1123452 - Try to enter dormant state when document is hidden

Status: RESOLVED FIXED

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

Description

[Sotaro Ikeda [:sotaro]]

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().

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Is it different from what we do in

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

?

Comment 3

[Sotaro Ikeda [:sotaro]]

(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.

Comment 4

[Sotaro Ikeda [:sotaro]]

This bug should not degrade the Firefox OS's system sound's performance.

Comment 5

[Sotaro Ikeda [:sotaro]]

(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.

Comment 6

[Sotaro Ikeda [:sotaro]]

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?

Comment 7

[Sotaro Ikeda [:sotaro]]

(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.

Comment 8

[Sotaro Ikeda [:sotaro]]

(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.

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: Bug 1123452 - Try to enter dormant state when document is hidden

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Try to enter dormant state when document is hidden

heuristic dormant is disabled by default.

Comment 12

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Try to enter dormant state when document is hidden

update nits.

Comment 13

[Sotaro Ikeda [:sotaro]]

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=100e76704a1d
 https://tbpl.mozilla.org/?tree=Try&rev=100e76704a1d

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

We also transition through dormant state when shutting down due to browser window close, which is not ideal, though it should not cause problems.

Comment 16

[Sotaro Ikeda [:sotaro]]

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.

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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.

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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.

Comment 19

[Sotaro Ikeda [:sotaro]]

I could reproduce the problem. I am investigating about it.

Comment 20

[Sotaro Ikeda [:sotaro]]

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.

Comment 21

[Sotaro Ikeda [:sotaro]]

When the problem happens, mVideoRequestStatus is not cleared during entering dormant.

Comment 22

[Sotaro Ikeda [:sotaro]]

(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.

Comment 23

[Sotaro Ikeda [:sotaro]]

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?

Comment 24

[Chris Pearce [:cpearce (Not reading bugmail)]]

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/

Comment 25

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Try to enter dormant state when document is hidden

Apply the comment. Carry "r=cpearce".

Comment 26

[Sotaro Ikeda [:sotaro]]

Wait to check in until bug 1125472 fixed.

Comment 27

[Chris Pearce [:cpearce (Not reading bugmail)]]

I landed this along with my other patches:
https://hg.mozilla.org/integration/mozilla-inbound/rev/304e3ccaf533

Comment 28

[Carsten Book [:Tomcat]]

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

Comment 29

[Sotaro Ikeda [:sotaro]]

(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() ||

Comment 30

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Try to enter dormant state when document is hidden

Add pointer check. Carry "r=cpearce".

Comment 31

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8558b7fe82bd
https://tbpl.mozilla.org/?tree=Try&rev=8558b7fe82bd

Comment 32

[Sotaro Ikeda [:sotaro]]

(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.

Comment 33

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Try to enter dormant state when document is hidden

Add shutdown check to MediaDecoder::StartDormantTimer(). Carry "r=cpearce".

Comment 34

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=78748e0ad9d4
https://tbpl.mozilla.org/?tree=Try&rev=78748e0ad9d4

Comment 35

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Try to enter dormant state when document is hidden

Safer patch. Carry "r=cpearce".

Comment 36

[Sotaro Ikeda [:sotaro]]

(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.

Comment 37

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7e3bcf58532
https://tbpl.mozilla.org/?tree=Try&rev=e7e3bcf58532

Comment 38

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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.

Comment 39

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fad4fd21ab1d

Comment 40

[Sotaro Ikeda [:sotaro]]

Thanks!

Comment 41

[Ryan VanderMeulen [:RyanVM]]

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

Comment 42

[Sotaro Ikeda [:sotaro]]

(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.

Comment 43

[Sotaro Ikeda [:sotaro]]

I am going to land the patch separately from bug 1123535.

Comment 44

[Sotaro Ikeda [:sotaro]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/73c166471592

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/73c166471592

Comment 46

[Ralph Giles (:rillian)]

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.

Comment 47

[Sylvestre Ledru [:Sylvestre]]

(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?

Comment 48

[Ralph Giles (:rillian)]

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.

Comment 49

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bc8330374bd7

Comment 50

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-beta/rev/7436b2d2e790

Comment 51

[JW Wang [:jwwang] [:jw_wang]]

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?

Comment 52

[Sotaro Ikeda [:sotaro]]

(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.

Comment 53

[JW Wang [:jwwang] [:jw_wang]]

Well, it doesn't do the job.

See the logs in bug 1273612 comment 7.

Comment 54

[JW Wang [:jwwang] [:jw_wang]]

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|?

Comment 55

[Sotaro Ikeda [:sotaro]]

(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.

Comment 56

[JW Wang [:jwwang] [:jw_wang]]

(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?

Comment 57

[Sotaro Ikeda [:sotaro]]

(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.

Comment 58

[JW Wang [:jwwang] [:jw_wang]]

(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?

Comment 59

[Sotaro Ikeda [:sotaro]]

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.

Comment 60

[Sotaro Ikeda [:sotaro]]

I rethink about comment 59. It is most harmless if MediaDecoder goes out of dormant mode in this case.