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

RESOLVED FIXED in Firefox 52

Status

()

Core
Audio/Video: Playback
P1
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: jya, Assigned: jwwang)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

(Reporter)

Description

a year ago
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.
(Reporter)

Updated

a year ago
Blocks: 1293937
(Assignee)

Comment 1

a year ago
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
(Assignee)

Updated

a year ago
Duplicate of this bug: 1311608
(Assignee)

Comment 3

a year ago
Let's continue the discussion in this bug.
(Assignee)

Updated

a year ago
Depends on: 1311594
(Assignee)

Updated

a year ago
Depends on: 1308147
(Assignee)

Updated

a year ago
Depends on: 1311924
Blocks: 1307864
(Assignee)

Updated

a year ago
Assignee: nobody → jwwang
Depends on: 1312337
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
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)
(Reporter)

Comment 12

a year ago
mozreview-review
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+
(Reporter)

Comment 13

a year ago
mozreview-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+
(Reporter)

Comment 14

a year ago
mozreview-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+
(Reporter)

Comment 15

a year ago
mozreview-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+
(Reporter)

Comment 16

a year ago
mozreview-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+
(Assignee)

Comment 17

a year ago
mozreview-review-reply
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.
(Assignee)

Comment 18

a year ago
mozreview-review-reply
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?
(Assignee)

Updated

a year ago
Blocks: 1314219
(Assignee)

Comment 19

a year ago
mozreview-review-reply
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.
(Assignee)

Updated

a year ago
Blocks: 1314222
How much of an impact on playback start time does this have in Fennec on mid-range android devices?

Comment 21

a year ago
mozreview-review
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 22

a year ago
mozreview-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 23

a year ago
mozreview-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 24

a year ago
mozreview-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 25

a year ago
mozreview-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+
(Assignee)

Comment 26

a year ago
mozreview-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.
(Assignee)

Updated

a year ago
Blocks: 1314524
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 32

a year ago
mozreview-review-reply
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 38

a year ago
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

Comment 39

a year ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Blocks: 1322169
(Assignee)

Updated

11 months ago
Duplicate of this bug: 1293937
You need to log in before you can comment on or make changes to this bug.