Enable adaptive playback for spitzer, use conservative size.

Enable adaptive playback for spitzer, use conservative size.

Android MediaCodec allocates memory for adaptive playback (dynamic
resolution change) based on hints provided to the codec during
construction about the maximum expected decoded frame dimensions.
That can signficantly increate the memory usage / reduce the number
of concurrent codec instances that chrome can use.

On some devices (Samsung S3), adaptive playback also entirely breaks
decoding.  Decoded frames can be tiled / black and white / generally
quite incorrect.

Spitzer had previously turned off dynamic resolution support to fix
both of these.

However, it turns out that enabling adaptive support is necessary:

- returning unused decoded buffers to MediaCodec without rendering
can incorrectly stop MediaCodec from decoding more frames without
adaptive playback enabled.
- some videos on some devices (N7) play very slowly otherwise.

This CL requests adaptive playback when AVDA creates the MediaCodec
instance.  It also plumbs the inital coded size to AVDA, and send it
to MediaCodecBridge.  MediaCodecBridge now uses this size, rather
than 1920x1080, as the max adaptive size hint to save memory.

For devices like the S3, MediaCodecBridge ignores the request for
adaptive playback, and keeps it off.

Providing the size to MediaCodec will also allow it to choose an
appropriate input buffer size.  Previously, it used the requested
size of 320x240 when estimating the size of an encoded frame.

BUG=599908, 596211, 601066
Committed: https://crrev.com/62cf4f1fab4e0cc2560983465d3fd15fde537f00
Cr-Commit-Position: refs/heads/master@{#386091}

[liberato (no reviews please), 2016-04-07 18:33:37]

Description was changed from

==========
Enable adaptive playback for spitzer, use conservative size.

Android MediaCodec allocates memory for adaptive playback (dynamic
resolution change) based on hints provided to the codec during
construction about the maximum expected decoded frame dimensions.
That can signficantly increate the memory usage / reduce the number
of concurrent codec instances that chrome can use.

On some devices (Samsung S3), adaptive playback also entirely breaks
decoding.  Decoded frames can be tiled / black and white / generally
quite incorrect.

Spitzer had previously turned off dynamic resolution support to fix
both of these.

However, it turns out that enabling adaptive support is necessary:

- returning unused decoded buffers to MediaCodec without rendering
can incorrectly stop MediaCodec from decoding more frames without
adaptive playback enabled.
- some videos on some devices (N7) play very slowly otherwise.

This CL requests adaptive playback when AVDA creates the MediaCodec
instance.  It also plumbs the inital coded size to AVDA, and send it
to MediaCodecBridge.  MediaCodecBridge now uses this size, rather
than 1920x1080, as the max adaptive size hint to save memory.

For devices like the S3, MediaCodecBridge ignores the request for
adaptive playback, and keeps it off.

Providing the size to MediaCodec will also allow it to choose an
appropriate input buffer size.  Previously, it used the requested
size of 320x240 when estimating the size of an encoded frame.

BUG=599908,596211,601066
==========

to

==========
Enable adaptive playback for spitzer, use conservative size.

Android MediaCodec allocates memory for adaptive playback (dynamic
resolution change) based on hints provided to the codec during
construction about the maximum expected decoded frame dimensions.
That can signficantly increate the memory usage / reduce the number
of concurrent codec instances that chrome can use.

On some devices (Samsung S3), adaptive playback also entirely breaks
decoding.  Decoded frames can be tiled / black and white / generally
quite incorrect.

Spitzer had previously turned off dynamic resolution support to fix
both of these.

However, it turns out that enabling adaptive support is necessary:

- returning unused decoded buffers to MediaCodec without rendering
can incorrectly stop MediaCodec from decoding more frames without
adaptive playback enabled.
- some videos on some devices (N7) play very slowly otherwise.

This CL requests adaptive playback when AVDA creates the MediaCodec
instance.  It also plumbs the inital coded size to AVDA, and send it
to MediaCodecBridge.  MediaCodecBridge now uses this size, rather
than 1920x1080, as the max adaptive size hint to save memory.

For devices like the S3, MediaCodecBridge ignores the request for
adaptive playback, and keeps it off.

Providing the size to MediaCodec will also allow it to choose an
appropriate input buffer size.  Previously, it used the requested
size of 320x240 when estimating the size of an encoded frame.

BUG=599908,596211,601066
==========

[liberato (no reviews please), 2016-04-07 18:33:37]

liberato@chromium.org changed reviewers:
+ dalecurtis@chromium.org, timav@chromium.org, watk@chromium.org

[DaleCurtis, 2016-04-07 18:40:44]

https://codereview.chromium.org/1869103002/diff/20001/content/common/gpu/medi...
File content/common/gpu/media/android_video_decode_accelerator.h (right):

https://codereview.chromium.org/1869103002/diff/20001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.h:187: // Initial
coded size.
Add a note that this may and probably will change over the lifetime of the
stream and should only be used as an initial hint.

https://codereview.chromium.org/1869103002/diff/20001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right):

https://codereview.chromium.org/1869103002/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:242: private
static boolean isAdaptivePlaybackBlacklisted(String mime) {
Instead of having this should we just add the GT-I9300 and SCH-I535 to the vp8
blacklist? It kind of unnerves me that a single bit has so much impact on the
viability of these devices.

[DaleCurtis, 2016-04-07 18:41:25]

https://codereview.chromium.org/1869103002/diff/20001/media/video/video_decod...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/1869103002/diff/20001/media/video/video_decod...
media/video/video_decode_accelerator.h:126: // Coded size of the video frame.
Ditto on adding a note, maybe even name it initial_expected_coded_size in all
cases.

[Tima Vaisburd, 2016-04-07 18:50:01]

I like the approach!
lgtm % nits

https://codereview.chromium.org/1869103002/diff/20001/content/common/gpu/medi...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1869103002/diff/20001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:958:
gfx::Size(codec_config->coded_size_.width(),
nit: will passing |codec_config_->coded_size_| work?

https://codereview.chromium.org/1869103002/diff/20001/content/common/gpu/medi...
File content/common/gpu/media/android_video_decode_accelerator.h (right):

https://codereview.chromium.org/1869103002/diff/20001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.h:188: gfx::Size
coded_size_;
nit: s/coded_size_/initial_frame_size_/ ? or just |frame_size_| ?

I noticed that the word 'size' is used a lot to denote the length of a buffer in
bytes and thus can be easily confused with geometrical size of a rectangle.

https://codereview.chromium.org/1869103002/diff/20001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
(right):

https://codereview.chromium.org/1869103002/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:469: final
int max_width = format.containsKey(MediaFormat.KEY_WIDTH)
I think KEY_WITH and KEY_HEIGHT are mandatory keys for Video configuration and
thus they will always be there, so you can simplify.
http://developer.android.com/reference/android/media/MediaFormat.html

[liberato (no reviews please), 2016-04-07 20:46:29]

https://codereview.chromium.org/1869103002/diff/20001/content/common/gpu/medi...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1869103002/diff/20001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:958:
gfx::Size(codec_config->coded_size_.width(),
On 2016/04/07 18:50:01, Tima Vaisburd wrote:
> nit: will passing |codec_config_->coded_size_| work?

indeed, thanks!

https://codereview.chromium.org/1869103002/diff/20001/content/common/gpu/medi...
File content/common/gpu/media/android_video_decode_accelerator.h (right):

https://codereview.chromium.org/1869103002/diff/20001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.h:187: // Initial
coded size.
On 2016/04/07 18:40:43, DaleCurtis wrote:
> Add a note that this may and probably will change over the lifetime of the
> stream and should only be used as an initial hint.

Done.

https://codereview.chromium.org/1869103002/diff/20001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.h:188: gfx::Size
coded_size_;
On 2016/04/07 18:50:01, Tima Vaisburd wrote:
> nit: s/coded_size_/initial_frame_size_/ ? or just |frame_size_| ?
> 
> I noticed that the word 'size' is used a lot to denote the length of a buffer
in
> bytes and thus can be easily confused with geometrical size of a rectangle.

renamed to |initial_expected_coded_size_|, per dale's suggestion.  since it
comes from |coded_size| in the decoder config, i'd like to keep the name
similar.

https://codereview.chromium.org/1869103002/diff/20001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
(right):

https://codereview.chromium.org/1869103002/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:469: final
int max_width = format.containsKey(MediaFormat.KEY_WIDTH)
On 2016/04/07 18:50:01, Tima Vaisburd wrote:
> I think KEY_WITH and KEY_HEIGHT are mandatory keys for Video configuration and
> thus they will always be there, so you can simplify.
> http://developer.android.com/reference/android/media/MediaFormat.html

Done.

https://codereview.chromium.org/1869103002/diff/20001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right):

https://codereview.chromium.org/1869103002/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:242: private
static boolean isAdaptivePlaybackBlacklisted(String mime) {
On 2016/04/07 18:40:44, DaleCurtis wrote:
> Instead of having this should we just add the GT-I9300 and SCH-I535 to the vp8
> blacklist? It kind of unnerves me that a single bit has so much impact on the
> viability of these devices.

i don't think that the s3 has hw vp8, just 264.  caleb showed me that it's
running fine in the lab for 264 now with adaptive disabled.

maybe i should remove the mime check..?

https://codereview.chromium.org/1869103002/diff/20001/media/video/video_decod...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/1869103002/diff/20001/media/video/video_decod...
media/video/video_decode_accelerator.h:126: // Coded size of the video frame.
On 2016/04/07 18:41:25, DaleCurtis wrote:
> Ditto on adding a note, maybe even name it initial_expected_coded_size in all
> cases. 

Done.

[DaleCurtis, 2016-04-07 22:55:17]

lgtm

https://codereview.chromium.org/1869103002/diff/40001/media/base/android/java...
File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
(right):

https://codereview.chromium.org/1869103002/diff/40001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:465:
format.setInteger(
FWIW, since I had to go check, VideoDecoderConfig::IsValidConfig() will ensure
the width and height are always > 0.

[DaleCurtis, 2016-04-07 22:55:34]

To confirm on the N4 you were able to get plenty of codecs?

[liberato (no reviews please), 2016-04-08 05:34:14]

On 2016/04/07 22:55:34, DaleCurtis wrote:
> To confirm on the N4 you were able to get plenty of codecs?

around three or four, don't remember.  i'll verify tomorrow before landing it.

[liberato (no reviews please), 2016-04-08 15:00:20]

On 2016/04/08 05:34:14, liberato wrote:
> On 2016/04/07 22:55:34, DaleCurtis wrote:
> > To confirm on the N4 you were able to get plenty of codecs?
> 
> around three or four, don't remember.  i'll verify tomorrow before landing it.

three once, four once.

-fl

[liberato (no reviews please), 2016-04-08 15:00:28]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2016-04-08 15:00:28]

The patchset sent to the CQ was uploaded after l-g-t-m from timav@chromium.org
Link to the patchset: https://codereview.chromium.org/1869103002/#ps40001
(title: "cl feedback.")

[commit-bot: I haz the power, 2016-04-08 15:00:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1869103002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1869103002/40001

[commit-bot: I haz the power, 2016-04-08 16:05:51]

Description was changed from

==========
Enable adaptive playback for spitzer, use conservative size.

Android MediaCodec allocates memory for adaptive playback (dynamic
resolution change) based on hints provided to the codec during
construction about the maximum expected decoded frame dimensions.
That can signficantly increate the memory usage / reduce the number
of concurrent codec instances that chrome can use.

On some devices (Samsung S3), adaptive playback also entirely breaks
decoding.  Decoded frames can be tiled / black and white / generally
quite incorrect.

Spitzer had previously turned off dynamic resolution support to fix
both of these.

However, it turns out that enabling adaptive support is necessary:

- returning unused decoded buffers to MediaCodec without rendering
can incorrectly stop MediaCodec from decoding more frames without
adaptive playback enabled.
- some videos on some devices (N7) play very slowly otherwise.

This CL requests adaptive playback when AVDA creates the MediaCodec
instance.  It also plumbs the inital coded size to AVDA, and send it
to MediaCodecBridge.  MediaCodecBridge now uses this size, rather
than 1920x1080, as the max adaptive size hint to save memory.

For devices like the S3, MediaCodecBridge ignores the request for
adaptive playback, and keeps it off.

Providing the size to MediaCodec will also allow it to choose an
appropriate input buffer size.  Previously, it used the requested
size of 320x240 when estimating the size of an encoded frame.

BUG=599908,596211,601066
==========

to

==========
Enable adaptive playback for spitzer, use conservative size.

Android MediaCodec allocates memory for adaptive playback (dynamic
resolution change) based on hints provided to the codec during
construction about the maximum expected decoded frame dimensions.
That can signficantly increate the memory usage / reduce the number
of concurrent codec instances that chrome can use.

On some devices (Samsung S3), adaptive playback also entirely breaks
decoding.  Decoded frames can be tiled / black and white / generally
quite incorrect.

Spitzer had previously turned off dynamic resolution support to fix
both of these.

However, it turns out that enabling adaptive support is necessary:

- returning unused decoded buffers to MediaCodec without rendering
can incorrectly stop MediaCodec from decoding more frames without
adaptive playback enabled.
- some videos on some devices (N7) play very slowly otherwise.

This CL requests adaptive playback when AVDA creates the MediaCodec
instance.  It also plumbs the inital coded size to AVDA, and send it
to MediaCodecBridge.  MediaCodecBridge now uses this size, rather
than 1920x1080, as the max adaptive size hint to save memory.

For devices like the S3, MediaCodecBridge ignores the request for
adaptive playback, and keeps it off.

Providing the size to MediaCodec will also allow it to choose an
appropriate input buffer size.  Previously, it used the requested
size of 320x240 when estimating the size of an encoded frame.

BUG=599908,596211,601066
==========

[commit-bot: I haz the power, 2016-04-08 16:05:52]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-08 16:08:21]

Description was changed from

==========
Enable adaptive playback for spitzer, use conservative size.

Android MediaCodec allocates memory for adaptive playback (dynamic
resolution change) based on hints provided to the codec during
construction about the maximum expected decoded frame dimensions.
That can signficantly increate the memory usage / reduce the number
of concurrent codec instances that chrome can use.

On some devices (Samsung S3), adaptive playback also entirely breaks
decoding.  Decoded frames can be tiled / black and white / generally
quite incorrect.

Spitzer had previously turned off dynamic resolution support to fix
both of these.

However, it turns out that enabling adaptive support is necessary:

- returning unused decoded buffers to MediaCodec without rendering
can incorrectly stop MediaCodec from decoding more frames without
adaptive playback enabled.
- some videos on some devices (N7) play very slowly otherwise.

This CL requests adaptive playback when AVDA creates the MediaCodec
instance.  It also plumbs the inital coded size to AVDA, and send it
to MediaCodecBridge.  MediaCodecBridge now uses this size, rather
than 1920x1080, as the max adaptive size hint to save memory.

For devices like the S3, MediaCodecBridge ignores the request for
adaptive playback, and keeps it off.

Providing the size to MediaCodec will also allow it to choose an
appropriate input buffer size.  Previously, it used the requested
size of 320x240 when estimating the size of an encoded frame.

BUG=599908,596211,601066
==========

to

==========
Enable adaptive playback for spitzer, use conservative size.

Android MediaCodec allocates memory for adaptive playback (dynamic
resolution change) based on hints provided to the codec during
construction about the maximum expected decoded frame dimensions.
That can signficantly increate the memory usage / reduce the number
of concurrent codec instances that chrome can use.

On some devices (Samsung S3), adaptive playback also entirely breaks
decoding.  Decoded frames can be tiled / black and white / generally
quite incorrect.

Spitzer had previously turned off dynamic resolution support to fix
both of these.

However, it turns out that enabling adaptive support is necessary:

- returning unused decoded buffers to MediaCodec without rendering
can incorrectly stop MediaCodec from decoding more frames without
adaptive playback enabled.
- some videos on some devices (N7) play very slowly otherwise.

This CL requests adaptive playback when AVDA creates the MediaCodec
instance.  It also plumbs the inital coded size to AVDA, and send it
to MediaCodecBridge.  MediaCodecBridge now uses this size, rather
than 1920x1080, as the max adaptive size hint to save memory.

For devices like the S3, MediaCodecBridge ignores the request for
adaptive playback, and keeps it off.

Providing the size to MediaCodec will also allow it to choose an
appropriate input buffer size.  Previously, it used the requested
size of 320x240 when estimating the size of an encoded frame.

BUG=599908,596211,601066

Committed: https://crrev.com/62cf4f1fab4e0cc2560983465d3fd15fde537f00
Cr-Commit-Position: refs/heads/master@{#386091}
==========

[commit-bot: I haz the power, 2016-04-08 16:08:23]

Patchset 3 (id:??) landed as
https://crrev.com/62cf4f1fab4e0cc2560983465d3fd15fde537f00
Cr-Commit-Position: refs/heads/master@{#386091}