Rockbox

Tasklist

FS#6317 - Lockup if stopping while still seeking to resume point

Attached to Project: Rockbox
Opened by Peter D'Hoye (petur) - Saturday, 11 November 2006, 23:27 GMT
Last edited by Michael Sevakis (MikeS) - Tuesday, 31 July 2007, 11:13 GMT
Task Type Bugs
Category Music playback
Status Closed
Assigned To No-one
Operating System SW-codec
Severity High
Priority Normal
Reported Version Daily build (which?)
Due in Version Version 3.0
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

Hitting stop while still reading the file to reach the resume point causes a lockup.

1) Play a fairly large WAV and seek quite far into it
2) stop layback
3) start playback using 'play' to resume where left off
4) press stop again before playback resumes (= while reading the file to rewach the resume point)

backlight thread still running but paperclip required to get a usable player again.

Observed on h340 but probably an issue on al SWCODEC
This task depends upon

Closed by  Michael Sevakis (MikeS)
Tuesday, 31 July 2007, 11:13 GMT
Reason for closing:  Fixed
Additional comments about closing:  Fixed is as fixed does. I thought I closed this one.
Comment by Nils Wallménius (nls) - Sunday, 12 November 2006, 13:34 GMT
This appears to be related to  FS#6129 
Comment by Peter D'Hoye (petur) - Monday, 13 November 2006, 07:27 GMT
so it seems... *looks at pondlife*
Comment by Steve Bavin (pondlife) - Monday, 13 November 2006, 07:30 GMT
Don't look at me, I only reported the problem! ;-) In fact, I didn't even do that - Deb did, but I filed it on Flyspray so it wouldn't get lost.

I am able to reproduce this, maybe will have time to look into it this week...
Comment by Mark Arigo (lowlight) - Friday, 17 November 2006, 18:59 GMT
Can reproduce on my H140 with a big flac file, but not the sim (bummer).

The audio thread is getting stuck in an infinite loop in audio_stop_codec_flush(), specifically the loop
while (audio_codec_loaded)
yield();
It's easy to see on the remote if you put a logf with a counter in there.

So, I guess the audio thread is waiting for the codec thread to stop, but the codec thread is still waiting for the audio thread to finish buffering. It looks like the audio thread has buffered some data, however, I'm not sure why control is not returned to the codec thread, but instead the audio thread continues to process the stop request.
Comment by Mark Arigo (lowlight) - Friday, 17 November 2006, 20:12 GMT
Ok, here's some logf dumps if anyone wants to take a look. One for a successful resume and one that locks up the player when stop is pressed during the rebuffer (I got that one by calling logfdump after 100 iterations in the infinite loop).

The sequence of events are as follows:
1. Start up
2. Play track (150MB flac)
3. Seek far enough to cause a rebuffer & seek
4. Stop (return to file tree)
5. Press play to resume playback
6a. For locked up case: Press stop while buffering (don't know if the timing is important here...I waited until I saw the wps and then pressed stop a couple of times).
6b. For ok resume case: wait for hdd activity to stop, then logfdump.

Note: there are a couple of extra logf's added here and there.
Comment by Michael Sevakis (MikeS) - Friday, 17 November 2006, 23:55 GMT
It's conceivable that the current codec could finish and the next audio codec could start loading before audio_codec_loaded is checked again. If a message is buffered queue_wait doesn't yield and neither does mutex_lock if the mutex isn't taken so audio_codec_loaded will be always seen to be true even if false for a brief period. Just my 0x02.

EDIT: ...though I have an idea of using a counter instead of a bool so every time an audio codec loads it increments the counter by 1 and you'd be able to detect this state. Simply adding yields somewhere is still a race condition.

EDIT2: Having the counter start at zero and increment when loading a codec and increment a again when unloading would make it even when no codec is loaded and odd when a codec is loaded and other checks can simply be if (encoder_version & 0x1) instead of if (audio_codec_loaded) but the aliasing problem would be nonexistent too.
Comment by Michael Sevakis (MikeS) - Saturday, 18 November 2006, 04:25 GMT
Well, forget all my BS. :P That doesn't stop the problem even though there's a possibility of what I described.
Comment by Mark Arigo (lowlight) - Saturday, 18 November 2006, 06:01 GMT
I don't think it's codec loading/unloading timing. It's just that the codec thread queue_waits for the rebuffer to finish, but somehow the audio thread processes the stop event before notifying the codec thread that it's done buffering. Thus, the code path never returns to the codec. If it did, there are checks to see if playback has stopped.

I think there may be some confusion in the buffering process for flac files. When a flac files resumes at a long offset, it must be buffered from the beginning in order to get the stream info & seek tables and then immediately rebuffered to the proper seek point. I think maybe the stop is coming between the initial fill buffer request and secondary seek & rebuffer. That seems to be what the logf dump is indicating for the lockup case (look at when Q_AUDIO_REBUFFER_SEEK is queued).

If that's the case, maybe a solution is to make codec_seek_buffer_callback return "busy" while the audio thread is filling and make the codec do a yield loop until it's ready.
Comment by Michael Sevakis (MikeS) - Saturday, 18 November 2006, 07:03 GMT
Works on a WAV too every time. It's clear the codec thread is locked somewhere too since it should exit the entry point and change the counter. It became apparent it is not and is likely stuck in a callback like you said.
Comment by Michael Sevakis (MikeS) - Saturday, 18 November 2006, 07:34 GMT
Ok, if Q_AUDIO_REBUFFER_SEEK is posted to the audio queue in codec_advance_buffer callback while the audio thread is in a loop not dequeueing messages and the codec waits on the codec_callback_queue but the audio thread is what posts messages there, you have a nice complex deadlock. The codec thread cannot take a path to exit the callback and return and release the audio thread from its loop. Viola!
Comment by Michael Sevakis (MikeS) - Saturday, 18 November 2006, 08:37 GMT
If I post Q_AUDIO_STOP (an unambiguous message that ensures ci.stop_codec is true) to the codec callback queue and call queue_remove_from_head to ensure that the message doesn't linger in the queue if nothing was dequeueing at the time, the problem goes away.
Comment by Michael Sevakis (MikeS) - Saturday, 18 November 2006, 08:46 GMT
The problem with doing that though is audio is stopped when doing a skip but it at least shows that the problem is a deadlock because of the wait on the callback queue for certain.
Comment by Michael Sevakis (MikeS) - Saturday, 18 November 2006, 09:02 GMT
Posting Q_CODEC_REQUEST_COMPLETE before the loop in audio_stop_codec_flush gives much better results and what seems like a good fix. I still dequeue the message after the loop to make sure it doesn't abort later if there was no dequeue about to happen. If there's any pitfalls to doing that perhaps a special message should be used.
Comment by Michael Sevakis (MikeS) - Saturday, 18 November 2006, 10:49 GMT
Check out and test drive...
Comment by Mark Arigo (lowlight) - Monday, 20 November 2006, 15:11 GMT
Should probably post Q_CODEC_REQUEST_FAILED so the codec exits immediately.

Just thinking out loud...shouldn't rebuffer_seek be able to abort the current fill processes? It's very inefficient in these cases to fill the buffer twice. Or maybe we need a function like audio_fill_buffer_exact(Nbytes) that reads at most Nbytes onto the buffer. Flac (and wav?) could use that function for the initial buffering of the stream info & seek table.
Comment by Peter D'Hoye (petur) - Saturday, 09 December 2006, 21:19 GMT
yup, this patch fixes the issue
Comment by Michael Sevakis (MikeS) - Sunday, 10 December 2006, 01:00 GMT
It's not a proper fix so that's why I haven't committed it. The audio thread can still get spurious messages after the stop and the codecs do not behave in a consistent way since not all of them check return values from the callbacks. Will have to fix any that do not (wavpack.codec comes to mind). You'll probably get codec failures later after the stop during seek since the audio thread will still receive messages posted before the codec waited on the private queue.

One thing I've wondered about but haven't tested (off topic):
What happens if you have a codec on the buffer followed by several files that use that codec and the first file is invalid and causes a codec failure? Do the remaining files play or die?
Comment by Michael Sevakis (MikeS) - Monday, 01 January 2007, 06:51 GMT
Working on this fix when simplifying playback. :)
Comment by Deb (fatoldpig) - Tuesday, 27 March 2007, 03:47 GMT
Any update on this or when this going to be addressed?
Comment by Michael Sevakis (MikeS) - Saturday, 12 May 2007, 00:11 GMT
Should be addressed as of a few days ago from this post's date. I got sick of it still hanging around...no pun.
Comment by Deb (fatoldpig) - Thursday, 17 May 2007, 01:52 GMT
it solved this problem but created another bug. now when i the player is plugged in my car with a car adapter (car adapter mode is on), player freezes when i stop the car and the player goes into pause mode.
Comment by Peter D'Hoye (petur) - Tuesday, 31 July 2007, 07:41 GMT
What's the status on this, Mike?

Loading...