This is the bug/patch tracker for Rockbox. Click here for more information.
Quick links: Bugs · Patches · Rockbox frontpage
FS#10466 - Real malloc for the Vorbis codec
Attached to Project:
Rockbox
Opened by Magnus Holmgren (learman) - Saturday, 25 July 2009, 17:43 GMT+2
Last edited by Magnus Holmgren (learman) - Saturday, 29 August 2009, 15:17 GMT+2
Opened by Magnus Holmgren (learman) - Saturday, 25 July 2009, 17:43 GMT+2
Last edited by Magnus Holmgren (learman) - Saturday, 29 August 2009, 15:17 GMT+2
|
DetailsThis 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. 
vorbis_malloc.diff
(10.9 KiB)
apps/codecs/vorbis.c | 29 ++++++------- apps/codecs/libtremor/os_types.h | 9 +--- apps/codecs/libtremor/SOURCES | 1 apps/codecs/libtremor/vorbisfile.c | 2 apps/codecs/libtremor/oggmalloc.c | 78 +++++++++++++---------------------- apps/codecs/libtremor/sharedbook.c | 26 ++++++----- apps/codecs/libtremor/libtremor.make | 4 + apps/codecs/libtremor/block.c | 2 8 files changed, 69 insertions(+), 82 deletions(-) |
This task depends upon
Closed by Magnus Holmgren (learman)
Saturday, 29 August 2009, 15:17 GMT+2
Reason for closing: Accepted
Saturday, 29 August 2009, 15:17 GMT+2
Reason for closing: Accepted
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.
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;
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?
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.
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...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.
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.
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?
/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?
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.
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;
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.
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.