975782 - MediaSource with no data delays the load event indefinitely

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P2)

Description

[Jesse Ruderman]

Attachment: 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)

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[alex_mayorga]

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?

Comment 3

[owpard]

(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>

Comment 4

[Matthew Gregan [:kinetik]]

This no longer asserts, but the throbber issue remains.

Comment 5

[Karl Tomlinson (:karlt)]

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.

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Karl Tomlinson (:karlt) from comment #5)
> bug 1088481 may have replaced that.
How is bug 1088481 related?

Comment 7

[Karl Tomlinson (:karlt)]

(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.

Comment 8

[Karl Tomlinson (:karlt)]

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

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: 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.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 11

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/78bde999fddd

Comment 12

[Karl Tomlinson (:karlt)]

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]

Comment 13

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Need to reland

Comment 14

[Karl Tomlinson (:karlt)]

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]

Comment 15

[Karl Tomlinson (:karlt)]

Attachment: 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.

Comment 16

[Karl Tomlinson (:karlt)]

Attachment: 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).

Comment 17

[Karl Tomlinson (:karlt)]

https://tbpl.mozilla.org/?tree=Try&rev=2354a6b66094
https://tbpl.mozilla.org/?tree=Try&rev=30323dc5e527

Comment 18

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/edaf95c2be57
https://hg.mozilla.org/integration/mozilla-inbound/rev/2393682aeb6a

Comment 19

[Karl Tomlinson (:karlt)]

Filed bug 1115835 on the remaining issue observed in comment 8.

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/edaf95c2be57
https://hg.mozilla.org/mozilla-central/rev/2393682aeb6a

Comment 21

[Ralph Giles (:rillian)]

Attachment: 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.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cdd335426a39
https://hg.mozilla.org/releases/mozilla-beta/rev/3abc61cb0abd