Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category Music playback
  • Assigned To
    learman
  • Operating System SW-codec
  • Severity Low
  • Priority Very Low
  • Reported Version Daily build (which?)
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by learman - 2009-02-22
Last edited by learman - 2009-03-08

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

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.

Closed by  learman
2009-03-08 13:50
Reason for closing:  Accepted

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.

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).

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.

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing