Closed Bug 1123535 Opened 10 years ago Closed 10 years ago

Make Mp4Reader dormant capable in desktop Firefox

Categories

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

Product:
Core
Component:
Audio/Video
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla38
Tracking Flags:
Tracking Status
firefox36 --- fixed
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(5 files, 4 obsolete files)

Patch 2: ReleaseMediaResources on Windows/MP4Reader
16.34 KB, patch
ajones
: review+
Sylvestre
: approval-mozilla-aurora+
Sylvestre
: approval-mozilla-beta+
Details | Diff | Splinter Review
Patch 1: Call Reader::ReleaseMediaResources on decode task queue
2.75 KB, patch
jwwang
: review+
Sylvestre
: approval-mozilla-aurora+
Details | Diff | Splinter Review
Patch 3: Turn on MP4Reader dormant on Windows.
1.91 KB, patch
ajones
: review+
Sylvestre
: approval-mozilla-aurora+
Details | Diff | Splinter Review
Patch 4: Don't hold decoder monitor while calling MDR::PrereadMetadata
2.75 KB, patch
ajones
: review+
Sylvestre
: approval-mozilla-aurora+
Details | Diff | Splinter Review
Patch 5: Make dormant ResetPlayback assertion more lenient (m-c patch)
1.09 KB, patch
jwwang
: review+
lmandel
: approval-mozilla-aurora+
Details | Diff | Splinter Review
We need to make MP4Readers in Desktop Firefox that aren't being used dormant, to reduce resource usage. This is pretty easy; we just need to shutdown the MediaDataDecoders and the PDM. We don't need to tear down the demuxer. We can re-create the PDM and decoders if the MP4Reader comes out of dormant when ReadMetadata() is called again. We'll need to change the logic in HTMLMediaElement to not be so aggressive in making <video> elements dormant; currently it makes the element dormant if it becomes non-visible. This is fine for B2G, but on desktop I think it makes sense to only make paused non-visible videos dormant after a timeout. 60s would be fine. We can do this in another bug.
Implement MP4Reader::ReleaseMediaResources(), to make the MP4Reader able to become dormant. This patch is based on top of Blake's patch in bug 1113527. MP4Reader goes dormant by simply shutting down and destroying the decoders and PDM. We do not shutdown the demuxer. When we want to leave dormant state, we will act as if we were loading metadata; we create a new PDM and decoders, which may still need to wait for resources. I've tested this patch on Windows, and it makes videos non-visible dormant, and re-activates them when the videos become visible again. Blake: Can you please test this patch on MP4Reader on B2G? I don't have a B2G device. Sotaro: Any feedback here is welcome.
Assignee: nobody → cpearce
Status: NEW → ASSIGNED
Attachment #8551591 - Flags: feedback?(sotaro.ikeda.g)
Attachment #8551591 - Flags: feedback?(bwu)
Comment on attachment 8551591 [details] [diff] [review] WIP Patch: Make MP4Reader dormant capable Review of attachment 8551591 [details] [diff] [review]: ----------------------------------------------------------------- I have tested it on my Flame. This patch can only work on non-MSE case (local playback) and on MSE it cannot work well. The difference is for MSE on B2G SharedDecoderManager plays a proxy role between MP4Reader and video decoder. ::: dom/media/fmp4/MP4Reader.cpp @@ +1020,3 @@ > if (mVideo.mDecoder) { > + Flush(kVideo); > + mVideo.mDecoder->Shutdown(); This can only work for non-MSE case(local playback). For MSE case, ShutDown will finally go to SharedDecoderManager's setIdle() @[1] which will not really release resource. mVideo.mDecoder->ReleaseMediaResources() should be the correct function to call. [1]https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/SharedDecoderManager.cpp#173
Attachment #8551591 - Flags: feedback?(bwu)
Comment on attachment 8551591 [details] [diff] [review] WIP Patch: Make MP4Reader dormant capable Review of attachment 8551591 [details] [diff] [review]: ----------------------------------------------------------------- I saw the following diagram. https://github.com/sotaroikeda/firefox-diagrams/blob/master/media/dom_media_MediaSource_FirefoxOS_2_2.pdf?raw=true From the diagram, When MSE is used, MediaSourceReader is instantiation of MediaDecoderReader from playback point of view. The control flow from MediaSourceReader is like the following and multiple MP4Readers could exist. - MediaSourceReader => TrackBuffer => SourceBufferDecoder => MP4Reader. If we want to enable dormant on MES, such control like for MES seems to becomes necessary. And it might be better to limit dormant only to widows and b2g.
Attachment #8551591 - Flags: feedback?(sotaro.ikeda.g)
Priority: -- → P1
Attached patch WIP Patch #2 (obsolete) — — Splinter Review
OK, another attempt. Make MP4Reader's B2G path also work on Windows. So the WMF MediaDataDecoder implements AllocateMediaResources and ReleaseMediaResources. The SharedDecoderManager also now calls these. Dormancy works with MSE on YouTube with this patch, as well as non-MSE video. I still think we should refactor the dormant code to shutdown the decoder instead of asking it to release/reallocate resources, but I think that will be easiest when we've refactored MSE to have multiple demuxers and one decoder instead of multiple Readers, each of which have (a proxy to) a decoder.
Attachment #8551591 - Attachment is obsolete: true
Patch 1: Split the dispatch to call Reader::ReleaseMediaResources() on the decode task queue out of bwu's patch in bug 1113527. I need this for my subsequent patch.
Attachment #8552990 - Attachment is obsolete: true
* Use SharedDecoderManager on Windows to share a single decoder in MSReader using MP4Readers. * Implement WMF MediaDataDecoder ReleaseMediaResources, and pipe that through the SharedDecoderProxy. * SharedDecoderManager uses the MP4Reader's PDM to create the MediaDataDecoder wrapped by SharedDecoderProxy. This is so that if we're decoding EME content, the PDM CDM wrapper is wrapped by the SharedDecoderProxy. This will work fine provided we don't happen to create the wrapped decoder first on a non-encrypted video, and try to subsequently use the decoder on an encrypted video. I haven't figured out how to handle that yet. This works for YouTube/MSE and non-MSE MP4 videos on Windows.
* Based on Sotaro's dormant patch in bug 1123452. * Make MP4Reader use the SharedDecoderManager on Windows, not just B2G. * Use the same "ReleaseMediaResources" approach as B2G. We can clean that up and shutdown the decoders properly once we've refactored the MSReader to have multiple demuxers and an explicit single decoder, rather than multiple sub-readers.
Attachment #8553514 - Attachment is obsolete: true
Attachment #8554396 - Flags: review?(ajones)
Run Reader::ReleaseMediaResources on decode task queue, rather than the state machine thread. I also noticed that we don't need to call AwaitIdle() before dispatching the ReleaseMediaResources() task from RunStateMachine either; the FlushDecoding() we call earlier in that case will do an AwaitIdle() anyway.
Attachment #8553510 - Attachment is obsolete: true
Attachment #8554398 - Flags: review?(jwwang)
Pref's match Sotaro's from bug 1123452.
Attachment #8554400 - Flags: review?(ajones)
Comment on attachment 8554398 [details] [diff] [review] Patch 1: Call Reader::ReleaseMediaResources on decode task queue Review of attachment 8554398 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaDecoderStateMachine.cpp @@ +1966,5 @@ > > SAMPLE_LOG("EnsureAudioDecodeTaskQueued isDecoding=%d status=%d", > IsAudioDecoding(), mAudioRequestStatus); > > + if (mState >= DECODER_STATE_COMPLETED || mState == DECODER_STATE_DORMANT) { I wonder if we should move this check to NeedToDecode{Audio,Video} to centralize the logic of checking whether to decode next audio/video samples.
Attachment #8554398 - Flags: review?(jwwang) → review+
(In reply to Chris Pearce (:cpearce) from comment #10) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7f50d403202 The failures in this push are caused by calling MP4Reader::PreReadMetadata() with the decoder monitor held. Stack: 02:38:28 INFO - 10 xul.dll!NS_DispatchToMainThread(nsIRunnable *,unsigned int) [nsThreadUtils.cpp:e7f50d403202 : 177 + 0x41] 02:38:28 INFO - rip = 0x000007fe26d989d7 rsp = 0x00000090db1bf000 02:38:28 INFO - Found by: call frame info 02:38:28 INFO - 11 xul.dll!mozilla::WMFVideoMFTManager::InitializeDXVA() [WMFVideoMFTManager.cpp:e7f50d403202 : 163 + 0xc] 02:38:28 INFO - rip = 0x000007fe28b1bff7 rsp = 0x00000090db1bf040 02:38:28 INFO - Found by: call frame info 02:38:28 INFO - 12 xul.dll!mozilla::WMFVideoMFTManager::Init() [WMFVideoMFTManager.cpp:e7f50d403202 : 172 + 0x4] 02:38:28 INFO - rip = 0x000007fe28b1baa3 rsp = 0x00000090db1bf080 02:38:28 INFO - Found by: call frame info 02:38:28 INFO - 13 xul.dll!mozilla::WMFMediaDataDecoder::AllocateMediaResources() [WMFMediaDataDecoder.cpp:e7f50d403202 : 158 + 0x37] 02:38:28 INFO - rip = 0x000007fe28b1a03a rsp = 0x00000090db1bf140 02:38:28 INFO - Found by: call frame info 02:38:28 INFO - 14 xul.dll!mozilla::MP4Reader::RequestCodecResource() [MP4Reader.cpp:e7f50d403202 : 260 + 0xc] 02:38:28 INFO - rip = 0x000007fe28b13298 rsp = 0x00000090db1bf180 02:38:28 INFO - Found by: call frame info 02:38:28 INFO - 15 xul.dll!mozilla::MP4Reader::PreReadMetadata() [MP4Reader.cpp:e7f50d403202 : 338 + 0x4] 02:38:28 INFO - rip = 0x000007fe28b1218b rsp = 0x00000090db1bf1c0 02:38:28 INFO - Found by: call frame info 02:38:28 INFO - 16 xul.dll!mozilla::MediaDecoderStateMachine::DecodeMetadata() [MediaDecoderStateMachine.cpp:e7f50d403202 : 2182 + 0x2d] 02:38:28 INFO - rip = 0x000007fe289c58e0 rsp = 0x00000090db1bf1f0 02:38:28 INFO - Found by: call frame info 02:38:28 INFO - 17 xul.dll!mozilla::MediaDecoderStateMachine::CallDecodeMetadata() [MediaDecoderStateMachine.cpp:e7f50d403202 : 2169 + 0x7] 02:38:28 INFO - rip = 0x000007fe289bfeaa rsp = 0x00000090db1bf340 02:38:28 INFO - Found by: call frame info 02:38:28 INFO - 18 xul.dll!nsRunnableMethodImpl<void ( mozilla::MediaDecoderStateMachine::*)(void),void,1>::Run() [nsThreadUtils.h:e7f50d403202 : 386 + 0x6] 02:38:28 INFO - rip = 0x000007fe289f7d2f rsp = 0x00000090db1bf3c0 The WMFMediaDataDecoder::AllocateMediaResources() path calls InitializeDXVA which dispatches synchronously to the main thread to create the DXVA context. The main thred is blocked waiting for the monitor. The fix is simple; don't call MediaDecoderReader::PreReadMetadata() with the monitor held. (Ideally, we should not need a PreReadMetadata() function, but we can clean that up later.)
Don't hold decoder monitor while calling MDR::PrereadMetadata.
Attachment #8554773 - Flags: review?(ajones)
Comment on attachment 8554396 [details] [diff] [review] Patch 2: ReleaseMediaResources on Windows/MP4Reader Review of attachment 8554396 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/fmp4/MP4Reader.cpp @@ +1005,5 @@ > > bool MP4Reader::IsDormantNeeded() > { > +#if defined(MOZ_GONK_MEDIACODEC) || defined(XP_WIN) > + return nit: Dreaded whitespace. ::: dom/media/fmp4/SharedDecoderManager.cpp @@ +60,4 @@ > , mActiveCallback(nullptr) > , mWaitForInternalDrain(false) > , mMonitor("SharedDecoderProxy") > + , mDecoderReleasedResources(false) Comment: It is a bit yuck that we have to store this state here. All it does is reduce the number of times we call AllocateMediaResources(). Making it fast and calling it all the time would make the code less stateful. ::: dom/media/fmp4/wmf/WMFMediaDataDecoder.h @@ +94,5 @@ > + ReleaseMftManager, > + RetainMftManager > + }; > + > + void ProcessShutdown(ReleaseMode aMode); Commnent: It might be cleaner to just have two functions. ProcessTemporaryShutdown() and ProcessShutdown() rather than passing a flag to ProcessShutdown().
Attachment #8554396 - Flags: review?(ajones) → review+
Comment on attachment 8554400 [details] [diff] [review] Patch 3: Turn on MP4Reader dormant on Windows. Review of attachment 8554400 [details] [diff] [review]: ----------------------------------------------------------------- ::: modules/libpref/init/all.js @@ +263,5 @@ > pref("media.play-stand-alone", true); > > +#if defined(XP_WIN) > +pref("media.decoder.heuristic.dormant.enabled", true); > +pref("media.decoder.heuristic.dormant.timeout", 60000); nit: whitespace ::: testing/profiles/prefs_general.js @@ +284,5 @@ > > user_pref("media.eme.enabled", true); > > +#if defined(XP_WIN) > +user_pref("media.decoder.heuristic.dormant.timeout", 0); nit: whitespace
Attachment #8554400 - Flags: review?(ajones) → review+
Attachment #8554773 - Flags: review?(ajones) → review+
(In reply to JW Wang [:jwwang] from comment #11) > Comment on attachment 8554398 [details] [diff] [review] > Patch 1: Call Reader::ReleaseMediaResources on decode task queue > > Review of attachment 8554398 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/media/MediaDecoderStateMachine.cpp > @@ +1966,5 @@ > > > > SAMPLE_LOG("EnsureAudioDecodeTaskQueued isDecoding=%d status=%d", > > IsAudioDecoding(), mAudioRequestStatus); > > > > + if (mState >= DECODER_STATE_COMPLETED || mState == DECODER_STATE_DORMANT) { > > I wonder if we should move this check to NeedToDecode{Audio,Video} to > centralize the logic of checking whether to decode next audio/video samples. I too like centralizing logic, but I don't think the above would work, as the NeedToDecodeVideo() etc are called and then the DecodeVideo() task is run, so the state could change in between NeedToDecodeVideo() being called and the DecodeVideo() task being run, and so we would not catch this case.
Blocks: 1113527
sorry had to back this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=043a03d002f2 seems one of this changes caused a Crashtest bustage on Windows Tests like: https://treeherder.mozilla.org/logviewer.html#?job_id=5955531&repo=mozilla-inbound
Flags: needinfo?(cpearce)
Blocks: 1107348
Bug 1125472 by itself was green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a4fad808ffa I've landed that. Patch 1 by itself was green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a4fad808ffa Patch 4 caused the Linux test failure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdb7af25bbc7 Enabling dormant for MP4Reader caused the WPT-4 failure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d43d549352e5 (Clearing ni?rillian until this is green and landed.)
Flags: needinfo?(giles)
Depends on: 1127414
Comment on attachment 8554396 [details] [diff] [review] Patch 2: ReleaseMediaResources on Windows/MP4Reader I'm requesting a blanket approval for all patches in this bug. Approval Request Comment [Feature/regressing bug #]: Media Source [User impact if declined]: Out Of Memory crashes. This patch shuts down video decoders that are paused and not visible for 60 seconds, releasing GPU and CPU memory. This should reduce memory consumption of idle video in background tabs, reducing chance of out of memory crashes on YouTube, which have spiked in the last week or so. [Describe test coverage new/current, TreeHerder]: This has been in m-c for a day and tested locally. [Risks and why]: There's a chance this could break video decoding on Windows completely. Most of the platform agnostic infrastructure has shipped in B2G already, this is just the Windows specific stuff, so I'm pretty confident it will be OK. We need this for reducing out of memory crashes, so I think the risk is worth it. [String/UUID change made/needed]: None.
Attachment #8554396 - Flags: approval-mozilla-beta?
Attachment #8554396 - Flags: approval-mozilla-aurora?
Comment on attachment 8554396 [details] [diff] [review] Patch 2: ReleaseMediaResources on Windows/MP4Reader I am concerned about the potential risk for beta. Anyway, the next good to build is next Monday. Until then, I am going to take in aurora to get more testing.
Attachment #8554396 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8554398 - Flags: approval-mozilla-aurora+
Attachment #8554400 - Flags: approval-mozilla-aurora+
Attachment #8554773 - Flags: approval-mozilla-aurora+
Comment on attachment 8554396 [details] [diff] [review] Patch 2: ReleaseMediaResources on Windows/MP4Reader Taking it for beta 6. Go to build tomorrow.
Attachment #8554396 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This is failing mochitest on beta with: Assertion failure: mState == DECODER_STATE_SEEKING || mState == DECODER_STATE_SHUTDOWN || mState == DECODER_STATE_DORMANT, at c:\builds\moz2_slave\m-beta-w32-d-00000000000000000\build\dom\media\MediaDecoderStateMachine.cpp:1631 I assume this means I forgot to uplift something. Any ideas, Chris?
Flags: needinfo?(cpearce)
Maybe bug 1124844. That's the only thing I can find that's not been uplifted in the bugs blocking Bug 1050031, the dormant tracking bug.
Flags: needinfo?(cpearce)
Pushed the patch from bug 1124844 on top of beta to try to see if that resolves the assertion. I don't see it with aurora though, so this may just re-arranging the timing. https://treeherder.mozilla.org/#/jobs?repo=try&revision=534a3717e242
Backed out for the test_reactivate.html debug asserts. The patch from bug 1124844 didn't help. https://hg.mozilla.org/releases/mozilla-beta/rev/bbc98a8c8142
status-firefox36: fixed → affected
I get a timeout with that test on a beta on my Mac (with the pref flipped) but haven't been able to reproduce the asserts.
I think the assert here is benign, and we can just fix the assert to handle this call path. I think what's happening here is we're running the state machine for DORMANT state, and inside the call to FlushDecoding() while we drop the monitor to flush the task queue the main thread moves the decoder out of dormant state into DECODER_STATE_DECODING_NONE (in MDSM::SetDormant()), and so when FlushDecoding re-takes the monitor and calls ResetPlayback(), the assertion on MDSM::mState fails. We can just change this assertion to include DECODER_STATE_DECODING_NONE state; we should complete the going dormant before we resume from being dormant. I think this is a timing issue. We should make the same fix on Aurora and Beta too.
This patch applies to mozilla-central. We'll need it on Aurora too. My diagnosis of the problem: (In reply to Chris Pearce (:cpearce) from comment #36) > I think the assert here is benign, and we can just fix the assert to handle > this call path. > > I think what's happening here is we're running the state machine for DORMANT > state, and inside the call to FlushDecoding() while we drop the monitor to > flush the task queue the main thread moves the decoder out of dormant state > into DECODER_STATE_DECODING_NONE (in MDSM::SetDormant()), and so when > FlushDecoding re-takes the monitor and calls ResetPlayback(), the assertion > on MDSM::mState fails. > > We can just change this assertion to include DECODER_STATE_DECODING_NONE > state; we should complete the going dormant before we resume from being > dormant. Ideally once the dust settles, we can change the state machine to only change its state on a single task queue, so we don't have these out-of-order issues.
Attachment #8558256 - Flags: review?(jwwang)
Comment on attachment 8558256 [details] [diff] [review] Patch 5: Make dormant ResetPlayback assertion more lenient (m-c patch) Review of attachment 8558256 [details] [diff] [review]: ----------------------------------------------------------------- This is fine but I wonder if the state could even change again before we enter the monitor in some extreme case. We should add a comment for as code changes it is getting harder to understand why we assert some states in a function. And I like the idea to only allow state transition in-between state machine runs not in the middle of it.
Attachment #8558256 - Flags: review?(jwwang) → review+
(In reply to JW Wang [:jwwang] from comment #39) > Comment on attachment 8558256 [details] [diff] [review] > Patch 5: Make dormant ResetPlayback assertion more lenient (m-c patch) Landed, with comment added: https://hg.mozilla.org/integration/mozilla-inbound/rev/347b91bccf4d Will need to uplift this to aurora 37, but it's already on beta 36. ni?me so I don't forget.
Flags: needinfo?(cpearce)
Comment on attachment 8558256 [details] [diff] [review] Patch 5: Make dormant ResetPlayback assertion more lenient (m-c patch) Request uplift of this follow-up patch. This fixed test failures on beta; we should have it in 37 as well in case further changes trigger the same problem. Approval Request Comment [Feature/regressing bug #]: MSE [User impact if declined]: Possible asserts (crashes) on debug builds. [Describe test coverage new/current, TreeHerder]: Landed on m-c, beta. [Risks and why]: Risk is low. At worst we'd crash somewhere less useful for debugging. [String/UUID change made/needed]: None
Attachment #8558256 - Flags: approval-mozilla-aurora?
Comment on attachment 8558256 [details] [diff] [review] Patch 5: Make dormant ResetPlayback assertion more lenient (m-c patch) Aurora+ for the follow-up.
Attachment #8558256 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1128357
Depends on: 1130311
Depends on: 1133167
No longer depends on: 1133167
Depends on: 1135304
No longer depends on: 1135304
Depends on: 1138238
Depends on: 1141816
Depends on: 1142527
No longer depends on: 1141816
No longer depends on: 1142527
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: