When using a MediaSource, media element shouldn't fire stalled events

RESOLVED FIXED in Firefox 62

Status

()

P3
normal
Rank:
25
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks: 1 bug)

unspecified
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

a year ago
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

11 months ago
Rank: 25
Priority: -- → P3
(Assignee)

Updated

10 months ago
Duplicate of this bug: 1461028
(Assignee)

Updated

10 months ago
Assignee: nobody → jyavenard
(Assignee)

Comment 2

10 months 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 5

10 months 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

10 months 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

10 months 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)
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

10 months 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)
(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

10 months 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

10 months 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

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b7c3ae209716
https://hg.mozilla.org/mozilla-central/rev/b3350fc6da3a
Status: NEW → RESOLVED
Last Resolved: 10 months 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.