support adaptive playback in MSE

support adaptive playback in MSE

In kitkat, android introduces a new API to support adaptive playback:
https://developer.android.com/about/versions/android-4.4.html

This change enables adaptive playback on kitkat devices for MSE so that we don't need to tear down decoders during config changes.

Tested with the youtube dash MSE demo page, and i saw no pauses during config changes.

BUG=357726
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276022

[qinmin, 2014-06-06 18:02:51]

PTAL

[wolenetz, 2014-06-06 23:28:07]

First round of comments. With 'adaptive/non-adaptive' video bridge support as
another test case dimension, I'm not clear if we this change needs to add
further coverage than what is in it.

https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
File media/base/android/media_decoder_job.cc (right):

https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
media/base/android/media_decoder_job.cc:292: // |current_demuxer_data_index_|
must be pointing to an EOS unit.
nit: or a kConfigChanged unit and drain_decoder_ is true, during which we will
just feed EOS to decoder?

https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
media/base/android/media_decoder_job.cc:338: config_changed_cb_.Run();
Should we wait to tell the renderer that the config has changed until we
complete draining (if any) and create the new decoder? I'm fine with this as a
separate CL.

https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
media/base/android/media_decoder_job.cc:339: if (!drain_decoder_) {
If !reconfigure_needed, we still drain previous decoder and do the old
OnDecoderDrained() logic. Or am I misreading?

https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
File media/base/android/media_source_player_unittest.cc (right):

https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
media/base/android/media_source_player_unittest.cc:639: else {
lint nit: put {} around clause prior to else to match clause formats before and
after else.

https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
media/base/android/media_source_player_unittest.cc:1812: // Test that video
config change notification results in creating a new
No new codec should have been created beyond the one used to decode the prefetch
media data prior to the kConfigChanged. The existing one was adaptive.

https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
media/base/android/media_source_player_unittest.cc:1817:
EXPECT_TRUE(GetMediaCodecBridge(false));
We need to somehow verify the resulting codec bridge is the same as the one
prior to kConfigChanged. Any ideas how (without pointer comparison flakiness)?
Check that the original bridge never needed draining?

https://codereview.chromium.org/323563002/diff/20001/media/base/android/video...
File media/base/android/video_decoder_job.cc (right):

https://codereview.chromium.org/323563002/diff/20001/media/base/android/video...
media/base/android/video_decoder_job.cc:111: IsAdaptivePlaybackSupported(width_,
height_);
s/width_/configs.video_size.width()/
s/height_/configs.video_size.height()/

[wolenetz, 2014-06-06 23:31:16]

On 2014/06/06 23:28:07, wolenetz wrote:
> First round of comments. With 'adaptive/non-adaptive' video bridge support as
> another test case dimension, I'm not clear if we this change needs to add
> further coverage than what is in it.
> 
>
https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
> File media/base/android/media_decoder_job.cc (right):
> 
>
https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
> media/base/android/media_decoder_job.cc:292: // |current_demuxer_data_index_|
> must be pointing to an EOS unit.
> nit: or a kConfigChanged unit and drain_decoder_ is true, during which we will
> just feed EOS to decoder?
> 
>
https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
> media/base/android/media_decoder_job.cc:338: config_changed_cb_.Run();
> Should we wait to tell the renderer that the config has changed until we
> complete draining (if any) and create the new decoder? I'm fine with this as a
> separate CL.
> 
>
https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
> media/base/android/media_decoder_job.cc:339: if (!drain_decoder_) {
> If !reconfigure_needed, we still drain previous decoder and do the old
> OnDecoderDrained() logic. Or am I misreading?
> 
>
https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
> File media/base/android/media_source_player_unittest.cc (right):
> 
>
https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
> media/base/android/media_source_player_unittest.cc:639: else {
> lint nit: put {} around clause prior to else to match clause formats before
and
> after else.
> 
>
https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
> media/base/android/media_source_player_unittest.cc:1812: // Test that video
> config change notification results in creating a new
> No new codec should have been created beyond the one used to decode the
prefetch
> media data prior to the kConfigChanged. The existing one was adaptive.
> 
>
https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
> media/base/android/media_source_player_unittest.cc:1817:
> EXPECT_TRUE(GetMediaCodecBridge(false));
> We need to somehow verify the resulting codec bridge is the same as the one
> prior to kConfigChanged. Any ideas how (without pointer comparison flakiness)?
> Check that the original bridge never needed draining?
> 
>
https://codereview.chromium.org/323563002/diff/20001/media/base/android/video...
> File media/base/android/video_decoder_job.cc (right):
> 
>
https://codereview.chromium.org/323563002/diff/20001/media/base/android/video...
> media/base/android/video_decoder_job.cc:111:
IsAdaptivePlaybackSupported(width_,
> height_);
> s/width_/configs.video_size.width()/
> s/height_/configs.video_size.height()/

Also, I forgot to add, but I haven't totally grok'ed this Android 4.4 adaptive
playback requirement:
"be certain that you provide the new Sequence Parameter Set (SPS) and Picture
Parameter Set (PPS) values together with the Instantaneous Decoder Refresh (IDR)
frame in a single buffer"
(https://developer.android.com/about/versions/android-4.4.html).

[qinmin, 2014-06-07 01:31:47]

Yes, the android requirement on SPS/PPS/IDR is quite unpleasant. So far I
haven't seen any issues when experimenting with my change.
I am wondering whether chunk demuxer can guarantee that SPS/PPS/IDR is in the
same AU? Anyway, added a TODO.

On 2014/06/06 23:31:16, wolenetz wrote:
> On 2014/06/06 23:28:07, wolenetz wrote:
> > First round of comments. With 'adaptive/non-adaptive' video bridge support
as
> > another test case dimension, I'm not clear if we this change needs to add
> > further coverage than what is in it.
> > 
> >
>
https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
> > File media/base/android/media_decoder_job.cc (right):
> > 
> >
>
https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
> > media/base/android/media_decoder_job.cc:292: //
|current_demuxer_data_index_|
> > must be pointing to an EOS unit.
> > nit: or a kConfigChanged unit and drain_decoder_ is true, during which we
will
> > just feed EOS to decoder?
> > 
> >
>
https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
> > media/base/android/media_decoder_job.cc:338: config_changed_cb_.Run();
> > Should we wait to tell the renderer that the config has changed until we
> > complete draining (if any) and create the new decoder? I'm fine with this as
a
> > separate CL.
> > 
> >
>
https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
> > media/base/android/media_decoder_job.cc:339: if (!drain_decoder_) {
> > If !reconfigure_needed, we still drain previous decoder and do the old
> > OnDecoderDrained() logic. Or am I misreading?
> > 
> >
>
https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
> > File media/base/android/media_source_player_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
> > media/base/android/media_source_player_unittest.cc:639: else {
> > lint nit: put {} around clause prior to else to match clause formats before
> and
> > after else.
> > 
> >
>
https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
> > media/base/android/media_source_player_unittest.cc:1812: // Test that video
> > config change notification results in creating a new
> > No new codec should have been created beyond the one used to decode the
> prefetch
> > media data prior to the kConfigChanged. The existing one was adaptive.
> > 
> >
>
https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
> > media/base/android/media_source_player_unittest.cc:1817:
> > EXPECT_TRUE(GetMediaCodecBridge(false));
> > We need to somehow verify the resulting codec bridge is the same as the one
> > prior to kConfigChanged. Any ideas how (without pointer comparison
flakiness)?
> > Check that the original bridge never needed draining?
> > 
> >
>
https://codereview.chromium.org/323563002/diff/20001/media/base/android/video...
> > File media/base/android/video_decoder_job.cc (right):
> > 
> >
>
https://codereview.chromium.org/323563002/diff/20001/media/base/android/video...
> > media/base/android/video_decoder_job.cc:111:
> IsAdaptivePlaybackSupported(width_,
> > height_);
> > s/width_/configs.video_size.width()/
> > s/height_/configs.video_size.height()/
> 
> Also, I forgot to add, but I haven't totally grok'ed this Android 4.4 adaptive
> playback requirement:
> "be certain that you provide the new Sequence Parameter Set (SPS) and Picture
> Parameter Set (PPS) values together with the Instantaneous Decoder Refresh
(IDR)
> frame in a single buffer"
> (https://developer.android.com/about/versions/android-4.4.html).

[qinmin, 2014-06-07 01:31:55]

https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
File media/base/android/media_decoder_job.cc (right):

https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
media/base/android/media_decoder_job.cc:292: // |current_demuxer_data_index_|
must be pointing to an EOS unit.
On 2014/06/06 23:28:06, wolenetz wrote:
> nit: or a kConfigChanged unit and drain_decoder_ is true, during which we will
> just feed EOS to decoder?

Done. Rephrased the comment.

https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
media/base/android/media_decoder_job.cc:338: config_changed_cb_.Run();
Yes, that should be the correct behavior. However, the fix needs to touch
multiple places where drain_decoder_ gets cleared. Added TODO here.

On 2014/06/06 23:28:07, wolenetz wrote:
> Should we wait to tell the renderer that the config has changed until we
> complete draining (if any) and create the new decoder? I'm fine with this as a
> separate CL.

https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
media/base/android/media_decoder_job.cc:339: if (!drain_decoder_) {
If !reconfigure_needed, we will keep the previous decoder and keep on decoding
the data it currently has. So there is no need to drain it.
However, if need_to_reconfig_decoder_job_ is true, we will recreate a new
decoder even if !reconfigure_needed is true. This could happen when surface
changes or a seek is conducted. In both cases, we should abadon the current
buffered data and recreate a new codec.


On 2014/06/06 23:28:07, wolenetz wrote:
> If !reconfigure_needed, we still drain previous decoder and do the old
> OnDecoderDrained() logic. Or am I misreading?

https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
File media/base/android/media_source_player_unittest.cc (right):

https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
media/base/android/media_source_player_unittest.cc:639: else {
On 2014/06/06 23:28:07, wolenetz wrote:
> lint nit: put {} around clause prior to else to match clause formats before
and
> after else.

Done.

https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
media/base/android/media_source_player_unittest.cc:1812: // Test that video
config change notification results in creating a new
On 2014/06/06 23:28:07, wolenetz wrote:
> No new codec should have been created beyond the one used to decode the
prefetch
> media data prior to the kConfigChanged. The existing one was adaptive.

Done.

https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
media/base/android/media_source_player_unittest.cc:1817:
EXPECT_TRUE(GetMediaCodecBridge(false));
we can use manager_.num_resources_requested().

On 2014/06/06 23:28:07, wolenetz wrote:
> We need to somehow verify the resulting codec bridge is the same as the one
> prior to kConfigChanged. Any ideas how (without pointer comparison flakiness)?
> Check that the original bridge never needed draining?

https://codereview.chromium.org/323563002/diff/20001/media/base/android/video...
File media/base/android/video_decoder_job.cc (right):

https://codereview.chromium.org/323563002/diff/20001/media/base/android/video...
media/base/android/video_decoder_job.cc:111: IsAdaptivePlaybackSupported(width_,
height_);
On 2014/06/06 23:28:07, wolenetz wrote:
> s/width_/configs.video_size.width()/
> s/height_/configs.video_size.height()/

Done. A stupid mistake :(

[wolenetz, 2014-06-09 18:22:39]

lgtm % the SPS/PPS/IDR stats question

https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
File media/base/android/media_decoder_job.cc (right):

https://codereview.chromium.org/323563002/diff/20001/media/base/android/media...
media/base/android/media_decoder_job.cc:339: if (!drain_decoder_) {
On 2014/06/07 01:31:55, qinmin wrote:
> If !reconfigure_needed, we will keep the previous decoder and keep on decoding
> the data it currently has. So there is no need to drain it.
> However, if need_to_reconfig_decoder_job_ is true, we will recreate a new
> decoder even if !reconfigure_needed is true. This could happen when surface
> changes or a seek is conducted. In both cases, we should abadon the current
> buffered data and recreate a new codec.
> 
> 
> On 2014/06/06 23:28:07, wolenetz wrote:
> > If !reconfigure_needed, we still drain previous decoder and do the old
> > OnDecoderDrained() logic. Or am I misreading?
> 

Thank you for clarifying. Yes, I was misreading.

https://codereview.chromium.org/323563002/diff/40001/media/base/android/media...
File media/base/android/media_decoder_job.cc (right):

https://codereview.chromium.org/323563002/diff/40001/media/base/android/media...
media/base/android/media_decoder_job.cc:294: // We'll reuse this unit to drain
the decoder until we hit output EOS.
nit: s/We'll/we'll/ and s/reuse this/feed an EOS input/

https://codereview.chromium.org/323563002/diff/40001/media/base/android/media...
media/base/android/media_decoder_job.cc:338: // TODO(qinmin):
|config_changed_cb_| should be ran after draining finishes.
nit: s/ran/run/ (http://wiki.answers.com/Q/Should_be_run_or_should_be_ran)

https://codereview.chromium.org/323563002/diff/40001/media/base/android/media...
media/base/android/media_decoder_job.cc:349: //
https://developer.android.com/about/versions/android-4.4.html.
How will we know if this SPS/PPS/IDR expectation is missed? Are there stats that
can show us something like % of failed adaptive video config changes?

[qinmin, 2014-06-09 18:55:39]

https://codereview.chromium.org/323563002/diff/40001/media/base/android/media...
File media/base/android/media_decoder_job.cc (right):

https://codereview.chromium.org/323563002/diff/40001/media/base/android/media...
media/base/android/media_decoder_job.cc:294: // We'll reuse this unit to drain
the decoder until we hit output EOS.
On 2014/06/09 18:22:39, wolenetz wrote:
> nit: s/We'll/we'll/ and s/reuse this/feed an EOS input/

Done.

https://codereview.chromium.org/323563002/diff/40001/media/base/android/media...
media/base/android/media_decoder_job.cc:338: // TODO(qinmin):
|config_changed_cb_| should be ran after draining finishes.
On 2014/06/09 18:22:39, wolenetz wrote:
> nit: s/ran/run/ (http://wiki.answers.com/Q/Should_be_run_or_should_be_ran)

Done.

https://codereview.chromium.org/323563002/diff/40001/media/base/android/media...
media/base/android/media_decoder_job.cc:349: //
https://developer.android.com/about/versions/android-4.4.html.
Since SPS/PPS/IDR is guaranteed to be the first AU after config change, removed
the TODO here.
On 2014/06/09 18:22:39, wolenetz wrote:
> How will we know if this SPS/PPS/IDR expectation is missed? Are there stats
that
> can show us something like % of failed adaptive video config changes?

[qinmin, 2014-06-09 19:05:58]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2014-06-09 19:08:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/qinmin@chromium.org/323563002/60001

[commit-bot: I haz the power, 2014-06-10 09:00:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[commit-bot: I haz the power, 2014-06-10 14:42:08]

Change committed as 276022