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

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

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.

Freeze decoding when exit dormant and resume decoding when seeking. try server: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a2397e117657

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

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

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

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

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

1. Rename: mFreezeDecodingAtStateMetadata => mDecodingFrozenAtStateMetadata mFreezeDecodingAtStateDecoding => mDecodingFrozenAtStateDecoding 2. Reset mDecodingFrozenAtStateDecoding at Play().

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

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.

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

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

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

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