1054207 - skipToNextKeyFrame in MediaDecoderStateMachine may not be required

Status: RESOLVED INVALID

(Core :: Audio/Video: Playback, defect)

Description

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

If we are low in the VideoQueue, skipping to next key frame may not make playback smoother after next key frame. A couple reasons are following.  
1. Usually low VideoQueue is due to the slow decoding from decoder or CPU. Skpping will not improve or increase the speed of decoding. 
2. For downloading data case, it may make users wait due to skipping. 
What I oberved on Flame by playing high resolution video (downloaded from http://people.mozilla.org/~cpearce/avatar.mp4 )via current PDM (not use graphic Buffer yet) is on average slow, not keeping jumping by disabling skipToNextKeyFrame. 
With skipToNextKeyFrame enabled, playback will keep jumping to next I-frame and playing slowly for a short while. 
3. No much benefits but introduce some complexity to current playback design.

Comment 1

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

Add one more reason. 
4. For those videos with longer GOP/ keyframe interval, skip-to-next-key-frame will jump over a long period of frames and playback may end earlier. The user experience may not be good.

Comment 2

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

I was never happy with the skip-to-next-keyframe logic. But what else can we do? If the CPU is too slow, we there's not much we can do...

Comment 3

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

I think there is no any way to make playback smoother if the CPU or HW decoder is too slow to decode a complicated content :(. That should be a limitation. 
skip-to-next-keyframe logic cannot impove either. IMO, we should remove it.

Comment 4

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

I've thought of removing that code too. If we do remove it, we should not remove it until we have async decoding working, as otherwise we'll get audio underruns, which are a very bad user experience.

Comment 5

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

I don't quite understand and got two questions for you as below :)  
1. In which cases or under what conditions, we will hit audio underruns? Normally, the speed of audio decoding is quite fast and faster than that of video decoding.
2. Why skip-to-next-keyframe can mitigate/fix the problem of audio underrun?

Comment 6

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

(In reply to Blake Wu [:bwu][:blakewu] from comment #5)
> I don't quite understand and got two questions for you as below :)  
> 1. In which cases or under what conditions, we will hit audio underruns?

We hit audio underruns when there is not enough audio to play. This can happen when the decode thread spends so much time decoding video, that while it's busy decoding video we run out of audio.

> Normally, the speed of audio decoding is quite fast and faster than that of
> video decoding.
> 2. Why skip-to-next-keyframe can mitigate/fix the problem of audio underrun?

skip-to-next-keyframe helps because it reduces the amount of video frames we decode. It's not a good solution.

Comment 7

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

(In reply to Chris Pearce (:cpearce) from comment #6)
> skip-to-next-keyframe helps because it reduces the amount of video frames we
> decode. It's not a good solution.

We don't know if it is a key frame or not until the frame is decoded, right? So we don't save decoding time but only render time.

Comment 8

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

Keyframe can be known from demuxer's side. But IIRC for B2G OmxDecoder currently it still continues decoding but drops those decoded frames before rendering. So what you said is correct. skip-to-next-keyframe doesn't save decoding time and so actually is not helpful. 
I forgot to put this one in comment 0. Thanks JW's mention here. :)

Comment 9

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

(In reply to JW Wang [:jwwang] from comment #7)
> (In reply to Chris Pearce (:cpearce) from comment #6)
> > skip-to-next-keyframe helps because it reduces the amount of video frames we
> > decode. It's not a good solution.
> 
> We don't know if it is a key frame or not until the frame is decoded, right?

As Blake says, whether a frame is a keyframe or not is known in advance by the demuxer.


(In reply to Blake Wu [:bwu][:blakewu] from comment #8)
> Keyframe can be known from demuxer's side. But IIRC for B2G OmxDecoder
> currently it still continues decoding but drops those decoded frames before
> rendering. So what you said is correct. skip-to-next-keyframe doesn't save
> decoding time and so actually is not helpful. 

Skip-to-next-keyframe is incorrectly implemented in B2G OmxDecoder. IIRC this is because the demuxers that OEMs ship do not reliably implement a seek-forwards-to-next-keyframe.

Skip-to-next-keyframe is helpful in other backends that correctly implement it. The user experience for synchronous Readers will be worse without skip-to-next-keyframe, as when video decode takess to long audio will underrun (there was a Tarako bug where this happened, IIRC).

I don't think we can consider removing skip-to-next-keyframe except for async Readers, and only after testing on a range of devices with a range of input files.

Comment 10

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

(In reply to Chris Pearce (:cpearce) from comment #9)
> (In reply to JW Wang [:jwwang] from comment #7)
> > (In reply to Chris Pearce (:cpearce) from comment #6)
> > > skip-to-next-keyframe helps because it reduces the amount of video frames we
> > > decode. It's not a good solution.
> > 
> > We don't know if it is a key frame or not until the frame is decoded, right?
> 
> As Blake says, whether a frame is a keyframe or not is known in advance by
> the demuxer.
> 
> 
> (In reply to Blake Wu [:bwu][:blakewu] from comment #8)
> > Keyframe can be known from demuxer's side. But IIRC for B2G OmxDecoder
> > currently it still continues decoding but drops those decoded frames before
> > rendering. So what you said is correct. skip-to-next-keyframe doesn't save
> > decoding time and so actually is not helpful. 
> 
> Skip-to-next-keyframe is incorrectly implemented in B2G OmxDecoder. IIRC
> this is because the demuxers that OEMs ship do not reliably implement a
> seek-forwards-to-next-keyframe.
> 
> Skip-to-next-keyframe is helpful in other backends that correctly implement
> it. The user experience for synchronous Readers will be worse without
> skip-to-next-keyframe, as when video decode takess to long audio will
> underrun (there was a Tarako bug where this happened, IIRC).
Sorry. I am still not clear why skip-to-next-keyframe can speed up video decoding and give audio more time to decode. Recently I found woodduck (bug 1068509) plays 720P video not very smoothly and skip-to-next-keyframe is triggered many times. I am going to disable it on OmxDecoder. 
Do you agree? If yes, I will create a bug for disabling OmxDecoder first.

Comment 11

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

(In reply to Blake Wu [:bwu][:blakewu] from comment #10)
> Sorry. I am still not clear why skip-to-next-keyframe can speed up video
> decoding and give audio more time to decode. 

The goal of skip-to-next-keyframe is to give more time to audio decode. It does not speed up video decoding. There is no way we can speed up video decoding if the CPU/GPU is too slow. It is impossible.

MediaOmxReader is synchronous. It decodes video and audio on the same thread. So if video decode is slow, audio decode may not get enough time. We may playback audio faster than we decode more audio, and thus run out of audio to play, and then audio underruns and we get stutters.

The MediaOmxReader does not implement skip-to-next-keyframe correctly. When skip-to-next-keyframe happens the MediaDecoderReader is supposed to *skip* the demuxer to the next keyframe. It's is not supposed to *decode* to the next keyframe, like the OmxDecoder does. The OmxDecoder does not implement skip-to-next-keyframe correctly. When MediaOmxReader engages skip-to-next-keyframe, it actually gives the audio decode even less time to run, so it makes the problem worse.



> Recently I found woodduck (bug
> 1068509) plays 720P video not very smoothly and skip-to-next-keyframe is
> triggered many times.

> I am going to disable it on OmxDecoder. 
> Do you agree? If yes, I will create a bug for disabling OmxDecoder first.

You should try to implement skip-to-next-keyframe correctly before we consider disabling it. If you disable skip-to-next-keyframe you will get audio underrun (compared to if skip-to-next-keyframe was implemented correctly).

The best solution to avoid audio glitches when the CPU/GPU is not fast enough to decode the video in real time is to use async decoding. The MediaCodecReader does async decoding, the video decode runs concurrently with the audio decode.

Comment 12

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

Thanks for your feedback!