Closed Bug 1062134 Opened 6 years ago Closed 5 years ago

Avoid unnecessary decoding when back from dormant state.

Categories

(Core :: Audio/Video, defect)

x86_64
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36
blocking-b2g 2.1+
Tracking Status
firefox34 --- wontfix
firefox35 --- wontfix
firefox36 --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: bechen, Assigned: bechen)

References

Details

(Whiteboard: [caf priority: p2][CR 741684])

Attachments

(2 files, 8 obsolete files)

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.
Attached patch bug-1062134.v01.patch (obsolete) — Splinter Review
Attachment #8489848 - Flags: review?(cpearce)
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.
Attachment #8489848 - Flags: review?(cpearce) → review+
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.
(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.
Tip: click the "[review]" button to add comments. It is easier to trace all review comments.
Attached patch bug-1062134.v01.patch (obsolete) — Splinter Review
r=cpearce
try server:
https://tbpl.mozilla.org/?tree=Try&rev=668aa69f6916
Attachment #8489848 - Attachment is obsolete: true
Attachment #8494951 - Flags: review+
https://tbpl.mozilla.org/?tree=Try&rev=19072e634dec

Looks like my patch breaks some testcases about texture..
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.
Attached patch bug-1062134.v02.patch (obsolete) — Splinter Review
Freeze decoding when exit dormant and resume decoding when seeking.

try server:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a2397e117657
Attachment #8494951 - Attachment is obsolete: true
Attachment #8506626 - Flags: feedback?(jwwang)
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?
Attachment #8506626 - Flags: feedback?(jwwang)
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 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.
See Also: → 1084629
Attached patch bug-1062134.v03.patch (obsolete) — Splinter Review
Attachment #8506626 - Attachment is obsolete: true
[Blocking Requested - why for this release]: Bug 1058429, which blocks bug 1082563, depends on this bug.
blocking-b2g: --- → 2.1?
This blocks a 2.1+ bug so I'm marking it 2.1+ as well.
blocking-b2g: 2.1? → 2.1+
Attachment #8507736 - Flags: feedback?(jwwang)
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.
Attachment #8507736 - Flags: feedback?(jwwang)
Attached patch bug-1062134.v03.patch (obsolete) — Splinter Review
Attachment #8507736 - Attachment is obsolete: true
Attachment #8509263 - Flags: feedback?(jwwang)
Attached patch bug-1062134.v04.patch (obsolete) — Splinter Review
Attachment #8509263 - Attachment is obsolete: true
Attachment #8509263 - Flags: feedback?(jwwang)
Attachment #8509276 - Flags: feedback?(jwwang)
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?
Attachment #8509276 - Flags: feedback?(jwwang)
(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.
Summary: Avoid the redundant update for mStartTime on B2G. → Avoid unnecessary decoding when back from dormant state.
Duplicate of this bug: 1058429
Attached patch bug-1062134.v04.patch (obsolete) — Splinter Review
1. Rename:
mFreezeDecodingAtStateMetadata => mDecodingFrozenAtStateMetadata
mFreezeDecodingAtStateDecoding => mDecodingFrozenAtStateDecoding

2. Reset mDecodingFrozenAtStateDecoding at Play().
Attachment #8509276 - Attachment is obsolete: true
Attachment #8510151 - Flags: review?(cpearce)
Attachment #8510151 - Flags: feedback?(jwwang)
Whiteboard: [CR 741684]
Whiteboard: [CR 741684] → [caf priority: p2][CR 741684]
(In reply to Chris Pearce (:cpearce) from comment #23)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/95e70cc80f82

Whoops, wrong bug!
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.
Attachment #8510151 - Flags: review?(jwwang)
Attachment #8510151 - Flags: review?(cpearce)
Attachment #8510151 - Flags: feedback?(jwwang)
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.
Attachment #8510151 - Flags: review?(jwwang) → review+
Blocks: 1088524
Attached patch bug-1062134.v04.patch (obsolete) — Splinter Review
r=jwwang

try server:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5eaaa87ab59b
Attachment #8510151 - Attachment is obsolete: true
Attachment #8510941 - Flags: review+
Rebased version, bug 946065.
content/media/... => dom/media/...
Attachment #8510941 - Attachment is obsolete: true
Attachment #8511819 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5e7e21442572
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
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
Attachment #8513188 - Flags: approval-mozilla-b2g34?
Attachment #8513188 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
Unable to verify as it is a back-end issue.
QA Whiteboard: [QAnalyst-Triage?][QAnalyst-verify-]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?][QAnalyst-verify-] → [QAnalyst-Triage+][QAnalyst-verify-]
Flags: needinfo?(ktucker)
See Also: → 1147090
Depends on: 1147090
You need to log in before you can comment on or make changes to this bug.