Closed Bug 1299105 Opened 8 years ago Closed 8 years ago

Video stutters when switching quality.

Categories

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

Unspecified
Android
defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- wontfix
firefox53 --- verified

People

(Reporter: jhlin, Assigned: JamesCheng)

References

Details

Attachments

(3 files, 11 obsolete files)

58 bytes, text/x-review-board-request
jya
: review+
Details
58 bytes, text/x-review-board-request
jya
: review+
Details
31.38 KB, patch
Details | Diff | Splinter Review
Go to http://s3.cf.web.us.aiv-cdn.net/HTML5PartnerTest.html?asin=B00TYBBNAW&videoType=Trailer&ui to watch the trailer: video stutters when there are quality switch.
It could be that recreating decoder when initialization segment is received takes some time and the new frame didn't arrive soon enough. Will do some measurements to verify that.
The log shows format reader and decoder processed all video frames when resolution changed and all of them were sent to video sink, but some didn’t arrived in time (A/V sync criteria) and were discard.

09-01 03:27:59.864  7760  8158 I Gecko   : [MediaPlayback #1]: V/MediaDecoder VideoSink=7fdf9ef0 playing video frame 3962291 (id=60) (vq-queued=1)
09-01 03:28:00.011  7760  8159 I Gecko   : [MediaPlayback #2]: V/MediaDecoder VideoSink=7fdf9ef0 discarding video frame mTime=4004000 clock_time=4110000
09-01 03:28:00.017  7760  8159 I Gecko   : [MediaPlayback #2]: V/MediaDecoder VideoSink=7fdf9ef0 discarding video frame mTime=4045708 clock_time=4117000
09-01 03:28:00.036  7760  8158 I Gecko   : [MediaPlayback #1]: V/MediaDecoder VideoSink=7fdf9ef0 discarding video frame mTime=4087416 clock_time=4137000
09-01 03:28:00.039  7760  8158 I Gecko   : [MediaPlayback #1]: V/MediaDecoder VideoSink=7fdf9ef0 playing video frame 4129125 (id=64) (vq-queued=1)
James, 
Please discuss with John to move this bug forward. 
Thanks!
Assignee: nobody → jacheng
Our gecko implementation will do the recreating and initializing decoder when resolution changed[1].

It will trigger the Android MediaCode configure[2] being called.

I have measured the performance on Decoder Init -> Promise Resolved [3], it cost about 200ms to do the initialization(MediaCodec.configure involved).

It is the root cause why dropping frame when resolution changed.


I attempt to disable the recreating and re-initializing behavior to see if the dropping frame symptom can be eased. 
First, I remove these two line [1]. The decoder will not recreate by this change but I still can see the MediaCodec.configure[2] being called. I think the caller is not gecko, maybe there is some reconfigure mechanism inside the Android framework. Thus, the dropping frame still happened.

Next, I will try on "enabling seamless change in resolution during playback" mentioned by [4] to see if we can stop MediaCodec.configure being called repeatedly.



[1]
http://searchfox.org/mozilla-central/rev/021e8e0cee4f446757b8b1ffd312272174d6fc7b/dom/media/MediaFormatReader.cpp#999-1000

[2] MediaCodec.configure
https://developer.android.com/reference/android/media/MediaCodec.html#configure(android.media.MediaFormat, android.view.Surface, android.media.MediaCrypto, int)

[3] Decoder Init
http://searchfox.org/mozilla-central/rev/021e8e0cee4f446757b8b1ffd312272174d6fc7b/dom/media/MediaFormatReader.cpp#467

[4] enabling seamless change in resolution during playback
https://developer.android.com/about/versions/android-4.4.html#Multimedia
Upload the test I did which is trying to avoid re-configure MediaCodec according to [1] referenced [2].
The result is failed, still got MediaCodec configured repeatedly and dropped frames.
The patch includes
(1) Try to disable re-create decoder when resolution changed.
(2) Add log to see if (1) works and add log to see if the codec supported adaptive playback feature(Yes, supported).
(3) Try to add  ```format->SetInteger(MediaFormat::KEY_MAX_WIDTH, 1920);``` and ```format->SetInteger(MediaFormat::KEY_MAX_HEIGHT, 1080);``` the same as [2] did.

Hi esawin,
Since you have more experience on Android playback, do you have any idea that I could try to move forward?

Thank you!




[1]
https://developer.android.com/about/versions/android-4.4.html#Multimedia
[2]
http://androidxref.com/5.1.1_r6/xref/external/chromium_org/media/base/android/java/src/org/chromium/media/MediaCodecBridge.java#509
[3] Test sample
http://s3.cf.web.us.aiv-cdn.net/HTML5PartnerTest.html?asin=B00TYBBNAW&videoType=Trailer&logging
Flags: needinfo?(esawin)
James, 
Could you attach the logs you captured? we should be able to check where and how Android framework decides to reconfigure. Based on that, we can know how to configure codec correctly.
Hi jya,

John told me that you said the repeatedly initializing decoder symptom is due to [1].
I was trying to remove these three lines to see if anything can improve.

But the playback get stuck... 

I think it is not easy for me to change the current code to make MSE don't re-create decoder.

Could you please give me some hints or guidance how to get rid of re-creating decoder mechanism that I can do further attempt? Or we can have a f2f discussion.

Thank you.



[1]
http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/dom/media/platforms/wrappers/H264Converter.cpp#279-281
Flags: needinfo?(esawin) → needinfo?(jyavenard)
Attachment #8806613 - Attachment is obsolete: true
update the patch consulted with jya.

But the playback still got frozen.
Attachment #8806652 - Attachment is obsolete: true
Run this patch on PC nightly browser, I will get numerous decoding errors.
update patch which is workable on PC(linux platform). Avoid recreating decoder.
Attachment #8807057 - Attachment is obsolete: true
Attachment #8807072 - Attachment is obsolete: true
The logic of this patch is the same as attachment 8807095 [details] [diff] [review], but the playback got frozen...

I even hard code the surface size to 1920*1080, but it still cannot solve this problem...
Hi esawin,

Since you have more experience on Android playback,

I would like to ask for your advice on this bug...

The video playback will be stuttered when the MSE recreating decoder(initialization and configuration) by resolution change or SPS change.

Base on comment 6 and comment 13

I have tried to disable the recreating decoder behavior to see if the stuttered symptom can be eased. The patch(attachment 8807095 [details] [diff] [review]) works fine on Desktop browser(my ubuntu PC) but fails on android (attachment 8807098 [details] [diff] [review]) with video frame frozen.

Currently, I have no idea how to do further to make it work...

Do you have any idea that I could try to move forward?

Thank you!
Flags: needinfo?(esawin)
Flags: needinfo?(jyavenard)
I haven't worked on adaptive video playback, so I don't know the peculiarities.
You can try flushing the decoder prior to updating the config in H264Convert to avoid mismatched samples.
Flags: needinfo?(esawin)
There should be no need to flush the decoder with the H264Converter, when the resolution changes, it makes sure that the next sample is a key frame. 
Also flushing is what we're trying to avoid to make the transition smoother.
(In reply to Jean-Yves Avenard [:jya] from comment #16)
> There should be no need to flush the decoder with the H264Converter, when
> the resolution changes, it makes sure that the next sample is a key frame. 
> Also flushing is what we're trying to avoid to make the transition smoother.

Unfortunately, MediaFormatReader request draining when res. change [1], which MediaCodecDataDecoder implements by sending EOS. According to [2], after that MediaCodec won't accept further input until flush() is called. :(

[1] http://searchfox.org/mozilla-central/source/dom/media/MediaFormatReader.cpp#986
[2] https://developer.android.com/images/media/mediacodec_states.png
As comment 17 said,

We should call flush as this patch did, the video can be played without recreating the decoder. The log from the testing website will not show dropping frame anymore which means the root cause for dropping frame is recreating and reinitializing MediaCodec decoder.

But as comment 16 said,
With "flush", the transition is not smooth... I still can feel the stuttering during playback.
Attachment #8807098 - Attachment is obsolete: true
This patch is to disable the recreating and do not drain decoder when resolution changed.

The stuttering symptom is improved a little bit but still not smooth.
Attachment #8807095 - Attachment is obsolete: true
Attachment #8808688 - Attachment is obsolete: true
Also update the patch for desktop.
If the improvements are negligible, I would strongly prefer that we leave things as-is...

As draining and reusing the decoder across resolutions only for android is bad IMHO. Much prefer that we have a common code path.
Depends on: 1317239
Hi jya,

As your suggestion on IRC, IIUC, I change the code [1] which may return 3 in my case into return 10 (hard coded)...

But the symptom is still existing especially on Youtube live stream which stream id changed very frequently(I do not exactly know the meaning of id change).

Observing the behavior of Chrome app, it create the decoder once and use it without recreation(by adding some log to android framework).

Unfortunately, the Amazon test link cannot be played by Chrome app on Android, so I cannot compare the difference between these two browser(maybe chrome also performs bad).

Do I ignore any changes or misunderstand your word. 


Thank you for your help.
[1]
http://searchfox.org/mozilla-central/rev/886d5186f0598ab2f8a9953eb5a4dab9750ef834/dom/media/MediaDecoderStateMachine.cpp#3214
Flags: needinfo?(jyavenard)
The theory on why you're seeing the symptoms described, is that the frames are being invalidated when the decoder is being shutdown.

As such, changing the size of the video queue will do nothing to help the problem. Quite the opposite, if the frames were to be invalidated, having more frames in the queue would cause the visual disruption to be more noticeable.

So what I would like you to investigate, is if it's possible to not invalidate the frames returned by the old decoder, or maybe have the Android MediaDataDecoder make a copy before returning them to the MediaFormatReader, etc...

My comment about increasing the video queue size was only to have it compensate for the time it takes to create a new decoder, But of course, this is assuming that the previous video frames returned are still valid !

Hope that clarifies the problem.
Flags: needinfo?(jyavenard)
(In reply to Jean-Yves Avenard [:jya] from comment #23)
> As such, changing the size of the video queue will do nothing to help the
> problem. Quite the opposite, if the frames were to be invalidated, having
> more frames in the queue would cause the visual disruption to be more
> noticeable.

How does deleting an old decoder invalidate the frames it generates?
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #24)
> How does deleting an old decoder invalidate the frames it generates?

very good question !

if it didn't, why would you see a visual change when the decoder is destroyed and re-created ?

It was said that the destroy/recreate took over 200ms, but with YouTube typical 15fps, all you need is more than 3 frames in the video queue to have sufficient video content for those 200ms be unoticeable (and why I suggested to increase the size of the MDSM video queue).
Seeing from comment 22, that even with a queue size of 10 (so 666ms worth) you still get the problem -> therefore it's something else. 
This put us back to the video frames being invalidated when the decoder is being shutdown.

It's entirely possible, it was happening on the mac VT decoder too, we have to make sure we increased the RefCount of the returned IOSurface so that when the decoder was shutdown and it deleted its pool of video frames, the ones returned to the MDSM were still usable.
(In reply to Jean-Yves Avenard [:jya] from comment #25)
> It's entirely possible, it was happening on the mac VT decoder too, we have
> to make sure we increased the RefCount of the returned IOSurface so that
> when the decoder was shutdown and it deleted its pool of video frames, the
> ones returned to the MDSM were still usable.

If that is the case, I wonder how pause-on-dormant works without causing visual artifacts where the decoders are deleted when entering dormant and the image container still keeps the last frame outputted by the decoder.
Blocks: 1286738
I make a stupid mistake that I forget to remove the decoder->Flush in H264Converter.cpp while I remove drain decoder when resolution changed mechanism.

This patch can immensely improved the Youtube live streaming performance issue which is very very bad in current Fennec release.

This patch can also fix the Amazon issue.

When adding some log in AOSP source code,

I found that Chrome will set 

format->SetInteger("feature-adaptive-playback", 1);  in MediaFormat object.

I cannot find any document mentioned this setting but I think this setting may take effect on certain situation. I will do the same thing.

I will start to let the code be landed.
Attachment #8809337 - Attachment is obsolete: true
Note that, i mentioned before, I do not want to reintroduce a different code path for decoding data.
That is reusing the same decoder across all resolution.

So please, do not continue down that path, it's the wrong way to go about it... and you still haven't explained on the why transition is bad when a new decoder is used.
(In reply to James Cheng[:JamesCheng] from comment #27)
> Created attachment 8811650 [details] [diff] [review]
> Fix smooth adaptive playback on android.patch
> 
> I make a stupid mistake that I forget to remove the decoder->Flush in
> H264Converter.cpp while I remove drain decoder when resolution changed
> mechanism.
> 
> This patch can immensely improved the Youtube live streaming performance
> issue which is very very bad in current Fennec release.
> 
> This patch can also fix the Amazon issue.
Good job! Now at least we have a solution to fix this bug. 



(In reply to Jean-Yves Avenard [:jya] from comment #28)
> Note that, i mentioned before, I do not want to reintroduce a different code
> path for decoding data.
Agree. Unless we don't have other choices...
> That is reusing the same decoder across all resolution.
> 
> So please, do not continue down that path, it's the wrong way to go about
> it... and you still haven't explained on the why transition is bad when a
> new decoder is used.
Yes. James will provide more information about that transition. I believe that transition (the logged time difference between the last decoded frame outputted from the old decoder and the first decoded frame outputted from the new decoder)is more time than 200ms. Per comment 5, 200ms is measured just for Decoder init time and might not represent the whole transition.
As comment 2 described, without increasing the queue size, the decoded sample will not arrived in time.

With increasing the queue size to 10, I cannot find why I still met unsmooth transition as shown on below logs[3].
The gap between the last low resolution sample and the first high resolution sample is about 5xx ms.
The gap between recreating decoder and the first high resolution sample decoded is about 3xx ms which can be covered by increasing the queue size.
And the videosink log shown that there is no discarding frame occurred.

In theory, when recreating a decoder we will recreate a surfacetexture[1] which may lead to realloc the graphic buffer. I didn't know the implementation of Android but it seems it is the limitation so they introduce a seamless playback approach by setting some config on MediaFormat[2].
Maybe power consumption is another reason for handheld devices.

I will try to prove this is the limitation on Android...


[1] 
http://searchfox.org/mozilla-central/rev/935627b015aaabbd2dffa90cce051521f22dd7e6/dom/media/platforms/android/MediaCodecDataDecoder.cpp#76

[2]
https://developer.android.com/about/versions/android-4.4.html#Multimedia

[3]
I/Gecko   (31748): 2016-11-21 09:02:08.948559 UTC - [MC Decoder]: V/MediaFormatReader MediaFormatReader(99cef000)::void mozilla::MediaFormatReader::Output(mozilla::MediaFormatReader::TrackType, mozilla::MediaData*): Decoded Video sample time=9968291 timecode=9968291 kf=0 dur=41709
I/Gecko   (31748): 2016-11-21 09:02:08.957809 UTC - [MediaPlayback #1]: V/MediaDecoder VideoSink=99df7660 playing video frame 9342666 (id=e1) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:08.957883 UTC - [MediaPlayback #1]: V/MediaDecoder VideoSink=99df7660 playing video frame 9384375 (id=e2) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:08.957942 UTC - [MediaPlayback #1]: V/MediaDecoder VideoSink=99df7660 playing video frame 9426083 (id=e3) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:08.957998 UTC - [MediaPlayback #1]: V/MediaDecoder VideoSink=99df7660 playing video frame 9467791 (id=e4) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:08.958056 UTC - [MediaPlayback #1]: V/MediaDecoder VideoSink=99df7660 playing video frame 9509500 (id=e5) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:08.958112 UTC - [MediaPlayback #1]: V/MediaDecoder VideoSink=99df7660 playing video frame 9551208 (id=e6) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:08.958167 UTC - [MediaPlayback #1]: V/MediaDecoder VideoSink=99df7660 playing video frame 9592916 (id=e7) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:08.958224 UTC - [MediaPlayback #1]: V/MediaDecoder VideoSink=99df7660 playing video frame 9634625 (id=e8) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:08.958280 UTC - [MediaPlayback #1]: V/MediaDecoder VideoSink=99df7660 playing video frame 9676333 (id=e9) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:08.958335 UTC - [MediaPlayback #1]: V/MediaDecoder VideoSink=99df7660 playing video frame 9718041 (id=ea) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:08.997944 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9384375 (id=e2) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:08.998030 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9426083 (id=e3) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:08.998089 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9467791 (id=e4) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:08.998147 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9509500 (id=e5) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:08.998203 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9551208 (id=e6) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:08.998259 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9592916 (id=e7) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:08.998315 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9634625 (id=e8) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:08.998371 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9676333 (id=e9) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:08.998427 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9718041 (id=ea) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:08.998483 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9759750 (id=eb) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.039682 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9384375 (id=e2) (vq-queued=11)
I/Gecko   (31748): 2016-11-21 09:02:09.039811 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9426083 (id=e3) (vq-queued=11)
I/Gecko   (31748): 2016-11-21 09:02:09.039871 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9467791 (id=e4) (vq-queued=11)
I/Gecko   (31748): 2016-11-21 09:02:09.039927 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9509500 (id=e5) (vq-queued=11)
I/Gecko   (31748): 2016-11-21 09:02:09.039982 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9551208 (id=e6) (vq-queued=11)
I/Gecko   (31748): 2016-11-21 09:02:09.040037 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9592916 (id=e7) (vq-queued=11)
I/Gecko   (31748): 2016-11-21 09:02:09.040093 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9634625 (id=e8) (vq-queued=11)
I/Gecko   (31748): 2016-11-21 09:02:09.040149 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9676333 (id=e9) (vq-queued=11)
I/Gecko   (31748): 2016-11-21 09:02:09.040204 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9718041 (id=ea) (vq-queued=11)
I/Gecko   (31748): 2016-11-21 09:02:09.040260 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9759750 (id=eb) (vq-queued=11)
I/Gecko   (31748): 2016-11-21 09:02:09.040315 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9801458 (id=ec) (vq-queued=11)
I/Gecko   (31748): 2016-11-21 09:02:09.049139 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9426083 (id=e3) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.049221 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9467791 (id=e4) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.049275 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9509500 (id=e5) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.049336 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9551208 (id=e6) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.049389 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9592916 (id=e7) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.049446 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9634625 (id=e8) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.049500 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9676333 (id=e9) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.049555 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9718041 (id=ea) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.049610 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9759750 (id=eb) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.049665 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9801458 (id=ec) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.083078 UTC - [MediaPlayback #2]: V/MediaDecoder VideoSink=99df7660 playing video frame 9467791 (id=e4) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.083142 UTC - [MediaPlayback #2]: V/MediaDecoder VideoSink=99df7660 playing video frame 9509500 (id=e5) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.083195 UTC - [MediaPlayback #2]: V/MediaDecoder VideoSink=99df7660 playing video frame 9551208 (id=e6) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.083249 UTC - [MediaPlayback #2]: V/MediaDecoder VideoSink=99df7660 playing video frame 9592916 (id=e7) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.083300 UTC - [MediaPlayback #2]: V/MediaDecoder VideoSink=99df7660 playing video frame 9634625 (id=e8) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.083355 UTC - [MediaPlayback #2]: V/MediaDecoder VideoSink=99df7660 playing video frame 9676333 (id=e9) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.083405 UTC - [MediaPlayback #2]: V/MediaDecoder VideoSink=99df7660 playing video frame 9718041 (id=ea) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.083458 UTC - [MediaPlayback #2]: V/MediaDecoder VideoSink=99df7660 playing video frame 9759750 (id=eb) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.083510 UTC - [MediaPlayback #2]: V/MediaDecoder VideoSink=99df7660 playing video frame 9801458 (id=ec) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.083563 UTC - [MediaPlayback #2]: V/MediaDecoder VideoSink=99df7660 playing video frame 9843166 (id=ed) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.122194 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9509500 (id=e5) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.122274 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9551208 (id=e6) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.122333 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9592916 (id=e7) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.122389 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9634625 (id=e8) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.122445 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9676333 (id=e9) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.122500 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9718041 (id=ea) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.122556 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9759750 (id=eb) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.122613 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9801458 (id=ec) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.122668 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9843166 (id=ed) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.122724 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9884875 (id=ee) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.164806 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9551208 (id=e6) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.164873 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9592916 (id=e7) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.164928 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9634625 (id=e8) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.164983 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9676333 (id=e9) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.165034 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9718041 (id=ea) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.165091 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9759750 (id=eb) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.165142 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9801458 (id=ec) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.165197 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9843166 (id=ed) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.165250 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9884875 (id=ee) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.165304 UTC - [MediaPlayback #4]: V/MediaDecoder VideoSink=99df7660 playing video frame 9926583 (id=ef) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.206168 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9592916 (id=e7) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.206253 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9634625 (id=e8) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.206311 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9676333 (id=e9) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.206366 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9718041 (id=ea) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.206422 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9759750 (id=eb) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.206477 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9801458 (id=ec) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.206532 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9843166 (id=ed) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.206587 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9884875 (id=ee) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.206642 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9926583 (id=ef) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.206698 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9968291 (id=f0) (vq-queued=10)
I/Gecko   (31748): 2016-11-21 09:02:09.207846 UTC - [MediaPlayback #3]: D/MediaFormatReader MediaFormatReader(99cef000)::void mozilla::MediaFormatReader::HandleDemuxedSamples(mozilla::MediaFormatReader::TrackType, mozilla::AbstractMediaDecoder::AutoNotifyDecoded&): Video stream id has changed from:3 to:4, recreating decoder.
I/Gecko   (31748): 2016-11-21 09:02:09.247070 UTC - [MediaPlayback #2]: V/MediaDecoder VideoSink=99df7660 playing video frame 9634625 (id=e8) (vq-queued=9)
I/Gecko   (31748): 2016-11-21 09:02:09.247112 UTC - [MediaPlayback #2]: V/MediaDecoder VideoSink=99df7660 playing video frame 9676333 (id=e9) (vq-queued=9)
I/Gecko   (31748): 2016-11-21 09:02:09.247144 UTC - [MediaPlayback #2]: V/MediaDecoder VideoSink=99df7660 playing video frame 9718041 (id=ea) (vq-queued=9)
I/Gecko   (31748): 2016-11-21 09:02:09.247178 UTC - [MediaPlayback #2]: V/MediaDecoder VideoSink=99df7660 playing video frame 9759750 (id=eb) (vq-queued=9)
I/Gecko   (31748): 2016-11-21 09:02:09.247209 UTC - [MediaPlayback #2]: V/MediaDecoder VideoSink=99df7660 playing video frame 9801458 (id=ec) (vq-queued=9)
I/Gecko   (31748): 2016-11-21 09:02:09.247240 UTC - [MediaPlayback #2]: V/MediaDecoder VideoSink=99df7660 playing video frame 9843166 (id=ed) (vq-queued=9)
I/Gecko   (31748): 2016-11-21 09:02:09.247269 UTC - [MediaPlayback #2]: V/MediaDecoder VideoSink=99df7660 playing video frame 9884875 (id=ee) (vq-queued=9)
I/Gecko   (31748): 2016-11-21 09:02:09.247300 UTC - [MediaPlayback #2]: V/MediaDecoder VideoSink=99df7660 playing video frame 9926583 (id=ef) (vq-queued=9)
I/Gecko   (31748): 2016-11-21 09:02:09.247330 UTC - [MediaPlayback #2]: V/MediaDecoder VideoSink=99df7660 playing video frame 9968291 (id=f0) (vq-queued=9)
I/Gecko   (31748): 2016-11-21 09:02:09.290021 UTC - [MediaPlayback #1]: V/MediaDecoder VideoSink=99df7660 playing video frame 9634625 (id=e8) (vq-queued=9)
I/Gecko   (31748): 2016-11-21 09:02:09.290136 UTC - [MediaPlayback #1]: V/MediaDecoder VideoSink=99df7660 playing video frame 9676333 (id=e9) (vq-queued=9)
I/Gecko   (31748): 2016-11-21 09:02:09.290198 UTC - [MediaPlayback #1]: V/MediaDecoder VideoSink=99df7660 playing video frame 9718041 (id=ea) (vq-queued=9)
I/Gecko   (31748): 2016-11-21 09:02:09.290256 UTC - [MediaPlayback #1]: V/MediaDecoder VideoSink=99df7660 playing video frame 9759750 (id=eb) (vq-queued=9)
I/Gecko   (31748): 2016-11-21 09:02:09.290314 UTC - [MediaPlayback #1]: V/MediaDecoder VideoSink=99df7660 playing video frame 9801458 (id=ec) (vq-queued=9)
I/Gecko   (31748): 2016-11-21 09:02:09.290370 UTC - [MediaPlayback #1]: V/MediaDecoder VideoSink=99df7660 playing video frame 9843166 (id=ed) (vq-queued=9)
I/Gecko   (31748): 2016-11-21 09:02:09.290425 UTC - [MediaPlayback #1]: V/MediaDecoder VideoSink=99df7660 playing video frame 9884875 (id=ee) (vq-queued=9)
I/Gecko   (31748): 2016-11-21 09:02:09.290483 UTC - [MediaPlayback #1]: V/MediaDecoder VideoSink=99df7660 playing video frame 9926583 (id=ef) (vq-queued=9)
I/Gecko   (31748): 2016-11-21 09:02:09.290543 UTC - [MediaPlayback #1]: V/MediaDecoder VideoSink=99df7660 playing video frame 9968291 (id=f0) (vq-queued=9)
I/Gecko   (31748): 2016-11-21 09:02:09.299152 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9676333 (id=e9) (vq-queued=8)
I/Gecko   (31748): 2016-11-21 09:02:09.299234 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9718041 (id=ea) (vq-queued=8)
I/Gecko   (31748): 2016-11-21 09:02:09.299295 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9759750 (id=eb) (vq-queued=8)
I/Gecko   (31748): 2016-11-21 09:02:09.299355 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9801458 (id=ec) (vq-queued=8)
I/Gecko   (31748): 2016-11-21 09:02:09.299412 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9843166 (id=ed) (vq-queued=8)
I/Gecko   (31748): 2016-11-21 09:02:09.299470 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9884875 (id=ee) (vq-queued=8)
I/Gecko   (31748): 2016-11-21 09:02:09.299529 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9926583 (id=ef) (vq-queued=8)
I/Gecko   (31748): 2016-11-21 09:02:09.299587 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9968291 (id=f0) (vq-queued=8)
I/Gecko   (31748): 2016-11-21 09:02:09.333387 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9718041 (id=ea) (vq-queued=7)
I/Gecko   (31748): 2016-11-21 09:02:09.333485 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9759750 (id=eb) (vq-queued=7)
I/Gecko   (31748): 2016-11-21 09:02:09.333521 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9801458 (id=ec) (vq-queued=7)
I/Gecko   (31748): 2016-11-21 09:02:09.333557 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9843166 (id=ed) (vq-queued=7)
I/Gecko   (31748): 2016-11-21 09:02:09.333591 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9884875 (id=ee) (vq-queued=7)
I/Gecko   (31748): 2016-11-21 09:02:09.333626 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9926583 (id=ef) (vq-queued=7)
I/Gecko   (31748): 2016-11-21 09:02:09.333659 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9968291 (id=f0) (vq-queued=7)
I/Gecko   (31748): 2016-11-21 09:02:09.373843 UTC - [MediaPlayback #2]: V/MediaDecoder VideoSink=99df7660 playing video frame 9759750 (id=eb) (vq-queued=6)
I/Gecko   (31748): 2016-11-21 09:02:09.373921 UTC - [MediaPlayback #2]: V/MediaDecoder VideoSink=99df7660 playing video frame 9801458 (id=ec) (vq-queued=6)
I/Gecko   (31748): 2016-11-21 09:02:09.373985 UTC - [MediaPlayback #2]: V/MediaDecoder VideoSink=99df7660 playing video frame 9843166 (id=ed) (vq-queued=6)
I/Gecko   (31748): 2016-11-21 09:02:09.374042 UTC - [MediaPlayback #2]: V/MediaDecoder VideoSink=99df7660 playing video frame 9884875 (id=ee) (vq-queued=6)
I/Gecko   (31748): 2016-11-21 09:02:09.374099 UTC - [MediaPlayback #2]: V/MediaDecoder VideoSink=99df7660 playing video frame 9926583 (id=ef) (vq-queued=6)
I/Gecko   (31748): 2016-11-21 09:02:09.374154 UTC - [MediaPlayback #2]: V/MediaDecoder VideoSink=99df7660 playing video frame 9968291 (id=f0) (vq-queued=6)
I/Gecko   (31748): 2016-11-21 09:02:09.415809 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9801458 (id=ec) (vq-queued=5)
I/Gecko   (31748): 2016-11-21 09:02:09.415886 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9843166 (id=ed) (vq-queued=5)
I/Gecko   (31748): 2016-11-21 09:02:09.415947 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9884875 (id=ee) (vq-queued=5)
I/Gecko   (31748): 2016-11-21 09:02:09.416006 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9926583 (id=ef) (vq-queued=5)
I/Gecko   (31748): 2016-11-21 09:02:09.416063 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9968291 (id=f0) (vq-queued=5)
I/Gecko   (31748): 2016-11-21 09:02:09.456406 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9843166 (id=ed) (vq-queued=4)
I/Gecko   (31748): 2016-11-21 09:02:09.456469 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9884875 (id=ee) (vq-queued=4)
I/Gecko   (31748): 2016-11-21 09:02:09.456525 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9926583 (id=ef) (vq-queued=4)
I/Gecko   (31748): 2016-11-21 09:02:09.456581 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9968291 (id=f0) (vq-queued=4)
I/Gecko   (31748): 2016-11-21 09:02:09.500303 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9884875 (id=ee) (vq-queued=3)
I/Gecko   (31748): 2016-11-21 09:02:09.500382 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9926583 (id=ef) (vq-queued=3)
I/Gecko   (31748): 2016-11-21 09:02:09.500445 UTC - [MediaPlayback #3]: V/MediaDecoder VideoSink=99df7660 playing video frame 9968291 (id=f0) (vq-queued=3)
I/Gecko   (31748): 2016-11-21 09:02:09.523045 UTC - [MC Decoder]: V/MediaFormatReader MediaFormatReader(99cef000)::void mozilla::MediaFormatReader::Output(mozilla::MediaFormatReader::TrackType, mozilla::MediaData*): Decoded Video sample time=10010000 timecode=10010000 kf=1 dur=41708
(In reply to James Cheng[:JamesCheng] from comment #30)
> As comment 2 described, without increasing the queue size, the decoded
> sample will not arrived in time.
> 
> With increasing the queue size to 10, I cannot find why I still met unsmooth
> transition as shown on below logs[3].
> The gap between the last low resolution sample and the first high resolution
> sample is about 5xx ms.
For 5xx ms, it seems remotely possible to find a way to make playback smooth with re-creating decoder approach.
Attachment #8814349 - Flags: review?(jyavenard)
Comment on attachment 8814349 [details]
Bug 1299105 - Part1 - Check if the decoder support recycling to prevent from recreating decoder.

https://reviewboard.mozilla.org/r/95618/#review95828

::: dom/media/MediaFormatReader.cpp:1306
(Diff revision 1)
>        if (samplesPending) {
>          // Let existing samples complete their decoding. We'll resume later.
>          return;
>        }
>  
> +      auto supportRecycling = decoder.mDecoder->SupportDecoderRecycling();

Shoudn't you take into account whehter the android decoder is supporting adaptive playback or not before doing the following steps ?

I mean, if the data decoder created from MediaCodec doesn't support adaptive playback, there's no chance to recreate data decoder here, right ?
Blocks: 1320618
(In reply to Kilik Kuo [:kikuo] from comment #34)
> Comment on attachment 8814349 [details]
> Bug 1299105 - Check if the decoder support recycling to prevent from
> recreating decoder.
> 
> https://reviewboard.mozilla.org/r/95618/#review95828
> 
> ::: dom/media/MediaFormatReader.cpp:1306
> (Diff revision 1)
> >        if (samplesPending) {
> >          // Let existing samples complete their decoding. We'll resume later.
> >          return;
> >        }
> >  
> > +      auto supportRecycling = decoder.mDecoder->SupportDecoderRecycling();
> 
> Shoudn't you take into account whehter the android decoder is supporting
> adaptive playback or not before doing the following steps ?
> 
> I mean, if the data decoder created from MediaCodec doesn't support adaptive
> playback, there's no chance to recreate data decoder here, right ?

Yes, it sounds reasonable but I prefer to do it in another follow up bug due to it is not that easy to ask for "adaptive playback" feature.

Bug 1320618 is the follow up bug. 

Thanks for the suggestion.
(In reply to James Cheng[:JamesCheng] from comment #35)
> 
> Yes, it sounds reasonable but I prefer to do it in another follow up bug due
> to it is not that easy to ask for "adaptive playback" feature.
> 
> Bug 1320618 is the follow up bug. 
> 
> Thanks for the suggestion.

If you have to create a decoder in Fennec process to ask for codec capability in out-of-process mode,
first, maybe we should sync the creation of data decoder for both in-process decode and out-of-process decode.

Current behavior, 
a) MediaCodecDataDecoder create decoder by mimetype [1].
b) JellyBeanAsyncCodec create decoder by a codec name which is obtained from MediaFormat [2].

[1] http://searchfox.org/mozilla-central/rev/9a3ab17838f039aab023837d905115f8990e442e/dom/media/platforms/android/MediaCodecDataDecoder.cpp#48
[2] http://searchfox.org/mozilla-central/rev/9a3ab17838f039aab023837d905115f8990e442e/mobile/android/base/java/org/mozilla/gecko/media/Codec.java#241-249

That may also need to be done before/in Bug 1320618.
Comment on attachment 8814349 [details]
Bug 1299105 - Part1 - Check if the decoder support recycling to prevent from recreating decoder.

https://reviewboard.mozilla.org/r/95618/#review95860

::: dom/media/MediaFormatReader.cpp:1306
(Diff revision 1)
>        if (samplesPending) {
>          // Let existing samples complete their decoding. We'll resume later.
>          return;
>        }
>  
> +      auto supportRecycling = decoder.mDecoder->SupportDecoderRecycling();

it's a bool, don't use auto. Otherwise have to wonder what the type is

::: dom/media/platforms/android/RemoteDataDecoder.cpp:248
(Diff revision 1)
>    {
>      RemoteDataDecoder::Input(aSample);
>      mInputDurations.Put(aSample->mDuration);
>    }
>  
> +  bool SupportDecoderRecycling() const override { return true; }

Are we sure about that ?

Isn't this supported only on certain version of Android?
Attachment #8814349 - Flags: review?(jyavenard) → review+
Comment on attachment 8814349 [details]
Bug 1299105 - Part1 - Check if the decoder support recycling to prevent from recreating decoder.

https://reviewboard.mozilla.org/r/95618/#review95868

::: dom/media/MediaFormatReader.cpp:1309
(Diff revision 1)
>        }
>  
> +      auto supportRecycling = decoder.mDecoder->SupportDecoderRecycling();
>        if (decoder.mNextStreamSourceID.isNothing() ||
>            decoder.mNextStreamSourceID.ref() != info->GetID()) {
> +        if (!supportRecycling) {

this MUST be behind a pref, and disabled by default as we discussed last week.

This should be our least preferred solution, and instead, as discussed, fix the buffer lifecycle issue.
Attachment #8815173 - Flags: review?(jyavenard)
Comment on attachment 8814349 [details]
Bug 1299105 - Part1 - Check if the decoder support recycling to prevent from recreating decoder.

https://reviewboard.mozilla.org/r/95618/#review95828

> Shoudn't you take into account whehter the android decoder is supporting adaptive playback or not before doing the following steps ?
> 
> I mean, if the data decoder created from MediaCodec doesn't support adaptive playback, there's no chance to recreate data decoder here, right ?

Thank you for the suggestion, will do it in Bug 1320618
Comment on attachment 8814349 [details]
Bug 1299105 - Part1 - Check if the decoder support recycling to prevent from recreating decoder.

https://reviewboard.mozilla.org/r/95618/#review95860

> it's a bool, don't use auto. Otherwise have to wonder what the type is

addressed, thanks

> Are we sure about that ?
> 
> Isn't this supported only on certain version of Android?

I will make it more conprehensive on Bug 1320618.
Comment on attachment 8814349 [details]
Bug 1299105 - Part1 - Check if the decoder support recycling to prevent from recreating decoder.

https://reviewboard.mozilla.org/r/95618/#review95868

> this MUST be behind a pref, and disabled by default as we discussed last week.
> 
> This should be our least preferred solution, and instead, as discussed, fix the buffer lifecycle issue.

Do it in part2 , thank you very much for helping and guidance on this bug.
Comment on attachment 8815173 [details]
Bug 1299105 - Part2 - Make Part1 works behind a pref.

https://reviewboard.mozilla.org/r/96206/#review96652

::: dom/media/MediaFormatReader.cpp:1307
(Diff revision 1)
>        if (samplesPending) {
>          // Let existing samples complete their decoding. We'll resume later.
>          return;
>        }
>  
> -      bool supportRecycling = decoder.mDecoder->SupportDecoderRecycling();
> +      bool supportRecycling = mCheckDecoderRecycling && decoder.mDecoder->SupportDecoderRecycling();

no need for a member here.

MediaPrefs is cached already and you use it once.

this only occurs when a stream id change, which isn't time critical

::: dom/media/MediaPrefs.h:152
(Diff revision 1)
>    DECL_MEDIA_PREF("media.webspeech.recognition.enable",       WebSpeechRecognitionEnabled, bool, false);
>    DECL_MEDIA_PREF("media.webspeech.recognition.force_enable", WebSpeechRecognitionForceEnabled, bool, false);
>  
>    DECL_MEDIA_PREF("media.num-decode-threads",                 MediaThreadPoolDefaultCount, uint32_t, 4);
>    DECL_MEDIA_PREF("media.decoder.limit",                      MediaDecoderLimit, int32_t, MediaDecoderLimitDefault());
> +  DECL_MEDIA_PREF("media.video_check-decoder-recycling",       MediaDecoderCheckRecycling, bool, false);

media.recycle-decoder.enabled

move it up with the other PDM preferences, and align it with the previous line (it's one character out)

::: dom/media/platforms/wrappers/H264Converter.h:77
(Diff revision 1)
>    MozPromiseRequestHolder<InitPromise> mInitPromiseRequest;
>    RefPtr<GMPCrashHelper> mGMPCrashHelper;
>    bool mNeedAVCC;
>    nsresult mLastError;
>    bool mNeedKeyframe = true;
> +  bool mCheckDecoderRecycling;

same comment as in MediaFormatReader, no need for a member.
Attachment #8815173 - Flags: review?(jyavenard) → review+
Thanks for review, rebasing the patch and check-in-needed.
Keywords: checkin-needed
Comment on attachment 8815173 [details]
Bug 1299105 - Part2 - Make Part1 works behind a pref.

https://reviewboard.mozilla.org/r/96206/#review96690

::: dom/media/MediaPrefs.h:133
(Diff revision 2)
>    DECL_MEDIA_PREF("media.wmf.decoder.thread-count",           PDMWMFThreadCount, int32_t, -1);
>  #endif
>    DECL_MEDIA_PREF("media.decoder.fuzzing.enabled",            PDMFuzzingEnabled, bool, false);
>    DECL_MEDIA_PREF("media.decoder.fuzzing.video-output-minimum-interval-ms", PDMFuzzingInterval, uint32_t, 0);
>    DECL_MEDIA_PREF("media.decoder.fuzzing.dont-delay-inputexhausted", PDMFuzzingDelayInputExhausted, bool, true);
> +  DECL_MEDIA_PREF("media.recycle-decoder.enabled",            MediaDecoderCheckRecycling, bool, false);

sorry for being a pain, but looking at the other preference name
media.decoder.recycle.enabled seems to be a better choice.
Sure, I will rename it. Thank you very much.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/00edc44f7cf8
Part 1: Check if the decoder support recycling to prevent from recreating decoder. r=jya
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a5772d0f6a0
Part 2: Make Part 1 works behind a pref. r=jya
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/00edc44f7cf8
https://hg.mozilla.org/mozilla-central/rev/2a5772d0f6a0
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Blocks: 1321727
It sounds very dangerous to me to want to uplift this immediately when it has received no testing coverage whatsoever and we don't know if all devices support it.
Blocks: 1321756
Sorina, 
Can you help test it?
Flags: qe-verify?
Flags: needinfo?(sorina.florean)
Hi all,

Tested and verified with: 
- Nexus 9 (Android 7.0)
- Asus ZenPad 8 (Android 6.0.1) and Nexus 5 (Android 6.0.1)
- LG G4 (Android 5.1)
- Lenovo A536 (Android 4.4.2)

The videos 1 and 2 is playing without difficulty or delays, on latest Nightly (2016-12-08).
1. http://s3.cf.web.us.aiv-cdn.net/HTML5PartnerTest.html?asin=amzn1.dv.lcid.4d5e5db9-7ca8-4bca-8181-898539e3e60d&videoType=External&clear&ui
2. http://s3.cf.web.us.aiv-cdn.net/HTML5PartnerTest.html?asin=B00TYBBNAW&videoType=Trailer&ui
Flags: needinfo?(sorina.florean)
Sorina, 
Thanks! 
Can you also help test live stream on Youtube? Just search "live stream" in youtube.
Flags: needinfo?(sorina.florean)
There's no reason testing should focus on live stream. Any videos using MSE will be impacted. This includes YouTube videos. 

You can manually change the resolution there too
Yeah. We should check normal video cases. 
Add more info here, previously we saw serious performance problems on the links in comment 55 and on Youtube live stream. This bug and those bug related to this bug fix those problems. But since our patches are in MSE pipeline, so we need to test all videos using MSE, like Youtube, as jya said.    
Sorina, 
Please also check normal videos on Youtube as well, not only for live stream. 
Thanks.
On normal youtube videos and live stream I don't see any change when I switch to another resolution. I've compared with Firefox 50, and the videos are playing the same: frozen and then playing.
(In reply to Sorina Florean [:sorina] from comment #59)
> On normal youtube videos and live stream I don't see any change when I
> switch to another resolution. I've compared with Firefox 50, and the videos

> are playing the same: frozen and then playing.
Hi Sorina,
If you didn't manually switch the quality,
Do you feel the playback is choppy periodically when playing live stream by Firefox50 and this symptom did not occur on the latest nightly?

I hope the answer is yes!

Thank you.
(In reply to James Cheng[:JamesCheng] from comment #60)
> (In reply to Sorina Florean [:sorina] from comment #59)
> > On normal youtube videos and live stream I don't see any change when I
> > switch to another resolution. I've compared with Firefox 50, and the videos
> 
> > are playing the same: frozen and then playing.
> Hi Sorina,
> If you didn't manually switch the quality,
> Do you feel the playback is choppy periodically when playing live stream by
> Firefox50 and this symptom did not occur on the latest nightly?
> 
> I hope the answer is yes!
> 
> Thank you.

The answer is yes! There is an improvement to Nightly compared to Firefox50.
Flags: needinfo?(sorina.florean)
Sorina,
Can you help test earlier Android version before 4.4, like 4.3?
Flags: needinfo?(sorina.florean)
Hi, for now I only have a tablet with Android 4.2.1, Asus Transformer Pad. Tested and the results on Nightly are the same like comment 55 and comment 61, compared with release.
Flags: needinfo?(sorina.florean)
This patch merged the related Bug 1299105, Bug 1317239, and Bug 1325005.

I've tested on my local Android device and file a try to validate it.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d332b31b02179c0c81de8084abba600b7e76a1da
Attachment #8809339 - Attachment is obsolete: true
Attachment #8811650 - Attachment is obsolete: true
Comment on attachment 8821023 [details] [diff] [review]
Merged-dependent-Bug-1299105-Bug-1317239-Bug-1320618.patch

Approval Request Comment
[Feature/Bug causing the regression]:Bug 1299105
[User impact if declined]: User may feel the video playback did not transit smoothly when resolution changed especially when playing live streaming.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes, it would be better to test it again.
(1) Open Fennec and make sure the preference "media.decoder.recycle.enabled" sets to true by default.
(2) Entering Youtube and search for any of "live stream" and play it.
Expected Result:
The playback is smooth compared with setting "media.decoder.recycle.enabled" to false(original flow, the playback will stutter periodically) manually.

[List of other uplifts needed for the feature/fix]: none.
[Is the change risky?]: No, and the change only affect on Fennec.
[Why is the change risky/not risky?]: not risky. We have asked QA for testing on legacy device already.
[String changes made/needed]: none.
Flags: needinfo?(gchang)
Attachment #8821023 - Flags: approval-mozilla-aurora?
Change ni to Julien as he is the owner of Aurora52.
Flags: needinfo?(gchang) → needinfo?(jcristau)
(In reply to James Cheng[:JamesCheng] from comment #66)
> [Why is the change risky/not risky?]: not risky. We have asked QA for
> testing on legacy device already.
> [String changes made/needed]: none.

What about https://bugzilla.mozilla.org/show_bug.cgi?id=1299068#c39?

this pointed out crashes with this change on at least the following devices:
- Android 4.4.2 (Device: Lenovo A536);
- Android 4.4.4 (Device: Motorola XT910).

For which bug 1323687 was created, shouldn't this be uplifted first?
(In reply to Jean-Yves Avenard [:jya] from comment #68)
> this pointed out crashes with this change on at least the following devices:
> - Android 4.4.2 (Device: Lenovo A536);
> - Android 4.4.4 (Device: Motorola XT910).

never mind... this is for the OOP decoding..
Liz, 
Since I don't know when Julien will come back, may we have your help to check comment 66?
Thanks.
Flags: needinfo?(lhenry)
Good that this has been tested on several different devices, but it sounds like it may risk breaking basic video functionality once it hits a wider audience. If we uplift to aurora now, we will only have the next two weeks to notice regressions or crashes before this change goes to our beta users. We can do this if you feel it's urgent, but if we see crashes or regressions, please back out the changes.   

This also has the changes from bug 1320618, right?

Do we gain anything here other than avoiding a pause/stutter when users switch video quality?
Flags: needinfo?(lhenry)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #71)
> Good that this has been tested on several different devices, but it sounds
> like it may risk breaking basic video functionality once it hits a wider
> audience. If we uplift to aurora now, we will only have the next two weeks
> to notice regressions or crashes before this change goes to our beta users.
> We can do this if you feel it's urgent, but if we see crashes or
> regressions, please back out the changes.   
Of course thank you!
> 
> This also has the changes from bug 1320618, right?
> 
Yes, the patch is included bug 1320618.
> Do we gain anything here other than avoiding a pause/stutter when users
> switch video quality?  
The quality will be changed automatically especially when you watch live streaming. You can play live streaming on current Fennec, you will feel that it is not acceptable.

Thank you!
Flags: needinfo?(lhenry)
Comment on attachment 8821023 [details] [diff] [review]
Merged-dependent-Bug-1299105-Bug-1317239-Bug-1320618.patch

Let's try this in aurora; should improve video playback on android.
Flags: needinfo?(lhenry)
Attachment #8821023 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Sorina,
Can you help test the FF52 version merged with this patch to see if there is any problems?
Thanks.
Flags: needinfo?(jcristau) → needinfo?(sorina.florean)
Tested with:
- Nexus 9 (Android 7.0)
- Nexus 5 (Android 6.0.1)
- LG G4 (Android 5.1)
- Lenovo A536 (Android 4.4.2)
Build: Latest Aurora 52.0a2 (2017-01-05).

The preference "media.decoder.recycle.enabled" is set to true by default.
Watched normal YouTube videos, live stream, [1] and [2]. The videos are playing smoothly, no delays so verified as fixed.

[1] http://s3.cf.web.us.aiv-cdn.net/HTML5PartnerTest.html?asin=B00TYBBNAW&videoType=Trailer&ui
[2] http://s3.cf.web.us.aiv-cdn.net/HTML5PartnerTest.html?asin=amzn1.dv.lcid.4d5e5db9-7ca8-4bca-8181-898539e3e60d&videoType=External&clear&ui
Flags: needinfo?(sorina.florean)
Depends on: 1336431
Depends on: 1340124
Sorina,
We need your help again. 
Can you help test the aurora version on [1] with the all android devices you have?
And check if there is any problem when resolution(it will be shown on the top of video) goes over 1920*1080. 
Thanks.

[1]http://players.akamai.com/dash
Flags: needinfo?(sorina.florean)
(In reply to Blake Wu [:bwu][:blakewu] from comment #77)
> Sorina,
> We need your help again. 
> Can you help test the aurora version on [1] with the all android devices you
> have?
> And check if there is any problem when resolution(it will be shown on the
> top of video) goes over 1920*1080. 
Some low-end devices may not be able to go over 1920*1080. Also it may require sufficient network bandwidth.
> Thanks.
> 
> [1]http://players.akamai.com/dash
(In reply to Blake Wu [:bwu][:blakewu] from comment #77)
> Sorina,
> We need your help again. 
> Can you help test the aurora version on [1] with the all android devices you
> have?

 what about beta? It takes priority over aurora, doesn't it?
(In reply to John Lin [:jolin][:jhlin] from comment #79)
> (In reply to Blake Wu [:bwu][:blakewu] from comment #77)
> > Sorina,
> > We need your help again. 
> > Can you help test the aurora version on [1] with the all android devices you
> > have?
> 
>  what about beta? It takes priority over aurora, doesn't it?
Yeah. You are right. I should get more sleep...
Sorina, 
Please test beta.
If the aim is to verify there's no out of bound memory access and. O vulnerability, simply running the test manually isn't conclusive at all.
You need proper analysis tools such as valgrind or asan, and even there, the problem is testing the java side of things which will be tricky
(In reply to Jean-Yves Avenard [:jya] from comment #81)
> If the aim is to verify there's no out of bound memory access and. O
> vulnerability, simply running the test manually isn't conclusive at all.
> You need proper analysis tools such as valgrind or asan, and even there, the
> problem is testing the java side of things which will be tricky

 The purpose of testing is just to see if video larger than the hard-coded max width/height still works.

 The code that decides what buffer size will be is indeed all native, some in stagefright and some in OMX decoder. FWICT, for video that changes resolution, the decoder supporting adaptive playback either allocates buffers on demand (dynamic buffer mode) or allocates max width/height buffers beforehand to avoid reallocation.

 Only buffer producer and consumer [1] have access to the buffer content, in this case the decoder itself and the GLES implementation where surface buffers are used as textures.

 Bottom line is, so far we don't have any viable solution other than adaptive playback for seamless playback on Android, it's either video stutters or as vulnerable as all other apps that depend on this feature documented in Android SDK.

 PS, there is a test case for this adaptive playback in Android CTS [2], so devices passing it should be fine (fingers crossed). :)

 [1] http://source.android.com/devices/graphics/arch-bq-gralloc.html
 [2] https://android.googlesource.com/platform/cts/+/f536c21/tests/tests/media/src/android/media/cts/AdaptivePlaybackTest.java
Hi,

Tested with: 
- Nexus 9 (Android 7.1.1) and Nexus 6 (Android 7.0)
- HTC Desire 820 (Android 6.0.1) and LG G4 (Android 6.)
- Samsung Galaxy Note 4 (Android 5.0.1), Huawei Honor 5X (Android 5.1.1), Huawei MediaPad (Android 5.1.1)
- Lenovo A536 (Android 4.4.2)

I didn't notice any problem when resolution was change with [1]. Maybe with Lenovo was like a freezing but probably because is more slower than the rest of the devices. If I need to do more testing, or other scenarios please let me know.
Thanks! 

[1]http://players.akamai.com/dash
Flags: needinfo?(sorina.florean)

Clearing the qe-verify? flag on this old flag. The flag should have been set to qe-verify+, but this is an old bug that doesn't need to be verified now.

Flags: qe-verify?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: