Closed Bug 1311872 Opened 8 years ago Closed 8 years ago

Aggressively put decoders into dormant mode as soon as element is paused

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: jya, Assigned: jwwang)

References

Details

Attachments

(5 files)

Bug 1311872. Part 1 - remove dormant code from MediaDecoder and its friends. We will let MDSM solely decide when to enter/exit dormant.
58 bytes, text/x-review-board-request
cpearce
: review+
jya
: review+
Details
Bug 1311872. Part 2 - remove the dormant test that doesn't work anymore. We will write new dormant tests in next bugs.
58 bytes, text/x-review-board-request
cpearce
: review+
jya
: review+
Details
Bug 1311872. Part 3 - enter dormant when being paused for a while.
58 bytes, text/x-review-board-request
cpearce
: review+
jya
: review+
Details
Bug 1311872. Part 4 - exit dormant in response to user actions.
58 bytes, text/x-review-board-request
cpearce
: review+
jya
: review+
Details
Bug 1311872. Part 5 - disable dormant when running mochitests.
58 bytes, text/x-review-board-request
cpearce
: review+
jya
: review+
Details 
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.
Blocks: 1293937
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.
Blocks: 1286129
Let's continue the discussion in this bug.
Depends on: 1311594
Depends on: 1308147
Depends on: 1311924
Blocks: 1307864
Assignee: nobody → jwwang
Depends on: 1312337
Attachment #8806224 - Flags: review?(jyavenard)
Attachment #8806224 - Flags: review?(cpearce)
Attachment #8806225 - Flags: review?(jyavenard)
Attachment #8806225 - Flags: review?(cpearce)
Attachment #8806226 - Flags: review?(jyavenard)
Attachment #8806226 - Flags: review?(cpearce)
Attachment #8806227 - Flags: review?(jyavenard)
Attachment #8806227 - Flags: review?(cpearce)
Attachment #8806228 - Flags: review?(jyavenard)
Attachment #8806228 - Flags: review?(cpearce)
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/
Attachment #8806224 - Flags: review?(jyavenard) → review+
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
Attachment #8806225 - Flags: review?(jyavenard) → review+
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)
Attachment #8806226 - Flags: review?(jyavenard) → review+
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.
Attachment #8806227 - Flags: review?(jyavenard) → review+
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)
Attachment #8806228 - Flags: review?(jyavenard) → review+
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?
Blocks: 1314219
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.
Blocks: 1314222
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!
Attachment #8806224 - Flags: review?(cpearce) → review+
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
Attachment #8806225 - Flags: review?(cpearce) → review+
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.
Attachment #8806226 - Flags: review?(cpearce) → review+
Comment on attachment 8806227 [details]
Bug 1311872. Part 4 - exit dormant in response to user actions.

https://reviewboard.mozilla.org/r/89710/#review89464
Attachment #8806227 - Flags: review?(cpearce) → review+
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.
Attachment #8806228 - Flags: review?(cpearce) → review+
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.
Blocks: 1314524
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: