1050664 - MediaCodecReader playback can't suspend/resume.

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Benjamin Chen [:bechen]]

STR:
1. enable MediaCodec preference
2. play a video or audio by browser
3. press home key
4. open browser again, the video/audio can't play anymore.

Comment 1

[Benjamin Chen [:bechen]]

Seems that we need to change the mPlayState to PLAY_STATE_LOADING like MediaOmxDecoder::ResumeStateMachine(), or the statemacheing keeps in dormant state.

http://dxr.mozilla.org/mozilla-central/source/content/media/omx/MediaOmxDecoder.cpp#120

Comment 2

[Benjamin Chen [:bechen]]

Attachment: bug-1050664.patch

As discussed with Bruce, we should not check the |mCodec| which mapping to real HW resource that will be released at dormant state. Makes the function |MediaCodecReader::IsDormantNeeded| not works.
We should check the mSource that indaicates the MediaCodec has video.

Comment 3

[Benjamin Chen [:bechen]]

Hmm, the patch only fix "can't play video after suspend/resume", there is still bugs inside.

STR:
1. enable MediaCodec preference, apply this patch
2. play a video by browser
3. seek to the middle of the video
4. suspend/resume by home key

We can observe that the timing on the control bar doesn't match to the video frame.
Seems that the timing of decoder is not sync to MediaDecoderStateMachine.
Symptoms:
1. EOS is triggered earlier. ex: a 3:50 video clip, the control bar jumps to the end when playing 3:43 frame.
2. Seek to the beginning of the clip, some frames are missing.

Comment 4

[Benjamin Chen [:bechen]]

Found root cause:
When the MediaDecoderStateMachine change the state from Dormant to Metadata, will invoke |mReader->ReadMetadata| then |mReader->RequestData|. After the mReader callback decoded data to StateMachine, the |mStartTime| in StateMachine will be updated.
Once the |mStartTime| is not zero, everything is wrong....

TODO: find out why the MediaOmxReader/Decoder works fine because the code flow is the same at StateMachine.

Comment 5

[Benjamin Chen [:bechen]]

Attachment: bug-1050664.patch

Invoke mSource->stop at the ReleaseMediaResources() function and also add a new member |mSourceIsStopped| to protect the mSource->start/stop function.

Comment 6

[Bruce Sun [:brsun]]

Comment on attachment 8475797 [details] [diff] [review]
bug-1050664.patch

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

The timing to call MediaSource::start() and MediaSource::stop() is a little bit subtle. The logic to call OmxDecoder::Play() and OmxDecoder::Pause() in MediaOmxReader is much clearer. Perhaps we could mimic that logic from MediaOmxReader to have a clearer flow.

Comment 7

[Benjamin Chen [:bechen]]

As expected, the suspend/resume issue occurs at Bug 1033910 for rtsp.

When the StateMachine is in DECODER_STATE_DECODING_METADATA state, the mStartTime will be updated.
That implies the Rtsp must seek to zero(or do something like seek to zero) when the state is DECODER_STATE_DECODING_METADATA or DECODER_STATE_DORMANT, in order to set the mStartTime correctly.
It will be a problem for rtsp.

Comment 8

[Benjamin Chen [:bechen]]

Attachment: bug-1050664.patch

(In reply to Bruce Sun [:brsun] from comment #6)
> Comment on attachment 8475797 [details] [diff] [review]
> bug-1050664.patch
> 
> Review of attachment 8475797 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The timing to call MediaSource::start() and MediaSource::stop() is a little
> bit subtle. The logic to call OmxDecoder::Play() and OmxDecoder::Pause() in
> MediaOmxReader is much clearer. Perhaps we could mimic that logic from
> MediaOmxReader to have a clearer flow.

From my view, it is fine to me and we can refine it later.
It is a little subtle because for now the MediaCodecReader.cpp replace OMXDecoder.cpp and MediaOmxReader.cpp. It plays two roles before.

The only difference in this patch is that |return UNKNOWN_ERROR| if mSource->start() fail.

try server:
https://tbpl.mozilla.org/?tree=Try&rev=eaf9e82c7dfd

Comment 9

[Benjamin Chen [:bechen]]

Attachment: WIP-StartTime.patch

Hi Chirs:
The wip patch tries to avoid the "redundant SetStartTime()" in StateMachine.

The idea is that: stream's mStartTime won't change if we suspend/resume.
So I try to check the |mStartTime == -1| in FinishDecodeMetadata to avoid SetStartTime(). 
For Rtsp case, the patch can reduce one seek network operation, improve the suspend/resume performance a lot. But for Http streaming case, I guess the patch doens't do any help because the MPEG4Extractor had already rewind the sampletable when we suspend the MediaElement.(I think the http stream still |read the data offset 0| before the seek triggered by resume, once the data is not in the MediaCache, it will trigger an extra download)

How do you think about the startime? We can refine it at another bug, not this one.

Comment 10

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

Comment on attachment 8478149 [details] [diff] [review]
WIP-StartTime.patch

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

::: content/media/MediaDecoderStateMachine.cpp
@@ +1918,5 @@
>        NS_NewRunnableMethod(this, &MediaDecoderStateMachine::DispatchVideoDecodeTaskIfNeeded));
>      VideoQueue().AddPopListener(decodeTask, mDecodeTaskQueue);
>    }
>  
>    if (mScheduler->IsRealTime()) {

I don't understand. I thought for RTSP IsRealTime() should return true? So you won't take the "if (mStartTime != -1)" branch anyway?

Will IsRealTime() not be true for RTSP? If so we can remove all IsRealTime() checks?

I assume we'll take the IsRealTime() branch, and so we'd be better to go:

if (mScheduler->IsRealTime()) {
  if (mStartTime == -1) {
    // Only set 0 start time if we're not resuming from dormant state.
    SetStartTime(0);
  }
  res = FinishDecodeMetadata();
  NS_ENSURE_SUCCESS(res, res);
} else {
  // ...


Is that correct? Or is RTSP not real time?

Comment 11

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

So the only users realtime mode are the RawReader:
http://mxr.mozilla.org/mozilla-central/source/content/media/raw/RawDecoder.cpp#13

And the RTSP reader:
http://mxr.mozilla.org/mozilla-central/source/content/media/omx/RtspOmxDecoder.cpp#21

Though it looks like the RTSP reader may not be realtime? If that is the case, then what you're doing here is probably fine.

Comment 12

[Benjamin Chen [:bechen]]

(In reply to Chris Pearce (:cpearce) from comment #10)

> 
> I don't understand. I thought for RTSP IsRealTime() should return true? So
> you won't take the "if (mStartTime != -1)" branch anyway?
> Is that correct? Or is RTSP not real time?
RTSP can be realtime or non-realtime.
RTSP realtime: a live stream without duration, non-seekable, usually used for live show.
RTSP non-realtime: a demand stream with fixed duration, like other streaming.

I'll file another bug for the performance issue when suspend/resume.

Comment 13

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

Comment on attachment 8478149 [details] [diff] [review]
WIP-StartTime.patch

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

OK. This is fine then.

Comment 14

[Benjamin Chen [:bechen]]

Attachment: bug-1050664.patch

r=cpearce.
The try server result looks good at comment 8.
We can check in the patch first then do some improvements at bug 1058429.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3bc4d864cce

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a3bc4d864cce