Closed
Bug 1451149
Opened 6 years ago
Closed 6 years ago
When using a MediaSource, media element shouldn't fire stalled events
Categories
(Core :: Audio/Video: Playback, enhancement, P3)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: jya, Assigned: jya)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
This was discussed on chromium side: https://bugs.chromium.org/p/chromium/issues/detail?id=517240 When using a media element with a Media Source, the resource fetching algorithm is to be called in "local" mode: https://www.w3.org/TR/media-source/#mediasource-attach "Continue the resource fetch algorithm by running the remaining "Otherwise (mode is local)" steps, with these clarifications" https://html.spec.whatwg.org/multipage/media.html#concept-media-load-resource Under the local mode, the steps that would cause the element to fire suspend, stalled or progress can never occur. As such, those events shouldn't be fired. As it appears that websites are making use of the progress event (like determining if the init segment has been parsed), we should make an exception for that event and amend the spec.
Updated•6 years ago
|
Rank: 25
Priority: -- → P3
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → jyavenard
Assignee | ||
Comment 2•6 years ago
|
||
:karlt in bug 1114885, you used the stalled and progress events as indicators to determine if a media element using MSE should be GCed or not. However, as per the bug description, the "stalled" and "progress" event should never be fired when using MSE. The plan as this stage is only remove the "stalled" event as "progress" is being used by sites like YouTube If we were to prevent the media element for handling such event, what would the impact be on what bug 114885 was fixing?
Flags: needinfo?(karlt)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 5•6 years ago
|
||
mozreview-review |
Comment on attachment 8975427 [details] Bug 1451149 - P1. Fix HTMLMediaElement style. https://reviewboard.mozilla.org/r/243718/#review249614 ::: dom/html/HTMLMediaElement.cpp:394 (Diff revision 1) > > /** > * This listener observes the first video frame to arrive with a non-empty size, > * and calls HTMLMediaElement::UpdateInitialMediaSize() with that size. > */ > -class HTMLMediaElement::StreamSizeListener : public DirectMediaStreamTrackListener { > +class HTMLMediaElement::StreamSizeListener : public DirectMediaStreamTrackListener I see this line get broken again by my clang-format for being > 80 chars. Could see if a reformat picks it up, not a big deal either way.
Attachment #8975427 -
Flags: review?(bvandyk) → review+
Assignee | ||
Comment 6•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8975427 [details] Bug 1451149 - P1. Fix HTMLMediaElement style. https://reviewboard.mozilla.org/r/243718/#review249614 > I see this line get broken again by my clang-format for being > 80 chars. Could see if a reformat picks it up, not a big deal either way. I only applied clang-format on the function definition and some code where it was obviously not formatted properly. not class definitions. I figured it would be better to minimize the changes while waiting for clang-format to be applied on everything at once (wonder when this will be done) But you're right, I'll apply more consistently everywhere... may as well do it now
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8975428 [details] Bug 1451149 - P2. Don't fire the "stalled" event when using MSE. https://reviewboard.mozilla.org/r/243720/#review250496
Attachment #8975428 -
Flags: review?(bvandyk) → review+
Comment on attachment 8975428 [details] Bug 1451149 - P2. Don't fire the "stalled" event when using MSE. LGTM, but I think :karlt should still look at this after hearing his thoughts on this.
Attachment #8975428 -
Flags: review?(karlt)
Comment 11•6 years ago
|
||
An brief summary of history: May 2016 The ME WG chose a resolution to https://www.w3.org/Bugs/Public/show_bug.cgi?id=28573 that made the stalled event more-or-less useless. August 2016 https://github.com/w3c/media-source/pull/143 incidentally but intentionally removed progress and stalled events, while still permitting implementations to queue them. https://github.com/w3c/media-source/issues/88 tracked progress and stalled events but was not closed (and is still open). April 2018 Chromium's intent to deprecate stalled events: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/x54XtrTyOP8 https://bugs.chromium.org/p/chromium/issues/detail?id=517240#c26 implies that HTML51 may still have some requirements for progress events even when mode is local.
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8975428 [details] Bug 1451149 - P2. Don't fire the "stalled" event when using MSE. https://reviewboard.mozilla.org/r/243720/#review250592 Thank you, Bryce. Jean-Yves was asking me to review the rationale behind the change, and confirm if the spec has been properly understood. I think that it is fine to remove the "stalled" event because it can no longer serve a useful purpose, the spec doesn't require the stalled event, and Chromium intends to remove. I'll skip reviewing the actual code change, as I'm not so familiar with the code now, and this already has review. I'm also not commenting on whether it is sensible to remove before Chrome, and I don't know whether this is significant enough for Intent to Unship m.d.p. post. ::: dom/html/HTMLMediaElement.cpp (Diff revision 2) > - if (mMediaSource) { > ChangeDelayLoadStatus(false); FYI, the WG also decided to change the delay load status in May 2016. https://github.com/w3c/media-source/issues/24#issuecomment-221993352
Attachment #8975428 -
Flags: review?(karlt)
Comment 13•6 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #2) > :karlt in bug 1114885, you used the stalled and progress events as > indicators to determine if a media element using MSE should be GCed or not. > > [...] > > If we were to prevent the media element for handling such event, what would > the impact be on what bug 114885 was fixing? The mProgressTimer test was because a "A pending 'stalled' event keeps the media element alive." If there is no stalled event remaining to be dispatched then there is no need to keep the media element alive long enough to dispatch the event.
Flags: needinfo?(karlt)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 16•6 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 56e5bdd606dec2c09751496562a1027511a04389 -d b0dc0094001b: rebasing 464477:56e5bdd606de "Bug 1451149 - P1. Fix HTMLMediaElement style. r=bryce" merging dom/html/HTMLMediaElement.cpp warning: conflicts while merging dom/html/HTMLMediaElement.cpp! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 19•6 years ago
|
||
Pushed by jyavenard@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b7c3ae209716 P1. Fix HTMLMediaElement style. r=bryce https://hg.mozilla.org/integration/autoland/rev/b3350fc6da3a P2. Don't fire the "stalled" event when using MSE. r=bryce
Comment 20•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b7c3ae209716 https://hg.mozilla.org/mozilla-central/rev/b3350fc6da3a
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in
before you can comment on or make changes to this bug.
Description
•