1334112 - <video autoplay> sometimes starts playing before text tracks are ready

Status: RESOLVED FIXED

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

Description

[Simon Pieters [:zcorpan]]

https://html.spec.whatwg.org/#ready-states says:

HAVE_FUTURE_DATA: "the text tracks are ready".
HAVE_ENOUGH_DATA: All the conditions described for the HAVE_FUTURE_DATA state are met, ...

When the ready state of a media element whose networkState is not NETWORK_EMPTY changes,
the user agent must follow the steps given below:
If the new ready state is HAVE_ENOUGH_DATA
(autoplay)

So if the text tracks are not yet ready, we can't autoplay.

Gecko *sometimes* does autoplay anyway, before text tracks are ready (so video.textTracks[0].cues.length is 0 when 'playing' is fired).

In https://github.com/w3c/web-platform-tests/pull/4385#issuecomment-268907045 I discovered that a few tests were unstable in Gecko because of this bug.

In https://github.com/w3c/web-platform-tests/pull/4385/commits/a13f65f90abe27e6902bba1223e56efc85343aee I added a new test specifically to test this bug. This test reliably fails in Gecko; reliably passes in Chromium and WebKit.

Comment 1

[Simon Pieters [:zcorpan]]

Fixed link to the new test:
https://github.com/w3c/web-platform-tests/blob/707d79318f6be6d7239453d52530f31bf5bcc769/html/semantics/embedded-content/media-elements/ready-states/autoplay-with-slow-text-tracks.html

Comment 2

[Blake Wu [:bwu][:blakewu]]

bechen,
Can you check this?

Comment 3

[Benjamin Chen [:bechen]]

We miss the implementation, our MediaElement and TrackElement load their resources separately not notify/inform to each other.

Comment 4

[Benjamin Chen [:bechen]]

Attachment: Bug 1334112 - part1: Add IsLoaded functions for TextTrack, TextTrackList, TextTrackManager.

Review commit: https://reviewboard.mozilla.org/r/113364/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/113364/

Comment 5

[Benjamin Chen [:bechen]]

Attachment: Bug 1334112 - part2: When changing the readyState to HAVE_FUTURE_DATA and HAVE_ENOUGH_DATA, the texttracks must be loaded.

Review commit: https://reviewboard.mozilla.org/r/113366/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/113366/

Comment 6

[Benjamin Chen [:bechen]]

Attachment: Bug 1334112 -part3 : Trigger UpdateReadyState after unbind TrackElement's and TextTrack::SetReadyState.

Review commit: https://reviewboard.mozilla.org/r/113368/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/113368/

Comment 7

[Ralph Giles (:rillian)]

Comment on attachment 8838425 [details]
Bug 1334112 - part1: Add IsLoaded functions for TextTrack, TextTrackList, TextTrackManager.

https://reviewboard.mozilla.org/r/113364/#review114962

::: dom/media/TextTrack.cpp:345
(Diff revision 1)
> +bool
> +TextTrack::IsLoaded()
> +{
> +  if (mMode == TextTrackMode::Disabled) {
> +    return true;
> +  } else {

The `else` here is redundant with the return. Just proceed assuming the condition has been asserted and save an indentation level.

```
if (mMode == TextTrackMode::Disabled) {
  return true;
}
// If the TrackElement's src is null, we can not block the
// MediaElement.
if (mTrackElement) { ...
```

::: dom/media/TextTrack.cpp:354
(Diff revision 1)
> +      nsAutoString src;
> +      if (!(mTrackElement->GetAttr(kNameSpaceID_None, nsGkAtoms::src, src))) {
> +        return true;
> +      }
> +    }
> +    return (mReadyState >= Loaded ? true : false);

`mReadyState >= Loaded` should already be a bool, so I don't think you need the ternary.

Comment 8

[Ralph Giles (:rillian)]

Comment on attachment 8838427 [details]
Bug 1334112 -part3 : Trigger UpdateReadyState after unbind TrackElement's and TextTrack::SetReadyState.

https://reviewboard.mozilla.org/r/113368/#review114966

Comment 9

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

Comment on attachment 8838426 [details]
Bug 1334112 - part2: When changing the readyState to HAVE_FUTURE_DATA and HAVE_ENOUGH_DATA, the texttracks must be loaded.

https://reviewboard.mozilla.org/r/113366/#review115322

::: dom/html/HTMLMediaElement.cpp:5574
(Diff revision 1)
>      // Force HAVE_CURRENT_DATA when buffering.
>      ChangeReadyState(nsIDOMHTMLMediaElement::HAVE_CURRENT_DATA);
>      return;
>    }
>  
> -  if (mDownloadSuspendedByCache && mDecoder && !mDecoder->IsEnded()) {
> +  // Bug 1334112: Check the TextTrack loading state for the

if (mTextTrackManager && !mTextTrackManager->IsLoaded()) {
  // Force HAVE_CURRENT_DATA if text tracks not loaded.
  ChangeReadyState(nsIDOMHTMLMediaElement::HAVE_CURRENT_DATA);
}

Add the if here so you don't have to spread |isTextTracksLoaded| checks below.

Comment 10

[Benjamin Chen [:bechen]]

Comment on attachment 8838425 [details]
Bug 1334112 - part1: Add IsLoaded functions for TextTrack, TextTrackList, TextTrackManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113364/diff/1-2/

Comment 11

[Benjamin Chen [:bechen]]

Comment on attachment 8838426 [details]
Bug 1334112 - part2: When changing the readyState to HAVE_FUTURE_DATA and HAVE_ENOUGH_DATA, the texttracks must be loaded.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113366/diff/1-2/

Comment 12

[Benjamin Chen [:bechen]]

Comment on attachment 8838427 [details]
Bug 1334112 -part3 : Trigger UpdateReadyState after unbind TrackElement's and TextTrack::SetReadyState.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113368/diff/1-2/

Comment 13

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

Comment on attachment 8838426 [details]
Bug 1334112 - part2: When changing the readyState to HAVE_FUTURE_DATA and HAVE_ENOUGH_DATA, the texttracks must be loaded.

https://reviewboard.mozilla.org/r/113366/#review115348

::: dom/html/HTMLMediaElement.cpp:5574
(Diff revision 2)
>      // Force HAVE_CURRENT_DATA when buffering.
>      ChangeReadyState(nsIDOMHTMLMediaElement::HAVE_CURRENT_DATA);
>      return;
>    }
>  
> +  // Bug 1334112: TextTracks must be loaded for the

The convention is only mentioning the bug number for TODO or FIXME.

Comment 14

[Ralph Giles (:rillian)]

Comment on attachment 8838425 [details]
Bug 1334112 - part1: Add IsLoaded functions for TextTrack, TextTrackList, TextTrackManager.

https://reviewboard.mozilla.org/r/113364/#review115418

::: dom/media/TextTrack.cpp
(Diff revisions 1 - 2)
> -      if (!(mTrackElement->GetAttr(kNameSpaceID_None, nsGkAtoms::src, src))) {
> +    if (!(mTrackElement->GetAttr(kNameSpaceID_None, nsGkAtoms::src, src))) {
> -        return true;
> +      return true;
> -      }
> +    }
> -    }
> +  }
> -    return (mReadyState >= Loaded ? true : false);
> +  return (mReadyState >= Loaded);
> -  }

Looks better, thanks!

Comment 15

[Benjamin Chen [:bechen]]

Comment on attachment 8838426 [details]
Bug 1334112 - part2: When changing the readyState to HAVE_FUTURE_DATA and HAVE_ENOUGH_DATA, the texttracks must be loaded.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113366/diff/2-3/

Comment 16

[Benjamin Chen [:bechen]]

Comment on attachment 8838427 [details]
Bug 1334112 -part3 : Trigger UpdateReadyState after unbind TrackElement's and TextTrack::SetReadyState.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113368/diff/2-3/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c0f61862e942
part1: Add IsLoaded functions for TextTrack, TextTrackList, TextTrackManager. r=rillian
https://hg.mozilla.org/integration/autoland/rev/f2efeff392e3
part2: When changing the readyState to HAVE_FUTURE_DATA and HAVE_ENOUGH_DATA, the texttracks must be loaded. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/c3c1ac0041b7
part3 : Trigger UpdateReadyState after unbind TrackElement's and TextTrack::SetReadyState. r=rillian

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/c0f61862e942
https://hg.mozilla.org/mozilla-central/rev/f2efeff392e3
https://hg.mozilla.org/mozilla-central/rev/c3c1ac0041b7