Rockbox

Tasklist

FS#10466 - Real malloc for the Vorbis codec

Attached to Project: Rockbox
Opened by Magnus Holmgren (learman) - Saturday, 25 July 2009, 15:43 GMT
Last edited by Magnus Holmgren (learman) - Saturday, 29 August 2009, 13:17 GMT
Task Type Patches
Category Codecs
Status Closed
Assigned To Magnus Holmgren (learman)
Operating System SW-codec
Severity Low
Priority Normal
Reported Version Version 3.3
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

This patch adds a real malloc implementation to Tremor, based on TLSF (also used by PDBox). As I'm not yet sure where TLSF should be located, it isn't included in the patch; just copy the TLSF-2.4.4/src folder from PDBox and place it in apps/codecs/libtremor and call it tlsf.

A few notes:

* It currently enables TLSF statistics for the simulator build, to report peak memory usage after having decoded a track.

* Some changes to the seek hack has been made, and some or all of them should really have been done when vorbisfile.c was updated recently, but it wasn't until now it caused any problems.

* ogg_tmpmalloc has been removed. Normal ogg_malloc is used instead.

* One change to vorbisfile.c has been made, to fix what appears to be a bug when opening files in non-seekable mode (which Rockbox does in the seek hack). This has been reported to Xiph.

* Added MIPS to the longjmp support.

* There's some (commented) debug code in oggmalloc.c that probably should be removed before commit.

I have tested this on a few tracks, mostly created with a recent encoder, but also a test file from an older encoder (which I believe uses floor0; it uses much more memory at least, about 370 kB vs 100 kB for the newer files). More tests would be good though.
This task depends upon

Closed by  Magnus Holmgren (learman)
Saturday, 29 August 2009, 13:17 GMT
Reason for closing:  Accepted
Comment by Magnus Holmgren (learman) - Sunday, 26 July 2009, 19:36 GMT
Hm, there may be a problem with TLSF... I found (and fixed) one problem in the realloc implementation, but it is still possible to run out of memory and not get a proper NULL return (my Sansa e200 freezes, rather than a clean exit via longjmp). That's at least what I think is happening; I don't have any good means to debug it on the target (and it is only certain cases that trigger the freeze).
Comment by Magnus Holmgren (learman) - Monday, 27 July 2009, 13:04 GMT
Hm, I'm starting to think the problem is related to seeking, rather than memory allocations. It happens on a file with a huge comment packet, and it may be that the (new) seeking code doesn't handle that extreme case well (either entering an infinite loop or just taking a very long time). It should be noted that such files require lots of memory, due to how the Ogg layer is designed (always read a full packet into allocated buffers). Comments aren't supposed to be that large. :)

Anyway, I found another malloc implemenation that I tested, heapmm. Not constant time as TLSF, but should be fast enough. Slightly lower memory overhead and slightly smaller binary size compared to TLSF, but we're only talking a few kB here. I can upload a patch, if there's any interest.
Comment by Magnus Holmgren (learman) - Thursday, 30 July 2009, 21:01 GMT
OK, my changes to the seeking hack weren't right; some OggVorbis_File data was in one form during open and another form after, so just adding a few frees right after open worked much better. Seeking in the big comment file now works.

Also removed some obsolete comments in vorbis.c.

Finally, the change to TLSF is in tlsf.c and looks like this:

864c859,860
< ptr_aux = malloc_ex(new_size, mem_pool);
---
> if (!(ptr_aux = malloc_ex(new_size, mem_pool)))
> return NULL;
Comment by alex wallis (alexwallis646) - Saturday, 08 August 2009, 17:43 GMT
Hi. Do you need a user to volunteer to test this on target?
or is this not at a stage to be run on target yet?
if you would like this run on target, what exactly should I be looking to report apart from the obvious freezes, crashes and general weird behaviour that is almost certain to result with early patches?
Comment by Magnus Holmgren (learman) - Saturday, 08 August 2009, 19:11 GMT
It works fine for me on an arm target. I can test a coldfire target as well. My main problem is that I don't have that many test files, produced with different encoder versions and processed by different taggers. Crashes and files not playing are the main things to look for.
Comment by alex wallis (alexwallis646) - Saturday, 08 August 2009, 21:57 GMT
OK, I see. Is it likely I might notice a slow down in how fast the file is decoded?

Also, do I still need to copy that file across from pdbox in other words, is there anything extra I need to do other than just apply the patch? as I am a little confused by some of the early comments for the task.
Comment by Magnus Holmgren (learman) - Sunday, 09 August 2009, 18:38 GMT
There shouldn't be any speed difference. Memory allocations are only done during decoder setup, when starting a new file.

Just copy the contents of TLSF-2.4.4/src from pdbox and change that malloc_ex call.

I tested on a coldfire target, with a some more test files, and it looks good. One file caused a crash, but that happens without this patch too. It's similar to  FS#10492 ...
Comment by alex wallis (alexwallis646) - Monday, 10 August 2009, 12:13 GMT
Hi. I am not really familiar with c, can you please give me exact instructions on what line I need to change, where it is in the file and what I should be changing it to? once I apply this patch, what sort of savings am I likely to see? and will this help with battery life? I mostly listen to 160 kbs stereo ogg files on my player.
Comment by Magnus Holmgren (learman) - Monday, 10 August 2009, 16:48 GMT
Well, you only need to be able to read the patch. :) Go to line 864. There you should find the code before the "---" (and after the "<"). Replace it with the code after "---" (excluding the ">" chars). The line numbers after the "c" are a bit different because I made a few other (unnecessary) changes I made.

Ideally, the changes will not be visible for the end-user, so no battery life changes. There's the possibility that files that previously didn't play will now play.
Comment by alex wallis (alexwallis646) - Monday, 10 August 2009, 18:23 GMT
Hi.

OK, I understand so can you please tell me if I have got this write?

the file that I need to be editing is tlsf.c

I need to go to line 864 in that file and delete it.

I then need to insert the following two lines of code that you pasted above

> if (!(ptr_aux = malloc_ex(new_size, mem_pool)))
> return NULL;

have I understood that right? sorry it just wasn't clear if it was tlsf.c that needs to be patched.
Comment by alex wallis (alexwallis646) - Monday, 10 August 2009, 20:22 GMT
OK, I had a look at the comments again, and I am fairly sure I am doing things right.
I went to line 864 in tlsf.c and pasted in the exact lines of code below it
in my text editor ptr_aux = malloc_ex(new_size, mem_pool); is line 864
I pasted
> if (!(ptr_aux = malloc_ex(new_size, mem_pool)))
> return NULL;
under it.
I then tried to compile under cygwin and got an error.

Would you like me to give it? I assume I should have included the > sign next to the if?
Comment by alex wallis (alexwallis646) - Tuesday, 11 August 2009, 11:03 GMT
OK, just to save time, the error I am getting from cygwin when I try to compile is
/home/Administrator/rockbox/apps/codecs/libtremor/tlsf/tlsf.c:865: error: syntax
error before '>' token

I am sure I have followed your instructions about changing the call in the right place. So do you know what might be going on?
Comment by Magnus Holmgren (learman) - Saturday, 15 August 2009, 11:04 GMT
You didn't fully follow my instructions. :) You missed this bit: 'excluding the ">" chars'.

Still, the fix isn't _that_ important. If not added, you can get a crash rather than skipping a track in certain cases where the file requires more memory than is available.
Comment by alex wallis (alexwallis646) - Sunday, 16 August 2009, 17:46 GMT
Hi. I tried removing the > signs. I take it the characters you are referring to are the ones in and just out side of the if statement?

I removed them, and I still got the same error under cygwin.

Could you please advise if what I did was correct?

This is how the fix currently looks in my source. If it is wrong, could you please paste me in how the code should look?
as I am sure I am following your instructions after having removed the > signs.
< ptr_aux = malloc_ex(new_size, mem_pool);
if (!(ptr_aux = malloc_ex(new_size, mem_pool)))
return NULL;

Comment by Magnus Holmgren (learman) - Sunday, 16 August 2009, 20:30 GMT
The change I mentioned above is part of the output from (svn) diff. The part above the "---" indicates the existing text to remove (that's what the "<" means), the text after the "---" indicates the text to insert instead of the removed text (that's what the ">" means).

So, in this case, line 864 contained this (before any changes):

ptr_aux = malloc_ex(new_size, mem_pool);

That line should be replaced with the following two lines:

if (!(ptr_aux = malloc_ex(new_size, mem_pool)))
return NULL;

Since it is a bug, I just committed a fix for it, so you can just copy the updated file instead.
Comment by alex wallis (alexwallis646) - Wednesday, 19 August 2009, 12:42 GMT
Hi. I now understand lol. I didn't actually realise you meant remove the original line from the file and replace it with the if statement, I thought I had to insert the if statement below the original line in the file. Anyway, I updated my source and put the updated file in the right place. Rockbox has now compiled fine with no problems. I have applied the v2 patch, so I will now listen to some files with it for a few days and report back any odd behaviour.
Comment by alex wallis (alexwallis646) - Sunday, 23 August 2009, 15:41 GMT
After several days of listening to files, I haven't had any weird crashes player freezes or any problems with rockbox at all. Perhaps this patch should be committed? as I have found no issues or any performance problems at all.
Though admittedly all the ogg files I listen to i think are 75 kbs mono, though I am not sure as winamp shows me an average bit rate of 75 kbs and a nominal one of 96 kbs. If there is any other technical information you would like about the files I have listened to using the patch please let me know.

Loading...