Closed Bug 1334112 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: zcorpan, Assigned: bechen)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

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.
Component: Audio/Video → Audio/Video: Playback
bechen,
Can you check this?
Flags: needinfo?(bechen)
We miss the implementation, our MediaElement and TrackElement load their resources separately not notify/inform to each other.
Flags: needinfo?(bechen)
Blocks: webvtt
Assignee: nobody → bechen
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.
Attachment #8838425 - Flags: review?(giles) → review+
Comment on attachment 8838427 [details]
Bug 1334112 -part3 : Trigger UpdateReadyState after unbind TrackElement's and TextTrack::SetReadyState.

https://reviewboard.mozilla.org/r/113368/#review114966
Attachment #8838427 - Flags: review?(giles) → review+
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.
Attachment #8838426 - Flags: review?(jwwang) → review-
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.
Attachment #8838426 - Flags: review?(jwwang) → review+
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!
Priority: -- → P1
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.