695 bytes, text/html
2.33 KB, text/plain
7.19 KB, patch
|Details | Diff | Splinter Review|
8.82 KB, patch
|Details | Diff | Splinter Review|
1.20 KB, patch
|Details | Diff | Splinter Review|
6.68 KB, patch
|Details | Diff | Splinter Review|
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)
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?
(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>
This no longer asserts, but the throbber issue remains.
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?
(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.
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
(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 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+
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]
Need to reland
Priority: -- → P2
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]
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)
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
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+
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
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?
Attachment #8548725 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.