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

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

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.

Comment 1

[Jean-Yves Avenard [:jya]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=53a2464101a0

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Only fire loadeddata once the first frame has been decoded.

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: P2. Do not transition to HAVE_ENOUGH_DATA readyState until we do have data.

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.

Comment 4

[Jean-Yves Avenard [:jya]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=37baf147022a

Comment 5

[Jean-Yves Avenard [:jya]]

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...

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: P2. Do not transition to HAVE_ENOUGH_DATA readyState until we do have data.

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.

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/590ee345a6ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/d673112518b5

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/590ee345a6ed
https://hg.mozilla.org/mozilla-central/rev/d673112518b5