Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#5172 - Musepack seeking patch

Attached to Project: Rockbox
Opened by Andrew Cupper (snowgoon) - Tuesday, 18 April 2006, 11:30 GMT+2
Last edited by Thom Johansen (preglow) - Tuesday, 18 April 2006, 17:29 GMT+2
Task Type Patches
Category Codecs
Status Closed
Assigned To Thom Johansen (preglow)
Player Type All players
Severity Medium
Priority Normal
Reported Version
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

Musepack seeking patch as discussed here: http://www.musepack.net/forum/viewtopic.php?t=299

Fast seeking for musepack files through the use of a seek table, and a hack to prevent possible white noise after seeking in files encoded with buggy encoder.
   rockbox_mpc_seek_patch_1_2_3h.diff (55.6 KiB) 
 mpc.c                      |   25 -
 libmusepack/config_types.h |    2 
 libmusepack/decoder.h      |    9 
 libmusepack/huffman.h      |    4 
 libmusepack/huffsv7.c      |   20 -
 libmusepack/mpc_decoder.c  |  783 ++++++++++++++++++++++++++++++++++++---------
 libmusepack/musepack.h     |    3 
 libmusepack/synth_filter.c |    3 
 8 files changed, 670 insertions(+), 179 deletions(-)

   rockbox_mpc_seek_patch_1_2_3h.zip (32.3 KiB)
This task depends upon

Closed by  Thom Johansen (preglow)
Thursday, 31 August 2006, 20:19 GMT+2
Reason for closing:  Accepted
Additional comments about closing:  Commited.
Comment by Tan Yu Sheng (Farpenoodle) - Tuesday, 18 April 2006, 15:28 GMT+2
Patch compiles for iPod Mini but codec fails when trying to play any MPC files.
Comment by MichaelGiacomelli (saratoga) - Saturday, 22 April 2006, 01:28 GMT+2
Has anyone tried this on a 4G, 5G or nano?
Comment by Thom Johansen (preglow) - Monday, 24 April 2006, 01:09 GMT+2
Confirmed to not work on Ipods. Patch works in x86 sim, so does not appear to be an endiannes issue. Will try to have a look when I get time.
Snowgoon: we use four spaces as indents in Rockbox. Could you please upload a patch which use that instead of tabs?
Comment by Thom Johansen (preglow) - Monday, 24 April 2006, 14:46 GMT+2
OK, fixed on Ipods. The char type is unsigned by default on ARM, so you need to use signed char explicitely for mpc_int8_t in config_types.h.
Other than that, it looks fine. Is this patch scheduled for inclusion in libmpcdec?
Comment by Andrew Cupper (snowgoon) - Monday, 24 April 2006, 23:49 GMT+2

Do you need me to update the patch before this can be committed? I don't have a running dev environment currently so I can't generate a new patch - I could manually change the patch but I'm not able to test it at the moment...

I don't know if/when my patch to libmpcdec will be committed, so I haven't updated the libmpcdec patch with all my latest changes. I'm not sure the seeking patch will be approved for libmpcdec because it really requires patching the encoder as well to ensure 100% artifact free playback after seeking (and repairing mpc files encoded with the current sv7 encoder).

Also, I have some new changes to improve performance, but these are specific to the Rockbox version (this involves reducing type sizes in the decoder struct to fit it into IRAM). I'm planning to submit these changes in a new patch once the current patch is committed.

Better performance on iAudio X5 is also possible with increased allocation of IRAM to the codec plugin. At the moment this is not possible because the extra IRAM on the iAudio is being allocated to the LCD buffer, which I don't think is the best use of the extra IRAM. This might also allow for improvements in performance of other codecs on the iAudio...
Comment by Thom Johansen (preglow) - Monday, 24 April 2006, 23:54 GMT+2
Nah, it'll be fine. I'll commit this patch when Rockbox playback has been fixed enough for me to validate the patch works correctly.
Comment by Thom Johansen (preglow) - Friday, 28 April 2006, 02:51 GMT+2
Updated patch sans tabs and plus config_types.h change is at http://www.pvv.org/~thomj/rockbox/musepack_seek_updated.patch

Snowgoon:
Right now, the seek table has one entry for each frame, yes? This seems to be a bit much to me. For files with length over an hour, it would actually oveflow our current half a megabyte malloc buffer. We would also very much like to get rid of this buffer, so we discourage use of malloc as much as possible. If the seek table is made smaller, it can be statically allocated in the mpc.c code.
Not a showstopper right now, since we have a couple of other codecs that use malloc as well, though.
Also, do you know how well your patch preserves gapless playback? I detect some small gaps between tracks now, but that also might be my previous commit's fault.
Comment by Andrew Cupper (snowgoon) - Friday, 28 April 2006, 14:50 GMT+2

Yeah, I did think about adding a frames/seek-entry option, but didn't think it was worth the extra development. In the short term you could try switching off the seek table, ie. UseSeekTable=false, or disable it for very large files. This would slow seeking backward, and/or seeking forward to a point that has already been played, but the performance decrease may be acceptable in the short term. Re gapless, I don't think my patch would affect gapless playback. I'm still out of action (no dev pc) so I won't be doing any development (or testing ;) for at least a couple of weeks.
Comment by Thom Johansen (preglow) - Saturday, 29 April 2006, 23:06 GMT+2
As it is, seeking backwards in a file seldom works for me. More often than the seek working, the current file is just skipped. You got any idea why this happens?
Comment by Andrew Cupper (snowgoon) - Monday, 01 May 2006, 15:32 GMT+2

It looks like short seeks backwards are ok (at least on my 2 week old iAudio build), so my guess is the seek (or decode after seek) is failing if the filebuf has to get reloaded - sorry, I can't test it... :( I thought this tested ok, at least with older builds... maybe not...
Comment by Andrew Cupper (snowgoon) - Saturday, 17 June 2006, 05:06 GMT+2

Thom, not sure if you're still watching this - I will pm you anyway. I've only just found the time to look at this again - it appears as though the problem is with AUDIO_REBUFFER_GUESS_SIZE in playback.c. Since musepack cannot resync this is causing problems with the seek. Setting this to 0 seems to fix the seeking problem. I would suggest making this configurable (eg. like AUDIO_DEFAULT_FILECHUNK), so that it can be switched off for musepack.
Comment by Thom Johansen (preglow) - Sunday, 18 June 2006, 14:22 GMT+2
Still watching it, but got limited time to do anything right now. Plus, I'm not sure if this should go in before 3.0 or not. It might very well introduce bugs.
Comment by Andrew Cupper (snowgoon) - Thursday, 24 August 2006, 23:41 GMT+2

I have updated the musepack seeking patch to use a static allocated seektable which avoids the use of malloc. It depends on the following patch that fixes a general problem with rockbox when seeking backwards in large files, which needs to be applied to the rockbox core first ->

http://www.rockbox.org/tracker/task/5879

Performance has been improved by squeezing the decoder struct into IRAM. Even more performance could be gained on the iAudio X5 if the codec IRAM allocation was increased so that the output samples buffer fits into IRAM too... eg. ~45Mhz decoding

   musepack.patch (74.2 KiB) 
 rockbox/apps/codecs/mpc.c                      |   32 
 rockbox/apps/codecs/libmusepack/config_types.h |    2 
 rockbox/apps/codecs/libmusepack/decoder.h      |   46 -
 rockbox/apps/codecs/libmusepack/huffman.h      |    4 
 rockbox/apps/codecs/libmusepack/huffsv7.c      |   20 
 rockbox/apps/codecs/libmusepack/internal.h     |    4 
 rockbox/apps/codecs/libmusepack/mpc_decoder.c  |  969 +++++++++++++++++++------
 rockbox/apps/codecs/libmusepack/musepack.h     |    6 
 rockbox/apps/codecs/libmusepack/synth_filter.c |    5 
 9 files changed, 836 insertions(+), 252 deletions(-)

Comment by Thom Johansen (preglow) - Wednesday, 30 August 2006, 19:58 GMT+2
Everything seems to work fine and dandy this time around, but playback of low bitrate files (radio, thumb profiles) seem to be broken now, and obviously needs to be fixed.
The time I tried shrinking the size of the decoder struct members from 32 bits to something more sane, this is exactly what happened to me too, so I suggest you look into that first.
A couple of other questions, I don't have time to read the patch in detail, so bear with me if some of these are stupid:
Did you do any testing on the impact of removing the output buffer from IRAM? Having the output buffer in IRAM is beneficial when further DSP processing is to be done, but I don't know by how much.
Do you make sure the seek table never overflows?
What's with the NULL pointer check on OutData in synth_filter.c? Can this ever be NULL?
Does this impact pre-SV7 streams at all? I'm planning on enabling the SV4-6 code some day, and would like to know if something extra needs to be done.
Apart from this, nice work! I would commit this right now if not for the low-bitrate issue.
Comment by Andrew Cupper (snowgoon) - Thursday, 31 August 2006, 17:40 GMT+2

I have fixed playback of low res files.

- this patch tested significantly faster than the previous one on my iAudio - this should be verified on other targets. I didn't test with EQ on or anything - is that when dsp processing usually occurs?...
- seek table step is increased for large files, ie. n frames per entry, so it should never overflow. The seek table entry size was also changed to 32 bits rather than 16.
- the NULL is passed at the end of a seek to synthesize the last frame before the one being seeked to - helps with bit accurate playback after seeking (well, it would for properly encoded files)...
- pre-sv7 seem to decode ok with one fix for big endian players - header needs to be byte-swapped in mpc_streaminfo_read - I will submit a seperate patch, along with filling id3 info in metadata.c
   musepack.patch (74.2 KiB) 
 rockbox/apps/codecs/mpc.c                      |   32 
 rockbox/apps/codecs/libmusepack/config_types.h |    2 
 rockbox/apps/codecs/libmusepack/decoder.h      |   46 -
 rockbox/apps/codecs/libmusepack/huffman.h      |    4 
 rockbox/apps/codecs/libmusepack/huffsv7.c      |   20 
 rockbox/apps/codecs/libmusepack/internal.h     |    4 
 rockbox/apps/codecs/libmusepack/mpc_decoder.c  |  969 +++++++++++++++++++------
 rockbox/apps/codecs/libmusepack/musepack.h     |    6 
 rockbox/apps/codecs/libmusepack/synth_filter.c |    5 
 9 files changed, 836 insertions(+), 252 deletions(-)

Loading...