MediaSource with no data delays the load event indefinitely

RESOLVED FIXED in Firefox 36

Status

()

P2
critical
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jruderman, Assigned: karlt)

Tracking

(Blocks: 3 bugs, {assertion, testcase})

Trunk
mozilla37
x86_64
Mac OS X
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed)

Details

Attachments

(6 attachments)

(Reporter)

Description

5 years ago
Created attachment 8380292 [details]
testcase

With:
  user_pref("media.mediasource.enabled", true);

Three things go wrong here:
1) The throbber never stops spinning, even though everything has loaded
2) Shutdown assert:
     Assertion failure: rc != 0 (destroyed timer off its target thread!)
3) Shutdown hang (bug 931388)
(Reporter)

Comment 1

5 years ago
Created attachment 8380293 [details]
stack

Comment 2

5 years ago
Jesse,

Is this why media.mediasource.enabled;true breaks http://www.youtube.com/watch?v=B1wYQc29weU&html5=1 on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 ID:2 or shall I file a new bug?
Flags: needinfo?(jruderman)

Comment 3

5 years ago
(In reply to alex_mayorga from comment #2)
> 
> Is this why media.mediasource.enabled;true breaks
> http://www.youtube.com/watch?v=B1wYQc29weU&html5=1 on Mozilla/5.0 (Windows
> NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 ID:2 or shall I file a
> new bug?

If YouTube serves videos via Media Source Extensions with that pref enabled then things will definitely break. We don't implement enough for the YouTube player to work yet.

We have the case of a single stream playing in their demo player but that's it at the moment:

<http://cd.pn/mse/ytdemo/dash-player.html?url=http://cd.pn/mse/ytdemo/feelings2.mpd>

We do work with some other Dash player implementations though. For example:

<http://cd.pn/mse/webm/dash-player.html>
(Reporter)

Updated

5 years ago
Flags: needinfo?(jruderman)
Blocks: 778617
(Reporter)

Updated

4 years ago
Blocks: 1059045
This no longer asserts, but the throbber issue remains.
(Assignee)

Comment 5

4 years ago
I'm looking at the delay of the load event.
Comment 4 observes that the assertion is now gone, but
bug 1088481 may have replaced that.

Hope you don't mind me using this bug to address the load event issue.
Assignee: nobody → karlt
Summary: MediaSource: "Assertion failure: rc != 0 (destroyed timer off its target thread!)" → MediaSource with no data delays the load event indefinitely
(In reply to Karl Tomlinson (:karlt) from comment #5)
> bug 1088481 may have replaced that.
How is bug 1088481 related?
(Assignee)

Comment 7

4 years ago
(In reply to JW Wang [:jwwang] from comment #6)
> (In reply to Karl Tomlinson (:karlt) from comment #5)
> > bug 1088481 may have replaced that.
> How is bug 1088481 related?

It may not be.  This testcase reproduces, but I suspect there are two different bugs here.
(Assignee)

Comment 8

4 years ago
In Chromium, the load event seems to fire on the earlier of loadeddata or
stalled, which is consistent with the embedded content spec.  The stalled
event is delivered after 3 seconds.

In Firefox, stalled is not fired.  Before bug 1065827 landed, the load event
fired between loadedmetadata and loadeddata, all in quick succession.  Now it
seems to fire after the updateend that follows (for the appendBuffer() several
seconds after) loadeddata.

http://people.mozilla.org/~karlt/mse/mediasource_demo.html?file=mediasource_test.webm&chunks=50&eos_chunk=0&eos_delay=-1&delay_chunk=6000&start=-1
(Assignee)

Updated

4 years ago
Depends on: 1108838
(Assignee)

Comment 9

4 years ago
Created attachment 8533443 [details] [diff] [review]
part 2: stop delaying the load event when media fetch has stalled

(In reply to Karl Tomlinson (:karlt) from comment #8)
> In Chromium, the load event seems to fire on the earlier of loadeddata or
> stalled, which is consistent with the embedded content spec.  The stalled
> event is delivered after 3 seconds.

It's consistent but not clearly spelled out in the spec, which seems to mix up
discussion of stall and suspend, and I'm not sure how much of that is
intentional.

http://www.w3.org/TR/2014/REC-html5-20141028/embedded-content-0.html#concept-media-load-resource

"User agents may allow users to selectively block or slow media data
 downloads. When a media element's download has been blocked altogether, the
 user agent must act as if it was stalled (as opposed to acting as if the
 connection was closed).

 User agents may decide to not download more content at any time, e.g. after
 buffering five minutes of a one hour media resource, while waiting for the
 user to decide whether to play the resource or not, while waiting for user
 input in an interactive resource, or when the user navigates away from the
 page. When a media element's download has been suspended, the user agent must
 queue a task, to set the networkState to NETWORK_IDLE and fire a simple event
 named suspend at the element. If and when downloading of the resource
 resumes, the user agent must queue a task to set the networkState to
 NETWORK_LOADING. Between the queuing of these tasks, the load is suspended
 (so progress events don't fire, as described above)."

 When a user agent decides to completely stall a download, e.g. if it is
 waiting until the user starts playback before downloading any further
 content, the user agent must queue a task to set the element's
 delaying-the-load-event flag to false."

I think we need to do something to allow the load event to fire even when a
MediaSource is attached to a media element before the load event has been
dispatched.

This patch adopts the Blink behavior.

I guess we could instead make this a special case for MSE if we wanted to keep
the existing behavior for non-MSE media elements, but MSE do not specify many
differences in the resource fetch algorithm, so there is no specified special
case for MSE.

http://www.w3.org/TR/2014/CR-media-source-20140717/#mediasource-attach

"Text in the resource fetch algorithm that refers to "the download" or "bytes
 received" refer to data passed in via appendBuffer() and
 appendStream(). References to HTTP in the resource fetch algorithm do not
 apply because the HTMLMediaElement does not fetch media data via HTTP when a
 MediaSource is attached."

An alternative might be to take advantage of the option to suspend "at any
time".  We could pick that time to be any time that there is no (async)
appendBuffer in progress.  I don't think we can block the "download" from from
appendBuffer(), and so we'd switch back to NETWORK_LOADING automatically on
each appendBuffer.  I suspect though that switches to NETWORK_IDLE would imply
that no more bytes are wanted, which is not (in general) the right thing to do
here.

f?kinetik re the approach to stop delaying the load event when stalled, and
whether you have any other ideas.

In some ways stalled is like a network timeout, but it is not as final as
giving up on an HTTP request.
Attachment #8533443 - Flags: review?(cpearce)
Attachment #8533443 - Flags: feedback?(kinetik)
Comment on attachment 8533443 [details] [diff] [review]
part 2: stop delaying the load event when media fetch has stalled

Review of attachment 8533443 [details] [diff] [review]:
-----------------------------------------------------------------

I think this makes sense, including in the non-MSE case.
Attachment #8533443 - Flags: review?(cpearce) → review+
Attachment #8533443 - Flags: feedback?(kinetik) → feedback+
(Assignee)

Comment 12

4 years ago
Backed out for assertion failure in /tests/dom/imptests/editing/conformancetest/test_runtest.html

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4490301&repo=mozilla-inbound

ASSERTION: Load event not delayed during source selection?: 'mDelayingLoadEvent', file /builds/slave/m-in-l64-d-0000000000000000000/build/src/dom/html/HTMLMediaElement.cpp, line 695
#01: mozilla::dom::HTMLMediaElement::MediaLoadListener::OnStartRequest(nsIRequest*, nsISupports*) [dom/html/HTMLMediaElement.cpp:342]
#02: mozilla::net::nsHttpChannel::CallOnStartRequest() [netwerk/protocol/http/nsHttpChannel.cpp:941]
#03: mozilla::net::nsHttpChannel::ContinueProcessNormal(tag_nsresult) [netwerk/protocol/http/nsHttpChannel.cpp:1722]
#04: mozilla::net::nsHttpChannel::ProcessNormal() [netwerk/protocol/http/nsHttpChannel.cpp:1656]
#05: mozilla::net::nsHttpChannel::ProcessResponse() [netwerk/protocol/http/nsHttpChannel.cpp:1567]
#06: mozilla::net::nsHttpChannel::OnStartRequest(nsIRequest*, nsISupports*) [netwerk/protocol/http/nsHttpChannel.cpp:5276]
#07: nsInputStreamPump::OnStateStart() [netwerk/base/src/nsInputStreamPump.cpp:532]
#08: nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) [netwerk/base/src/nsInputStreamPump.cpp:433]
#09: nsInputStreamReadyEvent::Run() [xpcom/io/nsStreamUtils.cpp:90]
#10: nsThread::ProcessNextEvent(bool, bool*) [xpcom/threads/nsThread.cpp:830]
Keywords: leave-open
Need to reland
Flags: needinfo?(karlt)
Priority: -- → P2
(Assignee)

Comment 14

4 years ago
The assertion in comment 12 is easy to fix by modifying the assertion.

I also need to fix an assertion that sometimes occurs in dom/media/test/test_closing_connections.html after a load has stalled and then receives more data because ChangeDelayLoadStatus(true) is not moving existing loads out of background.

###!!! ASSERTION: Why are you calling this more than once?: '!mLoadInBackground', file /builds/slave/try-lx-d-000000000000000000000/build/src/dom/media/MediaResource.cpp, line 1581
#01: mozilla::BaseMediaResource::MoveLoadsToBackground() [dom/media/MediaResource.cpp:1581]
#02: mozilla::MediaDecoder::MoveLoadsToBackground() [dom/media/MediaDecoder.cpp:1401]
#03: mozilla::dom::HTMLMediaElement::ChangeDelayLoadStatus(bool) [dom/html/HTMLMediaElement.cpp:3745]
#04: mozilla::dom::HTMLMediaElement::FirstFrameLoaded() [dom/html/HTMLMediaElement.cpp:2939]
#05: mozilla::MediaDecoder::FirstFrameLoaded(nsAutoPtr<mozilla::MediaInfo>) [dom/media/MediaDecoder.cpp:752]
#06: mozilla::MetadataUpdatedEventRunner::Run() [dom/media/AbstractMediaDecoder.h:218]
#07: nsThread::ProcessNextEvent(bool, bool*) [xpcom/threads/nsThread.cpp:830]
#08: NS_ProcessNextEvent(nsIThread*, bool) [xpcom/glue/nsThreadUtils.cpp:265]
#09: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) [ipc/glue/MessagePump.cpp:100]
#10: MessageLoop::RunInternal() [ipc/chromium/src/base/message_loop.cc:233]
#11: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:508]
#12: nsBaseAppShell::Run() [widget/nsBaseAppShell.cpp:166]
#13: nsAppStartup::Run() [toolkit/components/startup/nsAppStartup.cpp:282]
#14: XREMain::XRE_mainRun() [toolkit/xre/nsAppRunner.cpp:4151]
#15: XREMain::XRE_main(int, char**, nsXREAppData const*) [toolkit/xre/nsAppRunner.cpp:4226]
#16: XRE_main [toolkit/xre/nsAppRunner.cpp:4447]
#17: do_main [browser/app/nsBrowserApp.cpp:292]
#18: main [browser/app/nsBrowserApp.cpp:663]
Flags: needinfo?(karlt)
(Assignee)

Comment 15

4 years ago
Created attachment 8540550 [details] [diff] [review]
part 1: bring media resource loads out of background while they delay the load event

This will happen after a stalled load doesn't delay the load event but such a
load then delays the load event again when it receives progress.
Attachment #8540550 - Flags: review?(cpearce)
(Assignee)

Updated

4 years ago
Attachment #8533443 - Attachment description: stop delaying the load event when media fetch has stalled → part 2: stop delaying the load event when media fetch has stalled
(Assignee)

Comment 16

4 years ago
Created attachment 8540551 [details] [diff] [review]
part 2 fold-in: allow not delaying the load event at MEDIA_ERR_SRC_NOT_SUPPORTED

This assertion was added in
https://hg.mozilla.org/integration/mozilla-inbound/rev/e79927f90d0b
which was mostly about AddRemoveSelfReference().
NETWORK_LOADING provides the self reference, and actually makes considering
mDelayingLoadEvent for the self reference unnecessary (bug 1114888).
Attachment #8540551 - Flags: review?(cpearce)
Attachment #8540550 - Flags: review?(cpearce) → review+
Attachment #8540551 - Flags: review?(cpearce) → review+
(Assignee)

Comment 19

4 years ago
Filed bug 1115835 on the remaining issue observed in comment 8.
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/edaf95c2be57
https://hg.mozilla.org/mozilla-central/rev/2393682aeb6a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
(Assignee)

Updated

4 years ago
Depends on: 1116676
Created attachment 8548725 [details] [diff] [review]
Backport Part 1 to 36 beta "bring media resource loads"

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent testing, video playback stalls with YouTube more likely.
[Describe test coverage new/current, TBPL]: Landed on m-c for some time. Fixes some web platform tests.
[Risks and why]: This affects non-MSE video playback, but has been stable on the 37 branch for some time. The main risk is a dependency on other changes we haven't uplifted.
[String/UUID change made/needed]: None.

This request applies to both patches in the bug. Part 1 needed manual rebasing.
Attachment #8548725 - Flags: approval-mozilla-beta?
status-firefox36: --- → affected
status-firefox37: --- → fixed
Attachment #8548725 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.