1091992 - MediaCodecReader: skipToNextKeyFrame always false when play a 720p video clip.

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Benjamin Chen [:bechen]]

Enable MediaCodec:
pref("media.omx.async.enabled", true);

HTTP streaming:
http://10.247.24.86/H264_720p_HP_L3.1_2Mbps_30fps_AAC_hinted.mp4

When I try to enable MediaCodec, the playback performance is bad if we play a http 720p video.

The |skipToNextKeyFrame| should be true if the video is slower than audio.

But I found the |skipToNextKeyFrame| always false because the |VideoQueue()| has data inside (a stale video frame)
http://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoderStateMachine.cpp?from=mediadecoderstatemachine.cpp#610

That means under the MediaCodec thread modeling, the |skipToNextKeyFrame| mechanism fails to accelerate the video decoding.

Comment 1

[Benjamin Chen [:bechen]]

http://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoderStateMachine.cpp?from=MediaDecoderStateMachine.cpp#608

The current code logic here assumes that all the frames in VideoQueue() had been processed by AdvanceFrame(). Hence check the | VideoQueue().GetSize())< LOW_VIDEO_FRAMES * mPlaybackRate | is reasonable. If there are frames in VideoQueue(), these frames' presentation time should larger than clocktime, so do not raise the flag skipToNextKeyFrame.

But the problem is : AdvanceFrame() runs on the StateMachine thread, DecodeVideo() runs on decode thread.
There might some stale frames in VideoQueue() prevent raising the flag skipToNextKeyFrame.

Comment 2

[Benjamin Chen [:bechen]]

Attachment: bug-1091992.v01.patch

Comment 3

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

Comment on attachment 8524402 [details] [diff] [review]
bug-1091992.v01.patch

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +608,2 @@
>             // we are still in the safe range without underrunning video frames
> +           GetClock() > mLatestVideoFrameEndTimeInQueue)) &&

I am nervous about removing the mPlaybackRate check... Do we need to remove it? If the mPlaybackRate check here causes problems, instead of removing it could we drop late frames here and keep the mPlaybackRate check?

Apart from that, I think this is OK.

Comment 4

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

Comment on attachment 8524402 [details] [diff] [review]
bug-1091992.v01.patch

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +608,2 @@
>             // we are still in the safe range without underrunning video frames
> +           GetClock() > mLatestVideoFrameEndTimeInQueue)) &&

How about checking |mLatestVideoFrameEndTimeInQueue - GetClock() < SOME_THRESHOLD * mPlaybackRate|?

::: dom/media/MediaDecoderStateMachine.h
@@ +821,5 @@
>    // The presentation end time of the last video frame which has been displayed
>    // in microseconds. Accessed from the state machine thread.
>    int64_t mVideoFrameEndTime;
>  
> +  // The presentation end time of the latest video frame which stores in

The comment should be: The end time of the latest decoded video frame.

Comment 5

[Benjamin Chen [:bechen]]

Attachment: bug-1091992.v02.patch

Address comment 4.

Comment 6

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

Comment on attachment 8525906 [details] [diff] [review]
bug-1091992.v02.patch

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +608,3 @@
>             // we are still in the safe range without underrunning video frames
> +           ((GetClock() - mLatestVideoFrameEndTimeInQueue) >
> +            (AUDIO_DURATION_USECS * mPlaybackRate)))) &&

The expression is wrong. We should kick off frame skip when remaining decoded video duration (mLatestVideoFrameEndTimeInQueue - GetClock()) is less than some threshold.

AUDIO_DURATION_USECS is 40ms which is a larger than a frame duration when FPS is 30. I think we should choose a smaller value.

Comment 7

[Benjamin Chen [:bechen]]

Attachment: bug-1091992.v03.patch

Address comment 6.

Comment 8

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

Comment on attachment 8526591 [details] [diff] [review]
bug-1091992.v03.patch

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

overall looks good to me.

::: dom/media/MediaDecoderStateMachine.cpp
@@ +608,5 @@
>            GetDecodedAudioDuration() < mLowAudioThresholdUsecs * mPlaybackRate) ||
>            (!mIsVideoPrerolling && IsVideoDecoding() &&
> +           // don't skip frame when
> +           // |clock time + LOW_VIDEO_THRESHOLD_USECS * mPlaybackRate| <=
> +           // |mLatestVideoFrameEndTimeInQueue| for

The comment is just a rephrasing of the code below. It should be "Don't skip when we still have enough decoded data beyond current playback position".

::: dom/media/MediaDecoderStateMachine.h
@@ +822,5 @@
>    // in microseconds. Accessed from the state machine thread.
>    int64_t mVideoFrameEndTime;
>  
> +  // The end time of the latest decoded video frame.
> +  int64_t mLatestVideoFrameEndTimeInQueue;

See bug 1098668. We should have a consistent naming style.

Comment 9

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

Comment on attachment 8526591 [details] [diff] [review]
bug-1091992.v03.patch

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

r+ with comments addressed.

::: dom/media/MediaDecoderStateMachine.cpp
@@ +107,5 @@
>  // we're not "prerolling video", we'll skip the video up to the next keyframe
>  // which is at or after the current playback position.
>  static const uint32_t LOW_VIDEO_FRAMES = 1;
>  
> +// Compare the latest video frame end time and clock time to see if we need to

This comment doesn't actually describe how LOW_VIDEO_THRESHOLD_USECS, it's describing an action that is performed, and it's not clear from the comment whether LOW_VIDEO_THRESHOLD_USECS is used during the described action, and when the described action is performed.

Please make this comment clearly describe where LOW_VIDEO_THRESHOLD_USECS is used, and what it is used for.

::: dom/media/MediaDecoderStateMachine.h
@@ +822,5 @@
>    // in microseconds. Accessed from the state machine thread.
>    int64_t mVideoFrameEndTime;
>  
> +  // The end time of the latest decoded video frame.
> +  int64_t mLatestVideoFrameEndTimeInQueue;

Please change the name of this to mDecodedVideoEndTime, so it is consistent with JW's patch in bug 1098668.

Comment 10

[Benjamin Chen [:bechen]]

Attachment: bug-1091992.v03.patch

r=cpearce, r=jwwang

tryserver:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=347270631a23

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f013d802a78

Comment 12

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4278493&repo=mozilla-inbound

Comment 13

[Benjamin Chen [:bechen]]

Because the patch changes the skipToNextKeyFrame flag policy, it turns out we are more aggressive to raise the flag.
Then we guess something wrong in SendStreamData(). For example, if the stream has only 1 I-frame and we still raise the flag to skip it, we may send nothing to downstream MedisStream..

I have tried to assign skipToNextKeyFrame to true however in MediaDecoderStateMachine::DecodeVideo, then run the test_streams_element_capture_reset.html. It always fail at desktop build.

Comment 14

[Benjamin Chen [:bechen]]

Attachment: bug-1091992.v04.patch

If it is a video only stream and decode to stream, we won't raise the flag skipToNextKeyFrame.

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

Comment 15

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

Comment on attachment 8538310 [details] [diff] [review]
bug-1091992.v04.patch

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +620,3 @@
>          !HasLowUndecodedData())
>      {
>        skipToNextKeyFrame = true;

The frame-skip logic is getting complicated. Please create a new function to encapsulate the logic. E.g.

skipToNextKeyFrame = NeedToSkipFrame();

bool NeedsToSkipFrame()
{
  // Don't skip frame when we are seeking or decoding metadata...
  if (mState != DECODER_STATE_DECODING) { return false; }
  if (!sVideoDecoding()) { return false; }
  // Don't skip frame for a video-only decoded stream because XXX
  if (mDecoder->GetDecodedStream() && !HasAudio()) { return false; }
  ...
}

Please also comment each condition to make the logic more comprehensive.

Comment 16

[Benjamin Chen [:bechen]]

Attachment: bug-1091992.v04.patch

Address comment 15.

Comment 17

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

Comment on attachment 8538983 [details] [diff] [review]
bug-1091992.v04.patch

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

Looks good to me with nits addressed.

::: dom/media/MediaDecoderStateMachine.cpp
@@ +583,5 @@
> +             mState == DECODER_STATE_BUFFERING ||
> +             mState == DECODER_STATE_SEEKING);
> +
> +  // We are in seeking or buffering states, don't skip frame.
> +  if (!IsVideoDecoding() || mState == DECODER_STATE_BUFFERING ||

It should be if (!IsVideoDecoding() || mState != DECODER_STATE_DECODING).
Don't change the original logic.

@@ +589,5 @@
> +    return false;
> +  }
> +
> +  // We are prerolling, don't skip frame.
> +  if (mIsVideoPrerolling && mIsAudioPrerolling) {

You can remove this if block since it will be included in the case of isLowOnDecodedAudio and isLowOnDecodedVideo below.

@@ +595,5 @@
> +  }
> +
> +  // Don't skip frame for video-only decoded stream because the clock time of
> +  // the stream relies on the video frame.
> +  if (mDecoder->GetDecodedStream() && HasVideo() && !HasAudio()) {

It should be sufficient to check !HasAudio() only. It doesn't make sense to have both !HasVideo() and !HasAudio().

Comment 18

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

Comment on attachment 8538983 [details] [diff] [review]
bug-1091992.v04.patch

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

Looks good to me with nits addressed.

::: dom/media/MediaDecoderStateMachine.cpp
@@ +583,5 @@
> +             mState == DECODER_STATE_BUFFERING ||
> +             mState == DECODER_STATE_SEEKING);
> +
> +  // We are in seeking or buffering states, don't skip frame.
> +  if (!IsVideoDecoding() || mState == DECODER_STATE_BUFFERING ||

It should be if (!IsVideoDecoding() || mState != DECODER_STATE_DECODING).
Don't change the original logic.

@@ +584,5 @@
> +             mState == DECODER_STATE_SEEKING);
> +
> +  // We are in seeking or buffering states, don't skip frame.
> +  if (!IsVideoDecoding() || mState == DECODER_STATE_BUFFERING ||
> +      mState == DECODER_STATE_SEEKING) {

Just notice the assertion above. It is fine without changing it.

@@ +589,5 @@
> +    return false;
> +  }
> +
> +  // We are prerolling, don't skip frame.
> +  if (mIsVideoPrerolling && mIsAudioPrerolling) {

You can remove this if block since it will be included in the case of isLowOnDecodedAudio and isLowOnDecodedVideo below.

@@ +595,5 @@
> +  }
> +
> +  // Don't skip frame for video-only decoded stream because the clock time of
> +  // the stream relies on the video frame.
> +  if (mDecoder->GetDecodedStream() && HasVideo() && !HasAudio()) {

It should be sufficient to check !HasAudio() only. It doesn't make sense to have both !HasVideo() and !HasAudio().

Comment 19

[Benjamin Chen [:bechen]]

Attachment: bug-1091992.v04.patch

Address comment 18.

Push to try server again:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b4e445c0f823

Comment 20

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

Comment on attachment 8539171 [details] [diff] [review]
bug-1091992.v04.patch

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

::: dom/media/MediaDecoderStateMachine.h
@@ +711,5 @@
>    void DoNotifyWaitingForResourcesStatusChanged();
>  
> +  // Return true if the video decoder's decode speed can not catch up the
> +  // play time.
> +  bool NeedToSkipVideoFrame();

Please make this NeedToSkipToNextKeyframge(), so that the name matches what it does; when this returns true we don't skip one video frame, we skip a run of frames up to the next keyframe.

Comment 21

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

NeedToSkipToNextKeyframe() I mean; spelling mistake! ;)

Comment 22

[Benjamin Chen [:bechen]]

Attachment: bug-1091992.v04.patch

Rename to NeedToSkipToNextKeyframe().

Comment 23

[Carsten Book [:Tomcat]]

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

Comment 24

[Carsten Book [:Tomcat]]

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

Comment 25

[Ralph Giles (:rillian)]

Comment on attachment 8539910 [details] [diff] [review]
bug-1091992.v04.patch

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent testing. Sites more likely to serve Flash video.
[Describe test coverage new/current, TBPL]: Landed on m-c and deployed in Nightly.
[Risks and why]: Risk is moderate. This affects normal playback in slow-network conditions. We think it's better behaviour overall, though.
[String/UUID change made/needed]: None.

Comment 26

[Ralph Giles (:rillian)]

Sorry, that's should be slow-cpu, not slow-network conditions.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/38577fafb0ed