Closed
Bug 1334112
Opened 7 years ago
Closed 7 years ago
<video autoplay> sometimes starts playing before text tracks are ready
Categories
(Core :: Audio/Video: Playback, defect, P1)
Core
Audio/Video: Playback
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.
Reporter | ||
Comment 1•7 years ago
|
||
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
Updated•7 years ago
|
Component: Audio/Video → Audio/Video: Playback
Assignee | ||
Comment 3•7 years ago
|
||
We miss the implementation, our MediaElement and TrackElement load their resources separately not notify/inform to each other.
Flags: needinfo?(bechen)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → bechen
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 7•7 years ago
|
||
mozreview-review |
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 8•7 years ago
|
||
mozreview-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 9•7 years ago
|
||
mozreview-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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 13•7 years ago
|
||
mozreview-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 14•7 years ago
|
||
mozreview-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!
Updated•7 years ago
|
Priority: -- → P1
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 17•7 years ago
|
||
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
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c0f61862e942 https://hg.mozilla.org/mozilla-central/rev/f2efeff392e3 https://hg.mozilla.org/mozilla-central/rev/c3c1ac0041b7
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•