1152218 - Make MediaDecoderStateMachine skip-to-next-keyframe less aggressively

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Blake Wu [:bwu][:blakewu]]

Current decision[1] to check if the video data is low or not is too aggressive and causes unnecessary skipToNextKeyframe. 
We should change it. 

[1]https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoderStateMachine.cpp?from=MediaDecoderStateMachine.cpp&case=true#653
bool isLowOnDecodedVideo = !mIsVideoPrerolling &&
                             (mDecodedVideoEndTime - GetClock() <
                              LOW_VIDEO_THRESHOLD_USECS * mPlaybackRate);

Comment 1

[Blake Wu [:bwu][:blakewu]]

Too main reasons to change the decision rule as comment 0. 
1.For some video files or system busy case, decoder may take a little bit longer to decode a keyframe which may make isLowOnDecodedVideo true and skip to next keyframe (if the keyframe is far, that would make situation worse). However, after keyframe decoder can decode the following P/B frames quickly. For a couple of late frames, we might not need to trigger skip-to-next-keyframe and it should be a nice-to-have and should not affect normal case.    
2.Take seek for example, after seeking video timestamp (mDecodedVideoEndTime) will be close to audio timestamp(GetClock()), so isLowOnDecodedVideo would be easily to be true.

Comment 2

[Blake Wu [:bwu][:blakewu]]

Attachment: Have-a-larger-tolerance-to-decide-if-video-data-is-low-or-not.patch

As comment 0 and 1. 
Have a larger tolerance to avoid unnecessary skip-to-next-keyframe.

Comment 3

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

When the MP4Reader is enabled, we only skip-to-next-keyframe when the current playback position has fallen behind the next keyframe (we ignore the aSkipToNextKeyframe parameter to MP4Reader::RequestVideoData()). Then we will only skip frames that we could not play anyway.

Do you see the issues with skip-to-next-keyframe when you're playing using the MP4Reader?

Comment 4

[Blake Wu [:bwu][:blakewu]]

(In reply to Chris Pearce (:cpearce) from comment #3)
> When the MP4Reader is enabled, we only skip-to-next-keyframe when the
> current playback position has fallen behind the next keyframe (we ignore the
> aSkipToNextKeyframe parameter to MP4Reader::RequestVideoData()). Then we
> will only skip frames that we could not play anyway.
> 
> Do you see the issues with skip-to-next-keyframe when you're playing using
> the MP4Reader?
I found this problem in the seek case in MediaOmxReader in bug 1147241. I have not checked it on MP4Reader yet. Will check it.

Comment 5

[Blake Wu [:bwu][:blakewu]]

I checked it using MP4Reader. skip-to-next-keyframe is not triggered since video mDecodedVideoEndTime is not *close* to GetClock() after seeking, which is different from that in MediaOmxReader.

Comment 6

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

Comment on attachment 8589549 [details] [diff] [review]
Have-a-larger-tolerance-to-decide-if-video-data-is-low-or-not.patch

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

The long term strategy with skip-to-next-keyframe is for our Readers to do it like the MP4Reader does.

In the short term, we can land this patch to make skip-to-next-keyframe less aggressive.

::: dom/media/MediaDecoderStateMachine.cpp
@@ +126,5 @@
>  // Threshold in usecs that used to check if we are low on decoded video.
> +// If (clock time - the last video frame's end time, mDecodedVideoEndTime)*
> +// mPlaybackRate doesn't exceed LOW_VIDEO_THRESHOLD_USECS. We are low on
> +// decoded video frames and trying to skip to next keyframe.
> +static const int32_t LOW_VIDEO_THRESHOLD_USECS = 33000;

I think this comment should be clearer. Please make it:

// Threshold in usecs that used to check if we are low on decoded video.
// If the last video frame's end time |mDecodedVideoEndTime| is more than
// |LOW_VIDEO_THRESHOLD_USECS*mPlaybackRate| after the current clock in
// Advanceframe(), the video decode is lagging, and we skip to next keyframe.

How about we make this value 60000? That would be almost 1.5 frames late in 24fps video.

Comment 7

[Blake Wu [:bwu][:blakewu]]

(In reply to Chris Pearce (:cpearce) from comment #6)
Thanks for your review! 
> Comment on attachment 8589549 [details] [diff] [review]
> Have-a-larger-tolerance-to-decide-if-video-data-is-low-or-not.patch
> 
> Review of attachment 8589549 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The long term strategy with skip-to-next-keyframe is for our Readers to do
> it like the MP4Reader does.
Yes. Agree. This is pretty tricky to have a same rule for all the Readers. 
> 
> In the short term, we can land this patch to make skip-to-next-keyframe less
> aggressive.
> 
> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +126,5 @@
> >  // Threshold in usecs that used to check if we are low on decoded video.
> > +// If (clock time - the last video frame's end time, mDecodedVideoEndTime)*
> > +// mPlaybackRate doesn't exceed LOW_VIDEO_THRESHOLD_USECS. We are low on
> > +// decoded video frames and trying to skip to next keyframe.
> > +static const int32_t LOW_VIDEO_THRESHOLD_USECS = 33000;
> 
> I think this comment should be clearer. Please make it:
Will do. 
> 
> // Threshold in usecs that used to check if we are low on decoded video.
> // If the last video frame's end time |mDecodedVideoEndTime| is more than
> // |LOW_VIDEO_THRESHOLD_USECS*mPlaybackRate| after the current clock in
> // Advanceframe(), the video decode is lagging, and we skip to next keyframe.
> 
> How about we make this value 60000? That would be almost 1.5 frames late in
> 24fps video.
I choose 33000 because it is the frame-to-frame interval of 30 fps. However, I think 60000 would be better since it will be more tolerate :)

Comment 8

[Blake Wu [:bwu][:blakewu]]

Attachment: Bug-1152218-Have-a-larger-tolerance-to-decide-if-video-data-is-low.patch

Carry r+ from cpearce and have some changes according to comment 6.

Comment 9

[Blake Wu [:bwu][:blakewu]]

Besides enlarging the tolerance, in long term we should need to check more frames to decide if we should trigger skip-to-next-keyframe or not. Currently only checking one frame might be too strict.

Comment 10

[Blake Wu [:bwu][:blakewu]]

Test results all look good.  
https://treeherder.mozilla.org/#/jobs?repo=try&revision=61863cb5b980

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/29ee242b0b0c

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/29ee242b0b0c

Comment 13

[Blake Wu [:bwu][:blakewu]]

Attachment: v2.2-Bug-1152218-Have-a-larger-tolerance-to-decide-if-vid.patch

Rebase attachment 8590604 [details] [diff] [review] to v2.2 and carry r+ from cpearce.

Comment 14

[Blake Wu [:bwu][:blakewu]]

Comment on attachment 8591338 [details] [diff] [review]
v2.2-Bug-1152218-Have-a-larger-tolerance-to-decide-if-vid.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Bug 1091992
User impact if declined: 
2.2+ bug 1147241 depends on this bug. 
Testing completed: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f483e2f03579
Risk to taking this patch (and alternatives if risky):
Low. 
String or UUID changes made by this patch:
NA

Comment 15

[Blake Wu [:bwu][:blakewu]]

Set 2.2? since bug 1147241 depends on this bug and we need to have approval-mozilla-b2g37+ approval for attachment 8591338 [details] [diff] [review].

Comment 16

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: per comment 15

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Please request b2g37 approval on this patch when you get a chance.

Comment 18

[Blake Wu [:bwu][:blakewu]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #17)
> Please request b2g37 approval on this patch when you get a chance.
It's done. 
Could you help check the test results, https://treeherder.mozilla.org/#/jobs?repo=try&revision=f483e2f03579 in comment 14? I don't think those failures and build busted caused by my patch.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Looks fine. The M5 is a known issue, Mc is trunk-only, and M20 looks like a known infra issue. You're clear for uplift :)

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/50a69167311b

Comment 21

[Blake Wu [:bwu][:blakewu]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #19)
> Looks fine. The M5 is a known issue, Mc is trunk-only, and M20 looks like a
> known infra issue. You're clear for uplift :)
Thanks! \o/