Closed Bug 1091992 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Audio/Video, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: bechen, Assigned: bechen)

References

Details

Attachments

(1 file, 7 obsolete files)

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.
No longer blocks: 1090989
Blocks: 1091989
Assignee: nobody → 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.
Status: NEW → ASSIGNED
Attached patch bug-1091992.v01.patch (obsolete) — Splinter Review
Attachment #8524402 - Flags: feedback?(jwwang)
Attachment #8524402 - Flags: feedback?(cpearce)
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.
Attachment #8524402 - Flags: feedback?(cpearce) → feedback+
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.
Attachment #8524402 - Flags: feedback?(jwwang) → feedback+
Attached patch bug-1091992.v02.patch (obsolete) — Splinter Review
Address comment 4.
Attachment #8524402 - Attachment is obsolete: true
Attachment #8525906 - Flags: review?(jwwang)
Attachment #8525906 - Flags: review?(cpearce)
Blocks: 1102047
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.
Attachment #8525906 - Flags: review?(jwwang) → review-
Attached patch bug-1091992.v03.patch (obsolete) — Splinter Review
Address comment 6.
Attachment #8525906 - Attachment is obsolete: true
Attachment #8525906 - Flags: review?(cpearce)
Attachment #8526591 - Flags: review?(jwwang)
Attachment #8526591 - Flags: review?(cpearce)
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.
Attachment #8526591 - Flags: review?(jwwang) → review+
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.
Attachment #8526591 - Flags: review?(cpearce) → review+
Attached patch bug-1091992.v03.patch (obsolete) — Splinter Review
r=cpearce, r=jwwang

tryserver:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=347270631a23
Attachment #8526591 - Attachment is obsolete: true
Attachment #8529508 - Flags: review+
Keywords: checkin-needed
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.
See Also: → 1112445
Attached patch bug-1091992.v04.patch (obsolete) — Splinter Review
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
Attachment #8529508 - Attachment is obsolete: true
Attachment #8538310 - Flags: review?(jwwang)
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.
Attachment #8538310 - Flags: review?(jwwang)
Attached patch bug-1091992.v04.patch (obsolete) — Splinter Review
Address comment 15.
Attachment #8538310 - Attachment is obsolete: true
Attachment #8538983 - Flags: review?(jwwang)
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().
Attachment #8538983 - Flags: review?(jwwang) → review+
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().
Attached patch bug-1091992.v04.patch (obsolete) — Splinter Review
Address comment 18.

Push to try server again:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b4e445c0f823
Attachment #8538983 - Attachment is obsolete: true
Attachment #8539171 - Flags: review+
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.
NeedToSkipToNextKeyframe() I mean; spelling mistake! ;)
Rename to NeedToSkipToNextKeyframe().
Attachment #8539171 - Attachment is obsolete: true
Attachment #8539910 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e64795aab6c9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
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.
Attachment #8539910 - Flags: approval-mozilla-aurora?
Sorry, that's should be slow-cpu, not slow-network conditions.
Attachment #8539910 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.