AndroidVideoEncodeAccelerator is born!

AndroidVideoEncodeAccelerator is born!

AVEA is the encode-side analogue of AndroidVideoDecodeAccelerator, or the
Android analogue of ExynosVideoEncodeAccelerator, depending on your POV.
Also included in this CL:
- MediaCodecBridge learns how to be an encoder, too.
- MediaCodecBridge::Start is sunk into Create since they were always called
  together.
- android.os.Log() is given an exception parameter instead of concatenating its
  .toString() (and losing its stacktrace!)
- MediaCodecBridge exposes its buffers to reduce unnecessary memcpy'ing

Performance impact: isolating encode performance by making Android decode only
240p and no audio send/receive with the following URLs:
z620/gprecise: https://apprtc.appspot.com/?video=maxHeight=240&audio=false&r=<ROOM>;
Nexus5: https://apprtc.appspot.com/?video=minHeight=720,maxHeight=720,minWidth=1280,maxWidth=1280&audio=false&r=<ROOM>;
All 4 cores are max'd are running at top speed (ondemand governor ramps them up
on its own with this workload).
SW encode: CPU utilization 80% and desktop receives 0.1-0.5FPS (jankily).
HW encode: CPU utilization 60-70% and desktop receives 30FPS reliably.

Comparing an easier workload of encoding 360p:
z620/gprecise: https://apprtc.appspot.com/?video=maxHeight=240&audio=false&r=<ROOM>;
Nexus5: https://apprtc.appspot.com/?video=minHeight=360,maxHeight=360,minWidth=640,maxWidth=640&audio=false&r=<ROOM>;
Set all 4 cores to "performance" governor for stable comparison.
SW encode: CPU utilization 63% and desktop receives 30FPS reliably.
HW encode: CPU utilization 53% and desktop receives 30FPS reliably.

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

[Ami GONE FROM CHROMIUM, 2013-11-16 01:26:19]

@xhwang: PTAL.  I hope you're happy to review the encoder but if not let me know
and I'll see if I can find someone else who can (but please still review the
misc stuff in this CL).

@yujie.mao & @wolenetz: FYI since you've expressed interest.

Before submitting I'll add details on the impact on CPU utilization of using
this HW encoder to the CL's description.

[xhwang, 2013-11-19 00:11:24]

This CL is looking pretty good. Mostly nits.

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
File content/common/gpu/media/android_video_encode_accelerator.cc (right):

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.cc:28: // Subset of
MediaCodecInfo.CodecCapabilities.
Should this be a MediaCodecBridge constant? It'd be nice to keep all Android
specific things together.

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.cc:29:
kColorFormatYUV420SemiPlanar = 21,
ditto about enum naming style

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.cc:47: return
base::TimeDelta::FromMilliseconds(10);
// arbitrary choice?

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.cc:76:
VideoCodecBridge::IsKnownUnaccelerated(media::kCodecVP8, true)) {
Comment that we are only looking for "hardware vp8" encoder?

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.cc:82:
profile.max_framerate.numerator = 30;
Where are the max resolution and size coming from? Make them const?

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.cc:156: DVLOG(3) <<
"AndroidVideoEncodeAccelerator::Encode: " << force_keyframe;
Here and below, will __FUNCTION__ work for you?

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.cc:177: void
AndroidVideoEncodeAccelerator::UseOutputBitstreamBuffer(
nit comment for the interface: "Use" is more like a "Provide" ?

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.cc:200: // bitrate,
and we set that explicitly.
Will we hit unused variable for |framerate| in release mode?

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.cc:222: if
(!client_ptr_factory_.GetWeakPtr() || pending_frames_.empty()) return;
nit: I like your use of if-return-in-one-line in many previous cases. But I
found when the if-line gets long it's a little bit hard to read :) Just sayin'

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.cc:236: if (input.b) {
I have to go to the .h file to check what "b" is. How about:

bool& is_key_frame = input.b;
if (is_key_frame)

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.cc:250:
frame->AllocationSize(VideoFrame::I420, frame->coded_size());
VideoFrame::AllocationSize()?

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.cc:291: LOG(ERROR) <<
"Need more/bigger buffers; before: " << num_output_buffers_
Use LOG_IF(ERROR, !ret)?

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.cc:336: } while
(buf_index < 0);
In what case would buf_index < 0?

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
File content/common/gpu/media/android_video_encode_accelerator.h (right):

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.h:21: class
MessageLoopProxy;
not used?

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.h:63: kIFrameInterval
= kint32max,
we use UPPER_CASE format for enums (from
http://www.chromium.org/developers/coding-style):

Though the Google C++ Style Guide now says to use kConstantNaming for enums,
Chromium was written using MACRO_STYLE naming.  Continue to use this style for
consistency.

Currently we have a lot of kCamelCase for enums. The plan is to convert them in
the future. For now, we use UPPER_CASE for new code.

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.h:111: uint32
last_set_bitrate_;
unit? in bps?

https://codereview.chromium.org/74563002/diff/30001/media/base/android/java/s...
File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
(right):

https://codereview.chromium.org/74563002/diff/30001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:151:
Map<String, CodecInfo> decoderInfoMap = new HashMap<String, CodecInfo>();
It's a bit odd to have two maps here then merge them together. But I don't know
a better way to do it yet. Also, shall we comment that we prefer the first
returned encoder/decoder in the list?

https://codereview.chromium.org/74563002/diff/30001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:399: }
Move l.375-l.399 to l.317?

https://codereview.chromium.org/74563002/diff/30001/media/base/android/media_...
File media/base/android/media_codec_bridge.cc (right):

https://codereview.chromium.org/74563002/diff/30001/media/base/android/media_...
media/base/android/media_codec_bridge.cc:221: int index, const uint8* data, int
orig_data_size,
Also update the header file.

https://codereview.chromium.org/74563002/diff/30001/media/base/android/media_...
media/base/android/media_codec_bridge.cc:235: int index, const uint8* data, int
orig_data_size, const uint8* key_id,
ditto

https://codereview.chromium.org/74563002/diff/30001/media/base/android/media_...
File media/base/android/media_codec_bridge.h (right):

https://codereview.chromium.org/74563002/diff/30001/media/base/android/media_...
media/base/android/media_codec_bridge.h:138: // optionally render the buffer.
nit: merge with the previous line?

https://codereview.chromium.org/74563002/diff/30001/media/base/android/media_...
media/base/android/media_codec_bridge.h:152: // Returns an input buffers' base
pointer and capacity.
s/buffers'/buffer's

[Ami GONE FROM CHROMIUM, 2013-11-21 22:59:07]

Thanks for the review, Xiaohan.  In addition to addressing your comments, I:
- Added perf impact details to CL description
- Ran clang-format (sorry, thought I'd already done that)
- Added about:flags support to be able to disable this if necessary.

I'd rather the flag went the other way for android, but I'd also rather not have
two polar-opposite flags.  I'm looking into how I can effect this via
blacklisting.

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
File content/common/gpu/media/android_video_encode_accelerator.cc (right):

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.cc:28: // Subset of
MediaCodecInfo.CodecCapabilities.
On 2013/11/19 00:11:25, xhwang wrote:
> Should this be a MediaCodecBridge constant? It'd be nice to keep all Android
> specific things together.

Maybe; I think the real answer is to convert MediaCodecBridge to use
auto-generated hotness.
TODO'd in media_codec_bridge.h.

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.cc:29:
kColorFormatYUV420SemiPlanar = 21,
On 2013/11/19 00:11:25, xhwang wrote:
> ditto about enum naming style

Done.

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.cc:47: return
base::TimeDelta::FromMilliseconds(10);
On 2013/11/19 00:11:25, xhwang wrote:
> // arbitrary choice?

Done.

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.cc:76:
VideoCodecBridge::IsKnownUnaccelerated(media::kCodecVP8, true)) {
On 2013/11/19 00:11:25, xhwang wrote:
> Comment that we are only looking for "hardware vp8" encoder?

Done.

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.cc:82:
profile.max_framerate.numerator = 30;
On 2013/11/19 00:11:25, xhwang wrote:
> Where are the max resolution and size coming from? Make them const?

I don't think it's worth hoisting these to named constants that are used only
once, but commented on their origin.

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.cc:156: DVLOG(3) <<
"AndroidVideoEncodeAccelerator::Encode: " << force_keyframe;
On 2013/11/19 00:11:25, xhwang wrote:
> Here and below, will __FUNCTION__ work for you?

__PRETTY_FUNCTION__, even, since we're guaranteed gcc/clang here.
(these aren't C++98, sadly)

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.cc:177: void
AndroidVideoEncodeAccelerator::UseOutputBitstreamBuffer(
On 2013/11/19 00:11:25, xhwang wrote:
> nit comment for the interface: "Use" is more like a "Provide" ?

"Provide" in the decode API is "request", so... ;)

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.cc:200: // bitrate,
and we set that explicitly.
On 2013/11/19 00:11:25, xhwang wrote:
> Will we hit unused variable for |framerate| in release mode?

Unused warnings don't apply to function parameters.

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.cc:222: if
(!client_ptr_factory_.GetWeakPtr() || pending_frames_.empty()) return;
On 2013/11/19 00:11:25, xhwang wrote:
> nit: I like your use of if-return-in-one-line in many previous cases. But I
> found when the if-line gets long it's a little bit hard to read :) Just sayin'

I obey clang-format.  Wrapped now.

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.cc:236: if (input.b) {
On 2013/11/19 00:11:25, xhwang wrote:
> I have to go to the .h file to check what "b" is. How about:
> 
> bool& is_key_frame = input.b;
> if (is_key_frame)
> 

Done, except I refuse to use bool& instead of bool :)

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.cc:250:
frame->AllocationSize(VideoFrame::I420, frame->coded_size());
On 2013/11/19 00:11:25, xhwang wrote:
> VideoFrame::AllocationSize()?

Done.

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.cc:291: LOG(ERROR) <<
"Need more/bigger buffers; before: " << num_output_buffers_
On 2013/11/19 00:11:25, xhwang wrote:
> Use LOG_IF(ERROR, !ret)?

Oh wow I never use LOG_IF.  Done.

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.cc:336: } while
(buf_index < 0);
On 2013/11/19 00:11:25, xhwang wrote:
> In what case would buf_index < 0?

In all of the cases where status!=MEDIA_CODEC_OK.
In practice the only path that can reach here is
MEDIA_CODEC_OUTPUT_{FORMAT,BUFFERS}_CHANGED.

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
File content/common/gpu/media/android_video_encode_accelerator.h (right):

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.h:21: class
MessageLoopProxy;
On 2013/11/19 00:11:25, xhwang wrote:
> not used?

Done.

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.h:63: kIFrameInterval
= kint32max,
On 2013/11/19 00:11:25, xhwang wrote:
> we use UPPER_CASE format for enums (from
> http://www.chromium.org/developers/coding-style%29:
> 
> Though the Google C++ Style Guide now says to use kConstantNaming for enums,
> Chromium was written using MACRO_STYLE naming.  Continue to use this style for
> consistency.
> 
> Currently we have a lot of kCamelCase for enums. The plan is to convert them
in
> the future. For now, we use UPPER_CASE for new code.

Done.

https://codereview.chromium.org/74563002/diff/30001/content/common/gpu/media/...
content/common/gpu/media/android_video_encode_accelerator.h:111: uint32
last_set_bitrate_;
On 2013/11/19 00:11:25, xhwang wrote:
> unit? in bps?

Done.

https://codereview.chromium.org/74563002/diff/30001/media/base/android/java/s...
File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
(right):

https://codereview.chromium.org/74563002/diff/30001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:151:
Map<String, CodecInfo> decoderInfoMap = new HashMap<String, CodecInfo>();
On 2013/11/19 00:11:25, xhwang wrote:
> It's a bit odd to have two maps here then merge them together.

Agreed.

> But I don't know a better way to do it yet. 

This whole API is Doing It Wrong(tm) IMO.  The C++ code should be using the
MediaCodec API as proxied by auto-generation, hence my added TODO in this
patchset.

> Also, shall we comment that we prefer the first
> returned encoder/decoder in the list?

Done.

https://codereview.chromium.org/74563002/diff/30001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:399: }
On 2013/11/19 00:11:25, xhwang wrote:
> Move l.375-l.399 to l.317?

Done.

https://codereview.chromium.org/74563002/diff/30001/media/base/android/media_...
File media/base/android/media_codec_bridge.cc (right):

https://codereview.chromium.org/74563002/diff/30001/media/base/android/media_...
media/base/android/media_codec_bridge.cc:221: int index, const uint8* data, int
orig_data_size,
On 2013/11/19 00:11:25, xhwang wrote:
> Also update the header file.

Done.

https://codereview.chromium.org/74563002/diff/30001/media/base/android/media_...
media/base/android/media_codec_bridge.cc:235: int index, const uint8* data, int
orig_data_size, const uint8* key_id,
On 2013/11/19 00:11:25, xhwang wrote:
> ditto

Done.

https://codereview.chromium.org/74563002/diff/30001/media/base/android/media_...
File media/base/android/media_codec_bridge.h (right):

https://codereview.chromium.org/74563002/diff/30001/media/base/android/media_...
media/base/android/media_codec_bridge.h:138: // optionally render the buffer.
On 2013/11/19 00:11:25, xhwang wrote:
> nit: merge with the previous line?

Done.

https://codereview.chromium.org/74563002/diff/30001/media/base/android/media_...
media/base/android/media_codec_bridge.h:152: // Returns an input buffers' base
pointer and capacity.
On 2013/11/19 00:11:25, xhwang wrote:
> s/buffers'/buffer's

Done.

[Ami GONE FROM CHROMIUM, 2013-11-22 02:07:23]

On 2013/11/21 22:59:07, Ami Fischman wrote:
> I'd rather the flag went the other way for android, but I'd also rather not
have
> two polar-opposite flags.  I'm looking into how I can effect this via
> blacklisting.

This is now done in PS#3.

[xhwang, 2013-11-26 18:08:27]

lgtm % nits, sorry for the delay

https://codereview.chromium.org/74563002/diff/330001/content/browser/gpu/comp...
File content/browser/gpu/compositor_util.cc (right):

https://codereview.chromium.org/74563002/diff/330001/content/browser/gpu/comp...
content/browser/gpu/compositor_util.cc:148: "Accelerated video encode has been
disabled, either via about:flags"
Shall we mention "RTC" in the description?

https://codereview.chromium.org/74563002/diff/330001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/74563002/diff/330001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_data_manager_impl_private.cc:651:
command_line->AppendSwitch(switches::kDisableWebRtcHWEncoding);
Since the if statement spans multiple lines, we should use {} for it. Ditto for
the two "if" blocks above.

[Ami GONE FROM CHROMIUM, 2013-11-26 19:11:58]

Thanks for the review.

https://codereview.chromium.org/74563002/diff/330001/content/browser/gpu/comp...
File content/browser/gpu/compositor_util.cc (right):

https://codereview.chromium.org/74563002/diff/330001/content/browser/gpu/comp...
content/browser/gpu/compositor_util.cc:148: "Accelerated video encode has been
disabled, either via about:flags"
On 2013/11/26 18:08:28, xhwang wrote:
> Shall we mention "RTC" in the description?

Thought about it but didn't see a natural phrasing that was less awkward than
the existing.

https://codereview.chromium.org/74563002/diff/330001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/74563002/diff/330001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_data_manager_impl_private.cc:651:
command_line->AppendSwitch(switches::kDisableWebRtcHWEncoding);
On 2013/11/26 18:08:28, xhwang wrote:
> Since the if statement spans multiple lines, we should use {} for it. Ditto
for
> the two "if" blocks above.

I was using the "consistency" overriding style rule.
I used to believe that your comment was part of the style guide (and may in fact
have been the person to make you believe this :)) but it is not:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...

[Ami GONE FROM CHROMIUM, 2013-11-26 19:17:11]

@avi: please OWNERS-review content/content_common.gypi and
content/browser/resources/gpu/info_view.js
@kbr: please OWNERS-review everything else that doesn't have /media/ in its
path.

[xhwang, 2013-11-26 19:19:25]

https://codereview.chromium.org/74563002/diff/330001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/74563002/diff/330001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_data_manager_impl_private.cc:651:
command_line->AppendSwitch(switches::kDisableWebRtcHWEncoding);
On 2013/11/26 19:11:59, Ami Fischman wrote:
> On 2013/11/26 18:08:28, xhwang wrote:
> > Since the if statement spans multiple lines, we should use {} for it. Ditto
> for
> > the two "if" blocks above.
> 
> I was using the "consistency" overriding style rule.
> I used to believe that your comment was part of the style guide (and may in
fact
> have been the person to make you believe this :)) but it is not:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...

The guide says:

... conditional or loop statements with complex conditions or statements may be
more readable with curly braces.

But I agree this is not a hard rule and "complex condition" is an ambiguous
term.

[Ken Russell (switch to Gerrit), 2013-11-27 00:18:20]

LGTM for any parts for which I'm an OWNER. Couple of minor comments.

https://codereview.chromium.org/74563002/diff/330001/content/common/gpu/media...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/74563002/diff/330001/content/common/gpu/media...
content/common/gpu/media/android_video_decode_accelerator.cc:87: if
(media::VideoCodecBridge::IsKnownUnaccelerated(codec_, false))
It would be better to use an enum for this new argument so the call site is
readable. The casual reader will wonder what "false" means in this context.

https://codereview.chromium.org/74563002/diff/330001/gpu/config/software_rend...
File gpu/config/software_rendering_list_json.cc (right):

https://codereview.chromium.org/74563002/diff/330001/gpu/config/software_rend...
gpu/config/software_rendering_list_json.cc:977: "description": "MediaCodec is
still too buggy to use for encoding.",  // b/11536167.
The end-of-line comment wouldn't be valid JSON if this weren't a .cc file;
please just put the bug ID in the description.

[Ami GONE FROM CHROMIUM, 2013-11-27 01:13:12]

https://codereview.chromium.org/74563002/diff/330001/content/common/gpu/media...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/74563002/diff/330001/content/common/gpu/media...
content/common/gpu/media/android_video_decode_accelerator.cc:87: if
(media::VideoCodecBridge::IsKnownUnaccelerated(codec_, false))
On 2013/11/27 00:18:20, Ken Russell wrote:
> It would be better to use an enum for this new argument so the call site is
> readable. The casual reader will wonder what "false" means in this context.

It's a PITA but I agree it's an improvement.  Done.

https://codereview.chromium.org/74563002/diff/330001/gpu/config/software_rend...
File gpu/config/software_rendering_list_json.cc (right):

https://codereview.chromium.org/74563002/diff/330001/gpu/config/software_rend...
gpu/config/software_rendering_list_json.cc:977: "description": "MediaCodec is
still too buggy to use for encoding.",  // b/11536167.
On 2013/11/27 00:18:20, Ken Russell wrote:
> The end-of-line comment wouldn't be valid JSON if this weren't a .cc file;
> please just put the bug ID in the description.

Even though it's google-internal only?  OK.

[Ken Russell (switch to Gerrit), 2013-11-27 01:16:37]

Still LGTM.

https://codereview.chromium.org/74563002/diff/330001/content/common/gpu/media...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/74563002/diff/330001/content/common/gpu/media...
content/common/gpu/media/android_video_decode_accelerator.cc:87: if
(media::VideoCodecBridge::IsKnownUnaccelerated(codec_, false))
On 2013/11/27 01:13:13, Ami Fischman wrote:
> On 2013/11/27 00:18:20, Ken Russell wrote:
> > It would be better to use an enum for this new argument so the call site is
> > readable. The casual reader will wonder what "false" means in this context.
> 
> It's a PITA but I agree it's an improvement.  Done.

Thanks.

https://codereview.chromium.org/74563002/diff/330001/gpu/config/software_rend...
File gpu/config/software_rendering_list_json.cc (right):

https://codereview.chromium.org/74563002/diff/330001/gpu/config/software_rend...
gpu/config/software_rendering_list_json.cc:977: "description": "MediaCodec is
still too buggy to use for encoding.",  // b/11536167.
On 2013/11/27 01:13:13, Ami Fischman wrote:
> On 2013/11/27 00:18:20, Ken Russell wrote:
> > The end-of-line comment wouldn't be valid JSON if this weren't a .cc file;
> > please just put the bug ID in the description.
> 
> Even though it's google-internal only?  OK.

Thanks. I don't think it matters that it's an internal bug ID.

[Avi (use Gerrit), 2013-12-02 22:12:38]

stampity stamp LGTM

[commit-bot: I haz the power, 2013-12-02 22:24:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fischman@chromium.org/74563002/430001

[commit-bot: I haz the power, 2013-12-02 22:43:55]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Ami GONE FROM CHROMIUM, 2013-12-02 23:27:25]

@fbarchard: please OWNERS-review the addition of third_party/libyuv to
content/common/gpu/media/DEPS:
https://codereview.chromium.org/74563002/diff/430001/content/common/gpu/media...

(this is the presubmit script's way of ensuring that people don't add
inappropriate deps on your libs; in this case I don't think you'll mind, as it
lets me use libyuv::I420ToNV12 from chromium's GPU process)

[fbarchard, 2013-12-03 04:51:09]

lgtm

https://codereview.chromium.org/74563002/diff/430001/content/common/gpu/media...
File content/common/gpu/media/android_video_encode_accelerator.cc (right):

https://codereview.chromium.org/74563002/diff/430001/content/common/gpu/media...
content/common/gpu/media/android_video_encode_accelerator.cc:291:
frame->rows(VideoFrame::kYPlane);
indent of line 291 looks wrong -> should be 4.

https://codereview.chromium.org/74563002/diff/430001/gpu/config/software_rend...
File gpu/config/software_rendering_list_json.cc (right):

https://codereview.chromium.org/74563002/diff/430001/gpu/config/software_rend...
gpu/config/software_rendering_list_json.cc:977: "description": "MediaCodec is
still too buggy to use for encoding (b/11536167).",
prefer 80 cols

[Ami GONE FROM CHROMIUM, 2013-12-03 06:46:19]

Thanks for the review.

https://codereview.chromium.org/74563002/diff/430001/content/common/gpu/media...
File content/common/gpu/media/android_video_encode_accelerator.cc (right):

https://codereview.chromium.org/74563002/diff/430001/content/common/gpu/media...
content/common/gpu/media/android_video_encode_accelerator.cc:291:
frame->rows(VideoFrame::kYPlane);
On 2013/12/03 04:51:09, fbarchard wrote:
> indent of line 291 looks wrong -> should be 4.

I obey clang-format and this is what it does.

https://codereview.chromium.org/74563002/diff/430001/gpu/config/software_rend...
File gpu/config/software_rendering_list_json.cc (right):

https://codereview.chromium.org/74563002/diff/430001/gpu/config/software_rend...
gpu/config/software_rendering_list_json.cc:977: "description": "MediaCodec is
still too buggy to use for encoding (b/11536167).",
On 2013/12/03 04:51:09, fbarchard wrote:
> prefer 80 cols

Not in this file, apparently.
(this is a .cc consisting entirely of a single stringized JSON string; best to
avoid attempts at continuation).

[commit-bot: I haz the power, 2013-12-03 08:05:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fischman@chromium.org/74563002/430001

[commit-bot: I haz the power, 2013-12-03 11:23:18]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-12-03 18:49:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fischman@chromium.org/74563002/450001

[commit-bot: I haz the power, 2013-12-03 19:11:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fischman@chromium.org/74563002/450001

[commit-bot: I haz the power, 2013-12-03 19:47:05]

Retried try job too often on android_aosp for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...

[commit-bot: I haz the power, 2013-12-03 20:53:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fischman@chromium.org/74563002/480001

[commit-bot: I haz the power, 2013-12-03 23:37:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fischman@chromium.org/74563002/480001

[commit-bot: I haz the power, 2013-12-04 01:02:22]

Retried try job too often on android_aosp for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...

[commit-bot: I haz the power, 2013-12-04 01:29:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fischman@chromium.org/74563002/500001

[commit-bot: I haz the power, 2013-12-04 03:05:17]

Retried try job too often on android_aosp for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...

[commit-bot: I haz the power, 2013-12-04 03:58:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fischman@chromium.org/74563002/520001

[commit-bot: I haz the power, 2013-12-04 05:19:51]

Retried try job too often on android_aosp for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...

[commit-bot: I haz the power, 2013-12-04 06:40:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fischman@chromium.org/74563002/540001

[commit-bot: I haz the power, 2013-12-04 09:28:00]

Change committed as 238658