Rockbox

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

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#11004 - Buffering crashes when skipping back from end of song

Attached to Project: Rockbox
Opened by Thomas Martitz (kugel.) - Friday, 12 February 2010, 17:08 GMT+2
Last edited by Thomas Martitz (kugel.) - Thursday, 18 February 2010, 16:41 GMT+2
Task Type Bugs
Category Music playback
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version Release 3.4
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

This bug is very apparent on the Fuze, I assume it relates to the small audio buffer size.

If you have track skip set to "skip to outro" you can skip to the almost end of the song (~5s before).
If you do so, everything is good. but if you then skip back again to the beginning, weird this happen. The display can corrupt, data aborts can happen (the simulator simply segfaults) and more.

I've tracked down this bug to buffer_handle().
r23680 commented a part out which should handle overwriting the next handle (it didn't work). This can indeed happen (but shouldn't). Accessing that handle later leads to bad effects.
The other problem is, that when rebuffer_handle() runs (skipping to the beginning on the fuze leads to rebuffering often because of the small buffer), the WPS isn't notified and doesn't refetch metadata. This leads to garbage in the next track info [at the end of the song it gets the next track metadata from the buffer, but at the beginning it gets it from the static mp3entry struct. when skipping back it still tries to get the data from the buffer).

My patch tries to fix both problems. I'm not entirely sure if it's correct but I can't reproduce the problems on my fuze.

I'll commit soon if nobody spots a problem or blames me for coding shit.
This task depends upon

Closed by  Thomas Martitz (kugel.)
Thursday, 18 February 2010, 16:41 GMT+2
Reason for closing:  Accepted
Additional comments about closing:  r24755
Comment by Thomas Martitz (kugel.) - Friday, 12 February 2010, 17:08 GMT+2
forgot patch
   fix_skipping_back.patch (3.4 KiB) 
 b/apps/buffering.c |   38 +++++++++++++++++++++-----------------
 b/apps/playback.c  |    1 +
 2 files changed, 22 insertions(+), 17 deletions(-)

Comment by Thomas Martitz (kugel.) - Sunday, 14 February 2010, 21:43 GMT+2
From matsch:
<quote>
Hello Thomas,
I do not know why I cannot login into Flyspray. There I use private mail for reply.
After analyzing the patch I think it is OK, but I do not know the functions being called by skip to outro. Even normal skipping is done by the codec. I assume skipping to outro or skipping to begining of track is done by bufadvance and bufseek.

There are 3 cases.

1. The full track is in the audio buffer. Skipping is no problem with bufadvance,
because there is enough space for rebuffering
This is the case for large mem targets. But if you skip back less than 1 second before
track end, shink buffer will shink the track in the buffer.

2. A part of the track is in the audio buffer, the corresponding handle is current handle.
No problem with skipping, because rebuffering stops at buf_ridx. There is no next
handle
This is the case for small mem targets like clip except for very small tracks

3 A part of the track is in the audio buffer, the corresponding handle is not current handle.
buffer_handle is in trouble, because there is a next handle before buf_ridx. But
buffer_handle buffers until buf_ridx.
This is the case for medium mem targets like fuze.

Your patch fixes this, but I would prefer to reset the whole audio buffer, when skipping back and there is h->filerem > 0. At least verify that the size of audio_handle is not to small (e.g. > 100kB) if you don't want to do this.

Regards,

Matthias
</quote>


The problem seems to be that if you use skip to outro, the next track is being (partly) buffered, creating a handle for this (what your case 3 describes). Skipping back to the beginning causes reset_handle() to be called because the playing track needs to be rebuffered since the audio buffer is quite small. The handle for the next track is apparently not correctly invalidated (it seems to be by returning false in buffer_handle()). Additionally the WPS doesn't switch back from the in-buffer mp3entry struct to the static one in this scenario (the playback.c change fixes that).

It appears you understood the problem. I would be glad if you propose a better patch if you think mine isn't good enough.
Comment by Matthias Schneider (matsch) - Tuesday, 16 February 2010, 21:24 GMT+2
Your patch is an improvement. I recommend to commit it. I do not see an influence on other targets.
After skipping back the buffer size can be quite small.
This causes many rebuffering steps. I think this is OK for a flash target. Target without flash have large mem.
Perhaps one should think about the smallest buffer size being possible. I think it is 0.5 sec playing time
times bitrate. (rebuffering with shrink_buffer while h-> filerem = 1 and then skip back) The probability is quite low for this and maybe rebuffering is still possible.


Comment by Thomas Martitz (kugel.) - Tuesday, 16 February 2010, 22:23 GMT+2
Why should this patch work bad on a hdd targets?

I haven't noticed more buffering steps when skipping back using it.
Comment by Matthias Schneider (matsch) - Wednesday, 17 February 2010, 22:53 GMT+2
I did not write bad. for hdd targets. Rebuffering means spinning up the HDD and more current consumption.
buffering.c is very complicated. Maybe my interpretation is not right. I leave it up to other people to
correct it. For instance, I do not know what happens after skipping back to the beginning of playing track. Is there a rebubbering of the whole
ringbuffer or is bufadvance called?
Comment by Thomas Martitz (kugel.) - Thursday, 18 February 2010, 05:18 GMT+2
As I see it, my patch shouldn't introduce extra buffering actions. It mostly post-pones buffering of the current handle to the next yield() (if I got this right) which is when h->next is actually invalidated. I didn't see rebuffers on my fuze, but I will try to have a look at high mem targets as well.

Loading...