Closed
Bug 1382303
Opened 7 years ago
Closed 7 years ago
[MSE] playing event incorrectly fired even when no data has been added
Categories
(Core :: Audio/Video: Playback, defect)
Tracking
()
RESOLVED
FIXED
mozilla56
People
(Reporter: jya, Assigned: jya)
References
Details
(Keywords: regression)
Attachments
(3 files)
59 bytes,
text/x-review-board-request
|
jwwang
:
review+
jcristau
:
approval-mozilla-beta+
gchang
:
approval-mozilla-esr52+
|
Details |
59 bytes,
text/x-review-board-request
|
jwwang
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jwwang
:
review+
|
Details |
Issue: Create a MediaSource Attach to HTML media element Create a source buffer Add Init segment. the playing event is fired. This is incorrect. Per spec: https://html.spec.whatwg.org/multipage/media.html#event-media-playing "readyState is newly equal to or greater than HAVE_FUTURE_DATA and paused is false, or paused is newly false and readyState is equal to or greater than HAVE_FUTURE_DATA." here we have at best readyState = HAVE_METADATA https://jyavenard.github.io/htmltests/tests/mse_mp4/facebook-init.html We can see the playing event being fired right after the play event. (reported by BBC, this is breaking their player) https://codepen.io/anon/pen/xrojbE for a narrowed case. It appears to be related to autoplay settings
Assignee | ||
Comment 1•7 years ago
|
||
In bug 1304134, we read "(https://dev.w3.org/html5/spec-preview/media-elements.html#ready-states) 'playing' should be fired before activating autoplay" I find no such description stating playing should be fired before autoplay in both w3.org and whatwg.org we have: w3.org: "readyState is newly equal to or greater than HAVE_FUTURE_DATA and paused is false, or paused is newly false and readyState is equal to or greater than HAVE_FUTURE_DATA. " whatwg.org: "readyState is newly equal to or greater than HAVE_FUTURE_DATA and paused is false, or paused is newly false and readyState is equal to or greater than HAVE_FUTURE_DATA" now in the example above, we see that playing was fired twice. even though playback never actually started. The only reference to the autoplaying attribute is in the section "If the new ready state is HAVE_ENOUGH_DATA" in bug 1304134 comment 1, we read "Small correction: just noticed that the "playing" event was fired, but some 20 seconds after the video started playing." so the issue wasn't that playing wasn't fired, but that it fired after playback had started. bug 1304134 comment 16, there's reference that this change would be wrong with MSE. Not even adding an init segment cause "playing" to be fired. The problem here is that we have readyState == HAVE_NOTHING If you attempt to seek when readyState == HAVE_NOTHING then per spec, the seek is ignored. This is likely the issue the BBC is seeing. IMHO bug 1304134 should be reversed, or modified such as with MSE that incorrect behaviour doesn't apply.
Comment 2•7 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #1) > In bug 1304134, we read > "(https://dev.w3.org/html5/spec-preview/media-elements.html#ready-states) > 'playing' should be fired before activating autoplay" > > I find no such description stating playing should be fired before autoplay https://html.spec.whatwg.org/multipage/media.html#ready-states If the new ready state is HAVE_ENOUGH_DATA If the element is not eligible for autoplay, then the user agent must abort these substeps. The user agent may run the following substeps: Set the paused attribute to false. If the element's show poster flag is true, set it to false and run the time marches on steps. Queue a task to fire an event named play at the element. Notify about playing for the element. <-- fire 'playing'.
Flags: needinfo?(jwwang)
Comment 3•7 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #1) > in bug 1304134 comment 1, we read "Small correction: just noticed that the > "playing" event was fired, but some 20 seconds after the video started > playing." > > so the issue wasn't that playing wasn't fired, but that it fired after > playback had started. There are in fact 2 issues: 1. comment 0 which I was able to reproduce. 2. comment 1 which was reproducible by the reporter. Issue 1 is fixed and issue 2 is not bug at all.
Comment 4•7 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #1) > bug 1304134 comment 16, there's reference that this change would be wrong > with MSE. > > Not even adding an init segment cause "playing" to be fired. The problem > here is that we have readyState == HAVE_NOTHING > > If you attempt to seek when readyState == HAVE_NOTHING then per spec, the > seek is ignored. > > This is likely the issue the BBC is seeing. > > IMHO bug 1304134 should be reversed, or modified such as with MSE that > incorrect behaviour doesn't apply. http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/dom/html/HTMLMediaElement.cpp#5989-5990 We should remove the special logic about MediaStream and MediaSource. However, it will break some sites or tests if I remember correctly. A quick workaround would be check readyState in CheckAutoplayDataReady() again before firing 'playing' to make MSE autoplay less broken.
Assignee | ||
Comment 5•7 years ago
|
||
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #2) > (In reply to Jean-Yves Avenard [:jya] from comment #1) > > In bug 1304134, we read > > "(https://dev.w3.org/html5/spec-preview/media-elements.html#ready-states)y > > 'playing' should be fired before activating autoplay" > > > > I find no such description stating playing should be fired before autoplay > > https://html.spec.whatwg.org/multipage/media.html#ready-states > > If the new ready state is HAVE_ENOUGH_DATA > > If the element is not eligible for autoplay, then the user agent must > abort these substeps. > The user agent may run the following substeps: > > Set the paused attribute to false. > If the element's show poster flag is true, set it to false and run the > time marches on steps. > Queue a task to fire an event named play at the element. > Notify about playing for the element. <-- fire 'playing'. The key point to get there is “if the new ready state is HAVE_ENOUGH_DATA”, at no stage did we reach readyState having that value. I’m sorry, but the reason behind bug 1304134 is incorrect. This change should be backed out There’s nothing in the spec that states playing should be fired before readyState having the right value.
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #3) > (In reply to Jean-Yves Avenard [:jya] from comment #1) > > in bug 1304134 comment 1, we read "Small correction: just noticed that the > > "playing" event was fired, but some 20 seconds after the video started > > playing." > > > > so the issue wasn't that playing wasn't fired, but that it fired after > > playback had started. > > There are in fact 2 issues: > 1. comment 0 which I was able to reproduce. > 2. comment 1 which was reproducible by the reporter. > > Issue 1 is fixed and issue 2 is not bug at all. Where/when was 1 fixed? Tests were done using today’s nightly
Comment 7•7 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #6) > Where/when was 1 fixed? > Tests were done using today’s nightly I mean bug 1304134 comment 0 and bug 1304134 comment 1 respectively.
Comment 8•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3891407491fb We should revert this change so autoplay is activated only when readyState is HAVE_ENOUGH_DATA.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8888483 [details] Bug 1382303 - P1. Add mochitest. https://reviewboard.mozilla.org/r/159440/#review164948 ::: dom/media/mediasource/test/test_PlayEventsAutoPlaying.html:64 (Diff revision 1) > + promises.push(once(el, 'playing')); > + promises.push(once(el, 'ended')); > + // We're only adding 1.6s worth of data, not enough for readyState to change to HAVE_ENOUGH_DATA > + // So we end the media source so that all the playable data is available. > + promises.push(fetchAndLoad(videosb, 'bipbop/bipbop_video', range(1, 3), '.m4s') > + .then(() => ms.endOfStream())); We should have ENOUGH_DATA after calling endOfStream() since: 1. playback hasn't reached the end. 2. we have all the data needed to play till the end without stopping. You can fix it in a new bug.
Attachment #8888483 -
Flags: review?(jwwang) → review+
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8888484 [details] Bug 1382303 - P2. Do not activate autoplay early. https://reviewboard.mozilla.org/r/159442/#review164952 ::: commit-message-64659:3 (Diff revision 1) > +Bug 1382303 - P2. Do not activate autoplay early. r?jwwang > + > +Per spec, autoplay should only gets triggered once readyState is equal or greater than HAVE_ENOUGH_DATA It is impossible for readyState to be greater than HAVE_ENOUGH_DATA.
Attachment #8888484 -
Flags: review?(jwwang) → review+
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8888485 [details] Bug 1382303 - P3. Reduce enough data threadhold to 10s. https://reviewboard.mozilla.org/r/159444/#review164956 ::: dom/media/mediasource/MediaSourceDecoder.cpp:316 (Diff revision 1) > - // If we have data up to the mediasource's duration or 30s ahead, we can > + // If we have data up to the mediasource's duration or 10s ahead, we can > // assume that we can play without interruption. > TimeIntervals buffered = GetBuffered(); > buffered.SetFuzz(MediaSourceDemuxer::EOS_FUZZ / 2); > TimeUnit timeAhead = > - std::min(duration, currentPosition + TimeUnit::FromSeconds(30)); > + std::min(duration, currentPosition + TimeUnit::FromSeconds(10)); It is more flexible to read the value from MediaPrefs.
Attachment #8888485 -
Flags: review?(jwwang) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 18•7 years ago
|
||
Pushed by jyavenard@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6568f892a5c4 P1. Add mochitest. r=jwwang https://hg.mozilla.org/integration/autoland/rev/0c6a7835fef4 P2. Do not activate autoplay early. r=jwwang https://hg.mozilla.org/integration/autoland/rev/5e11375d63c7 P3. Reduce enough data threadhold to 10s. r=jwwang
Assignee | ||
Comment 19•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8888483 [details] Bug 1382303 - P1. Add mochitest. https://reviewboard.mozilla.org/r/159440/#review164948 > We should have ENOUGH_DATA after calling endOfStream() since: > 1. playback hasn't reached the end. > 2. we have all the data needed to play till the end without stopping. > > You can fix it in a new bug. not sure I understand your comment here. It's exactly why we call endOfStream The reason we call endOfStream is precisely to move to HAVE_ENOUGH_DATA , otherwise readyState is HAVE_FUTURE_DATA And we do want to play something What is there to fix?
Assignee | ||
Comment 20•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8888485 [details] Bug 1382303 - P3. Reduce enough data threadhold to 10s. https://reviewboard.mozilla.org/r/159444/#review164956 > It is more flexible to read the value from MediaPrefs. yes, will open a new bug for that.
Comment 21•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6568f892a5c4 https://hg.mozilla.org/mozilla-central/rev/0c6a7835fef4 https://hg.mozilla.org/mozilla-central/rev/5e11375d63c7
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 22•7 years ago
|
||
Breaking the BBC player sounds bad. Please request Beta/ESR52 approval on this when you get a chance.
Assignee: nobody → jyavenard
status-firefox54:
--- → wontfix
status-firefox55:
--- → affected
status-firefox-esr52:
--- → affected
Flags: needinfo?(jyavenard)
Flags: in-testsuite+
Version: 54 Branch → 51 Branch
Comment 23•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8888483 [details] Bug 1382303 - P1. Add mochitest. https://reviewboard.mozilla.org/r/159440/#review164948 > not sure I understand your comment here. It's exactly why we call endOfStream > > The reason we call endOfStream is precisely to move to HAVE_ENOUGH_DATA , otherwise readyState is HAVE_FUTURE_DATA > > And we do want to play something > > What is there to fix? Forget it. I just misread the comments at #61-62.
Comment 24•7 years ago
|
||
Comment on attachment 8888483 [details] Bug 1382303 - P1. Add mochitest. Approval Request Comment [Feature/Bug causing the regression]: Bug 1304134 [User impact if declined]: Broken video playback on an unknown number of sites (the issue was originally reported to us by the BBC) [Is this code covered by automated tests?]: Yes, a new test was added. [Has the fix been verified in Nightly?]: No, but per jya, the BBC already worked around our bug with a change on their end, so verifying could be difficult at this point. [Needs manual test from QE? If yes, steps to reproduce]: No, the new test covers the spec compliance bug that was fixed here. [List of other uplifts needed for the feature/fix]: All 3 patches from this bug. [Is the change risky?]: Not risky. [Why is the change risky/not risky?]: Gets us spec compliant where we previously were not. New test added to cover this scenario. [String changes made/needed]: None.
Flags: needinfo?(jyavenard)
Attachment #8888483 -
Flags: approval-mozilla-esr52?
Attachment #8888483 -
Flags: approval-mozilla-beta?
Comment 25•7 years ago
|
||
Comment on attachment 8888483 [details] Bug 1382303 - P1. Add mochitest. mse fix, beta55+ (all three patches)
Attachment #8888483 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 26•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/759690a68aa0 https://hg.mozilla.org/releases/mozilla-beta/rev/f8e138d2f0ff https://hg.mozilla.org/releases/mozilla-beta/rev/b49e5e19d10f
Comment 27•7 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #24) > [Is this code covered by automated tests?]: Yes, a new test was added. > [Has the fix been verified in Nightly?]: No, but per jya, the BBC already > worked around our bug with a change on their end, so verifying could be > difficult at this point. > [Needs manual test from QE? If yes, steps to reproduce]: No, the new test > covers the spec compliance bug that was fixed here. Setting qe-verify- based on Ryan's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Comment 28•7 years ago
|
||
Comment on attachment 8888483 [details] Bug 1382303 - P1. Add mochitest. MSE fix. Uplift it to ESR52.3.
Attachment #8888483 -
Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Comment 29•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/688c9284fb12 https://hg.mozilla.org/releases/mozilla-esr52/rev/7f969ba7b6e8 https://hg.mozilla.org/releases/mozilla-esr52/rev/7e7b4f104462
Comment 31•7 years ago
|
||
backout |
Backed out from ESR52 at Julien's request for causing bug 1389844. https://hg.mozilla.org/releases/mozilla-esr52/rev/f01155fe4d54
You need to log in
before you can comment on or make changes to this bug.
Description
•