1062134 - Avoid unnecessary decoding when back from dormant state.

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Benjamin Chen [:bechen]]

Fork from Bug 1058429 comment 0.

Assume one stream's start time won't change after suspend/resume.
So, when the statemachine back from dormant state, we should not update mStartTime.

Comment 1

[Benjamin Chen [:bechen]]

Attachment: bug-1062134.v01.patch

Comment 2

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

Comment on attachment 8489848 [details] [diff] [review]
bug-1062134.v01.patch

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

::: content/media/MediaDecoderStateMachine.cpp
@@ +1965,5 @@
>      SetStartTime(0);
>      res = FinishDecodeMetadata();
>      NS_ENSURE_SUCCESS(res, res);
> +  } else if (mStartTime != -1) {
> +    SetStartTime(mStartTime);

Add a comment explaining *why* this code is doing what it's doing.

Comment 3

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

Comment on attachment 8489848 [details] [diff] [review]
bug-1062134.v01.patch

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

::: content/media/omx/RtspMediaCodecReader.cpp
@@ +105,5 @@
>  void
>  RtspMediaCodecReader::codecReserved(Track& aTrack)
>  {
>    // TODO: fix me, we need a SeekTime(0) here because the
>    // MediaDecoderStateMachine will update the mStartTime after ReadMetadata.

This comment is no longer valid and needs to be removed?

@@ +108,5 @@
>    // TODO: fix me, we need a SeekTime(0) here because the
>    // MediaDecoderStateMachine will update the mStartTime after ReadMetadata.
>    MediaCodecReader::codecReserved(aTrack);
>    if (aTrack.mCodec != nullptr) {
> +    EnsureActive();

Why do we need EnsureActive() here? It should be called in MediaCodecReader when seeking or decoding and RtspMediaCodecReader won't have to overwrite Request{Audio,Video}Data in order just to call EnsureActive() which should be called in MediaCodecReader::Request{Audio,Video}Data.

Comment 4

[Benjamin Chen [:bechen]]

(In reply to JW Wang [:jwwang] from comment #3)
> Comment on attachment 8489848 [details] [diff] [review]
> bug-1062134.v01.patch
> 
> Review of attachment 8489848 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/omx/RtspMediaCodecReader.cpp
> @@ +105,5 @@
> >  void
> >  RtspMediaCodecReader::codecReserved(Track& aTrack)
> >  {
> >    // TODO: fix me, we need a SeekTime(0) here because the
> >    // MediaDecoderStateMachine will update the mStartTime after ReadMetadata.
> 
> This comment is no longer valid and needs to be removed?
> 
> @@ +108,5 @@
> >    // TODO: fix me, we need a SeekTime(0) here because the
> >    // MediaDecoderStateMachine will update the mStartTime after ReadMetadata.
> >    MediaCodecReader::codecReserved(aTrack);
> >    if (aTrack.mCodec != nullptr) {
> > +    EnsureActive();
> 
> Why do we need EnsureActive() here? It should be called in MediaCodecReader
> when seeking or decoding and RtspMediaCodecReader won't have to overwrite
> Request{Audio,Video}Data in order just to call EnsureActive() which should
> be called in MediaCodecReader::Request{Audio,Video}Data.

Yes, seems that we don't need EnsureActive() here, and I can remove RtspMediaCodecReader::codecReserved function.

Comment 5

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

Tip: click the "[review]" button to add comments. It is easier to trace all review comments.

Comment 6

[Benjamin Chen [:bechen]]

Attachment: bug-1062134.v01.patch

r=cpearce
try server:
https://tbpl.mozilla.org/?tree=Try&rev=668aa69f6916

Comment 7

[Benjamin Chen [:bechen]]

https://tbpl.mozilla.org/?tree=Try&rev=19072e634dec

Looks like my patch breaks some testcases about texture..

Comment 8

[Benjamin Chen [:bechen]]

My patch breaks the testcase because the WebMReader will set mStartTime to zero by MediaDecoderStateMachine::SetDuration function. And then skip the "RenderVideoFrame" in MediaDecoderStateMachine::FinishDecodeMetadata(), then update the readyState to fire the "playing" dom event to JS. Once the JS receive the "playing" event, it will get the video frame container to check the RGB value. We skip the "RenderVideoFrame" so the JS will get nothing cause the testcase failure.

Comment 9

[Benjamin Chen [:bechen]]

Attachment: bug-1062134.v02.patch

Freeze decoding when exit dormant and resume decoding when seeking.

try server:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a2397e117657

Comment 10

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

Comment on attachment 8506626 [details] [diff] [review]
bug-1062134.v02.patch

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

::: content/html/content/src/HTMLMediaElement.cpp
@@ +2226,5 @@
>        DispatchAsyncEvent(NS_LITERAL_STRING("waiting"));
>        break;
>      case nsIDOMHTMLMediaElement::HAVE_FUTURE_DATA:
>      case nsIDOMHTMLMediaElement::HAVE_ENOUGH_DATA:
> +      printf("\ngggggg1 \n");

You would like to remove this.

::: content/media/MediaDecoderStateMachine.h
@@ +346,5 @@
>    void OnDecodeError();
>  
> +  // Set/clear mBackFromDormantFreezeDecoding flag.
> +  void SetBackFromDormantFreezeDecoding();
> +  void ClearBackFromDormantFreezeDecoding();

Can the logic be embedded into MediaDecoderStateMachine::SetDormant() without adding 2 functions?

Comment 11

[Benjamin Chen [:bechen]]

Comment on attachment 8506626 [details] [diff] [review]
bug-1062134.v02.patch

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

::: content/media/MediaDecoderStateMachine.h
@@ +346,5 @@
>    void OnDecodeError();
>  
> +  // Set/clear mBackFromDormantFreezeDecoding flag.
> +  void SetBackFromDormantFreezeDecoding();
> +  void ClearBackFromDormantFreezeDecoding();

Yes, the logic can all be done in state machine, the approach is the same with attachment 8483227 [details] [diff] [review].
See bug 1058429 comment 5.

Comment 12

[Benjamin Chen [:bechen]]

Comment on attachment 8506626 [details] [diff] [review]
bug-1062134.v02.patch

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

::: content/html/content/src/HTMLMediaElement.cpp
@@ +2226,5 @@
>        DispatchAsyncEvent(NS_LITERAL_STRING("waiting"));
>        break;
>      case nsIDOMHTMLMediaElement::HAVE_FUTURE_DATA:
>      case nsIDOMHTMLMediaElement::HAVE_ENOUGH_DATA:
> +      printf("\ngggggg1 \n");

Got it.

Comment 13

[Benjamin Chen [:bechen]]

Attachment: bug-1062134.v03.patch

Comment 14

[Russ Nicoletti [:russn]]

[Blocking Requested - why for this release]: Bug 1058429, which blocks bug 1082563, depends on this bug.

Comment 15

[Andrew Overholt [:overholt]]

This blocks a 2.1+ bug so I'm marking it 2.1+ as well.

Comment 16

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

Comment on attachment 8507736 [details] [diff] [review]
bug-1062134.v03.patch

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

::: content/media/MediaDecoder.cpp
@@ +150,5 @@
>      mIsDormant = true;
>      mIsExitingDormant = false;
>      ChangeState(PLAY_STATE_LOADING);
> +    // Freeze decoding because we will trigger a seek later.
> +    mDecoderStateMachine->SetFreezeDecodingAtStateDecoding(true);

Since mDecoderStateMachine->SetFreezeDecodingAtStateDecoding(true) always goes with mDecoderStateMachine->SetDormant(true), we should have the state machine figure out what to do with dormant state without intervention of media decoder.

@@ +155,5 @@
>    } else if (!aDormant && mPlayState == PLAY_STATE_LOADING) {
>      // exit dormant state
>      // trigger to state machine.
> +    // Freeze decoding because we don't need to SetStartTime again.
> +    mDecoderStateMachine->SetFreezeDecodingAtStateMetadata(true);

Ditto.

@@ +1144,5 @@
>    }
>  
>    ApplyStateToStateMachine(mPlayState);
>  
> +  if (aState != PLAY_STATE_LOADING) {

Ditto. These flags can be reset in MediaDecoderStateMachine::Seek or MediaDecoderStateMachine::Play without intervention of media decoder.

Comment 17

[Benjamin Chen [:bechen]]

Attachment: bug-1062134.v03.patch

Comment 18

[Benjamin Chen [:bechen]]

Attachment: bug-1062134.v04.patch

Comment 19

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

Comment on attachment 8509276 [details] [diff] [review]
bug-1062134.v04.patch

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

::: content/media/MediaDecoderStateMachine.cpp
@@ +1517,5 @@
>  {
>    NS_ASSERTION(NS_IsMainThread(), "Should be on main thread.");
>    ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
>  
> +  mFreezeDecodingAtStateDecoding = false;

You should also reset this flag in Play() to reduce coupling with MediaDecoder which might change the seek-after-wake-up-from-dormant policy one day.

::: content/media/MediaDecoderStateMachine.h
@@ +929,5 @@
> +  // mFreezeDecodingAtStateMetadata: turn on/off at
> +  //                                 SetDormant/FinishDecodeMetadata.
> +  // mFreezeDecodingAtStateDecoding: turn on/off at
> +  //                                 SetDormant/Seek.
> +  bool mFreezeDecodingAtStateMetadata;

How about the name "mSkipDecodeFirstFrame"?

@@ +930,5 @@
> +  //                                 SetDormant/FinishDecodeMetadata.
> +  // mFreezeDecodingAtStateDecoding: turn on/off at
> +  //                                 SetDormant/Seek.
> +  bool mFreezeDecodingAtStateMetadata;
> +  bool mFreezeDecodingAtStateDecoding;

How about mDecodingFrozen?

Comment 20

[Benjamin Chen [:bechen]]

(In reply to JW Wang [:jwwang] from comment #19)
> Comment on attachment 8509276 [details] [diff] [review]
> bug-1062134.v04.patch
> 
> Review of attachment 8509276 [details] [diff] [review]:
> -----------------------------------------------------------------
>  
> ::: content/media/MediaDecoderStateMachine.h
> @@ +929,5 @@
> > +  // mFreezeDecodingAtStateMetadata: turn on/off at
> > +  //                                 SetDormant/FinishDecodeMetadata.
> > +  // mFreezeDecodingAtStateDecoding: turn on/off at
> > +  //                                 SetDormant/Seek.
> > +  bool mFreezeDecodingAtStateMetadata;
> 
> How about the name "mSkipDecodeFirstFrame"?
>

It is hard to naming the flag here because at Metadata state, the state machine call request(Video/audio) directly, not specify the "first frame". The decoded frame here is the first frame because the extractor rewind the sampletable implicitly.

> @@ +930,5 @@
> > +  //                                 SetDormant/FinishDecodeMetadata.
> > +  // mFreezeDecodingAtStateDecoding: turn on/off at
> > +  //                                 SetDormant/Seek.
> > +  bool mFreezeDecodingAtStateMetadata;
> > +  bool mFreezeDecodingAtStateDecoding;
> 
> How about mDecodingFrozen?

I do the check |mState == DECODER_STATE_DECODING && mFreezeDecodingAtStateDecoding| in my patch, so
I'd prefer to keep the state information in the flag name.

Comment 22

[Benjamin Chen [:bechen]]

Attachment: bug-1062134.v04.patch

1. Rename:
mFreezeDecodingAtStateMetadata => mDecodingFrozenAtStateMetadata
mFreezeDecodingAtStateDecoding => mDecodingFrozenAtStateDecoding

2. Reset mDecodingFrozenAtStateDecoding at Play().

Comment 23

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/95e70cc80f82

Comment 24

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

(In reply to Chris Pearce (:cpearce) from comment #23)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/95e70cc80f82

Whoops, wrong bug!

Comment 25

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

Comment on attachment 8510151 [details] [diff] [review]
bug-1062134.v04.patch

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

JW can review this.

I think you should try to write an explicit test for this case, so that we don't regress this or forget about this case and remove your code unwittingly in future.

Comment 26

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

Comment on attachment 8510151 [details] [diff] [review]
bug-1062134.v04.patch

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

LGTM. Per Chris said, we need a test case.

Comment 27

[Benjamin Chen [:bechen]]

Attachment: bug-1062134.v04.patch

r=jwwang

try server:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5eaaa87ab59b

Comment 28

[Benjamin Chen [:bechen]]

Attachment: bug-1062134.v04.patch

Rebased version, bug 946065.
content/media/... => dom/media/...

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e7e21442572

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5e7e21442572

Comment 31

[Benjamin Chen [:bechen]]

Attachment: bug-1062134.b2g34v2_1.patch

Comment 32

[Benjamin Chen [:bechen]]

Comment on attachment 8513188 [details] [diff] [review]
bug-1062134.b2g34v2_1.patch


[Approval Request Comment]
Bug caused by (feature/regressing bug #): feature, bug 871485
User impact if declined: Bug 1084629, when video playback is resumed from dormant state. The first video frame is shown briefly.
Testing completed: manual test on m-c
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/ead9ef6db95b

Comment 34

[Yeojin Chung [:YeojinC]]

Unable to verify as it is a back-end issue.