1170589 - MPEG4 video playback is not smooth on master flame-kk

Status: VERIFIED FIXED

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

Description

[Sotaro Ikeda [:sotaro]]

On master flame, MPEG4 video playback is not smooth.

[STR]
-[1] Start video app
-[2] choose the video
-[3] start the video playback

I used the video at Bug 1131951 Comment 1 for the test.

Since Bug 1060900 fix, MP4Reader is used for mp4 video playback on gonk, It seems to cause the regression. When I set the following preference, MediaOmxReader is used for the mp4 video playback. In this case I did not sees the problem.
> pref("media.fragmented-mp4.exposed", false);

Comment 1

[Sotaro Ikeda [:sotaro]]

[Blocking Requested - why for this release]:

bwu, is there a similar bug already?

Comment 2

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

(In reply to Sotaro Ikeda [:sotaro] from comment #1)
> [Blocking Requested - why for this release]:
> 
> bwu, is there a similar bug already?
Sorry to reply you late since I was on a vacation for one week. 
I have not seen a similar bug, but previously I found a 720P video doesn't play smooth well. 
Could you attach your test file? 
Thanks!

Comment 3

[Sotaro Ikeda [:sotaro]]

Link to test file exists on Bug 1131951 as in Comment 0.

The problem seems not happen on MediaCodecReader and MediaOmxReader.

Comment 4

[Sotaro Ikeda [:sotaro]]

Bug 1171257 might be related partially.

Comment 5

[Mahendra Potharaju [:mahe]]

Can you confirm if this is still happening?

Comment 6

[Pi Wei Cheng [:piwei] (inactive)]

This issue is still reproducible on today's Flame master build using the video at Bug 1131951 Comment 1. Playback of video stutters.

Device: Flame (KK, 319MB, full flashed)
BuildID: 20150817061040
Gaia: 60489c1ff8c5d1633fc4837d4f8019623d4e1940
Gecko: a6eeb28458fd2652e12e57334f046b7776d75bb4
Gonk: c4779d6da0f85894b1f78f0351b43f2949e8decd
Version: 43.0a1 (2.5 Master) 
Firmware Version: v18Dv4
User Agent: Mozilla/5.0 (Mobile; rv:43.0) Gecko/43.0 Firefox/43.0

-----

I also tested on Aries and issue does not happen there. As title suggests this is specific to Flame.

Device: Aries
BuildID: 20150817141354
Gaia: 60489c1ff8c5d1633fc4837d4f8019623d4e1940
Gecko: a6eeb28458fd2652e12e57334f046b7776d75bb4
Gonk: 2916e2368074b5383c80bf5a0fba3fc83ba310bd
Version: 43.0a1 (2.5 Master) 
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:43.0) Gecko/43.0 Firefox/43.0

Comment 7

[Sotaro Ikeda [:sotaro]]

(In reply to Mahendra Potharaju [:mahe] from comment #5)
> Can you confirm if this is still happening?

In comment 6, it is tested on flame.

Comment 8

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #0)
> Since Bug 1060900 fix, MP4Reader is used for mp4 video playback on gonk, It
> seems to cause the regression. When I set the following preference,
> MediaOmxReader is used for the mp4 video playback. In this case I did not
> sees the problem.
> > pref("media.fragmented-mp4.exposed", false);

Latest master flame with pref("media.fragmented-mp4.exposed", false) still do not have a problem. When the pref is disabled, MediaOmxReader is used instead of MediaFormatReader.

On codeaurora platform, mpeg4 decoder allocate less out buffers than H.264. From the symptom, omx decoder becomes intermittent out of out buffers because gecko side keeps more buffers than MediaOmxReader. From the past experience, even using MediaOmxReader case, if gecko side steadily grab one more buffers, the video playback failed because of omx codec's time out.

Comment 9

[Sotaro Ikeda [:sotaro]]

One big difference between MediaFormatReader and MediaOmxReader is that MediaFormatReader uses MediaCodec and MediaOmxReader uses OMXCodec. MediaCodec is newer class and expose more capabilities.

Comment 10

[Al Tsai [:atsai]]

[Triage] We should be blocked by this. 

Blake, can you have someone work on it please? Thanks.

Comment 11

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

Benjamin,
Could you help on this bug? 
Thanks!

Comment 12

[Benjamin Chen [:bechen]]

Debugging now.
As comment 0, I can reproduce it on my flame-kk.

Comment 13

[Benjamin Chen [:bechen]]

Problem here, the |aSkipToNextKeyframe| and |MediaFormatReader::ShouldSkip|.
1. Now the MediaFormatReader::ShouldSkip doesn't respect the |aSkipToNextKeyframe|, it always check the |aTimeThreshold| and demuxer's nextKeyFrameTime.
2. Since the MediaOMXReader doesn't really implement the SkipToNextKeyframe, the problem might be the performance between "seek to next keyFrame" v.s. "keeping decoding".

Comment 14

[Benjamin Chen [:bechen]]

I try to fix the aSkipToNextKeyframe by introduce the seek/flush overhead to reduce the skip in my local patch:

MediaFormatReader::ShouldSkip(media::TimeUnit aTimeThreshold)
 {
   media::TimeUnit nextKeyframe;
   nsresult rv = mVideo.mTrackDemuxer->GetNextRandomAccessPoint(&nextKeyframe);

+
+  media::TimeUnit flushCost = mVideo.mDecoder->QueryFlushTimeCost();
+  printf_stderr("flushCost %lld %lld %lld",flushCost.ToMicroseconds(),
+      mVideo.mLastSampleTime.ToMicroseconds(),aTimeThreshold.ToMicroseconds());
+
+  return nextKeyframe < aTimeThreshold && nextKeyframe.ToMicroseconds() >= 0 &&
+         (mVideo.mLastSampleTime + flushCost) < aTimeThreshold;
 }

==
It works but improve the performance slightly.

Then I found the MediaOMXReader doesn't raise the aSkipToNextKeyframe flag during playback.
It indicate the performance of PDM regression is serrious (56fps -> 29fps)

Comment 15

[Benjamin Chen [:bechen]]

https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaFormatReader.cpp?from=mediaformatreader.cpp#714

Here is the root cause, since the MediaCodec has multiple input buffers, we should send more input samples.
I try to change the value from 1 to 2, the FPS increase to 55.

Comment 16

[Benjamin Chen [:bechen]]

Attachment: bug-1170589.v01.patch

Since the b2g platform usually have multiple input buffers for the demuxed samples, we can feed the samples more aggressively to make the decoder faster.

I add a new function |QueryInputAheadNumber| in MediaDataDecoder, in order to update the |decoder.mDecodeAhead|.
Then GonkMediaDataDecoder implements QueryInputAheadNumber by calculate its own input buffer number. Now the input buffer number is fixed to 3, we can file another bug for it.

Comment 17

[Jean-Yves Avenard [:jya]]

Comment on attachment 8657700 [details] [diff] [review]
bug-1170589.v01.patch

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

I don't see the need to amend the MediaDataDecoder once again just to cater for some special gonk case.
We've done a lot of work to make the reader/decoder as platform agnostic as possible; so let's not revert that trend again.

If you think gonk should have a decode ahead with a value of 3; then simply create a pref value of 3 and assume the value is 3. There's already a pref for that.

Also unfortunately, changing the number of frames to be demuxed require a tad more work than simply changing the call to Get Samples

Comment 18

[Benjamin Chen [:bechen]]

(In reply to [PTO Until 08/Sep] Jean-Yves Avenard [:jya] from comment #17)
> Comment on attachment 8657700 [details] [diff] [review]
> bug-1170589.v01.patch
> 
> Review of attachment 8657700 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't see the need to amend the MediaDataDecoder once again just to cater
> for some special gonk case.
> We've done a lot of work to make the reader/decoder as platform agnostic as
> possible; so let's not revert that trend again.
> 
> If you think gonk should have a decode ahead with a value of 3; then simply
> create a pref value of 3 and assume the value is 3. There's already a pref
> for that.

Because the number of input buffer for gonk might not be a fixed value, so I wish to update the ahead value at runtime.
In this case, the FPS at different ahead value from 1 ~ 4 are 22fps 55fps 100fps 100fps, so I set it to 3 temporarily.

> 
> Also unfortunately, changing the number of frames to be demuxed require a
> tad more work than simply changing the call to Get Samples

What's the extra work?

Comment 19

[Jean-Yves Avenard [:jya]]

(In reply to Benjamin Chen [:bechen] from comment #18)
> 
> Because the number of input buffer for gonk might not be a fixed value, so I
> wish to update the ahead value at runtime.
> In this case, the FPS at different ahead value from 1 ~ 4 are 22fps 55fps
> 100fps 100fps, so I set it to 3 temporarily.

in which case, why take such a convoluted approach ?

Simply create a MediaDataDecoder API that takes a TrackInfo object and returns by default the value of media.audio-decode-ahead or media.video-decode-ahead depending on the trackinfo's mimetype.

Then you won't need the DecoderData::mDecodeAhead member and simply query the MediaDataDecoder to find out how much in advance it should decode.


> 
> > 
> > Also unfortunately, changing the number of frames to be demuxed require a
> > tad more work than simply changing the call to Get Samples
> 
> What's the extra work?

Not all MediaDataDemuxer can demux more than a single frame (they do 1 or all of them with -1).
I'm also not confident that the MediaFormatReader will properly handle resolution change when it happens in the middle of the demuxed sampled. This needs to be checked.

In any case, this is the wrong way to solve your problem as just like bug 1171257, you're masking the core problem.

First, in your patch you'll always demux 2 (or 3) regardless on how many it actually needs.
But more importantly, feeding a single sample or multiples in rapid succession to the MediaDataDecoder should make no difference on how fast the decoder can decode simply because demuxing is *much* faster than decoding. You'd typically be able to demux close to 2000 samples before the MediaDataDecoder has time to complete the decoding of a frame.

*IF* you are seeing an improvement it's because the Gonk decoder wouldn't call InputExhausted as it should. That can be easily confirmed by modifying GonkVideoDecoderManager::Input to call InputExhausted() upon completion. Right now it doesn't.

This is something I'm guessing bug 1198664 would fix.

John, could you confirm? Could we make the GonkVideoDecoderManager call InputExhausted() more rapidly so it can request new samples from the reader?

Comment 20

[John Lin [:jhlin][:jolin]]

I've modified GonkVideoDecoderManager::Input() to call InputExhausted() and the log shows it's called ~100 times per second, which I think is more rapidly than enough. But the video is still janky.
I agree with jya that current implementation doesn't call InputExhausted() as reader expects, and ProcessDecode() should request as many inputs as the decoder can handle.
And, currently the output data is returned to reader only if reader sends input sample, which doesn't seem right to me either.
For bug 1198664 I will:
1. request as many input samples as decoder can handle from reader, by calling InputExhausted() when GonkVideoDecoderManager::mQueuedSamples is empty
2. introduce asynchronous output function to GonkVideoDecoderManager so output will be sent back to reader as soon as possible

Comment 21

[Jean-Yves Avenard [:jya]]

(In reply to John Lin [:jolin][:jhlin] from comment #20)
> I've modified GonkVideoDecoderManager::Input() to call InputExhausted() and
> the log shows it's called ~100 times per second, which I think is more
> rapidly than enough. But the video is still janky.

yeah, that's what I thought.. 
There's no need to feed multiple input in a loop : demuxing is fast enough already.

Not that we can't fix MediaFormatReader to ask the demuxer for more than one sample if it supports it : just that it won't make a difference worth worrying about at this stage. 

Now John, if you up media.video-decode-ahead from 2 to 3 or 4 ; does it play any better?
http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaFormatReader.cpp#69

> I agree with jya that current implementation doesn't call InputExhausted()
> as reader expects, and ProcessDecode() should request as many inputs as the
> decoder can handle.
> And, currently the output data is returned to reader only if reader sends
> input sample, which doesn't seem right to me either.

Ah yes!! that would be a problem ; especially as the MediaFormatReader could likely be waiting for the decoder to output something and not feed anything more to the decoder.

This sounds to be the likely explanation on why forcing the decode ahead to higher value fixes playback on gonk (which never really made sense to me in the first place): this simply causes more samples to be fed to the decoder, and so it returns samples more often to the decoder.

> For bug 1198664 I will:
> 1. request as many input samples as decoder can handle from reader, by
> calling InputExhausted() when GonkVideoDecoderManager::mQueuedSamples is
> empty
> 2. introduce asynchronous output function to GonkVideoDecoderManager so
> output will be sent back to reader as soon as possible

excellent.

Comment 22

[John Lin [:jhlin][:jolin]]

(In reply to Jean-Yves Avenard [:jya] from comment #21)
> Now John, if you up media.video-decode-ahead from 2 to 3 or 4 ; does it play
> any better?
> http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaFormatReader.
> cpp#69

 Tried that. No luck.

Comment 23

[Mahendra Potharaju [:mahe]]

The other dependent bug 1198664 is due for check-in. Can you please confirm if this one also gets fixed with the other bug? If not please look into this bug, its a P1 for 2.5 release

Comment 24

[John Lin [:jhlin][:jolin]]

Attachment: Use all allocated graphic buffers for output.

With patches in bug 1198664 and this one, I can play 60FPS video (http://pan.baidu.com/s/1eQ3s0Sy) without stuttering on Flame.

Comment 25

[Benjamin Chen [:bechen]]

Comment on attachment 8671144 [details] [diff] [review]
Use all allocated graphic buffers for output.

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

The patch works great on my flame.

Comment 26

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

Comment on attachment 8671144 [details] [diff] [review]
Use all allocated graphic buffers for output.

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

LGTM!
Please add a comment to describe the purpose to set it. 
You can refer to bug 1009410 comment 58.

Comment 27

[Benjamin Chen [:bechen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=512480c9c8f2

Comment 28

[Benjamin Chen [:bechen]]

Attachment: bug1170589-use-all-buffers.patch

Add comment. r=bwu
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2a1065f4b34

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3084c9574401

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3084c9574401

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3084c9574401

Comment 32

[Lance（leave from Mozilla）]

Attachment: Virified video: Flame KK v2.5.3GP

This bug has been verified as "pass" on the latest build of Flame KK 2.5 by the STR in comment 0 .

Actual results: The MPEG4 video playback is smooth .
See attachment: Flame KK v2.5.3GP
Reproduce rate: 0/10

Device: Flame KK v2.5 (Pass)
Build ID               20151019150205
Gaia Revision          a87f947366c2e044bd6336e1982419ac45378969
Gaia Date              2015-10-19 15:22:08
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/9605da94e75d61598d3c00f01a12d1b6bc427a6c
Gecko Version          44.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20151019.182947
Firmware Date          Mon Oct 19 18:29:58 EDT 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0