481488 - video readyState wrong for file:// media, throbber doesn't go away when seeking

Status: VERIFIED FIXED

(Core :: Audio/Video, defect, P2)

Description

[Justin Dolske [:Dolske]]

If I open a file:// OGG in the browser, it plays fine and the video controls work. But if I grab the scrubber and change the playback position, the that also works but the video throbber overlay never goes away (the video continues playing under it).

Probably not getting an expected event from the <video> element. Will turn on debugging and see what's not happening...

Comment 1

[Justin Dolske [:Dolske]]

Hmm. So, here's the sequence of events I get for a file:// (left) and http:// video. Video is initially paused, then I click the scrubber bar to trigger a seek, wait a while, and then click play...

    file://                      http://
--------------------------------------------------
seeking                   |  seeking
(throbber triggered)      |  (throbber triggered)
                          |  ...progress events...
timeupdate                |  timeupdate
seeked                    |  seeked
                          |  canplaythrough
                          |  ...progress events...
play                      |  play
waiting                   |
                          |  playing
...timeupdate events...   |  ...timeupdate events...


The http:// version fires canplaythough and playing events, which are where the throbber is normally halted. The file:// version doesn't. I think this is technically correct -- "4.8.10.7 The ready states" says these events are only fired during certain .readyState transitions, and so depending on how the video is buffered it's possible these wouldn't be expected to fire after a seek. [I'm not sure how our not-so-good buffering works with file:// data, though. Do we internally handle things as if the entire video is buffered, even after seeking?]

I anticipated this in the video controls, though, and the |seeked| handler is checking to see if the .readyState is already HAVE_ENOUGH_DATA (which implies these events wouldn't fire)... But when the controls get the seeked event for the file:// video, .readyState is  only HAVE_CURRENT_DATA.

So, it looks like either .readyState should change later (triggering the events) but doesn't, or .readyState should always be HAVE_ENOUGH_DATA for file:// sources.

Also note the oddball |waiting| event for the file://... That's probably a side effect of the same readyState problem... It's also technically correct if playback begins when we don't have future data.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

In DECODER_STATE_SEEKING case of nsOggDecodeStateMachine::Run(), after the seek returns, we deocode a frame, so we can draw a poster for the new seeked-to position. Then we synchronously call nsOggDecoder::SeekingStopped() on the main thread. This calls nsOggDecoder::UpdateReadyStateForData(), which asks the decode state machine if it has any more frames, via nsOggDecodeStateMachine::HaveNextFrameData(). nsOggDecodeStateMachine::Run() will most likely have only decoded one frame, and so HaveNextFrameData() returns NEXT_FRAME_UNAVAILABLE. nsOggDecoder::UpdateReadyStateForData() then passes NEXT_FRAME_UNAVAILABLE onto nsHTMLMediaElement::UpdateReadyStateForData(), which then changes readyState to HAVE_CURRENT_DATA. In the case where we're loading from disk, we're doing this, despite the fact that we've downloaded the file entirely.

I think the fix is to not change readyState to HAVE_CURRENT_DATA when we've downloaded the entire file, we should change to HAVE_ENOUGH_DATA.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1

Changes nsHTMLMediaElement::UpdateReadyStateForData() so that it does not set readyState to HAVE_CURRENT_DATA if it has downloaded the entire resource, it will change state to HAVE_ENOUGH_DATA instead. This patch is based on the media cache patch from bug 475441.

Comment 5

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

Comment on attachment 368448 [details] [diff] [review]
Patch v1

+      stats.mTotalBytes != stats.mDownloadPosition) {

Use <= here, just in case.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2

As v1, with "stats.mTotalBytes <= stats.mDownloadPosition".

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v3 - It must be ">" not "<="

Oops... That's wrong... That needs to be "stats.mTotalBytes > stats.mDownloadPosition".

Or stats.mDownloadPosition < stats.mTotalBytes for readibility.

It can't be "stats.mDownloadPosition <= stats.mTotalBytes". We're trying to exclude the case where stats.mTotalBytes == stats.mDownloadPosition...

Can mDownloadPosition ever be > mTotalBytes?

Comment 8

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

I wonder if we could have a test here. Loading file URLs from mochitests is a bit of a pain but I think you already did it for access-controls?

Comment 9

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

http://hg.mozilla.org/mozilla-central/rev/f5ae6b3f6120

Comment 10

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

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

Comment 11

[Aaron Train [:aaronmt]]

Verified FIXED

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090603 Shiretoko/3.5pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090603 Minefield/3.6a1pre