Closed Bug 1217304 Opened 4 years ago Closed 4 years ago

loadeddata event shouldn't be fired prior the first frame being decoded

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(2 files, 1 obsolete file)

The loadeddata event is fired with our HTMLMediaElement as soons a readyState is >= HAVE_CURRENT_DATA

Deciding the value of readyState is based on how the file content being cached or available. This operation is disabled for small files

https://hg.mozilla.org/mozilla-central/file/f029ccdee154/dom/html/HTMLMediaElement.cpp#l3799
checks that NextFrameStatus() != MediaDecoderOwner::NEXT_FRAME_AVAILABLE and set readyState to HAVE_CURRENT_DATA.

This is the wrong behaviour, as even we if had a decode error and were unable to decode the first frame we will fire the loadeddata event.
Attachment #8677382 - Flags: review?(jwwang) → review+
We must have at least a decoded frame available to transition to HAVE_ENOUGH_DATA, as otherwise canplay/canplaythrough will always be fired even if the data was invalid so long that it was small.
Attachment #8677890 - Flags: review?(jwwang)
Comment on attachment 8677890 [details] [diff] [review]
P2. Do not transition to HAVE_ENOUGH_DATA readyState until we do have data.

got a better idea...
Attachment #8677890 - Attachment is obsolete: true
Attachment #8677890 - Flags: review?(jwwang)
We must have at least a decoded frame available to transition to HAVE_ENOUGH_DATA, as otherwise canplay/canplaythrough will always be fired even if the data was invalid so long that it was small.
Attachment #8677896 - Flags: review?(jwwang)
Comment on attachment 8677896 [details] [diff] [review]
P2. Do not transition to HAVE_ENOUGH_DATA readyState until we do have data.

Review of attachment 8677896 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/HTMLMediaElement.cpp
@@ +3817,5 @@
>      }
>      return;
>    }
>  
> +  if (!mFirstFrameLoaded) {

Might worth adding an assertion somewhere to ensure we only transition to HAVE_CURRENT_DATA or above when mFirstFrameLoaded is true. It will help check if we regress the readyState transition algorithm.
Attachment #8677896 - Flags: review?(jwwang) → review+
https://hg.mozilla.org/mozilla-central/rev/590ee345a6ed
https://hg.mozilla.org/mozilla-central/rev/d673112518b5
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Depends on: 1270154
No longer depends on: 1270154
You need to log in before you can comment on or make changes to this bug.