Closed Bug 1123452 Opened 9 years ago Closed 9 years ago

Try to enter dormant state when document is hidden

Categories

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

Product:
Core
Component:
Audio/Video
Platform:
x86_64
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla38
Tracking Flags:
Tracking Status
firefox36 --- fixed
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(1 file, 6 obsolete files)

patch - Try to enter dormant state when document is hidden
10.47 KB, patch
sotaro
: review+
Sylvestre
: approval-mozilla-aurora+
Sylvestre
: approval-mozilla-beta+
Details | Diff | Splinter Review 
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.
Depends on: 1122228
Assignee: nobody → sotaro.ikeda.g
Priority: -- → P1
Is it different from what we do in

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

?
(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.
This bug should not degrade the Firefox OS's system sound's performance.
(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.
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)
(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.
(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)
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+
heuristic dormant is disabled by default.
Attachment #8552895 - Attachment is obsolete: true
update nits.
Attachment #8553180 - Attachment is obsolete: true
Depends on: 1124844
No longer depends on: 1124844
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.
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.
I could reproduce the problem. I am investigating about it.
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.
When the problem happens, mVideoRequestStatus is not cleared during entering dormant.
Depends on: 1125472
(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 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+
Apply the comment. Carry "r=cpearce".
Attachment #8553187 - Attachment is obsolete: true
Attachment #8554607 - Flags: review+
Wait to check in until bug 1125472 fixed.
(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() ||
Add pointer check. Carry "r=cpearce".
Attachment #8554607 - Attachment is obsolete: true
Attachment #8555249 - Flags: review+
(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.
Add shutdown check to MediaDecoder::StartDormantTimer(). Carry "r=cpearce".
Attachment #8555249 - Attachment is obsolete: true
Attachment #8555448 - Flags: review+
Safer patch. Carry "r=cpearce".
Attachment #8555448 - Attachment is obsolete: true
Attachment #8555450 - Flags: review+
(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.
Thanks!
(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.
I am going to land the patch separately from bug 1123535.
https://hg.mozilla.org/mozilla-central/rev/73c166471592
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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+
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)
(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)
(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)
(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)
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)
I rethink about comment 59. It is most harmless if MediaDecoder goes out of dormant mode in this case.
Flags: needinfo?(cpearce)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: