- Status Closed
- Percent Complete
- Task Type Patches
- Category Codecs
-
Assigned To
preglow - Operating System All players
- Severity Medium
- Priority Very Low
- Reported Version
- Due in Version Undecided
-
Due Date
Undecided
- Votes
- Private
FS#5172 - Musepack seeking patch
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...
rockbox_mpc_seek_patch_1_2_3h...
Closed by preglow
2006-08-31 18:19
Reason for closing: Accepted
Additional comments about closing: Warning: Undefined array key "typography" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 371 Warning: Undefined array key "camelcase" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 407
2006-08-31 18:19
Reason for closing: Accepted
Additional comments about closing: Warning: Undefined array key "typography" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 371 Warning: Undefined array key "camelcase" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 407
Commited.
Loading...
Available keyboard shortcuts
- Alt + ⇧ Shift + l Login Dialog / Logout
- Alt + ⇧ Shift + a Add new task
- Alt + ⇧ Shift + m My searches
- Alt + ⇧ Shift + t focus taskid search
Tasklist
- o open selected task
- j move cursor down
- k move cursor up
Task Details
- n Next task
- p Previous task
- Alt + ⇧ Shift + e ↵ Enter Edit this task
- Alt + ⇧ Shift + w watch task
- Alt + ⇧ Shift + y Close Task
Task Editing
- Alt + ⇧ Shift + s save task
Patch compiles for iPod Mini but codec fails when trying to play any MPC files.
Has anyone tried this on a 4G, 5G or nano?
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?
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?
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…
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.
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.
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.
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?
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…
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.
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.
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
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.
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