Refactor MediaCodecBridge to add support for NDK APIs

Refactor MediaCodecBridge to add support for NDK APIs

Currently MediaCodecBridge is implemented with android MediaCodec SDK API.
On android-21, MediaCodec NDK APIs have been added.
This CL makes MediaCodecBridge a common ancestor inherited by the
SDK and NDK implementation.
Some common java functions are moved to MediaCodecUtil.java.
The original native MediaCodecBridge class is renamed to
SdkMediaCodecBridge.
The NdkMediaCodecBridge is still WIP.

BUG=560451
Committed: https://crrev.com/5d52e0f8e63a294179df028a0dabc76a042cf9ec
Cr-Commit-Position: refs/heads/master@{#363374}

Committed: https://crrev.com/3e916b14c8494614216940c10f8be67fc668cc25
Cr-Commit-Position: refs/heads/master@{#365737}

[qinmin, 2015-11-23 19:59:33]

Patchset #1 (id:1) has been deleted

[qinmin, 2015-11-23 20:45:42]

qinmin@chromium.org changed reviewers:
+ timav@chromium.org, xhwang@chromium.org

[qinmin, 2015-11-23 20:45:42]

PTAL

[DaleCurtis, 2015-11-23 20:48:00]

Is there a performance or power improvement by switching to the NDK APIs?

[qinmin, 2015-11-23 21:03:40]

On 2015/11/23 20:48:00, DaleCurtis wrote:
> Is there a performance or power improvement by switching to the NDK APIs?

There is definitely a performance improvement as we no longer need the JNI layer
in the middle.
For power, since most power are consumed by the decoder, so this shouldn't have
great impact on that.
And unfortunately, NDK is not backward compatible (App build with new NDK cannot
run on a device with old NDKs). 
So arm32 chrome build cannot switch to NDK unless we want to build 2 separate
apks for play store.

[Tima Vaisburd, 2015-11-24 02:24:19]

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

https://codereview.chromium.org/1472943002/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:29: protected
static final int MEDIA_CODEC_DECODER = 0;
If default (so-called "package") scope works here I would use it instead of
|protected| since this class does not have subclasses.

https://codereview.chromium.org/1472943002/diff/20001/media/base/android/medi...
File media/base/android/media_codec_bridge.h (right):

https://codereview.chromium.org/1472943002/diff/20001/media/base/android/medi...
media/base/android/media_codec_bridge.h:36: // Codec direction. Keep this in
sync with MediaCodecBridge.java.
Now MediaCodecUtil.java?

https://codereview.chromium.org/1472943002/diff/20001/media/base/android/medi...
media/base/android/media_codec_bridge.h:202: static bool
RegisterMediaCodecUtil(JNIEnv* env);
Would it be cleaner to create a new C++ class MediaCodecUtil and have this
static method there, and get rid of JNI in this file? Then MediaCodecDirection
might go there, too.

https://codereview.chromium.org/1472943002/diff/20001/media/base/android/medi...
media/base/android/media_codec_bridge.h:221: virtual bool StartInternal()
WARN_UNUSED_RESULT = 0;
I would be nice to have a public method Start() instead. The only place this
method is used as virtual (i.e. through the pointer) that I found was in
VideoCodecBridge::CreateEncoder and VideoCodecBridge::CreateDecoder. Would it be
possible to just do a simple renaming and rename AudioCodecBridge::Start into
something like ConfigureAndStart (this is what it does)?

https://codereview.chromium.org/1472943002/diff/20001/media/base/android/sdk_...
File media/base/android/sdk_media_codec_bridge.h (right):

https://codereview.chromium.org/1472943002/diff/20001/media/base/android/sdk_...
media/base/android/sdk_media_codec_bridge.h:71: static bool
RegisterMediaCodecBridge(JNIEnv* env);
I would s/RegisterMediaCodecBridge/RegisterSdkMediaCodecBridge/ to be consistent
with other Register... methods.

[qinmin, 2015-11-24 20:44:06]

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

https://codereview.chromium.org/1472943002/diff/20001/media/base/android/java...
media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:29: protected
static final int MEDIA_CODEC_DECODER = 0;
On 2015/11/24 02:24:19, Tima Vaisburd wrote:
> If default (so-called "package") scope works here I would use it instead of
> |protected| since this class does not have subclasses.

Done.

https://codereview.chromium.org/1472943002/diff/20001/media/base/android/medi...
File media/base/android/media_codec_bridge.h (right):

https://codereview.chromium.org/1472943002/diff/20001/media/base/android/medi...
media/base/android/media_codec_bridge.h:36: // Codec direction. Keep this in
sync with MediaCodecBridge.java.
On 2015/11/24 02:24:19, Tima Vaisburd wrote:
> Now MediaCodecUtil.java?

Done.

https://codereview.chromium.org/1472943002/diff/20001/media/base/android/medi...
media/base/android/media_codec_bridge.h:202: static bool
RegisterMediaCodecUtil(JNIEnv* env);
On 2015/11/24 02:24:19, Tima Vaisburd wrote:
> Would it be cleaner to create a new C++ class MediaCodecUtil and have this
> static method there, and get rid of JNI in this file? Then MediaCodecDirection
> might go there, too.

Done.

https://codereview.chromium.org/1472943002/diff/20001/media/base/android/medi...
media/base/android/media_codec_bridge.h:221: virtual bool StartInternal()
WARN_UNUSED_RESULT = 0;
On 2015/11/24 02:24:19, Tima Vaisburd wrote:
> I would be nice to have a public method Start() instead. The only place this
> method is used as virtual (i.e. through the pointer) that I found was in
> VideoCodecBridge::CreateEncoder and VideoCodecBridge::CreateDecoder. Would it
be
> possible to just do a simple renaming and rename AudioCodecBridge::Start into
> something like ConfigureAndStart (this is what it does)?

Done.

https://codereview.chromium.org/1472943002/diff/20001/media/base/android/sdk_...
File media/base/android/sdk_media_codec_bridge.h (right):

https://codereview.chromium.org/1472943002/diff/20001/media/base/android/sdk_...
media/base/android/sdk_media_codec_bridge.h:71: static bool
RegisterMediaCodecBridge(JNIEnv* env);
On 2015/11/24 02:24:19, Tima Vaisburd wrote:
> I would s/RegisterMediaCodecBridge/RegisterSdkMediaCodecBridge/ to be
consistent
> with other Register... methods.

Done.

[xhwang, 2015-11-30 23:11:50]

I am not super familiar with the NDK API so I only skimmed the implementation.
Here are my comments.

https://chromiumcodereview.appspot.com/1472943002/diff/60001/media/base/andro...
File media/base/android/media_codec_bridge.cc (right):

https://chromiumcodereview.appspot.com/1472943002/diff/60001/media/base/andro...
media/base/android/media_codec_bridge.cc:27: bool
MediaCodecBridge::IsKnownUnaccelerated(const std::string& mime_type,
Now this file is small enough that I feel it make sense to make the interface
pure virtual and move (almost) everything to other places:

- IsKnownUnaccelerated() can be moved to MediaCodecUtil?

- QueueSecureInputBuffer() is exists only to translate a slightly different
interface. We should consolidate the interfaces and avoid this translation.

- I suppose we'll want to override GetOutputBuffersCount() etc in the NDK
version as well? Let's override it and put NOTIMPLEMENTED for now?

- Now we have only FillInputBuffer() left. I feel fine to duplicate it. It'd be
nice to look at either the SDK or the NDK implementation without looking back to
MediaCodecBridge (knowing that it's pure virtual).

https://chromiumcodereview.appspot.com/1472943002/diff/60001/media/base/andro...
File media/base/android/media_codec_util.cc (right):

https://chromiumcodereview.appspot.com/1472943002/diff/60001/media/base/andro...
media/base/android/media_codec_util.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
2015

https://chromiumcodereview.appspot.com/1472943002/diff/60001/media/base/andro...
File media/base/android/media_codec_util.h (right):

https://chromiumcodereview.appspot.com/1472943002/diff/60001/media/base/andro...
media/base/android/media_codec_util.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
2015

https://chromiumcodereview.appspot.com/1472943002/diff/60001/media/base/andro...
media/base/android/media_codec_util.h:36: bool MEDIA_EXPORT SupportsGetName();
Some functions in this file, e.g. media::SupportsGetName(), sound too generic. I
feel we should provide more context to make it more readable at the caller site.
Another example is media::CanDecode().

Can we put these calls in a class MediaCodecUtil? So media::SupportsGetName()
becomes media::MediaCodecUtil::SupportsGetName().

WDYT?

https://chromiumcodereview.appspot.com/1472943002/diff/60001/media/base/andro...
File media/base/android/ndk_media_codec_bridge.cc (right):

https://chromiumcodereview.appspot.com/1472943002/diff/60001/media/base/andro...
media/base/android/ndk_media_codec_bridge.cc:17: const char*
kMediaFormatKeyCropTop = "crop-top";
we prefer const array to const pointer.

where are these defined in the NDK?

https://chromiumcodereview.appspot.com/1472943002/diff/60001/media/base/andro...
media/base/android/ndk_media_codec_bridge.cc:76: *height = top - bottom + 1;
so "right" and "bottom" are inclusive? maybe worth a comment

https://chromiumcodereview.appspot.com/1472943002/diff/60001/media/base/andro...
media/base/android/ndk_media_codec_bridge.cc:78: AMediaFormat_getInt32(format,
AMEDIAFORMAT_KEY_WIDTH, width);
Q: Why we can't always use AMEDIAFORMAT_KEY_WIDTH?

https://chromiumcodereview.appspot.com/1472943002/diff/60001/media/base/andro...
media/base/android/ndk_media_codec_bridge.cc:86: AMediaFormat_getInt32(format,
AMEDIAFORMAT_KEY_SAMPLE_RATE, &sample_rate);
what if this returns false?

https://chromiumcodereview.appspot.com/1472943002/diff/60001/media/base/andro...
media/base/android/ndk_media_codec_bridge.cc:146: memcpy(iv_array, iv, 16);
The caller actually has the key and iv as vectors. We convert then to raw
pointers and sizes when making the QueueSecureInputBuffer() call and here we are
converting it back. Can we just pass in the vector directly?

https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...

https://chromiumcodereview.appspot.com/1472943002/diff/60001/media/base/andro...
File media/base/android/ndk_media_codec_bridge.h (right):

https://chromiumcodereview.appspot.com/1472943002/diff/60001/media/base/andro...
media/base/android/ndk_media_codec_bridge.h:8: #include <media/NdkMediaCodec.h>
Add a ref link to the NDK doc (if it exists)?

https://chromiumcodereview.appspot.com/1472943002/diff/60001/media/base/andro...
media/base/android/ndk_media_codec_bridge.h:21: 
// MediaCodecBridge implementation.

https://chromiumcodereview.appspot.com/1472943002/diff/60001/media/base/andro...
File media/base/android/sdk_media_codec_bridge.cc (right):

https://chromiumcodereview.appspot.com/1472943002/diff/60001/media/base/andro...
media/base/android/sdk_media_codec_bridge.cc:306: memcpy(dst,
static_cast<uint8*>(src_data) + offset, dst_size);
This is a non-trivial change. How did it work before?

https://chromiumcodereview.appspot.com/1472943002/diff/60001/media/media.gyp
File media/media.gyp (right):

https://chromiumcodereview.appspot.com/1472943002/diff/60001/media/media.gyp#...
media/media.gyp:1917: ['target_arch=="arm64" or target_arch=="x64" or
target_arch=="mips64el"', {
add a comment about why NDK is only used under these conditions?

https://chromiumcodereview.appspot.com/1472943002/diff/60001/media/media.gyp#...
media/media.gyp:1920: 'base/android/ndk_media_codec_bridge.h',
do we also want to exclude base/android/sdk_media_codec_bridge.*?

[qinmin, 2015-12-03 22:46:24]

PTAL again

https://codereview.chromium.org/1472943002/diff/60001/media/base/android/medi...
File media/base/android/media_codec_bridge.cc (right):

https://codereview.chromium.org/1472943002/diff/60001/media/base/android/medi...
media/base/android/media_codec_bridge.cc:27: bool
MediaCodecBridge::IsKnownUnaccelerated(const std::string& mime_type,
On 2015/11/30 23:11:50, xhwang wrote:
> Now this file is small enough that I feel it make sense to make the interface
> pure virtual and move (almost) everything to other places:
> 
> - IsKnownUnaccelerated() can be moved to MediaCodecUtil?
> 
> - QueueSecureInputBuffer() is exists only to translate a slightly different
> interface. We should consolidate the interfaces and avoid this translation.
> 
> - I suppose we'll want to override GetOutputBuffersCount() etc in the NDK
> version as well? Let's override it and put NOTIMPLEMENTED for now?
> 
> - Now we have only FillInputBuffer() left. I feel fine to duplicate it. It'd
be
> nice to look at either the SDK or the NDK implementation without looking back
to
> MediaCodecBridge (knowing that it's pure virtual).

Moved IsKnownUnaccelerated() to MediaCodecUtil.

GetOutputBuffersCount() is deprecated and will not be supported in NDK. We
should remove using GetOutputBuffersCount() in our code.

I will leave QueueSecureInputBuffer() as it is. Current it is used by AVDA and
MediaCodecPlayer. Since we are in the transition from WMPA->WMPI, i will wait
until that's finished to make a decision on which interface we want to keep.

https://codereview.chromium.org/1472943002/diff/60001/media/base/android/medi...
File media/base/android/media_codec_util.cc (right):

https://codereview.chromium.org/1472943002/diff/60001/media/base/android/medi...
media/base/android/media_codec_util.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2015/11/30 23:11:50, xhwang wrote:
> 2015

Done.

https://codereview.chromium.org/1472943002/diff/60001/media/base/android/medi...
File media/base/android/media_codec_util.h (right):

https://codereview.chromium.org/1472943002/diff/60001/media/base/android/medi...
media/base/android/media_codec_util.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2015/11/30 23:11:50, xhwang wrote:
> 2015

Done.

https://codereview.chromium.org/1472943002/diff/60001/media/base/android/medi...
media/base/android/media_codec_util.h:36: bool MEDIA_EXPORT SupportsGetName();
On 2015/11/30 23:11:50, xhwang wrote:
> Some functions in this file, e.g. media::SupportsGetName(), sound too generic.
I
> feel we should provide more context to make it more readable at the caller
site.
> Another example is media::CanDecode().
> 
> Can we put these calls in a class MediaCodecUtil? So media::SupportsGetName()
> becomes media::MediaCodecUtil::SupportsGetName().
> 
> WDYT?

Done.

https://codereview.chromium.org/1472943002/diff/60001/media/base/android/ndk_...
File media/base/android/ndk_media_codec_bridge.cc (right):

https://codereview.chromium.org/1472943002/diff/60001/media/base/android/ndk_...
media/base/android/ndk_media_codec_bridge.cc:17: const char*
kMediaFormatKeyCropTop = "crop-top";
On 2015/11/30 23:11:50, xhwang wrote:
> we prefer const array to const pointer.
> 
> where are these defined in the NDK?

These are hidden keys, but you can check MediaCodecBridge as we also use these
keys there.

https://codereview.chromium.org/1472943002/diff/60001/media/base/android/ndk_...
media/base/android/ndk_media_codec_bridge.cc:76: *height = top - bottom + 1;
On 2015/11/30 23:11:50, xhwang wrote:
> so "right" and "bottom" are inclusive? maybe worth a comment

Done.

https://codereview.chromium.org/1472943002/diff/60001/media/base/android/ndk_...
media/base/android/ndk_media_codec_bridge.cc:78: AMediaFormat_getInt32(format,
AMEDIAFORMAT_KEY_WIDTH, width);
On 2015/11/30 23:11:50, xhwang wrote:
> Q: Why we can't always use AMEDIAFORMAT_KEY_WIDTH?

It is not as accurate as crop_width, see b/18224769

https://codereview.chromium.org/1472943002/diff/60001/media/base/android/ndk_...
media/base/android/ndk_media_codec_bridge.cc:86: AMediaFormat_getInt32(format,
AMEDIAFORMAT_KEY_SAMPLE_RATE, &sample_rate);
On 2015/11/30 23:11:50, xhwang wrote:
> what if this returns false?

This is a mandatory key, so it should never return false. added a DCHECK here.

https://codereview.chromium.org/1472943002/diff/60001/media/base/android/ndk_...
media/base/android/ndk_media_codec_bridge.cc:146: memcpy(iv_array, iv, 16);
On 2015/11/30 23:11:50, xhwang wrote:
> The caller actually has the key and iv as vectors. We convert then to raw
> pointers and sizes when making the QueueSecureInputBuffer() call and here we
are
> converting it back. Can we just pass in the vector directly?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...

Done.

https://codereview.chromium.org/1472943002/diff/60001/media/base/android/ndk_...
File media/base/android/ndk_media_codec_bridge.h (right):

https://codereview.chromium.org/1472943002/diff/60001/media/base/android/ndk_...
media/base/android/ndk_media_codec_bridge.h:8: #include <media/NdkMediaCodec.h>
On 2015/11/30 23:11:50, xhwang wrote:
> Add a ref link to the NDK doc (if it exists)?

NDK don't have a reference page.

https://codereview.chromium.org/1472943002/diff/60001/media/base/android/ndk_...
media/base/android/ndk_media_codec_bridge.h:21: 
On 2015/11/30 23:11:50, xhwang wrote:
> // MediaCodecBridge implementation.

Done.

https://codereview.chromium.org/1472943002/diff/60001/media/base/android/sdk_...
File media/base/android/sdk_media_codec_bridge.cc (right):

https://codereview.chromium.org/1472943002/diff/60001/media/base/android/sdk_...
media/base/android/sdk_media_codec_bridge.cc:306: memcpy(dst,
static_cast<uint8*>(src_data) + offset, dst_size);
On 2015/11/30 23:11:50, xhwang wrote:
> This is a non-trivial change. How did it work before?

This call is only used in AVEA, which is not used by anyone. So we luckily
didn't hit this bug previously.

https://codereview.chromium.org/1472943002/diff/60001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/1472943002/diff/60001/media/media.gyp#newcode...
media/media.gyp:1917: ['target_arch=="arm64" or target_arch=="x64" or
target_arch=="mips64el"', {
On 2015/11/30 23:11:50, xhwang wrote:
> add a comment about why NDK is only used under these conditions?

Done.

https://codereview.chromium.org/1472943002/diff/60001/media/media.gyp#newcode...
media/media.gyp:1920: 'base/android/ndk_media_codec_bridge.h',
On 2015/11/30 23:11:50, xhwang wrote:
> do we also want to exclude base/android/sdk_media_codec_bridge.*?

Not now, the ndk work is still WIP, removing sdk_media_codec_bridge will break
MSE on 64 bit devices.

[Tima Vaisburd, 2015-12-04 02:04:48]

lgtm % nits

https://codereview.chromium.org/1472943002/diff/80001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/1472943002/diff/80001/media/BUILD.gn#newcode398
media/BUILD.gn:398: if (current_cpu == "arm64" || current_cpu == "x64" ||
Please explain why we only use NDK for 64 bits here (in GN) too.

https://codereview.chromium.org/1472943002/diff/80001/media/base/android/medi...
File media/base/android/media_codec_bridge.h (right):

https://codereview.chromium.org/1472943002/diff/80001/media/base/android/medi...
media/base/android/media_codec_bridge.h:8: #include <jni.h>
<jni.h> is not needed any more?

https://codereview.chromium.org/1472943002/diff/80001/media/base/android/medi...
media/base/android/media_codec_bridge.h:55: virtual bool Start()
WARN_UNUSED_RESULT = 0;
Can we remove WARN_UNUSED_RESULT here?

https://codereview.chromium.org/1472943002/diff/80001/media/base/android/medi...
File media/base/android/media_codec_util.h (right):

https://codereview.chromium.org/1472943002/diff/80001/media/base/android/medi...
media/base/android/media_codec_util.h:34: class MEDIA_EXPORT MediaCodecUtil {
[Q] This is a general question. This class contains static method only. When to
prefer such class and when to have a set of regular functions, perhaps in a
namespace? I'm not saying I'm proposing a change here, it is just a question.

https://codereview.chromium.org/1472943002/diff/80001/media/base/android/medi...
media/base/android/media_codec_util.h:61:
DISALLOW_COPY_AND_ASSIGN(MediaCodecUtil);
I would just drop the DISALLOW, it seems the class is only to group the
functions together?

[xhwang, 2015-12-04 18:07:58]

lgtm % nits

https://chromiumcodereview.appspot.com/1472943002/diff/80001/content/renderer...
File content/renderer/media/webrtc/peer_connection_dependency_factory.cc
(right):

https://chromiumcodereview.appspot.com/1472943002/diff/80001/content/renderer...
content/renderer/media/webrtc/peer_connection_dependency_factory.cc:23: 
empty line not needed? but probably this is from a rebase.

https://chromiumcodereview.appspot.com/1472943002/diff/80001/media/base/andro...
File media/base/android/media_codec_bridge.h (right):

https://chromiumcodereview.appspot.com/1472943002/diff/80001/media/base/andro...
media/base/android/media_codec_bridge.h:104: std::vector<char> iv,
pass by const ref

also, it seems these two are passed as strings. since we need to convert them at
the JNI layer anyways, can we also pass strings here to avoid one layer of
conversion?

https://chromiumcodereview.appspot.com/1472943002/diff/80001/media/base/andro...
File media/base/android/media_codec_util.h (right):

https://chromiumcodereview.appspot.com/1472943002/diff/80001/media/base/andro...
media/base/android/media_codec_util.h:34: class MEDIA_EXPORT MediaCodecUtil {
On 2015/12/04 02:04:48, Tima Vaisburd wrote:
> [Q] This is a general question. This class contains static method only. When
to
> prefer such class and when to have a set of regular functions, perhaps in a
> namespace? I'm not saying I'm proposing a change here, it is just a question.

We don't use an additional namespace for this purpose.

That being said, if all functions are generally useful in the media/ namespace,
we'll declare them as regular functions.

If they are only useful for one class, we typically declare these in that class,
or in the .cc file as static functions.

This is a special case where these functions are used by two classes. This
actually make me wonder why we don't just put them in MediaCodecBridge?

https://chromiumcodereview.appspot.com/1472943002/diff/80001/media/base/andro...
File media/base/android/ndk_media_codec_bridge.cc (right):

https://chromiumcodereview.appspot.com/1472943002/diff/80001/media/base/andro...
media/base/android/ndk_media_codec_bridge.cc:146:
reinterpret_cast<uint8*>(&key_id[0]),
we can use vector::data() now

https://chromiumcodereview.appspot.com/1472943002/diff/80001/media/base/andro...
File media/base/android/ndk_media_codec_bridge.h (right):

https://chromiumcodereview.appspot.com/1472943002/diff/80001/media/base/andro...
media/base/android/ndk_media_codec_bridge.h:8: #include <media/NdkMediaCodec.h>
hmm, without any reference page. How are people supposed to maintain this code?

[Tima Vaisburd, 2015-12-04 18:52:18]

https://chromiumcodereview.appspot.com/1472943002/diff/80001/media/base/andro...
File media/base/android/media_codec_util.h (right):

https://chromiumcodereview.appspot.com/1472943002/diff/80001/media/base/andro...
media/base/android/media_codec_util.h:34: class MEDIA_EXPORT MediaCodecUtil {
On 2015/12/04 18:07:58, xhwang wrote:
> On 2015/12/04 02:04:48, Tima Vaisburd wrote:
> > [Q] This is a general question. This class contains static method only. When
> to
> > prefer such class and when to have a set of regular functions, perhaps in a
> > namespace? I'm not saying I'm proposing a change here, it is just a
question.
> 
> We don't use an additional namespace for this purpose.
> 
> That being said, if all functions are generally useful in the media/
namespace,
> we'll declare them as regular functions.
> 
> If they are only useful for one class, we typically declare these in that
class,
> or in the .cc file as static functions.
> 
> This is a special case where these functions are used by two classes. This
> actually make me wonder why we don't just put them in MediaCodecBridge?

In the original CL they were part of MediaCodecBridge, if I remember correctly.
But then it was suggested to keep the MediaCodecBridge to be pure interface, and
move non pure virtual methods to another place. I quite like that suggestion,
so, in my opinion, having them in MediaCodecUtil - as it is right now - is
cleaner.

[qinmin, 2015-12-04 20:26:03]

qinmin@chromium.org changed reviewers:
+ sievers@chromium.org

[qinmin, 2015-12-04 20:26:04]

+sievers for content/ OWNER

https://codereview.chromium.org/1472943002/diff/80001/content/renderer/media/...
File content/renderer/media/webrtc/peer_connection_dependency_factory.cc
(right):

https://codereview.chromium.org/1472943002/diff/80001/content/renderer/media/...
content/renderer/media/webrtc/peer_connection_dependency_factory.cc:23: 
On 2015/12/04 18:07:58, xhwang wrote:
> empty line not needed? but probably this is from a rebase.

Done.

https://codereview.chromium.org/1472943002/diff/80001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/1472943002/diff/80001/media/BUILD.gn#newcode398
media/BUILD.gn:398: if (current_cpu == "arm64" || current_cpu == "x64" ||
On 2015/12/04 02:04:48, Tima Vaisburd wrote:
> Please explain why we only use NDK for 64 bits here (in GN) too.

Done.

https://codereview.chromium.org/1472943002/diff/80001/media/base/android/medi...
File media/base/android/media_codec_bridge.h (right):

https://codereview.chromium.org/1472943002/diff/80001/media/base/android/medi...
media/base/android/media_codec_bridge.h:8: #include <jni.h>
On 2015/12/04 02:04:48, Tima Vaisburd wrote:
> <jni.h> is not needed any more?

Done.

https://codereview.chromium.org/1472943002/diff/80001/media/base/android/medi...
media/base/android/media_codec_bridge.h:55: virtual bool Start()
WARN_UNUSED_RESULT = 0;
On 2015/12/04 02:04:48, Tima Vaisburd wrote:
> Can we remove WARN_UNUSED_RESULT here?

Done.

https://codereview.chromium.org/1472943002/diff/80001/media/base/android/medi...
media/base/android/media_codec_bridge.h:104: std::vector<char> iv,
On 2015/12/04 18:07:58, xhwang wrote:
> pass by const ref
> 
> also, it seems these two are passed as strings. since we need to convert them
at
> the JNI layer anyways, can we also pass strings here to avoid one layer of
> conversion?

We already have the string version defined above. The Java class uses byte
arrays, so the string needs to be converted to jbyteArray. Currently
MediaCodecPlayer uses this vector version, and AVDA uses the string version. For
MediaCodecPlayer, convert a vector into string and then back into an array seems
redundant work. Eventually we will remove the vector version after we move to
WMPI.

https://codereview.chromium.org/1472943002/diff/80001/media/base/android/medi...
File media/base/android/media_codec_util.h (right):

https://codereview.chromium.org/1472943002/diff/80001/media/base/android/medi...
media/base/android/media_codec_util.h:61:
DISALLOW_COPY_AND_ASSIGN(MediaCodecUtil);
On 2015/12/04 02:04:48, Tima Vaisburd wrote:
> I would just drop the DISALLOW, it seems the class is only to group the
> functions together?

Done.

https://codereview.chromium.org/1472943002/diff/80001/media/base/android/ndk_...
File media/base/android/ndk_media_codec_bridge.cc (right):

https://codereview.chromium.org/1472943002/diff/80001/media/base/android/ndk_...
media/base/android/ndk_media_codec_bridge.cc:146:
reinterpret_cast<uint8*>(&key_id[0]),
On 2015/12/04 18:07:58, xhwang wrote:
> we can use vector::data() now

Done.

https://codereview.chromium.org/1472943002/diff/80001/media/base/android/ndk_...
File media/base/android/ndk_media_codec_bridge.h (right):

https://codereview.chromium.org/1472943002/diff/80001/media/base/android/ndk_...
media/base/android/ndk_media_codec_bridge.h:8: #include <media/NdkMediaCodec.h>
On 2015/12/04 18:07:58, xhwang wrote:
> hmm, without any reference page. How are people supposed to maintain this
code?

like what happened to opensles_output.cc code, there is no reference for
OpenSLES_Android. But the header file included here should provide good
information for developers to understand how to use the calls.

[no sievers, 2015-12-04 22:14:10]

content lgtm

[qinmin, 2015-12-04 22:15:00]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2015-12-04 22:43:23]

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

[commit-bot: I haz the power, 2015-12-04 23:38:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-04 23:38:19]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[qinmin, 2015-12-05 00:10:42]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2015-12-05 00:10:43]

The patchset sent to the CQ was uploaded after l-g-t-m from timav@chromium.org,
xhwang@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1472943002/#ps100001
(title: "fix presubmit warnings")

[commit-bot: I haz the power, 2015-12-05 00:16:04]

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

[commit-bot: I haz the power, 2015-12-05 02:26:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-05 02:26:21]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)

[qinmin, 2015-12-05 04:57:35]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2015-12-05 04:58:43]

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

[commit-bot: I haz the power, 2015-12-05 07:01:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-05 07:01:19]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)

[qinmin, 2015-12-05 07:12:46]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2015-12-05 07:14:12]

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

[commit-bot: I haz the power, 2015-12-05 09:15:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-05 09:15:42]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)

[qinmin, 2015-12-05 15:56:12]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2015-12-05 15:56:28]

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

[commit-bot: I haz the power, 2015-12-05 17:57:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-05 17:57:53]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)

[qinmin, 2015-12-06 17:48:57]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2015-12-06 17:49:09]

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

[commit-bot: I haz the power, 2015-12-06 18:56:23]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-12-06 18:57:29]

Description was changed from

==========
Refactor MediaCodecBridge to add support for NDK APIs

Currently MediaCodecBridge is implemented with android MediaCodec SDK API.
On android-21, MediaCodec NDK APIs have been added.
This CL makes MediaCodecBridge a common ancestor inherited by the
SDK and NDK implementation.
Some common java functions are moved to MediaCodecUtil.java.
The original native MediaCodecBridge class is renamed to
SdkMediaCodecBridge.
The NdkMediaCodecBridge is still WIP.

BUG=560451
==========

to

==========
Refactor MediaCodecBridge to add support for NDK APIs

Currently MediaCodecBridge is implemented with android MediaCodec SDK API.
On android-21, MediaCodec NDK APIs have been added.
This CL makes MediaCodecBridge a common ancestor inherited by the
SDK and NDK implementation.
Some common java functions are moved to MediaCodecUtil.java.
The original native MediaCodecBridge class is renamed to
SdkMediaCodecBridge.
The NdkMediaCodecBridge is still WIP.

BUG=560451

Committed: https://crrev.com/5d52e0f8e63a294179df028a0dabc76a042cf9ec
Cr-Commit-Position: refs/heads/master@{#363374}
==========

[commit-bot: I haz the power, 2015-12-06 18:57:30]

Patchset 5 (id:??) landed as
https://crrev.com/5d52e0f8e63a294179df028a0dabc76a042cf9ec
Cr-Commit-Position: refs/heads/master@{#363374}

[acleung1, 2015-12-14 23:12:43]

A revert of this CL (patchset #5 id:100001) has been created in
https://codereview.chromium.org/1525003002/ by acleung@chromium.org.

The reason for reverting is: This breaks 64bit ARM

https://code.google.com/p/chromium/issues/detail?id=568923.

[qinmin, 2015-12-15 21:34:35]

Description was changed from

==========
Refactor MediaCodecBridge to add support for NDK APIs

Currently MediaCodecBridge is implemented with android MediaCodec SDK API.
On android-21, MediaCodec NDK APIs have been added.
This CL makes MediaCodecBridge a common ancestor inherited by the
SDK and NDK implementation.
Some common java functions are moved to MediaCodecUtil.java.
The original native MediaCodecBridge class is renamed to
SdkMediaCodecBridge.
The NdkMediaCodecBridge is still WIP.

BUG=560451

Committed: https://crrev.com/5d52e0f8e63a294179df028a0dabc76a042cf9ec
Cr-Commit-Position: refs/heads/master@{#363374}
==========

to

==========
Refactor MediaCodecBridge to add support for NDK APIs

Currently MediaCodecBridge is implemented with android MediaCodec SDK API.
On android-21, MediaCodec NDK APIs have been added.
This CL makes MediaCodecBridge a common ancestor inherited by the
SDK and NDK implementation.
Some common java functions are moved to MediaCodecUtil.java.
The original native MediaCodecBridge class is renamed to
SdkMediaCodecBridge.
The NdkMediaCodecBridge is still WIP.

BUG=560451

Committed: https://crrev.com/5d52e0f8e63a294179df028a0dabc76a042cf9ec
Cr-Commit-Position: refs/heads/master@{#363374}
==========

[qinmin, 2015-12-15 21:36:27]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2015-12-15 21:36:28]

The patchset sent to the CQ was uploaded after l-g-t-m from timav@chromium.org,
xhwang@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1472943002/#ps120001
(title: "fixing crazylinker issue")

[commit-bot: I haz the power, 2015-12-15 21:37:15]

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

[commit-bot: I haz the power, 2015-12-15 21:43:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-15 21:43:34]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[qinmin, 2015-12-15 22:57:38]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2015-12-15 22:57:38]

The patchset sent to the CQ was uploaded after l-g-t-m from timav@chromium.org,
xhwang@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1472943002/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2015-12-15 23:00:17]

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

[commit-bot: I haz the power, 2015-12-16 00:13:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-16 00:13:09]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)

[qinmin, 2015-12-17 00:35:22]

Patchset #6 (id:120001) has been deleted

[qinmin, 2015-12-17 00:35:38]

Patchset #6 (id:140001) has been deleted

[qinmin, 2015-12-17 00:35:44]

The CQ bit was checked by qinmin@chromium.org

[qinmin, 2015-12-17 00:35:44]

The patchset sent to the CQ was uploaded after l-g-t-m from timav@chromium.org,
xhwang@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1472943002/#ps160001
(title: "fix crazy linker issue")

[commit-bot: I haz the power, 2015-12-17 00:38:07]

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

[commit-bot: I haz the power, 2015-12-17 03:59:50]

Description was changed from

==========
Refactor MediaCodecBridge to add support for NDK APIs

Currently MediaCodecBridge is implemented with android MediaCodec SDK API.
On android-21, MediaCodec NDK APIs have been added.
This CL makes MediaCodecBridge a common ancestor inherited by the
SDK and NDK implementation.
Some common java functions are moved to MediaCodecUtil.java.
The original native MediaCodecBridge class is renamed to
SdkMediaCodecBridge.
The NdkMediaCodecBridge is still WIP.

BUG=560451

Committed: https://crrev.com/5d52e0f8e63a294179df028a0dabc76a042cf9ec
Cr-Commit-Position: refs/heads/master@{#363374}
==========

to

==========
Refactor MediaCodecBridge to add support for NDK APIs

Currently MediaCodecBridge is implemented with android MediaCodec SDK API.
On android-21, MediaCodec NDK APIs have been added.
This CL makes MediaCodecBridge a common ancestor inherited by the
SDK and NDK implementation.
Some common java functions are moved to MediaCodecUtil.java.
The original native MediaCodecBridge class is renamed to
SdkMediaCodecBridge.
The NdkMediaCodecBridge is still WIP.

BUG=560451

Committed: https://crrev.com/5d52e0f8e63a294179df028a0dabc76a042cf9ec
Cr-Commit-Position: refs/heads/master@{#363374}
==========

[commit-bot: I haz the power, 2015-12-17 03:59:51]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2015-12-17 04:01:18]

Description was changed from

==========
Refactor MediaCodecBridge to add support for NDK APIs

Currently MediaCodecBridge is implemented with android MediaCodec SDK API.
On android-21, MediaCodec NDK APIs have been added.
This CL makes MediaCodecBridge a common ancestor inherited by the
SDK and NDK implementation.
Some common java functions are moved to MediaCodecUtil.java.
The original native MediaCodecBridge class is renamed to
SdkMediaCodecBridge.
The NdkMediaCodecBridge is still WIP.

BUG=560451

Committed: https://crrev.com/5d52e0f8e63a294179df028a0dabc76a042cf9ec
Cr-Commit-Position: refs/heads/master@{#363374}
==========

to

==========
Refactor MediaCodecBridge to add support for NDK APIs

Currently MediaCodecBridge is implemented with android MediaCodec SDK API.
On android-21, MediaCodec NDK APIs have been added.
This CL makes MediaCodecBridge a common ancestor inherited by the
SDK and NDK implementation.
Some common java functions are moved to MediaCodecUtil.java.
The original native MediaCodecBridge class is renamed to
SdkMediaCodecBridge.
The NdkMediaCodecBridge is still WIP.

BUG=560451

Committed: https://crrev.com/5d52e0f8e63a294179df028a0dabc76a042cf9ec
Cr-Commit-Position: refs/heads/master@{#363374}

Committed: https://crrev.com/3e916b14c8494614216940c10f8be67fc668cc25
Cr-Commit-Position: refs/heads/master@{#365737}
==========

[commit-bot: I haz the power, 2015-12-17 04:01:19]

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