FS#9938 - Better handling of out of memory errors in Tremor

Attached to Project: Rockbox
Opened by Magnus Holmgren (learman) - Sunday, 22 February 2009, 13:17 GMT
Last edited by Magnus Holmgren (learman) - Sunday, 08 March 2009, 13:50 GMT
Task Type Patches
Category Music playback
Status Closed
Assigned To Magnus Holmgren (learman)
Operating System SW-codec
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No


Tremor, the Vorbis decoder library, doesn't always check the result of malloc, which can cause Rockbox to crash on some files (e.g., if they have really big comments, as in  FS#9866 ). To fix this, I've added setjmp/longjmp to the codec lib, which is used to jump back to the main codec loop if ogg_malloc fails, rather than return NULL. This causes the track to be skipped (it would be nice to have some sort of user indication for this).

The setjmp/longjmp implementation is from newlib 1.17.0, with a few minor changes (mainly the combination of a few files, and removal of support for some architectures we don't need). Some sort of file header should probably be added to the files from newlib, but I'm not sure exactly what that should look like.

The changes to the "previous_section" variable were done to avoid a warning about it being clobbered by longjmp.

I've (quickly) tested this on ARM (e200) and ColdFire (h140) and it seems to work as expected. It would be good to test this on some other ARM-based targets (assuming this fix is acceptable). Having MIPS support could be nice too (this would involve adding newlib/libc/machine/mips/setjmp.S, perhaps with a few adjustments, as well as updating the CPU check in setjmp.h), but I don't have any MIPS target to test with.
This task depends upon

Closed by  Magnus Holmgren (learman)
Sunday, 08 March 2009, 13:50 GMT
Reason for closing:  Accepted
Comment by MichaelGiacomelli (saratoga) - Sunday, 22 February 2009, 21:16 GMT
Wouldn't it make more sense to just statically allocate a comment buffer and then truncate if we exceed the buffer size?

I could see this being really useful for lowmem targets, since in theory mallocs inside the codec itself could fail.
Comment by Magnus Holmgren (learman) - Sunday, 22 February 2009, 22:45 GMT
That would solve the comment problem, but it requires bigger changes to Tremor. A simpler fix would be to just skip the comments, as the codec doesn't need them anyway. However, that won't fix all cases (e.g., certain floor0 files trigger the problem too).
Comment by MichaelGiacomelli (saratoga) - Sunday, 22 February 2009, 23:36 GMT
I have several thoughts.

1) We should pull floor0 support entirely. Its old, depreciated, virtually unused and we don't have enough memory to insure that it works consistently.

2) We should not be mallocing memory for comments at all.

3) This patch will probably be really useful once we cut down the size of the codec buffer, and will be useful regardless on the lowmem AMS players as I expect them to fail to decode a substantial number of files.

I say commit it.