Rockbox

forgot patch

   fix_skipping_back.patch (3.4 KiB)

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.

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.

Why should this patch work bad on a hdd targets?

I haven't noticed more buffering steps when skipping back using it.

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?

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.