If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Try to enter dormant state when document is hidden

RESOLVED FIXED in Firefox 36

Status

()

Core
Audio/Video
P1
normal
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: sotaro, Assigned: sotaro)

Tracking

Trunk
mozilla38
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed, firefox38 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

3 years ago
When a document is not shown to user and MediaDecoder is not doing playback, MediaDecoderStateMachine could enter dormant state without affecting to user. But it could slow down resuming playback.

The document state could be checked by nsIDocument::Hidden().
We only need to do this for the MP4Reader on Windows first, other platforms/Readers can be done later.

I think we should set a timer when a <video> element becomes hidden and is paused, and only make the decoder dormant when the timer expires.
(Assignee)

Updated

3 years ago
Depends on: 1122228
(Assignee)

Updated

3 years ago
Assignee: nobody → sotaro.ikeda.g

Updated

3 years ago
Priority: -- → P1
Is it different from what we do in

HTMLMediaElement::NotifyOwnerDocumentActivityChanged() {
  mDecoder->SetDormantIfNecessary(ownerDoc->Hidden());
}

?
(Assignee)

Comment 3

3 years ago
(In reply to JW Wang [:jwwang] from comment #2)
> Is it different from what we do in
> 
> HTMLMediaElement::NotifyOwnerDocumentActivityChanged() {
>   mDecoder->SetDormantIfNecessary(ownerDoc->Hidden());
> }
> 
> ?

On b2g, it is expected to become dormant if element is video element. But in other cases, they are not expected to enter dormant. This bug is for to enter dormant state, in the other cases if some timer expires and resume from dormant automatically if it is necessary even when the document is in hidden state.
(Assignee)

Comment 4

3 years ago
This bug should not degrade the Firefox OS's system sound's performance.
(Assignee)

Comment 5

3 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #4)
> This bug should not degrade the Firefox OS's system sound's performance.

From Bug 1122228 Comment 11, it seems better to limit only to video in this bug.
(Assignee)

Comment 6

3 years ago
During checking the local implementation, I faced another problem.

When MediaDecoderStateMachineScheduler::Schedule(0) is called, callback is not always called. When argument of the function is 0, MediaDecoderStateMachineScheduler::TimeoutExpired() is called before, incrementing mTimerId in the Schedule().

cpearce, is it a already recognized problem?
Flags: needinfo?(cpearce)
(Assignee)

Comment 7

3 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #6)
> During checking the local implementation, I faced another problem.
> 
> When MediaDecoderStateMachineScheduler::Schedule(0) is called, callback is
> not always called. When argument of the function is 0,
> MediaDecoderStateMachineScheduler::TimeoutExpired() is called before,
> incrementing mTimerId in the Schedule().

It could be temporarily fixed locally. It seems better to handle this problem as a different bug.
(Assignee)

Comment 8

3 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #7)
> (In reply to Sotaro Ikeda [:sotaro] from comment #6)
> > During checking the local implementation, I faced another problem.
> > 
> > When MediaDecoderStateMachineScheduler::Schedule(0) is called, callback is
> > not always called. When argument of the function is 0,
> > MediaDecoderStateMachineScheduler::TimeoutExpired() is called before,
> > incrementing mTimerId in the Schedule().
> 
> It could be temporarily fixed locally. It seems better to handle this
> problem as a different bug.

Sorry, this was just my local implementation's problem.
Flags: needinfo?(cpearce)
(Assignee)

Comment 9

3 years ago
Created attachment 8552895 [details] [diff] [review]
Bug 1123452 - Try to enter dormant state when document is hidden
(Assignee)

Updated

3 years ago
Attachment #8552895 - Flags: feedback?(cpearce)
Comment on attachment 8552895 [details] [diff] [review]
Bug 1123452 - Try to enter dormant state when document is hidden

Review of attachment 8552895 [details] [diff] [review]:
-----------------------------------------------------------------

It works!

I think it would be good to make DORMANT_TIMEOUT_MSECS a pref. And have a pref to disable dormant when hidden after timeout; we can set the pref true on Windows, and false on other platforms until we get their dormant code implemented.
Attachment #8552895 - Flags: feedback?(cpearce) → feedback+
(Assignee)

Comment 11

3 years ago
Created attachment 8553180 [details] [diff] [review]
patch - Try to enter dormant state when document is hidden

heuristic dormant is disabled by default.
Attachment #8552895 - Attachment is obsolete: true
(Assignee)

Comment 12

3 years ago
Created attachment 8553187 [details] [diff] [review]
patch - Try to enter dormant state when document is hidden

update nits.
Attachment #8553180 - Attachment is obsolete: true
(Assignee)

Comment 13

3 years ago
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=100e76704a1d
 https://tbpl.mozilla.org/?tree=Try&rev=100e76704a1d
(Assignee)

Updated

3 years ago
Depends on: 1124844
(Assignee)

Updated

3 years ago
No longer depends on: 1124844
(Assignee)

Updated

3 years ago
Attachment #8553187 - Flags: review?(cpearce)
Comment on attachment 8553187 [details] [diff] [review]
patch - Try to enter dormant state when document is hidden

Review of attachment 8553187 [details] [diff] [review]:
-----------------------------------------------------------------

If I apply this patch, and patches from bug 1113527 and bug 1123535, I discovered a bug; (on Windows) set media.decoder.heuristic.dormant.enabled=true and media.decoder.heuristic.dormant.timeout=0, then I load a YouTube video, pause, seek, then quickly change tab to make YouTube hidden. Focus the YouTube tab again. The YouTube video has seeked correctly, but cannot be played at the new position. We'll need that fixed. Note: if I don't do the seek, dormant works perfectly.
Attachment #8553187 - Flags: review?(cpearce) → review-
We also transition through dormant state when shutting down due to browser window close, which is not ideal, though it should not cause problems.
(Assignee)

Comment 16

3 years ago
cpearce, thanks for the review. I just set up windows develop environment. I will debug the problem tomorrow(Fri).

I have a question to a patch. Which patch is used for Bug 1113527? I think latest one seem not correct patch.
Flags: needinfo?(cpearce)
(In reply to Sotaro Ikeda [:sotaro] from comment #16)
> cpearce, thanks for the review. I just set up windows develop environment. I
> will debug the problem tomorrow(Fri).
> 
> I have a question to a patch. Which patch is used for Bug 1113527? I think
> latest one seem not correct patch.

My patch in bug 1123535 is based on bwu's v3 patch in bug 1113527.
Flags: needinfo?(cpearce)
(In reply to Chris Pearce (:cpearce) from comment #17)
> (In reply to Sotaro Ikeda [:sotaro] from comment #16)
> > cpearce, thanks for the review. I just set up windows develop environment. I
> > will debug the problem tomorrow(Fri).
> > 
> > I have a question to a patch. Which patch is used for Bug 1113527? I think
> > latest one seem not correct patch.
> 
> My patch in bug 1123535 is based on bwu's v3 patch in bug 1113527.

I have refactored my work in bug 1123535 to have the bits I need from bwu's patch in a separate patch in bug 1123535.
(Assignee)

Comment 19

3 years ago
I could reproduce the problem. I am investigating about it.
(Assignee)

Comment 20

3 years ago
The problem seems to exist to seek. Since recovery from dormant state, MediaDecoder try seek, but the seek does not end.

-[1] MediaDecoder: Enter dormant
-[2] MediaDecoder: Exit dormant
-[3] MediaDecoder: MetadataLoaded()
-[4] MediaDecoder: FirstFrameLoaded()
-[5] MediaDecoder: start seek to previous position.
                       But seek does not complete.
(Assignee)

Comment 21

3 years ago
When the problem happens, mVideoRequestStatus is not cleared during entering dormant.
(Assignee)

Updated

3 years ago
Depends on: 1125472
(Assignee)

Comment 22

3 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #21)
> When the problem happens, mVideoRequestStatus is not cleared during entering
> dormant.

Bug 1125472 is created for the above problem.
(Assignee)

Comment 23

3 years ago
Comment on attachment 8553187 [details] [diff] [review]
patch - Try to enter dormant state when document is hidden

Can you review again with Bug 1125472 patch?
Attachment #8553187 - Flags: review?(cpearce)
Comment on attachment 8553187 [details] [diff] [review]
patch - Try to enter dormant state when document is hidden

Review of attachment 8553187 [details] [diff] [review]:
-----------------------------------------------------------------

r=cpearce, assuming we get bug 1125472 fixed.

::: dom/media/MediaDecoder.cpp
@@ +158,4 @@
>      mIsDormant = true;
>    }
>  #endif
> +  // Try to enable dormant by heuristic, when the owner is hidden.

s/dormant by heuristic/dormant by idle heuristic/.

The heuristic is "if the decoder is idle for 'a while' and not visible, make it dormant". We should be mentioning that the decoder should be "idle" in the comment here.

::: dom/media/MediaDecoder.h
@@ +1233,5 @@
> +
> +  // True if heuristic dormant is supported.
> +  const bool mIsHeuristicDormantSupported;
> +
> +  // Timeout ms of heurisctic dormant timer.

s/heurisctic/heuristic/
Attachment #8553187 - Flags: review?(cpearce)
Attachment #8553187 - Flags: review-
Attachment #8553187 - Flags: review+
(Assignee)

Comment 25

3 years ago
Created attachment 8554607 [details] [diff] [review]
patch - Try to enter dormant state when document is hidden

Apply the comment. Carry "r=cpearce".
Attachment #8553187 - Attachment is obsolete: true
Attachment #8554607 - Flags: review+
(Assignee)

Comment 26

3 years ago
Wait to check in until bug 1125472 fixed.
I landed this along with my other patches:
https://hg.mozilla.org/integration/mozilla-inbound/rev/304e3ccaf533
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
(Assignee)

Comment 29

3 years ago
(In reply to Carsten Book [:Tomcat] from comment #28)
> 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

It seems to be caused by a pointer check of mOwner.

>       !mOwner->IsHidden() ||
(Assignee)

Comment 30

3 years ago
Created attachment 8555249 [details] [diff] [review]
patch - Try to enter dormant state when document is hidden

Add pointer check. Carry "r=cpearce".
Attachment #8554607 - Attachment is obsolete: true
Attachment #8555249 - Flags: review+
(Assignee)

Comment 31

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8558b7fe82bd
https://tbpl.mozilla.org/?tree=Try&rev=8558b7fe82bd
(Assignee)

Comment 32

3 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #31)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=8558b7fe82bd
> https://tbpl.mozilla.org/?tree=Try&rev=8558b7fe82bd

Even when adding pointer check, MediaDecoder::StartDormantTimer() caused the crash. It seem that mOwner is dangling pointer.
(Assignee)

Comment 33

3 years ago
Created attachment 8555448 [details] [diff] [review]
patch - Try to enter dormant state when document is hidden

Add shutdown check to MediaDecoder::StartDormantTimer(). Carry "r=cpearce".
Attachment #8555249 - Attachment is obsolete: true
Attachment #8555448 - Flags: review+
(Assignee)

Comment 34

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=78748e0ad9d4
https://tbpl.mozilla.org/?tree=Try&rev=78748e0ad9d4
(Assignee)

Comment 35

3 years ago
Created attachment 8555450 [details] [diff] [review]
patch - Try to enter dormant state when document is hidden

Safer patch. Carry "r=cpearce".
Attachment #8555448 - Attachment is obsolete: true
Attachment #8555450 - Flags: review+
(Assignee)

Comment 36

3 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #34)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=78748e0ad9d4
> https://tbpl.mozilla.org/?tree=Try&rev=78748e0ad9d4

cancelled.
(Assignee)

Comment 37

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7e3bcf58532
https://tbpl.mozilla.org/?tree=Try&rev=e7e3bcf58532
(In reply to Sotaro Ikeda [:sotaro] from comment #37)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7e3bcf58532
> https://tbpl.mozilla.org/?tree=Try&rev=e7e3bcf58532

That's green. I will re-land my patch queue with this fix once the tree re-opens.
https://hg.mozilla.org/integration/mozilla-inbound/rev/fad4fd21ab1d
(Assignee)

Comment 40

3 years ago
Thanks!
Backed out because something in the push was causing Linux32 opt mochitest-3 timeouts and Windows w-p-t failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcbff95e6d21

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=5997815&repo=mozilla-inbound
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=5999248&repo=mozilla-inbound
(Assignee)

Comment 42

3 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #41)
> Backed out because something in the push was causing Linux32 opt mochitest-3
> timeouts and Windows w-p-t failures.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/bcbff95e6d21
> 
> https://treeherder.mozilla.org/ui/logviewer.
> html#?job_id=5997815&repo=mozilla-inbound
> https://treeherder.mozilla.org/ui/logviewer.
> html#?job_id=5999248&repo=mozilla-inbound

Linux32 opt mochitest-3 seem to be caused by a different path in a series of patches.
(Assignee)

Comment 43

3 years ago
I am going to land the patch separately from bug 1123535.
(Assignee)

Comment 44

3 years ago
 https://hg.mozilla.org/integration/mozilla-inbound/rev/73c166471592
https://hg.mozilla.org/mozilla-central/rev/73c166471592
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
status-firefox36: --- → affected
status-firefox37: --- → affected
status-firefox38: --- → fixed
Comment on attachment 8555450 [details] [diff] [review]
patch - Try to enter dormant state when document is hidden

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: YouTube uses more resources; OOM crashes more likely.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: This affects all media playback and is moderately risky.
[String/UUID change made/needed]: None.
Attachment #8555450 - Flags: approval-mozilla-beta?
Attachment #8555450 - Flags: approval-mozilla-aurora?
(In reply to Ralph Giles (:rillian) from comment #46)
> [Risks and why]: This affects all media playback and is moderately risky.
That is a bit scary. Don't you have a way to mitigate this risk?
Flags: needinfo?(giles)
Not other than getting wider testing. To reduce resource consumption we have to be able to shut down idle video decoders, and there's no reasonable way to do that separately for just the MSE code.

Ideally we'd let it bake on Nightly (and possibly Aurora) for a few days, and delaying uplift until after 36 beta 5 is tagged would do that. But given the closeness to release, we feel it's safer to get it onto all branches so we learn more quickly whether there are regressions.

In particular, we want data on whether this reduces the incidence of OOM crashes we're getting.
Flags: needinfo?(giles)
Attachment #8555450 - Flags: approval-mozilla-beta?
Attachment #8555450 - Flags: approval-mozilla-beta+
Attachment #8555450 - Flags: approval-mozilla-aurora?
Attachment #8555450 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/bc8330374bd7
status-firefox37: affected → fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/7436b2d2e790
status-firefox36: affected → fixed
Comment on attachment 8555450 [details] [diff] [review]
patch - Try to enter dormant state when document is hidden

Review of attachment 8555450 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoder.cpp
@@ +702,5 @@
>  nsresult MediaDecoder::Play()
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>    ReentrantMonitorAutoEnter mon(GetReentrantMonitor());
> +  UpdateDormantState(false /* aDormantTimeout */, true /* aActivity */);

What is the purpose of calling UpdateDormantState(false, true) when Play() or Seek() starts?
Flags: needinfo?(sotaro.ikeda.g)
(Assignee)

Comment 52

a year ago
(In reply to JW Wang [:jwwang] from comment #51)
> ::: dom/media/MediaDecoder.cpp
> @@ +702,5 @@
> >  nsresult MediaDecoder::Play()
> >  {
> >    MOZ_ASSERT(NS_IsMainThread());
> >    ReentrantMonitorAutoEnter mon(GetReentrantMonitor());
> > +  UpdateDormantState(false /* aDormantTimeout */, true /* aActivity */);
> 
> What is the purpose of calling UpdateDormantState(false, true) when Play()
> or Seek() starts?

It was called to trigger wake up from dormant if MediaDecoder was in dormant state.
Flags: needinfo?(sotaro.ikeda.g)
Well, it doesn't do the job.

See the logs in bug 1273612 comment 7.
It looks like we need to fix the logic of UpdateDormantState(). Can you explain the meaning and usage of the parameters of |aDormantTimeout| and |aActivity|?
Flags: needinfo?(sotaro.ikeda.g)
(Assignee)

Comment 55

a year ago
(In reply to JW Wang [:jwwang] from comment #54)
> It looks like we need to fix the logic of UpdateDormantState(). Can you
> explain the meaning and usage of the parameters of |aDormantTimeout| and
> |aActivity|?

aDormantTimeout true is set when UpdateDormantState() is called from DormantTimerExpired(). It is used to detect timing of entering dormant except gonk. Because except gonk, MediaDecoder needed to enter dormant state after inactivity period(Heuristic Dormant Timeout time).

aActivity true is set when UpdateDormantState() is called from MediaDecoder's owner's controlling function calls that user could notice MediaDecoder working (via audio and video).

Except gonk, MediaDecoder entered and exited silently from users. It was done as users did not recognize that the MediaDecoder was in dormant state. MediaDecoder had to exit the dormant state in correct timing.
Flags: needinfo?(sotaro.ikeda.g)
(In reply to Sotaro Ikeda [:sotaro] from comment #55)
> aActivity true is set when UpdateDormantState() is called from
> MediaDecoder's owner's controlling function calls that user could notice
> MediaDecoder working (via audio and video).

So can I say UpdateDormantState(false, true) (which is called by Play() and Seek()) should always exit the dormant state?
Flags: needinfo?(sotaro.ikeda.g)
(Assignee)

Comment 57

a year ago
(In reply to JW Wang [:jwwang] from comment #56)
> 
> So can I say UpdateDormantState(false, true) (which is called by Play() and
> Seek()) should always exit the dormant state?

Basically yes, but there is 2 cases that MediaDecoder does not exit the dormant.

On gonk and when MediaDecoder has video, it does not exit dormant if the document is hidden. It is for limiting video decoder usages and intentional.

If the owner document is not active, MediaDecoder does not exit dormant. It was implemented just to align to HTMLMediaElement. The following says that "We should consider any bfcached page or inactive document as non-playing."

  https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#4997



By the way, in current implementation, MediaDecoder only fall into dormant when it has video. The dormant was originally implemented to reduce video resource consumption.
Flags: needinfo?(sotaro.ikeda.g)
(In reply to Sotaro Ikeda [:sotaro] from comment #57)
> If the owner document is not active, MediaDecoder does not exit dormant. It
> was implemented just to align to HTMLMediaElement. The following says that
> "We should consider any bfcached page or inactive document as non-playing."

That is exactly the problem described in bug 1273612 comment 7 where SetCurrentTime() is called when the owner document is inactive and hits the assertion for not exiting the dormant state.


Should we just reject the seek request for an inactive document is regarded as non-playing?
Flags: needinfo?(sotaro.ikeda.g)
(Assignee)

Comment 59

a year ago
Hmm, at first, it seems better to confirm the policy of HTMLMediaElement about how HTMLMediaElement should work when owner document is inactive. From the implementation, the policy seems not clear enough. Current implementation just keep dormant since HTMLMediaElement seems not expect to be played when owner document is bfcached page or inactive document. I do not know well about the policy, it seems better to confirm to :cpearce.

If we could continue dormant mode, in this case, there seems two choices.
 -[1] Just remove assert
   + Just logical position of MediaDecoder is updated in this case.
     Actual playback does not happen by MediaDecoderStateMachine.
 -[2] Change HTMLMediaElement as not to call seek when the owner document is inactive.

I do not think it is a good idea to reject the seek. The dormant state is not recognized by web content side. It try to be as transparent as possible.
Flags: needinfo?(sotaro.ikeda.g) → needinfo?(cpearce)
(Assignee)

Comment 60

a year ago
I rethink about comment 59. It is most harmless if MediaDecoder goes out of dormant mode in this case.
(Assignee)

Updated

a year ago
Flags: needinfo?(cpearce)
You need to log in before you can comment on or make changes to this bug.