Right now, a decoder enter dormant mode only if it's paused and the tab isn't being displayed. We should put the decoder into dormant as soon as the element is paused. The dormant logic would need to be modified so that the current frame isn't being cleared (the element still must display the last frame) The dormant logic could be greatly simplified because we wouldn't need to monitor the visibility of the element and so on.

Text copied from bug 1311608: Chances are the user has moved focus away from the media element. We can tell MDSM to enter dormant to release memory or hardware resources. When the user wants to play again, MDSM will exit dormant and seek to the current position to resume playback without causing noticeable effects to the user provided that seek is fast enough. We will also add telemetry for tweaking pause timeout in next bugs to strive for best user experience.

Let's continue the discussion in this bug.

Comment on attachment 8806224 [details] Bug 1311872. Part 1 - remove dormant code from MediaDecoder and its friends. We will let MDSM solely decide when to enter/exit dormant. https://reviewboard.mozilla.org/r/89704/#review89142 \o/

Comment on attachment 8806225 [details] Bug 1311872. Part 2 - remove the dormant test that doesn't work anymore. We will write new dormant tests in next bugs. https://reviewboard.mozilla.org/r/89706/#review89144

Comment on attachment 8806226 [details] Bug 1311872. Part 3 - enter dormant when being paused for a while. https://reviewboard.mozilla.org/r/89708/#review89146 ::: dom/media/MediaDecoderStateMachine.cpp:568 (Diff revision 2) > { > public: > - explicit DecodingState(Master* aPtr) : StateObject(aPtr) {} > + explicit DecodingState(Master* aPtr) > + : StateObject(aPtr) > + , mDormantTimer(OwnerThread()) > + { } { } the { } is only to be used on single line declaration ::: dom/media/MediaDecoderStateMachine.cpp:732 (Diff revision 2) > } > > + void StartDormantTimer() > + { > + auto timeout = MediaPrefs::DormantOnPauseTimeout(); > + if (timeout == 0) { if (!timeout)

Comment on attachment 8806227 [details] Bug 1311872. Part 4 - exit dormant in response to user actions. https://reviewboard.mozilla.org/r/89710/#review89150 ::: dom/media/MediaDecoderStateMachine.cpp:425 (Diff revision 2) > > /** > * Purpose: release decoder resources to save memory and hardware resources. > * > * Transition to: > - * DECODING_FIRSTFRAME when being asked to exit dormant. > + * DECODING_FIRSTFRAME when play state changes to PLAYING. shouldn't this state be renamed? it not longer matches what it describes.

Comment on attachment 8806228 [details] Bug 1311872. Part 5 - disable dormant when running mochitests. https://reviewboard.mozilla.org/r/89712/#review89152 ::: testing/profiles/prefs_general.js:47 (Diff revision 2) > user_pref("media.preload.default", 2); // default = metadata > user_pref("media.preload.auto", 3); // auto = enough > user_pref("media.cache_size", 1000); > user_pref("media.volume_scale", "0.01"); > user_pref("media.test.dumpDebugInfo", true); > +user_pref("media.dormant-on-pause-timeout-ms", 0); // Disable dormant for it break some tests. s/break/breaks ::: testing/profiles/prefs_general.js:47 (Diff revision 2) > user_pref("media.preload.default", 2); // default = metadata > user_pref("media.preload.auto", 3); // auto = enough > user_pref("media.cache_size", 1000); > user_pref("media.volume_scale", "0.01"); > user_pref("media.test.dumpDebugInfo", true); > +user_pref("media.dormant-on-pause-timeout-ms", 0); // Disable dormant for it break some tests. I'm puzzled by this. This can only breaks some tests if we modify readyState while resuming from dormant. I believe anything dormant related should be transparent to JS. No events should be fired because we're resuming from dormant. Please open a following bug to ensure that dormant is transparent, so that we never have to disable dormant for the mochitest to pass. Can also do this in an additional patch, so this issue doesn't get forgotten. I'm fairly certain we're going to cause regression in the various DASH players out there if we fire events more than once when the players only expected once (such as canplay / canplaythrough etc)

Comment on attachment 8806226 [details] Bug 1311872. Part 3 - enter dormant when being paused for a while. https://reviewboard.mozilla.org/r/89708/#review89146 > if (!timeout) I can't find the guidelines about it. The c++ guidelines say using |!x| for bool or pointer. https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#JavaScript_practices JavaScript practices says |Compare objects to null, numbers to 0 or strings to "" if there is chance for confusion.|. I prefer |x == 0| over |!x| to avoid confusion when x is an integer.

Comment on attachment 8806227 [details] Bug 1311872. Part 4 - exit dormant in response to user actions. https://reviewboard.mozilla.org/r/89710/#review89150 > shouldn't this state be renamed? it not longer matches what it describes. Do you mean DECODING_FIRSTFRAME or DORMANT?

Comment on attachment 8806228 [details] Bug 1311872. Part 5 - disable dormant when running mochitests. https://reviewboard.mozilla.org/r/89712/#review89152 > I'm puzzled by this. > > This can only breaks some tests if we modify readyState while resuming from dormant. > > I believe anything dormant related should be transparent to JS. > No events should be fired because we're resuming from dormant. > > Please open a following bug to ensure that dormant is transparent, so that we never have to disable dormant for the mochitest to pass. > Can also do this in an additional patch, so this issue doesn't get forgotten. > > I'm fairly certain we're going to cause regression in the various DASH players out there if we fire events more than once when the players only expected once (such as canplay / canplaythrough etc) Bug 1314219 opened.

How much of an impact on playback start time does this have in Fennec on mid-range android devices?

Comment on attachment 8806224 [details] Bug 1311872. Part 1 - remove dormant code from MediaDecoder and its friends. We will let MDSM solely decide when to enter/exit dormant. https://reviewboard.mozilla.org/r/89704/#review89456 I wish I got more review requests like this!

Comment on attachment 8806225 [details] Bug 1311872. Part 2 - remove the dormant test that doesn't work anymore. We will write new dormant tests in next bugs. https://reviewboard.mozilla.org/r/89706/#review89458

Comment on attachment 8806226 [details] Bug 1311872. Part 3 - enter dormant when being paused for a while. https://reviewboard.mozilla.org/r/89708/#review89460 ::: dom/media/MediaDecoderStateMachine.cpp:732 (Diff revision 2) > } > > + void StartDormantTimer() > + { > + auto timeout = MediaPrefs::DormantOnPauseTimeout(); > + if (timeout == 0) { I prefer x==0 as well. But yes, the style guide says no.

Comment on attachment 8806227 [details] Bug 1311872. Part 4 - exit dormant in response to user actions. https://reviewboard.mozilla.org/r/89710/#review89464

Comment on attachment 8806228 [details] Bug 1311872. Part 5 - disable dormant when running mochitests. https://reviewboard.mozilla.org/r/89712/#review89466 ::: testing/profiles/prefs_general.js:47 (Diff revision 2) > user_pref("media.preload.default", 2); // default = metadata > user_pref("media.preload.auto", 3); // auto = enough > user_pref("media.cache_size", 1000); > user_pref("media.volume_scale", "0.01"); > user_pref("media.test.dumpDebugInfo", true); > +user_pref("media.dormant-on-pause-timeout-ms", 0); // Disable dormant for it break some tests. If this breaks some tests, that indicates it's not safe to expose to our release population. We should get the tests green before we enable this. Please make this feature preffed on only in Nightly until its effects are unobservable to JS.

Comment on attachment 8806226 [details] Bug 1311872. Part 3 - enter dormant when being paused for a while. https://reviewboard.mozilla.org/r/89708/#review89476 ::: dom/media/MediaDecoderStateMachine.cpp:732 (Diff revision 2) > } > > + void StartDormantTimer() > + { > + auto timeout = MediaPrefs::DormantOnPauseTimeout(); > + if (timeout == 0) { I find it is useful to enter dormant immediately without waiting for the timer when debugging how this feature regresses the mochitests. Therefore, I decide to slightly change the meaning of the timeout: |> 0|: schedule a timer |== 0|: call HandleDormant(true) without scheduling a timer |< 0|: never enter dormant.

Comment on attachment 8806226 [details] Bug 1311872. Part 3 - enter dormant when being paused for a while. https://reviewboard.mozilla.org/r/89708/#review89146 > I can't find the guidelines about it. The c++ guidelines say using |!x| for bool or pointer. > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#JavaScript_practices > JavaScript practices says |Compare objects to null, numbers to 0 or strings to "" if there is chance for confusion.|. > I prefer |x == 0| over |!x| to avoid confusion when x is an integer. you're right... When testing a pointer, use (!myPtr) or (myPtr); don't use myPtr != nullptr or myPtr == nullptr. Do not compare x == true or x == false. Use (x) or (!x) instead. x == true, in fact, is different from if (x)! so only for true/false test or testing nullptr. my bad

Pushed by jwwang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4a4d45357bc7 Part 1 - remove dormant code from MediaDecoder and its friends. We will let MDSM solely decide when to enter/exit dormant. r=cpearce,jya https://hg.mozilla.org/integration/autoland/rev/1ed409c6df2f Part 2 - remove the dormant test that doesn't work anymore. We will write new dormant tests in next bugs. r=cpearce,jya https://hg.mozilla.org/integration/autoland/rev/80ee79d7a32a Part 3 - enter dormant when being paused for a while. r=cpearce,jya https://hg.mozilla.org/integration/autoland/rev/159aabd71aaa Part 4 - exit dormant in response to user actions. r=cpearce,jya https://hg.mozilla.org/integration/autoland/rev/6efd141313d4 Part 5 - disable dormant when running mochitests. r=cpearce,jya

https://hg.mozilla.org/mozilla-central/rev/4a4d45357bc7 https://hg.mozilla.org/mozilla-central/rev/1ed409c6df2f https://hg.mozilla.org/mozilla-central/rev/80ee79d7a32a https://hg.mozilla.org/mozilla-central/rev/159aabd71aaa https://hg.mozilla.org/mozilla-central/rev/6efd141313d4