493443 - Should transition to HAVE_CURRENT_DATA whenever Ogg frame queue is empty

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think (but I'm not 100% sure) than when we empty the frame queue during seeking, we should enter HAVE_CURRENT_DATA and stay there until we get a frame queued up after the current frame, even if we have the entire resource buffered in the media cache.

Basically the question is what readyState means for our Ogg decoder. I propose that we simply say this:
-- if we have a frame queued up after the current frame, then readyState is either HAVE_FUTURE_DATA or HAVE_ENOUGH_DATA, depending on how much data is buffered in the media cache.
-- otherwise we have no frame queued up after the current frame and readyState is HAVE_CURRENT_DATA.

Currently we have something more complicated, like this:
-- if we have a frame queued up after the current frame, OR the decoder IsEnded, OR all the data has been downloaded, then readyState is either HAVE_FUTURE_DATA or HAVE_ENOUGH_DATA, depending on how much data is buffered in the media cache.
-- otherwise readyState is HAVE_CURRENT_DATA.

So currently when we seek, if not all of the resource has been buffered (e.g. all bytes except the last one buffered) we enter HAVE_CURRENT_DATA, otherwise we stay in HAVE_ENOUGH_DATA. That seems wrong.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If we do this change, then we can also simplify the treatment of the ended state. In the ended state, the frame queue is empty so UpdateReadyStateForData will automatically set readyState to HAVE_CURRENT_DATA.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

By passing the NEXT_FRAME_UNAVAILABLE_BUFFERING frame-queue status while seeking, we fire a 'waiting' event during seeking. This is seems like the right thing according to the spec.

I've made nsOggDecoder::PlaybackEnded be dispatched synchronously. Otherwise I'm afraid someone may trigger a seek and seeking and decoding activity could happen before PlaybackEnded actually runs, which seems bad.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

The Wave decoder needs to be updated to call UpdateReadyStateForData a bit more, so that removing the ChangeReadyState call from nsHTMLMediaElement::PlaybackEnded doesn't regress Wave.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v3

One more change to UpdateReadyStateForData: we should check IsBuffering and IsSeeking before we check HaveNextFrame, because it's possible for us to be buffering while we have multiple frames in the queue, but we should still show the throbber while buffering.

Comment 5

[cajbir (:cajbir)]

Comment on attachment 377976 [details] [diff] [review]
fix v3

+  PRBool IsSeeking() const {
+    return mState == nsOggDecodeStateMachine::DECODER_STATE_SEEKING;
+  }

Missing comment about needing decoder monitor held. Maybe assert it?

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Tested by test in bug 493187.

http://hg.mozilla.org/mozilla-central/rev/8ae3e659599b

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/dafc03dcb93f