1113527 - [FFOS] Sometimes dormant is not set successfully during DecodeMetadata is requesting the codec resource.

Status: RESOLVED FIXED

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

Description

[Blake Wu [:bwu][:blakewu]]

This is a a sub-bug from Bug 1098298 comment 6.

Comment 1

[Blake Wu [:bwu][:blakewu]]

We should let setDormant as a media task and enqueue it in media task queue(mDecodeTaskQueue) to avoid this timing issue.

Comment 2

[Blake Wu [:bwu][:blakewu]]

Attachment: (v2.0)Bug-1113527-make-setDormant-done-in-a-MediaTaskQueue.patch

As comment 1.

Comment 3

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

Comment on attachment 8539104 [details] [diff] [review]
(v2.0)Bug-1113527-make-setDormant-done-in-a-MediaTaskQueue.patch

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

::: content/media/MediaDecoderStateMachine.cpp
@@ +1624,4 @@
>    NS_ASSERTION(OnStateMachineThread() || OnDecodeThread(),
>                 "Should be on state machine or decode thread.");
>  
> +  if (mState >= DECODER_STATE_COMPLETED || mState == DECODER_STATE_DORMANT) {

Why is checking |mState == DECODER_STATE_DORMANT| needed? This patch doesn't change how/when mState is set.

@@ +2198,4 @@
>        }
> +      // Now that those threads are stopped, there's no possibility of
> +      // mPendingWakeDecoder being needed again. Revoke it.
> +      mPendingWakeDecoder = nullptr;

Why moving this down mReader->ReleaseMediaResources()?

@@ +2882,5 @@
> +nsresult
> +MediaDecoderStateMachine::EnqueueSetDormantTask()
> +{
> +  AssertCurrentThreadInMonitor();
> +  if (mState == DECODER_STATE_SHUTDOWN) {

assert |mState == DECODER_STATE_DORMANT| since it is called from the DECODER_STATE_DORMANT case only.

@@ +2888,5 @@
> +  }
> +  nsresult rv = mDecodeTaskQueue->Dispatch(
> +    NS_NewRunnableMethod(this, &MediaDecoderStateMachine::SetDormantTask));
> +
> +  return NS_OK;

return rv instead.

@@ +2893,5 @@
> +}
> +
> +void
> +MediaDecoderStateMachine::SetDormantTask() {
> +  AssertCurrentThreadInMonitor();

You should acquire the monitor here.

::: content/media/omx/MediaOmxReader.h
@@ +90,5 @@
>  
>    virtual bool IsDormantNeeded();
> +
> +  // True if this reader is capable of being dormant
> +  virtual bool IsDormantCapable() { return true; }

I think we need to also override this function for MediaCodecReader and MP4Reader.

Comment 4

[Blake Wu [:bwu][:blakewu]]

Comment on attachment 8539104 [details] [diff] [review]
(v2.0)Bug-1113527-make-setDormant-done-in-a-MediaTaskQueue.patch

This patch is based on v2.0 since bug 1098298 is reported on v2.0.

Comment 5

[Blake Wu [:bwu][:blakewu]]

Thanks for the feedback! 
(In reply to JW Wang [:jwwang] from comment #3)
> Comment on attachment 8539104 [details] [diff] [review]
> (v2.0)Bug-1113527-make-setDormant-done-in-a-MediaTaskQueue.patch
> 
> Review of attachment 8539104 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/MediaDecoderStateMachine.cpp
> @@ +1624,4 @@
> >    NS_ASSERTION(OnStateMachineThread() || OnDecodeThread(),
> >                 "Should be on state machine or decode thread.");
> >  
> > +  if (mState >= DECODER_STATE_COMPLETED || mState == DECODER_STATE_DORMANT) {
> 
> Why is checking |mState == DECODER_STATE_DORMANT| needed? This patch doesn't
> change how/when mState is set.
This additional check is to avoid more decoding tasks being queued when we are going to set it dormant. 
> 
> @@ +2198,4 @@
> >        }
> > +      // Now that those threads are stopped, there's no possibility of
> > +      // mPendingWakeDecoder being needed again. Revoke it.
> > +      mPendingWakeDecoder = nullptr;
> 
> Why moving this down mReader->ReleaseMediaResources()?
Assuming you asked why move "mPendingWakeDecoder = nullptr;" down mReader->ReleaseMediaResources(). I didn't change the order. mReader->ReleaseMediaResources() will be called in EnqueueSetDormantTask();
> 
> @@ +2882,5 @@
> > +nsresult
> > +MediaDecoderStateMachine::EnqueueSetDormantTask()
> > +{
> > +  AssertCurrentThreadInMonitor();
> > +  if (mState == DECODER_STATE_SHUTDOWN) {
> 
> assert |mState == DECODER_STATE_DORMANT| since it is called from the
> DECODER_STATE_DORMANT case only.
For v2.0, it is possible that mState is changed to DECODER_STATE_SHUTDOWN. If mState is changed to DECODER_STATE_SHUTDOWN，dormant should not be set since shutdown is more important than dormant, right?
> 
> @@ +2888,5 @@
> > +  }
> > +  nsresult rv = mDecodeTaskQueue->Dispatch(
> > +    NS_NewRunnableMethod(this, &MediaDecoderStateMachine::SetDormantTask));
> > +
> > +  return NS_OK;
> 
> return rv instead.
Agree. 
> 
> @@ +2893,5 @@
> > +}
> > +
> > +void
> > +MediaDecoderStateMachine::SetDormantTask() {
> > +  AssertCurrentThreadInMonitor();
> 
> You should acquire the monitor here.
Yeah. 
> 
> ::: content/media/omx/MediaOmxReader.h
> @@ +90,5 @@
> >  
> >    virtual bool IsDormantNeeded();
> > +
> > +  // True if this reader is capable of being dormant
> > +  virtual bool IsDormantCapable() { return true; }
> 
> I think we need to also override this function for MediaCodecReader and
> MP4Reader.
For 2.0, it may not be required. But I should remember to overwrite in 2.1 and master branch.

Comment 6

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

Comment on attachment 8539104 [details] [diff] [review]
(v2.0)Bug-1113527-make-setDormant-done-in-a-MediaTaskQueue.patch

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

::: content/media/MediaDecoderStateMachine.cpp
@@ +1624,4 @@
>    NS_ASSERTION(OnStateMachineThread() || OnDecodeThread(),
>                 "Should be on state machine or decode thread.");
>  
> +  if (mState >= DECODER_STATE_COMPLETED || mState == DECODER_STATE_DORMANT) {

Do you mean this problem is already there without this patch that we could enqueuing decoding tasks while |mState == DECODER_STATE_DORMANT|?

@@ +2198,4 @@
>        }
> +      // Now that those threads are stopped, there's no possibility of
> +      // mPendingWakeDecoder being needed again. Revoke it.
> +      mPendingWakeDecoder = nullptr;

mPendingWakeDecoder used to be reset before mDecodeTaskQueue->AwaitIdle() but now it is after that. Is there a reason for that?

@@ +2882,5 @@
> +nsresult
> +MediaDecoderStateMachine::EnqueueSetDormantTask()
> +{
> +  AssertCurrentThreadInMonitor();
> +  if (mState == DECODER_STATE_SHUTDOWN) {

Ok, I see. The state chould change when mDecodeTaskQueue->AwaitIdle is running outside the monitor.

::: content/media/omx/MediaOmxReader.h
@@ +90,5 @@
>  
>    virtual bool IsDormantNeeded();
> +
> +  // True if this reader is capable of being dormant
> +  virtual bool IsDormantCapable() { return true; }

OK.

Comment 7

[Blake Wu [:bwu][:blakewu]]

(In reply to JW Wang [:jwwang] from comment #6)
> Comment on attachment 8539104 [details] [diff] [review]
> (v2.0)Bug-1113527-make-setDormant-done-in-a-MediaTaskQueue.patch
> 
> Review of attachment 8539104 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/MediaDecoderStateMachine.cpp
> @@ +1624,4 @@
> >    NS_ASSERTION(OnStateMachineThread() || OnDecodeThread(),
> >                 "Should be on state machine or decode thread.");
> >  
> > +  if (mState >= DECODER_STATE_COMPLETED || mState == DECODER_STATE_DORMANT) {
> 
> Do you mean this problem is already there without this patch that we could
> enqueuing decoding tasks while |mState == DECODER_STATE_DORMANT|?
Yes! 
> 
> @@ +2198,4 @@
> >        }
> > +      // Now that those threads are stopped, there's no possibility of
> > +      // mPendingWakeDecoder being needed again. Revoke it.
> > +      mPendingWakeDecoder = nullptr;
> 
> mPendingWakeDecoder used to be reset before mDecodeTaskQueue->AwaitIdle()
> but now it is after that. Is there a reason for that?
I thought it will be better to revoke it after all tasks are done. After checking codes together, it will be better to keep the original order to avoid more tasks pushed into task queue.  
> 
> @@ +2882,5 @@
> > +nsresult
> > +MediaDecoderStateMachine::EnqueueSetDormantTask()
> > +{
> > +  AssertCurrentThreadInMonitor();
> > +  if (mState == DECODER_STATE_SHUTDOWN) {
> 
> Ok, I see. The state chould change when mDecodeTaskQueue->AwaitIdle is
> running outside the monitor.
> 
> ::: content/media/omx/MediaOmxReader.h
> @@ +90,5 @@
> >  
> >    virtual bool IsDormantNeeded();
> > +
> > +  // True if this reader is capable of being dormant
> > +  virtual bool IsDormantCapable() { return true; }
> 
> OK.

Comment 8

[Blake Wu [:bwu][:blakewu]]

Attachment: (v2.0)Bug-1113527-make-setDormant-done-in-a-MediaTaskQueue-v2.patch

Have changes as discussed in comment 3~6

Comment 9

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

Comment on attachment 8541105 [details] [diff] [review]
(v2.0)Bug-1113527-make-setDormant-done-in-a-MediaTaskQueue-v2.patch

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

::: content/media/MediaDecoderStateMachine.cpp
@@ +1379,5 @@
>  
>    // The reader is no longer waiting for resources (say a hardware decoder),
>    // we can now proceed to decode metadata.
> +
> +  if (mState == DECODER_STATE_SHUTDOWN || mState == DECODER_STATE_DORMANT)

Ｉdon't think we need this check here since we've checked mState != DECODER_STATE_WAIT_FOR_RESOURCES above.

Comment 10

[Blake Wu [:bwu][:blakewu]]

Comment on attachment 8541105 [details] [diff] [review]
(v2.0)Bug-1113527-make-setDormant-done-in-a-MediaTaskQueue-v2.patch

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

::: content/media/MediaDecoderStateMachine.cpp
@@ +1379,5 @@
>  
>    // The reader is no longer waiting for resources (say a hardware decoder),
>    // we can now proceed to decode metadata.
> +
> +  if (mState == DECODER_STATE_SHUTDOWN || mState == DECODER_STATE_DORMANT)

Yeah. This check is redundant. I will remove it. Thanks!

Comment 11

[Blake Wu [:bwu][:blakewu]]

Attachment: (Master branch)make-setDormant-done-in-a-MediaTaskQueue.patch

Rebase attachment 8541105 [details] [diff] [review] to master branch plus some changes as comment 5 and comment 10.

Comment 12

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

Comment on attachment 8541399 [details] [diff] [review]
(Master branch)make-setDormant-done-in-a-MediaTaskQueue.patch

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

The idea of calling ReleaseMediaResources on the MediaTaskQueue makes sense, but I don't understand why we need both IsDormantNeeded() and IsDormantCapable().

Sotaro should also review this.

::: dom/media/MediaDecoder.cpp
@@ -124,4 @@
>    ReentrantMonitorAutoEnter mon(GetReentrantMonitor());
>  
>    if (!mDecoderStateMachine ||
> -      !mDecoderStateMachine->IsDormantNeeded() ||

Isn't this the only place where we call IsDormantNeeded() and then make a decision based upon its return value? If you remove this call to IsDormantNeeded(), then effectively we don't need IsDormantNeeded anymore, right?

I don't understand why we need both IsDormantNeeded() and IsDormantCapable(), or what the difference is and why it's significant.

Why can't you change IsDormantNeeded() to have the behaviour you need?

Comment 13

[Blake Wu [:bwu][:blakewu]]

(In reply to Chris Pearce (:cpearce) (PTO until 5 Jan) from comment #12)
> Comment on attachment 8541399 [details] [diff] [review]
> (Master branch)make-setDormant-done-in-a-MediaTaskQueue.patch
> 
> Review of attachment 8541399 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The idea of calling ReleaseMediaResources on the MediaTaskQueue makes sense,
> but I don't understand why we need both IsDormantNeeded() and
> IsDormantCapable().
> 
> Sotaro should also review this.
> 
> ::: dom/media/MediaDecoder.cpp
> @@ -124,4 @@
> >    ReentrantMonitorAutoEnter mon(GetReentrantMonitor());
> >  
> >    if (!mDecoderStateMachine ||
> > -      !mDecoderStateMachine->IsDormantNeeded() ||
> 
> Isn't this the only place where we call IsDormantNeeded() and then make a
> decision based upon its return value? If you remove this call to
> IsDormantNeeded(), then effectively we don't need IsDormantNeeded anymore,
> right?
> 
> I don't understand why we need both IsDormantNeeded() and
> IsDormantCapable(), or what the difference is and why it's significant.
> 
> Why can't you change IsDormantNeeded() to have the behaviour you need?

IsDormantCapable() is used to quickly check if the reader/decoder is capable of being dormant. If not (for browsers), we can ignore all the setDormant things. There is no timing issue for this return value. 
However, IsDormantNeeded() is based on the creation of video track which has a timing issue. When you call it @[1] video track may not be created, but shortly after you call it, video track is just created. But setDormant is aborted. Media Resource is not released and other app cannot use it. 

[1]http://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoder.cpp?from=MediaDecoder.cpp&case=true#127

Comment 14

[Blake Wu [:bwu][:blakewu]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #13)
> (In reply to Chris Pearce (:cpearce) (PTO until 5 Jan) from comment #12)
> > Comment on attachment 8541399 [details] [diff] [review]
> > (Master branch)make-setDormant-done-in-a-MediaTaskQueue.patch
> > I don't understand why we need both IsDormantNeeded() and
> > IsDormantCapable(), or what the difference is and why it's significant.
> > 
> > Why can't you change IsDormantNeeded() to have the behaviour you need?
> 
> IsDormantCapable() is used to quickly check if the reader/decoder is capable
> of being dormant. If not (for browsers), we can ignore all the setDormant
> things. There is no timing issue for this return value. 
> However, IsDormantNeeded() is based on the creation of video track which has
> a timing issue. When you call it @[1] video track may not be created, but
> shortly after you call it, video track is just created. But setDormant is
> aborted. Media Resource is not released and other app cannot use it. 
> 
> [1]http://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoder.
> cpp?from=MediaDecoder.cpp&case=true#127

IsDormantNeeded() should be used in a task in the MediaTaskQueue to avoid the timing issue.

Comment 15

[Benjamin Chen [:bechen]]

Comment on attachment 8541399 [details] [diff] [review]
(Master branch)make-setDormant-done-in-a-MediaTaskQueue.patch

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +3459,5 @@
> +  ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> +  if (!IsDormantNeeded()) {
> +    return;
> +  }
> +  mReader->ReleaseMediaResources();

At original logic, we should release the monitor when access |mReader|, maybe we should release the monitor here.

Comment 16

[Blake Wu [:bwu][:blakewu]]

(In reply to Benjamin Chen [:bechen] from comment #15)
> Comment on attachment 8541399 [details] [diff] [review]
> (Master branch)make-setDormant-done-in-a-MediaTaskQueue.patch
> 
> Review of attachment 8541399 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +3459,5 @@
> > +  ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> > +  if (!IsDormantNeeded()) {
> > +    return;
> > +  }
> > +  mReader->ReleaseMediaResources();
> 
> At original logic, we should release the monitor when access |mReader|,
> maybe we should release the monitor here.
Why that release is required?

Comment 17

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

(In reply to Blake Wu [:bwu][:blakewu] from comment #13)
> (In reply to Chris Pearce (:cpearce) (PTO until 5 Jan) from comment #12)
> > Comment on attachment 8541399 [details] [diff] [review]
> > (Master branch)make-setDormant-done-in-a-MediaTaskQueue.patch
> > 
> > Review of attachment 8541399 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > The idea of calling ReleaseMediaResources on the MediaTaskQueue makes sense,
> > but I don't understand why we need both IsDormantNeeded() and
> > IsDormantCapable().
> > 
> > Sotaro should also review this.
> > 
> > ::: dom/media/MediaDecoder.cpp
> > @@ -124,4 @@
> > >    ReentrantMonitorAutoEnter mon(GetReentrantMonitor());
> > >  
> > >    if (!mDecoderStateMachine ||
> > > -      !mDecoderStateMachine->IsDormantNeeded() ||
> > 
> > Isn't this the only place where we call IsDormantNeeded() and then make a
> > decision based upon its return value? If you remove this call to
> > IsDormantNeeded(), then effectively we don't need IsDormantNeeded anymore,
> > right?
> > 
> > I don't understand why we need both IsDormantNeeded() and
> > IsDormantCapable(), or what the difference is and why it's significant.
> > 
> > Why can't you change IsDormantNeeded() to have the behaviour you need?
> 
> IsDormantCapable() is used to quickly check if the reader/decoder is capable
> of being dormant. If not (for browsers), we can ignore all the setDormant
> things. There is no timing issue for this return value. 
> However, IsDormantNeeded() is based on the creation of video track which has
> a timing issue. When you call it @[1] video track may not be created, but
> shortly after you call it, video track is just created. But setDormant is
> aborted. Media Resource is not released and other app cannot use it. 
> 
> [1]http://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoder.
> cpp?from=MediaDecoder.cpp&case=true#127

But you remove all calls to IsDormantNeeded() in your patch, and only call IsDormantCapable() now. Why do you need both IsDormantNeeded() and IsDormantCapable(), since you never call IsDormantNeeded() after your patch?

Comment 18

[Blake Wu [:bwu][:blakewu]]

Comment on attachment 8541399 [details] [diff] [review]
(Master branch)make-setDormant-done-in-a-MediaTaskQueue.patch

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +3456,5 @@
> +
> +void
> +MediaDecoderStateMachine::SetDormantTask() {
> +  ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> +  if (!IsDormantNeeded()) {

IsDormantNeeded() is called in SetDormantTask() which is handled in DecodeTaskQueue.

Comment 19

[Blake Wu [:bwu][:blakewu]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #18)
> Comment on attachment 8541399 [details] [diff] [review]
> (Master branch)make-setDormant-done-in-a-MediaTaskQueue.patch
> 
> Review of attachment 8541399 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +3456,5 @@
> > +
> > +void
> > +MediaDecoderStateMachine::SetDormantTask() {
> > +  ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> > +  if (!IsDormantNeeded()) {
> 
> IsDormantNeeded() is called in SetDormantTask() which is handled in
> DecodeTaskQueue.
Are you looking for this?

Comment 20

[Benjamin Chen [:bechen]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #16)
> (In reply to Benjamin Chen [:bechen] from comment #15)
> > Comment on attachment 8541399 [details] [diff] [review]
> > (Master branch)make-setDormant-done-in-a-MediaTaskQueue.patch
> > 
> > Review of attachment 8541399 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/media/MediaDecoderStateMachine.cpp
> > @@ +3459,5 @@
> > > +  ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> > > +  if (!IsDormantNeeded()) {
> > > +    return;
> > > +  }
> > > +  mReader->ReleaseMediaResources();
> > 
> > At original logic, we should release the monitor when access |mReader|,
> > maybe we should release the monitor here.
> Why that release is required?

Because we don't know the mReader's member function is blocking call or not. Once the thread holds the lock and be blocked, the main thread is not responding anymore.

Comment 21

[Blake Wu [:bwu][:blakewu]]

(In reply to Benjamin Chen [:bechen] from comment #20)
> (In reply to Blake Wu [:bwu][:blakewu] from comment #16)
> > (In reply to Benjamin Chen [:bechen] from comment #15)
> > > Comment on attachment 8541399 [details] [diff] [review]
> > > (Master branch)make-setDormant-done-in-a-MediaTaskQueue.patch
> > > 
> > > Review of attachment 8541399 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: dom/media/MediaDecoderStateMachine.cpp
> > > @@ +3459,5 @@
> > > > +  ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> > > > +  if (!IsDormantNeeded()) {
> > > > +    return;
> > > > +  }
> > > > +  mReader->ReleaseMediaResources();
> > > 
> > > At original logic, we should release the monitor when access |mReader|,
> > > maybe we should release the monitor here.
> > Why that release is required?
> 
> Because we don't know the mReader's member function is blocking call or not.
> Once the thread holds the lock and be blocked, the main thread is not
> responding anymore.
It should be ok to block the main thread for dormant case since at that time app should be invisible. 
On the other hand, there should be no other tasks/actions running in the main thread except those functions for setting dormant.

Comment 22

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

(In reply to Blake Wu [:bwu][:blakewu] from comment #19)
> (In reply to Blake Wu [:bwu][:blakewu] from comment #18)
> > Comment on attachment 8541399 [details] [diff] [review]
> > (Master branch)make-setDormant-done-in-a-MediaTaskQueue.patch
> > 
> > Review of attachment 8541399 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/media/MediaDecoderStateMachine.cpp
> > @@ +3456,5 @@
> > > +
> > > +void
> > > +MediaDecoderStateMachine::SetDormantTask() {
> > > +  ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> > > +  if (!IsDormantNeeded()) {
> > 
> > IsDormantNeeded() is called in SetDormantTask() which is handled in
> > DecodeTaskQueue.
> Are you looking for this?

Yes, thank you.

Comment 23

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

Comment on attachment 8541399 [details] [diff] [review]
(Master branch)make-setDormant-done-in-a-MediaTaskQueue.patch

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

::: dom/media/MediaDecoderReader.h
@@ +70,5 @@
>    virtual bool IsWaitingOnCDMResource() { return false; }
>    // True when this reader need to become dormant state
>    virtual bool IsDormantNeeded() { return false; }
> +  // True if this reader is capable to be dormant.
> +  virtual bool IsDormantCapable() { return false; }

I think this should be called NeedsDormantNotifications(). That is a better name, as it describes how the function is used; when this returns true, we should always send the notification of entering dormant state.

This can also be const.

::: dom/media/MediaDecoderStateMachine.cpp
@@ +3455,5 @@
> +}
> +
> +void
> +MediaDecoderStateMachine::SetDormantTask() {
> +  ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());

Assert that this runs on the decode task queue.

We only call IsDormantNeeded() on the decode task queue with this patch now, right? If so, then we don't need to hold the monitor anymore either, since we only ever call this function from the same task queue, so it's threadsafe. If the IsDormantNeeded() implementation needs to take the monitor, it can do so if it chooses to.

@@ +3456,5 @@
> +
> +void
> +MediaDecoderStateMachine::SetDormantTask() {
> +  ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> +  if (!IsDormantNeeded()) {

Just call mReader->IsDormantNeeded(), and remove MediaDecoderStateMachine::IsDormantNeeded().

We don't need to expose the IsDormantNeeded() functionality outside of the state machine/reader.

@@ +3459,5 @@
> +  ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> +  if (!IsDormantNeeded()) {
> +    return;
> +  }
> +  mReader->ReleaseMediaResources();

If IsDormantNeeded() needs the monitor (and I don't think it should) we should *definitely* release the monitor here.

I don't see why the call to IsDormantNeeded() needs to hold the monitor at all now.

::: dom/media/MediaDecoderStateMachine.h
@@ +158,3 @@
>  
>    // Check if the decoder needs to become dormant state.
>    bool IsDormantNeeded();

Please remove IsDormantNeeded() from the state machine; IsDormantNeeded() is only called inside the state machine (right?) so instead of adding a public function, we can just call mReader->IsDormantNeeded() inside the state machine.

Comment 24

[Sotaro Ikeda [:sotaro]]

> 
> ::: dom/media/MediaDecoderStateMachine.h
> @@ +158,3 @@
> >  
> >    // Check if the decoder needs to become dormant state.
> >    bool IsDormantNeeded();
> 
> Please remove IsDormantNeeded() from the state machine; IsDormantNeeded() is
> only called inside the state machine (right?) so instead of adding a public
> function, we can just call mReader->IsDormantNeeded() inside the state
> machine.

Since Bug 1108728 fix, MediaDecoder removed dormant related state. But code of MediaDecoder::SetDormantIfNecessary() still have a dependency if dormant is handled in MediaDecoderStateMachine().

In OmxDecoder, dormant is done only when a media have a video. Audio only case, it is not expedted to be called.

Comment 25

[Sotaro Ikeda [:sotaro]]

From comment 24, if we want to remove IsDormantNeeded() from MediaDecoderStateMachine, more modification to MediaDecoder::SetDormantIfNecessary() and MediaDecoderStateMachine::SetDormant(). Just remove mReader->IsDormantNeeded() or replace by IsDormantCapable() causes state inconsistency.

Comment 26

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

(In reply to Sotaro Ikeda [:sotaro PTO Dec/24-Jan/6] from comment #25)
> From comment 24, if we want to remove IsDormantNeeded() from
> MediaDecoderStateMachine, more modification to
> MediaDecoder::SetDormantIfNecessary() and
> MediaDecoderStateMachine::SetDormant(). Just remove
> mReader->IsDormantNeeded() or replace by IsDormantCapable() causes state
> inconsistency.

I didn't understand Blake's patch properly when I made this comment. We don't need to remove IsDormantNeeded(). We still need it and use it in his patch.

Comment 27

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8541399 [details] [diff] [review]
(Master branch)make-setDormant-done-in-a-MediaTaskQueue.patch

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

::: dom/media/MediaDecoder.cpp
@@ +124,4 @@
>    ReentrantMonitorAutoEnter mon(GetReentrantMonitor());
>  
>    if (!mDecoderStateMachine ||
> +      !mDecoderStateMachine->IsDormantCapable() ||

Why is ti changed to IsDormantCapable()? It does not handle correctly the case of comment 24.

Comment 28

[Blake Wu [:bwu][:blakewu]]

(In reply to Sotaro Ikeda [:sotaro] from comment #27)
> Comment on attachment 8541399 [details] [diff] [review]
> (Master branch)make-setDormant-done-in-a-MediaTaskQueue.patch
> 
> Review of attachment 8541399 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaDecoder.cpp
> @@ +124,4 @@
> >    ReentrantMonitorAutoEnter mon(GetReentrantMonitor());
> >  
> >    if (!mDecoderStateMachine ||
> > +      !mDecoderStateMachine->IsDormantCapable() ||
> 
> Why is ti changed to IsDormantCapable()? It does not handle correctly the
> case of comment 24.
As comment 13 and 14, 
IsDormantCapable() is used to quickly check if the reader/decoder is capable of being dormant. If not (for browsers), we can ignore/skip all the setDormant things. There is no timing issue for this return value. 
However, IsDormantNeeded() is based on the creation of video track which has a timing issue. When you call it @MediaDecoder video track may not be created, but shortly after you call it, video track is just created. But setDormant is aborted. Media Resource is not released and other app cannot use it. 
IsDormantNeeded() would be better to be used in the SetDormantTask() within MediaTaskQueue to avoid the timing issue.
Does this answer your question? Sorry. I didn't quite get your point for the case of comment 24.

Comment 29

[Sotaro Ikeda [:sotaro]]

Thanks. Yeah, there is such timing related problem. But attachment 8541399 [details] [diff] [review] regress audio file playback.

Comment 30

[Blake Wu [:bwu][:blakewu]]

(In reply to Sotaro Ikeda [:sotaro] from comment #29)
> Thanks. Yeah, there is such timing related problem. But attachment 8541399 [details] [diff] [review]
> [details] [diff] [review] regress audio file playback.
Thanks for your reminder. I will add a check to see if there is video track and re-submit a new patch.

Comment 31

[Blake Wu [:bwu][:blakewu]]

Attachment: (Master branch)Bug-1113527-make-setDormant-done-in-a-MediaTaskQueue-v2.patch

Changes according to comment 23, 29, and 30.

Comment 32

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8549517 [details] [diff] [review]
(Master branch)Bug-1113527-make-setDormant-done-in-a-MediaTaskQueue-v2.patch

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

My concern about using mime type is that mime type might not be correct. If mime type is "audio/mp4" and the file contains video track, MediaDecoderReader instantiate video track.

Comment 33

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8549517 [details] [diff] [review]
(Master branch)Bug-1113527-make-setDormant-done-in-a-MediaTaskQueue-v2.patch

cpearce, can you do feedback about using mime type?

Comment 34

[Sotaro Ikeda [:sotaro]]

As in Comment 13, usage of mDecoderStateMachine->IsDormantNeeded() in MediaDecoder::::SetDormantIfNecessary() has a timing related problem. But using mime seems not fix the problem in all cases. In b2g, video decoder needed to be released when a document is hidden. In same use case, audio element should not be set to dormant state. But in current implementation, video decoder is created even on audio element. If audio element does not instantiate video decoder, the problem seems to become simplified.

Comment 35

[Sotaro Ikeda [:sotaro]]

cpearce, is there no problem about doing Comment 34?

Comment 36

[Sotaro Ikeda [:sotaro]]

If doing Comment 34 is OK, we can remove fragile IsDormantNeeded().

Comment 37

[Blake Wu [:bwu][:blakewu]]

(In reply to Sotaro Ikeda [:sotaro] from comment #32)
> Comment on attachment 8549517 [details] [diff] [review]
> (Master branch)Bug-1113527-make-setDormant-done-in-a-MediaTaskQueue-v2.patch
> 
> Review of attachment 8549517 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> My concern about using mime type is that mime type might not be correct. If
> mime type is "audio/mp4" and the file contains video track,
> MediaDecoderReader instantiate video track.
Yeah, mime type might not be correct, although we base on it to create the corresponding decoder/reader. 
To avoid this case, we can have demuxer to get correct mime type via MediaResource[1]. 
But it may take some time (~100ms?). 
How do you think?

[1]https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#2613

Comment 38

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

(In reply to Blake Wu [:bwu][:blakewu] from comment #37)
> (In reply to Sotaro Ikeda [:sotaro] from comment #32)
> > Comment on attachment 8549517 [details] [diff] [review]
> > (Master branch)Bug-1113527-make-setDormant-done-in-a-MediaTaskQueue-v2.patch
> > 
> > Review of attachment 8549517 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > My concern about using mime type is that mime type might not be correct. If
> > mime type is "audio/mp4" and the file contains video track,
> > MediaDecoderReader instantiate video track.
> Yeah, mime type might not be correct, although we base on it to create the
> corresponding decoder/reader. 

Instead of checking the MIME type, you can check whether the decoder has an ImageContainer?

If the decoder has an ImageContainer, the decoder belongs to a <video> element. If it does not, it belongs to an <audio> element. We can only display video if we're in a <video> element.

If a Reader is running for a MediaDecoder without an ImageContainer, it should not start up a video decoder. The WebMReader and MP4Reader do this:

https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/MP4Reader.cpp#355
https://dxr.mozilla.org/mozilla-central/source/dom/media/webm/WebMReader.cpp#368


(In reply to Sotaro Ikeda [:sotaro] from comment #35)
> cpearce, is there no problem about doing Comment 34?

Does checking if the MediaDecoder has an ImageContainer make this easier?


(In reply to Blake Wu [:bwu][:blakewu] from comment #13)
> If not (for browsers), we can ignore all the setDormant
> things. There is no timing issue for this return value. 

We want to make MP4Reader in Desktop Firefox able to become dormant so we can reduce system resource usage for non-active <video>s in Desktop Firefox.

Comment 39

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

Comment on attachment 8549517 [details] [diff] [review]
(Master branch)Bug-1113527-make-setDormant-done-in-a-MediaTaskQueue-v2.patch

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

::: dom/media/MediaDecoder.cpp
@@ +124,4 @@
>    ReentrantMonitorAutoEnter mon(GetReentrantMonitor());
>  
>    if (!mDecoderStateMachine ||
> +      !mOwner->IsMimeTypeVideo() ||

Does using mOwner->GetVideoFrameContainer() instead of mOwner->IsMimeTypeVideo() work here?

Comment 40

[Blake Wu [:bwu][:blakewu]]

(In reply to Chris Pearce (:cpearce) from comment #39)
> Comment on attachment 8549517 [details] [diff] [review]
> (Master branch)Bug-1113527-make-setDormant-done-in-a-MediaTaskQueue-v2.patch
> 
> Review of attachment 8549517 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaDecoder.cpp
> @@ +124,4 @@
> >    ReentrantMonitorAutoEnter mon(GetReentrantMonitor());
> >  
> >    if (!mDecoderStateMachine ||
> > +      !mOwner->IsMimeTypeVideo() ||
> 
> Does using mOwner->GetVideoFrameContainer() instead of
> mOwner->IsMimeTypeVideo() work here?
I think it can work! Besides that we need to add this check to our OmxDecoder[1] as you mentioned in MP4Reader and WebMReader do to make it consistent. Otherwise, for fake audio mime type it still will create video track but this check fails. 

[1]https://dxr.mozilla.org/mozilla-central/source/dom/media/omx/OmxDecoder.cpp?from=OmxDecoder.cpp&case=true#247

Comment 41

[Blake Wu [:bwu][:blakewu]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #40)
> (In reply to Chris Pearce (:cpearce) from comment #39)
> > Comment on attachment 8549517 [details] [diff] [review]
> > (Master branch)Bug-1113527-make-setDormant-done-in-a-MediaTaskQueue-v2.patch
> > 
> > Review of attachment 8549517 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/media/MediaDecoder.cpp
> > @@ +124,4 @@
> > >    ReentrantMonitorAutoEnter mon(GetReentrantMonitor());
> > >  
> > >    if (!mDecoderStateMachine ||
> > > +      !mOwner->IsMimeTypeVideo() ||
> > 
> > Does using mOwner->GetVideoFrameContainer() instead of
> > mOwner->IsMimeTypeVideo() work here?
> I think it can work! Besides that we need to add this check to our
> OmxDecoder[1] as you mentioned in MP4Reader and WebMReader do to make it
> consistent. Otherwise, for fake audio mime type it still will create video
> track but this check fails. 
> 
> [1]https://dxr.mozilla.org/mozilla-central/source/dom/media/omx/OmxDecoder.
> cpp?from=OmxDecoder.cpp&case=true#247

Sotaro, 
Do you think in the same way?

Comment 42

[Sotaro Ikeda [:sotaro]]

mOwner->GetVideoFrameContainer() should work to check if the owner is video element. And adding the check to [1] could suppress the video track. And it could suppress unintended video resource conflict between audio element and video element. But we also need to change OmxDecoder::HasVideo(), otherwise the code seems not work correctly.

The change to OmxDecoder::HasVideo() changes the way how audio element works. In current gecko, if video file is loaded to audio element, video track is also reported. It changes as not to report a video track when a video file is loaded to audio element.

cpearce, is there no problem about it?

Comment 43

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

I think it's ok to not report a video track in an <audio> element. There is no way to paint a video track in an <audio> element, so not decoding or reporting the presence of the video track seems fine to me. This is the same behaviour in MP4Reader and WebMReader.

Comment 44

[Blake Wu [:bwu][:blakewu]]

(In reply to Sotaro Ikeda [:sotaro] from comment #42)
> mOwner->GetVideoFrameContainer() should work to check if the owner is video
> element. And adding the check to [1] could suppress the video track. And it
> could suppress unintended video resource conflict between audio element and
> video element. But we also need to change OmxDecoder::HasVideo(), otherwise
> the code seems not work correctly.
OmxDecoder::HasVideo()@[2] checks mVideoSource to decide it has video or not. And mVideoSource is only created @[3], so it seems adding one check to [1] should be sufficient. 

[1]https://dxr.mozilla.org/mozilla-central/source/dom/media/omx/OmxDecoder.cpp?from=OmxDecoder.cpp#247 
[2]https://dxr.mozilla.org/mozilla-central/source/dom/media/omx/OmxDecoder.h?from=OmxDecoder.h#192
[3]https://dxr.mozilla.org/mozilla-central/source/dom/media/omx/OmxDecoder.cpp?from=OmxDecoder.cpp#287

Comment 45

[Blake Wu [:bwu][:blakewu]]

Attachment: Master-Bug-1113527-Make-setDormant-done-in-a-MediaTaskQueue-v3.patch

Changes according to comment 38~44.

Comment 46

[Sotaro Ikeda [:sotaro]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #44)
> (In reply to Sotaro Ikeda [:sotaro] from comment #42)
> > mOwner->GetVideoFrameContainer() should work to check if the owner is video
> > element. And adding the check to [1] could suppress the video track. And it
> > could suppress unintended video resource conflict between audio element and
> > video element. But we also need to change OmxDecoder::HasVideo(), otherwise
> > the code seems not work correctly.
> OmxDecoder::HasVideo()@[2] checks mVideoSource to decide it has video or
> not. And mVideoSource is only created @[3], so it seems adding one check to
> [1] should be sufficient. 
> 
> [1]https://dxr.mozilla.org/mozilla-central/source/dom/media/omx/OmxDecoder.
> cpp?from=OmxDecoder.cpp#247 
> [2]https://dxr.mozilla.org/mozilla-central/source/dom/media/omx/OmxDecoder.
> h?from=OmxDecoder.h#192
> [3]https://dxr.mozilla.org/mozilla-central/source/dom/media/omx/OmxDecoder.
> cpp?from=OmxDecoder.cpp#287

Yhea, OK.

Comment 47

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8551133 [details] [diff] [review]
Master-Bug-1113527-Make-setDormant-done-in-a-MediaTaskQueue-v3.patch

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

Don't we need to add the mDecoder->GetImageContainer() check also to MediaCodecReader? It always create video track if a media file has a video track.

Comment 48

[Blake Wu [:bwu][:blakewu]]

(In reply to Sotaro Ikeda [:sotaro] from comment #47)
> Comment on attachment 8551133 [details] [diff] [review]
> Master-Bug-1113527-Make-setDormant-done-in-a-MediaTaskQueue-v3.patch
> 
> Review of attachment 8551133 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Don't we need to add the mDecoder->GetImageContainer() check also to
> MediaCodecReader? It always create video track if a media file has a video
> track.
Yeah. We should add it to MediaCodecReader as well. 
Thanks for the reminder! I will re-submit a new patch.

Comment 49

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

Comment on attachment 8551133 [details] [diff] [review]
Master-Bug-1113527-Make-setDormant-done-in-a-MediaTaskQueue-v3.patch

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +3593,5 @@
> +}
> +
> +void
> +MediaDecoderStateMachine::SetDormantTask() {
> +  AssertCurrentThreadInMonitor();

You're asserting here that the monitor is held, but it won't be because this function is called by an RunnableMethod.

Also, it's a bad idea to call into mReader with the monitor held, please don't do that.

Comment 50

[Blake Wu [:bwu][:blakewu]]

(In reply to Chris Pearce (:cpearce) from comment #49)
> Comment on attachment 8551133 [details] [diff] [review]
> Master-Bug-1113527-Make-setDormant-done-in-a-MediaTaskQueue-v3.patch
> 
> Review of attachment 8551133 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +3593,5 @@
> > +}
> > +
> > +void
> > +MediaDecoderStateMachine::SetDormantTask() {
> > +  AssertCurrentThreadInMonitor();
> 
> You're asserting here that the monitor is held, but it won't be because this
> function is called by an RunnableMethod.
> 
> Also, it's a bad idea to call into mReader with the monitor held, please
> don't do that.
Yes. I should remove it. 
It is easy to make people (at lease to me :)) confused when/where to lock/unlock MediaDecoder's Monitor and similarly we have MediaTaskQueue to put some tasks into it to handle them in a proper sequence. IMHO, we should simplify this codes in MediaDecoder and MediaStateMachine.

Comment 51

[Blake Wu [:bwu][:blakewu]]

Attachment: Master-Bug-1113527-Make-setDormant-done-in-a-MediaTaskQueue-v4.patch

Based on attachment 8551133 [details] [diff] [review], 
add some changes according to comment 47 and 49.

Comment 52

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

Comment on attachment 8551596 [details] [diff] [review]
Master-Bug-1113527-Make-setDormant-done-in-a-MediaTaskQueue-v4.patch

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +3593,5 @@
> +}
> +
> +void
> +MediaDecoderStateMachine::SetDormantTask() {
> +  if (!IsDormantNeeded()) {

You should add an assertion here that you're running on the decode thread.

Comment 53

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8551596 [details] [diff] [review]
Master-Bug-1113527-Make-setDormant-done-in-a-MediaTaskQueue-v4.patch

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

I am going to check if the patch cause the regression.

Comment 54

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8551596 [details] [diff] [review]
Master-Bug-1113527-Make-setDormant-done-in-a-MediaTaskQueue-v4.patch

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

I confirmed that the patch regress video playback on latest master flame.

Comment 55

[Sotaro Ikeda [:sotaro]]

The following is the STR of comment 54.

[1] Open Browser app
[2] move to http://people.mozilla.org/~cpeterson/videos/
[3] Touch "H264_Baseline_Profile_Level_30_640x360p.mp4"

Expected: video playback start in browser tab.

Actual result: video playback did not start.

Comment 56

[Sotaro Ikeda [:sotaro]]

bwu, can you check Comment 55.

Comment 57

[Blake Wu [:bwu][:blakewu]]

I have tested the patch on Video APP and Youtube. It can work. 
But the case in comment 55 failed to work. 
Continue to check why...

Comment 58

[Blake Wu [:bwu][:blakewu]]

Comment on attachment 8551596 [details] [diff] [review]
Master-Bug-1113527-Make-setDormant-done-in-a-MediaTaskQueue-v4.patch

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

For comment 55 failure, 
It will call SetDormantIfNecessary with aDormant=true in the very beginning which will run through our dormant codes and make the state as DORMANT. That makes playback stop.

::: dom/media/MediaDecoder.cpp
@@ +124,5 @@
>    ReentrantMonitorAutoEnter mon(GetReentrantMonitor());
>  
>    if (!mDecoderStateMachine ||
> +      !mOwner->GetVideoFrameContainer() ||
> +      !mDecoderStateMachine->IsDormantCapable() ||

This is the reason that playback cannot be played. Previously we call isDormantNeeded here and will return here. So no dormant tasks will be done.

Comment 59

[Blake Wu [:bwu][:blakewu]]

Attachment: Master-Bug-1113527-Make-setDormant-done-in-a-MediaTaskQueue-v5.patch

Have changes for comment 52 and one more check in MediaDecoder to fix the problem mentioned in comment 55.

I have tested this patch on Video APP, Youtube, and the case in comment 55. All (play and resume from dormant state, switch between them) can work well.

Comment 60

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

Comment on attachment 8552333 [details] [diff] [review]
Master-Bug-1113527-Make-setDormant-done-in-a-MediaTaskQueue-v5.patch

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

This looks fine, but I still I don't think we need both MediaDecoderStateMachine::IsDormantNeeded() and IsDormantCapable(). We should reduce the size of our classes/interfaces where possible.

::: dom/media/MediaDecoderStateMachine.cpp
@@ +3586,5 @@
> +  if (mState == DECODER_STATE_SHUTDOWN) {
> +    return NS_OK;
> +  }
> +  nsresult rv = DecodeTaskQueue()->Dispatch(
> +    NS_NewRunnableMethod(this, &MediaDecoderStateMachine::SetDormantTask));

nsresult rv = DecodeTaskQueue()->Dispatch(	
  NS_NewRunnableMethod(mReader, &MediaDecoderReader::ReleaseMediaResources));

Then you don't need the IsDormantNeeded() check (it can be done inside the reader, and you don't need SetDormantTask().

@@ +3594,5 @@
> +
> +void
> +MediaDecoderStateMachine::SetDormantTask() {
> +  NS_ASSERTION(OnDecodeThread(), "Should be on decode thread.");
> +  if (!IsDormantNeeded()) {

This is the only place where IsDormantNeeded() is called right? You should remove it, and just call mReader->ReleaseMediaResources(); the Reader's ReleaseMediaResources() implementation can check whether it actually needs to become dormant. i.e.: MediaOmxReader::ReleaseMediaResources() should check whether it has a decoder that must become dormant.

Comment 61

[Blake Wu [:bwu][:blakewu]]

(In reply to Chris Pearce (:cpearce) from comment #60)
> Comment on attachment 8552333 [details] [diff] [review]
> Master-Bug-1113527-Make-setDormant-done-in-a-MediaTaskQueue-v5.patch
> 
> Review of attachment 8552333 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fine, but I still I don't think we need both
> MediaDecoderStateMachine::IsDormantNeeded() and IsDormantCapable(). We
> should reduce the size of our classes/interfaces where possible.
Cannot agree you any more! 
> 
> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +3586,5 @@
> > +  if (mState == DECODER_STATE_SHUTDOWN) {
> > +    return NS_OK;
> > +  }
> > +  nsresult rv = DecodeTaskQueue()->Dispatch(
> > +    NS_NewRunnableMethod(this, &MediaDecoderStateMachine::SetDormantTask));
> 
> nsresult rv = DecodeTaskQueue()->Dispatch(	
>   NS_NewRunnableMethod(mReader, &MediaDecoderReader::ReleaseMediaResources));
> 
> Then you don't need the IsDormantNeeded() check (it can be done inside the
> reader, and you don't need SetDormantTask().
> 
> @@ +3594,5 @@
> > +
> > +void
> > +MediaDecoderStateMachine::SetDormantTask() {
> > +  NS_ASSERTION(OnDecodeThread(), "Should be on decode thread.");
> > +  if (!IsDormantNeeded()) {
> 
> This is the only place where IsDormantNeeded() is called right? You should
> remove it, and just call mReader->ReleaseMediaResources(); the Reader's
> ReleaseMediaResources() implementation can check whether it actually needs
> to become dormant. i.e.: MediaOmxReader::ReleaseMediaResources() should
> check whether it has a decoder that must become dormant.
Thanks! It looks an awesome idea. Let me double check it.

Comment 62

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8552333 [details] [diff] [review]
Master-Bug-1113527-Make-setDormant-done-in-a-MediaTaskQueue-v5.patch

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

::: dom/media/MediaDecoder.cpp
@@ +130,5 @@
>      return;
>    }
>  
> +  // If mPlayState is at initial state, no need to set dormant
> +  if(aDormant && mPlayState != PLAY_STATE_LOADING) {

I do not like to add this check. It is not clear why it is necessary from logical point of view. This check could add timing related regression problem.

::: dom/media/MediaDecoderStateMachine.cpp
@@ +3595,5 @@
> +void
> +MediaDecoderStateMachine::SetDormantTask() {
> +  NS_ASSERTION(OnDecodeThread(), "Should be on decode thread.");
> +  if (!IsDormantNeeded()) {
> +    return;

I do not think the above IsDormantNeeded() is necessary.
We can check dormant necessity in MediaDecoder::SetDormantIfNecessary(). Bug 1122228 might help to understand it.

Comment 63

[Blake Wu [:bwu][:blakewu]]

Thanks for your review! 
(In reply to Sotaro Ikeda [:sotaro] from comment #62)
> Comment on attachment 8552333 [details] [diff] [review]
> Master-Bug-1113527-Make-setDormant-done-in-a-MediaTaskQueue-v5.patch
> 
> Review of attachment 8552333 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaDecoder.cpp
> @@ +130,5 @@
> >      return;
> >    }
> >  
> > +  // If mPlayState is at initial state, no need to set dormant
> > +  if(aDormant && mPlayState != PLAY_STATE_LOADING) {
> 
> I do not like to add this check. It is not clear why it is necessary from
> logical point of view. This check could add timing related regression
> problem.
The logic for this additional check is to 
1. Avoid doing unnecessary dormant things if MediaDecoder is already at an very initial state (PLAY_STATE_LOADING). 
2. Avoid setting dormant again if it is already at dormant state. Since aDormant is true, we set the mPlayState to PLAY_STATE_LOADING[1] as well.  

[1]https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoder.cpp?from=MediaDecoder.cpp&case=true#141
But the patch in bug 1122228 might fix this problem. I will check it further. 
> 
> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +3595,5 @@
> > +void
> > +MediaDecoderStateMachine::SetDormantTask() {
> > +  NS_ASSERTION(OnDecodeThread(), "Should be on decode thread.");
> > +  if (!IsDormantNeeded()) {
> > +    return;
> 
> I do not think the above IsDormantNeeded() is necessary.
> We can check dormant necessity in MediaDecoder::SetDormantIfNecessary(). Bug
> 1122228 might help to understand it.

IsDormantNeeded() is to check if there is video track/codec created or not. 
And the dormant necessity (mIsDormant?) in bug 1122228 is to check if the doc is active, video , and not hidden. They should be different. IIUC, the dormant necessity is to decide which situation we should set dormant true or false. But IsDormantNeeded is to check the decoder pipeline if there is any media resource created and we need to release them when the decision to set dormant (mIsDormant) is true.

Comment 64

[Sotaro Ikeda [:sotaro]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #63)
> Thanks for your review! 
> (In reply to Sotaro Ikeda [:sotaro] from comment #62)
> > Comment on attachment 8552333 [details] [diff] [review]
> > Master-Bug-1113527-Make-setDormant-done-in-a-MediaTaskQueue-v5.patch
> > 
> > Review of attachment 8552333 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/media/MediaDecoder.cpp
> > @@ +130,5 @@
> > >      return;
> > >    }
> > >  
> > > +  // If mPlayState is at initial state, no need to set dormant
> > > +  if(aDormant && mPlayState != PLAY_STATE_LOADING) {
> > 
> > I do not like to add this check. It is not clear why it is necessary from
> > logical point of view. This check could add timing related regression
> > problem.
> The logic for this additional check is to 
> 1. Avoid doing unnecessary dormant things if MediaDecoder is already at an
> very initial state (PLAY_STATE_LOADING). 
> 2. Avoid setting dormant again if it is already at dormant state. Since
> aDormant is true, we set the mPlayState to PLAY_STATE_LOADING[1] as well.  
> 

I do not understand well your comment. My concern about it is that It seems not handle well about the following case.
- MediaDecoder is in PLAY_STATE_LOADING and not in dormant. But MediaDecoderStateMachine is in the middle of loading resources.

I am going to confirm by myself about why attachment 8551596 [details] [diff] [review] does not work correctly.

Comment 65

[Sotaro Ikeda [:sotaro]]

I checked attachment 8551596 [details] [diff] [review] on flame. I confirmed that HTMLMediaElement side said to MediaDecoder that MediaDecoder is Hide state during the STR in Comment 55.

Therefore it is a HTMLMediaElement's problem. It is not a problem of MediaDecoder or MediaDecoderStateMachine. Therefore it should be fixed at TMLMediaElement, not at MediaDecoder.

Comment 66

[Sotaro Ikeda [:sotaro]]

From the investigation, the following things becomes clear.
- VideoDocument creates HTMLMediaElement during VideoDocument's load.
  VideoDocument::CreateSyntheticVideoDocument() is called in VideoDocument::StartDocumentLoad()
  https://dxr.mozilla.org/mozilla-central/source/dom/html/VideoDocument.cpp#84

- When document's visibility state is updated by nsDocument::SetScriptGlobalObject(), HTMLVideoElement::NotifyOwnerDocumentActivityChanged() is not called.
  From the following, it could happen at "coming out of bfcache" and "initial document load".
  https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#4711

Comment 67

[Sotaro Ikeda [:sotaro]]

From, comment 66, it seems to become a problem on only b2g && VideoDocument.

Comment 68

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #67)
> From, comment 66, it seems to become a problem on only b2g && VideoDocument.

Bug 1123452 seem not get effect from this problem.

Comment 69

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #65)
> I checked attachment 8551596 [details] [diff] [review] on flame. I confirmed
> that HTMLMediaElement side said to MediaDecoder that MediaDecoder is Hide
> state during the STR in Comment 55.
> 
> Therefore it is a HTMLMediaElement's problem. It is not a problem of
> MediaDecoder or MediaDecoderStateMachine. Therefore it should be fixed at
> TMLMediaElement, not at MediaDecoder.

Bug 1124844 is created for "MediaDecoder::NotifyOwnerActivityChanged() is not called" problem.

Comment 70

[Sotaro Ikeda [:sotaro]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #63)
> > 
> > I do not think the above IsDormantNeeded() is necessary.
> > We can check dormant necessity in MediaDecoder::SetDormantIfNecessary(). Bug
> > 1122228 might help to understand it.
> 
> IsDormantNeeded() is to check if there is video track/codec created or not. 
> And the dormant necessity (mIsDormant?) in bug 1122228 is to check if the
> doc is active, video , and not hidden. They should be different. IIUC, the
> dormant necessity is to decide which situation we should set dormant true or
> false. But IsDormantNeeded is to check the decoder pipeline if there is any
> media resource created and we need to release them when the decision to set
> dormant (mIsDormant) is true.

I still do not understand why it is necessary in current implementation. We could live without it on current gecko.

Comment 71

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #70)
> (In reply to Blake Wu [:bwu][:blakewu] from comment #63)
> > > 
> > > I do not think the above IsDormantNeeded() is necessary.
> > > We can check dormant necessity in MediaDecoder::SetDormantIfNecessary(). Bug
> > > 1122228 might help to understand it.
> > 
> > IsDormantNeeded() is to check if there is video track/codec created or not. 
> > And the dormant necessity (mIsDormant?) in bug 1122228 is to check if the
> > doc is active, video , and not hidden. They should be different. IIUC, the
> > dormant necessity is to decide which situation we should set dormant true or
> > false. But IsDormantNeeded is to check the decoder pipeline if there is any
> > media resource created and we need to release them when the decision to set
> > dormant (mIsDormant) is true.
> 
> I still do not understand why it is necessary in current implementation. We
> could live without it on current gecko.

This is not a main problem of this bug. This could be put off.

Comment 72

[Blake Wu [:bwu][:blakewu]]

OK. Thanks for your help to create Bug 1124844

(In reply to Sotaro Ikeda [:sotaro] from comment #69)
> (In reply to Sotaro Ikeda [:sotaro] from comment #65)
> > I checked attachment 8551596 [details] [diff] [review] on flame. I confirmed
> > that HTMLMediaElement side said to MediaDecoder that MediaDecoder is Hide
> > state during the STR in Comment 55.
> > 
> > Therefore it is a HTMLMediaElement's problem. It is not a problem of
> > MediaDecoder or MediaDecoderStateMachine. Therefore it should be fixed at
> > TMLMediaElement, not at MediaDecoder.
> 
> Bug 1124844 is created for "MediaDecoder::NotifyOwnerActivityChanged() is
> not called" problem.
Thanks for your help to create and fix this bug!

Comment 73

[Blake Wu [:bwu][:blakewu]]

(In reply to Sotaro Ikeda [:sotaro] from comment #71)
> (In reply to Sotaro Ikeda [:sotaro] from comment #70)
> > (In reply to Blake Wu [:bwu][:blakewu] from comment #63)
> > > > 
> > > > I do not think the above IsDormantNeeded() is necessary.
> > > > We can check dormant necessity in MediaDecoder::SetDormantIfNecessary(). Bug
> > > > 1122228 might help to understand it.
> > > 
> > > IsDormantNeeded() is to check if there is video track/codec created or not. 
> > > And the dormant necessity (mIsDormant?) in bug 1122228 is to check if the
> > > doc is active, video , and not hidden. They should be different. IIUC, the
> > > dormant necessity is to decide which situation we should set dormant true or
> > > false. But IsDormantNeeded is to check the decoder pipeline if there is any
> > > media resource created and we need to release them when the decision to set
> > > dormant (mIsDormant) is true.
> > 
> > I still do not understand why it is necessary in current implementation. We
> > could live without it on current gecko.
> 
> This is not a main problem of this bug. This could be put off.
Agree. I will remove it and if it is really a problem in the future we can create a bug then.

Comment 74

[Blake Wu [:bwu][:blakewu]]

Attachment: Bug-1113527-Make-setDormant-done-in-a-MediaTaskQueue-v6.patch

This patch is based on v4(attachment 8551596 [details] [diff] [review]) to add changes  
1. Remove IsDormantNeeded() from MediaDecoderStateMachine (Comment 60 and 62)
2. Change the name of EnqueueSetDormantTask to EnqueueReleaseMediaResource. 

For the problem in comment 55, it is fixed in bug 1124844. Thanks for Sotaro's help.

Comment 75

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

Comment on attachment 8553650 [details] [diff] [review]
Bug-1113527-Make-setDormant-done-in-a-MediaTaskQueue-v6.patch

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +1489,4 @@
>    mFragmentEndTime = aEndTime < 0 ? aEndTime : aEndTime + mStartTime;
>  }
>  
> +bool MediaDecoderStateMachine::IsDormantCapable()

Don't add IsDormantCapable(). Change IsDormantNeeded() to return true or false. Check inside MediaDecoderReader::ReleaseMediaResources() whether dormant is needed.

@@ +3586,5 @@
>    }
>  }
>  
> +nsresult
> +MediaDecoderStateMachine::EnqueueReleaseMediaResource()

Don't add MediaDecoderStateMachine::EnqueueReleaseMediaResource(). Please rebase your patch on my patch 1 in bug 1123535, which calls MediaDecoderReader::ReleaseMediaResources on the decode task queue.

Comment 76

[Blake Wu [:bwu][:blakewu]]

(In reply to Chris Pearce (:cpearce) from comment #75)
> Comment on attachment 8553650 [details] [diff] [review]
> Bug-1113527-Make-setDormant-done-in-a-MediaTaskQueue-v6.patch
> 
> Review of attachment 8553650 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +1489,4 @@
> >    mFragmentEndTime = aEndTime < 0 ? aEndTime : aEndTime + mStartTime;
> >  }
> >  
> > +bool MediaDecoderStateMachine::IsDormantCapable()
> 
> Don't add IsDormantCapable(). Change IsDormantNeeded() to return true or
> false. Check inside MediaDecoderReader::ReleaseMediaResources() whether
> dormant is needed.
> 
> @@ +3586,5 @@
> >    }
> >  }
> >  
> > +nsresult
> > +MediaDecoderStateMachine::EnqueueReleaseMediaResource()
> 
> Don't add MediaDecoderStateMachine::EnqueueReleaseMediaResource(). Please
> rebase your patch on my patch 1 in bug 1123535, which calls
> MediaDecoderReader::ReleaseMediaResources on the decode task queue.

Agree. That would make code simpler. 
I will modify it. Thanks!

Comment 77

[Blake Wu [:bwu][:blakewu]]

Attachment: Master-Bug-1113527-Make-setDormant-done-in-a-MediaTaskQueue-v6.patch

Per comment 75.

Comment 78

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8554347 [details] [diff] [review]
Master-Bug-1113527-Make-setDormant-done-in-a-MediaTaskQueue-v6.patch

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

::: dom/media/MediaDecoder.cpp
@@ +128,1 @@
>        !mDecoderStateMachine->IsDormantNeeded() ||

Why is this part is reverted? Is the above needed to be?

> !mDecoderStateMachine->IsDormantCapable() ||

Comment 79

[Blake Wu [:bwu][:blakewu]]

(In reply to Sotaro Ikeda [:sotaro] from comment #78)
> Comment on attachment 8554347 [details] [diff] [review]
> Master-Bug-1113527-Make-setDormant-done-in-a-MediaTaskQueue-v6.patch
> 
> Review of attachment 8554347 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaDecoder.cpp
> @@ +128,1 @@
> >        !mDecoderStateMachine->IsDormantNeeded() ||
> 
> Why is this part is reverted? Is the above needed to be?
> 
> > !mDecoderStateMachine->IsDormantCapable() ||
To make codes simpler as cpearce suggested in comment 75, I remove IsDormantCapable() from my previous patch and change the original IsDormantNeeded() to be what IsDormantCapable() does to avoid the timing issue.
And what IsDormantNeeded() does is moved to inside ReleaseMediaResources in each reader.

Comment 80

[Blake Wu [:bwu][:blakewu]]

Comment on attachment 8554347 [details] [diff] [review]
Master-Bug-1113527-Make-setDormant-done-in-a-MediaTaskQueue-v6.patch

Make this patch obsolete since the patch, attachment 8554398 [details] [diff] [review], in bug 1123535 fixes the problem that ReleaseMediaResources() is not handled in the MediaTaskQueue.

Comment 81

[Blake Wu [:bwu][:blakewu]]

Attachment: Bug-1113527-Change-isDormantNeeded-behavior.patch

Change the current behavior of IsDormantNeeded() to avoid the timing issue which was discussed in comment 13 and comment 75.

Comment 82

[Blake Wu [:bwu][:blakewu]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #81)
> Created attachment 8555056 [details] [diff] [review]
> Bug-1113527-Change-isDormantNeeded-behavior.patch
> 
> Change the current behavior of IsDormantNeeded() to avoid the timing issue
> which was discussed in comment 13 and comment 75.

This patch is based on the patch, attachment 8554398 [details] [diff] [review], in bug 1123535.

Comment 83

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8555056 [details] [diff] [review]
Bug-1113527-Change-isDormantNeeded-behavior.patch

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

::: dom/media/omx/MediaCodecReader.cpp
@@ +330,5 @@
>  MediaCodecReader::ReleaseMediaResources()
>  {
> +  if (mVideoTrack.mSource == nullptr) {
> +    return;
> +  }

Why is it check the video track here? It seems to cause normal shut down sequence from MediaDecoderReader::Shutdown().

Comment 84

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #83)
> -----------------------------------------------------------------
> 
> ::: dom/media/omx/MediaCodecReader.cpp
> @@ +330,5 @@
> >  MediaCodecReader::ReleaseMediaResources()
> >  {
> > +  if (mVideoTrack.mSource == nullptr) {
> > +    return;
> > +  }
> 
> Why is it check the video track here? It seems to cause normal shut down
> sequence from MediaDecoderReader::Shutdown().

Correction:
It seems to cause a problem to normal shut down sequence from MediaDecoderReader::Shutdown().

Comment 85

[Sotaro Ikeda [:sotaro]]

By the way, MediaOmxReader::ReleaseMediaResources() releases all decoders including audio decoder.

Comment 86

[Blake Wu [:bwu][:blakewu]]

Yeah. I should remove those additional checks which are from original isDormantNeeded(). Current ReleaseMediaResources() has sufficient checks and no need to add more.

Comment 87

[Blake Wu [:bwu][:blakewu]]

Attachment: Bug-1113527-Change-isDormantNeeded-behavior-v2.patch

Based on attachment 8555056 [details] [diff] [review], remove unnecessary additional checks and release audio decoder in MP4Reader::ReleaseMediaResources.

Comment 88

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8555648 [details] [diff] [review]
Bug-1113527-Change-isDormantNeeded-behavior-v2.patch

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

Looks good. This patch might be affected by Bug 1123452.

Comment 89

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

Comment on attachment 8555648 [details] [diff] [review]
Bug-1113527-Change-isDormantNeeded-behavior-v2.patch

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

Looking much better, but still two minor issues.

::: dom/media/fmp4/MP4Reader.cpp
@@ -1002,4 @@
>  bool MP4Reader::IsDormantNeeded()
>  {
>  #ifdef MOZ_GONK_MEDIACODEC
> -  return mVideo.mDecoder && mVideo.mDecoder->IsDormantNeeded();

Please don't make this change. I use this in my patches in bug 1123535. We'll need it until all PDMs support going dormant.

::: dom/media/omx/OmxDecoder.cpp
@@ +244,4 @@
>  
>  bool OmxDecoder::AllocateMediaResources()
>  {
> +  if ((mVideoTrack != nullptr) && mDecoder->GetImageContainer() &&

It seems to me it would be better to instead check if there's an image container in OmxDecoder::Init(), and not create a mVideoTrack in Init() if there's no image container.

Comment 90

[Blake Wu [:bwu][:blakewu]]

(In reply to Chris Pearce (:cpearce) from comment #89)
> Comment on attachment 8555648 [details] [diff] [review]
> Bug-1113527-Change-isDormantNeeded-behavior-v2.patch
> 
> Review of attachment 8555648 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking much better, but still two minor issues.
> 
> ::: dom/media/fmp4/MP4Reader.cpp
> @@ -1002,4 @@
> >  bool MP4Reader::IsDormantNeeded()
> >  {
> >  #ifdef MOZ_GONK_MEDIACODEC
> > -  return mVideo.mDecoder && mVideo.mDecoder->IsDormantNeeded();
> 
> Please don't make this change. I use this in my patches in bug 1123535.
> We'll need it until all PDMs support going dormant.
But this change will be required for B2G local playback with MP4Reader. Since I only change it within the define MOZ_GONK_MEDIACODEC. You can modify it once all PDMs support going dormant is done.  
> 
> ::: dom/media/omx/OmxDecoder.cpp
> @@ +244,4 @@
> >  
> >  bool OmxDecoder::AllocateMediaResources()
> >  {
> > +  if ((mVideoTrack != nullptr) && mDecoder->GetImageContainer() &&
> 
> It seems to me it would be better to instead check if there's an image
> container in OmxDecoder::Init(), and not create a mVideoTrack in Init() if
> there's no image container.
Yeah, that would be better.

Comment 91

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

(In reply to Blake Wu [:bwu][:blakewu] from comment #90)
> (In reply to Chris Pearce (:cpearce) from comment #89)
> > Comment on attachment 8555648 [details] [diff] [review]
> > Bug-1113527-Change-isDormantNeeded-behavior-v2.patch
> > 
> > Review of attachment 8555648 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looking much better, but still two minor issues.
> > 
> > ::: dom/media/fmp4/MP4Reader.cpp
> > @@ -1002,4 @@
> > >  bool MP4Reader::IsDormantNeeded()
> > >  {
> > >  #ifdef MOZ_GONK_MEDIACODEC
> > > -  return mVideo.mDecoder && mVideo.mDecoder->IsDormantNeeded();
> > 
> > Please don't make this change. I use this in my patches in bug 1123535.
> > We'll need it until all PDMs support going dormant.
> But this change will be required for B2G local playback with MP4Reader.
> Since I only change it within the define MOZ_GONK_MEDIACODEC. You can modify
> it once all PDMs support going dormant is done.  

Make GonkMediaDataDecoder::IsDormantNeeded() return true.

Comment 92

[Blake Wu [:bwu][:blakewu]]

Attachment: Bug-1113527-Change-isDormantNeeded-behavior-v3.patch

Changes per comment 89 and 91.

Comment 93

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

Comment on attachment 8557048 [details] [diff] [review]
Bug-1113527-Change-isDormantNeeded-behavior-v3.patch

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

Looks good!

Comment 94

[Blake Wu [:bwu][:blakewu]]

sotaro and cpearce, 
Thanks for your reviews!

Comment 95

[Blake Wu [:bwu][:blakewu]]

Attachment: Bug-1113527-Change-isDormantNeeded-behavior-hg.patch

Rebase attachment 8557048 [details] [diff] [review] to the latest code base and carry r+ from sotaro and cpearce.

Comment 96

[Blake Wu [:bwu][:blakewu]]

TryServer: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=590f066c032b

Comment 97

[Blake Wu [:bwu][:blakewu]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #96)
> TryServer: 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=590f066c032b
It looks red ones could be fixed by attachment 8558256 [details] [diff] [review] in bug 1123535. 
Wait it landed and retry again.

Comment 98

[Blake Wu [:bwu][:blakewu]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #97)
> (In reply to Blake Wu [:bwu][:blakewu] from comment #96)
> > TryServer: 
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=590f066c032b
> It looks red ones could be fixed by attachment 8558256 [details] [diff] [review]
> [review] in bug 1123535. 
> Wait it landed and retry again.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=998c0f90e1e5
Test results looks good with the patch in Bug 1123535.

Comment 99

[Blake Wu [:bwu][:blakewu]]

Please only check-in Bug-1113527-Change-isDormantNeeded-behavior-hg.patch. 
Thanks!

Comment 100

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2edd2f2ff1d9

Comment 101

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2edd2f2ff1d9

Comment 102

[Blake Wu [:bwu][:blakewu]]

I found the patches in the dependent bugs are uplifted to b2g37. The patch in this bug should be required as well.  
Besides bug 1098298 can also benefit from it.

Comment 103

[bhavana bajaj [:bajaj]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #102)
> I found the patches in the dependent bugs are uplifted to b2g37. The patch
> in this bug should be required as well.  
> Besides bug 1098298 can also benefit from it.

please request approval for 2.2.

Comment 104

[Blake Wu [:bwu][:blakewu]]

Attachment: b2g37-Bug-1113527-Change-isDormantNeeded-behavior-hg.patch

Rebase attachment 8557712 [details] [diff] [review] to v2.2 branch.

Comment 105

[Blake Wu [:bwu][:blakewu]]

Comment on attachment 8557712 [details] [diff] [review]
Bug-1113527-Change-isDormantNeeded-behavior-hg.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Bug 1050031 (Shut down dormant video/audio decoders)
User impact if declined: 
Testing completed: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6a02c121554
Risk to taking this patch (and alternatives if risky): 
Low.
String or UUID changes made by this patch:
NA

Comment 106

[Blake Wu [:bwu][:blakewu]]

Comment on attachment 8569566 [details] [diff] [review]
b2g37-Bug-1113527-Change-isDormantNeeded-behavior-hg.patch

Sorry to set the wrong patch to request approval for 2.2 in comment 105.
Re-request the right patch as below.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Bug 1050031 (Shut down dormant video/audio decoders)
User impact if declined: 
Testing completed: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6a02c121554
Risk to taking this patch (and alternatives if risky): 
Low.
String or UUID changes made by this patch:
NA

Comment 107

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/961f0cec70da