media: Remove the unused NdkMediaCodecBridge

media: Remove the unused NdkMediaCodecBridge

NdkMediaCodecBridge has been incomplete and unused since it was
introduced over a year ago. Since we don't have short term plans to
make it production ready, we'll remove it for now and bring it back if
we need it later.
Some rationale:
 * The NDK API is a second class citizen compared to the Java API, and
   is missing new APIs. For example, it's missing the callback API, and
   setOutputSurface(), which means we can't use it on Marshmallow.
 * It only works with 64 bit builds.
 * The benefit of removing JNI call overhead isn't expected to be very
   large anyway. JNI calls add 100s of nanoseconds to 10s of microseconds
   of overhead (according to internal mailing lists/docs). We have lower
   hanging fruit, like using MediaCodec's callback API instead of polling.

This change includes collapsing SdkMediaCodecBridge into
MediaCodecBridge. In doing that, MediaCodecLoop and its unit test no
longer build cross-platform, so this CL also removes the "anywhere"
targets. Alternatively we could have kept a platform independent
virtual MediaCodecBridge interface with a single impl.

BUG=560451
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2672313006
Cr-Commit-Position: refs/heads/master@{#450529}
Committed: https://chromium.googlesource.com/chromium/src/+/ddd685822dcdc6ec5e1fd32bb6ff3824d93c675c

[watk, 2017-02-07 02:42:28]

Description was changed from

==========
media: Remove the unused NdkMediaCodecBridge

NdkMediaCodecBridge has been incomplete and unused since it was
introduced over a year ago. Since we don't have short term plans to
make it production ready, we'll remove it for now and bring it back if
we need it later.
Some rationale:
 * The NDK API is a second class citizen compared to the Java API, and
   is missing new APIs. For example, it's missing the callback API, and
   setOutputSurface(), which means we can't use it on Marshmallow.
 * It only works with 64 bit builds.
 * The benefit of removing JNI call overhead isn't expected to be very
   large anyway. JNI calls add 100s of nanoseconds to 10s of microseconds
   of overhead (according to internal mailing lists/docs). We have lower
   hanging fruit, like using MediaCodec's callback API instead of polling.

This change includes collapsing SdkMediaCodecBridge into
MediaCodecBridge. In doing that, MediaCodecLoop and its unit test no
longer build cross-platform, so this CL also removes the "anywhere"
targets. Alternatively we could have kept a platform independent
virtual MediaCodecBridge interface with a single impl.

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

to

==========
media: Remove the unused NdkMediaCodecBridge

NdkMediaCodecBridge has been incomplete and unused since it was
introduced over a year ago. Since we don't have short term plans to
make it production ready, we'll remove it for now and bring it back if
we need it later.
Some rationale:
 * The NDK API is a second class citizen compared to the Java API, and
   is missing new APIs. For example, it's missing the callback API, and
   setOutputSurface(), which means we can't use it on Marshmallow.
 * It only works with 64 bit builds.
 * The benefit of removing JNI call overhead isn't expected to be very
   large anyway. JNI calls add 100s of nanoseconds to 10s of microseconds
   of overhead (according to internal mailing lists/docs). We have lower
   hanging fruit, like using MediaCodec's callback API instead of polling.

This change includes collapsing SdkMediaCodecBridge into
MediaCodecBridge. In doing that, MediaCodecLoop and its unit test no
longer build cross-platform, so this CL also removes the "anywhere"
targets. Alternatively we could have kept a platform independent
virtual MediaCodecBridge interface with a single impl.

BUG=560451
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[watk, 2017-02-07 02:45:03]

watk@chromium.org changed reviewers:
+ liberato@chromium.org

[watk, 2017-02-07 02:45:03]

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

https://codereview.chromium.org/2672313006/diff/1/media/base/android/java/src...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:242:
Log.w(TAG, "Codec released");
This change was kind of random. Happy to revert if you want.

[liberato (no reviews please), 2017-02-07 06:19:43]

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

https://codereview.chromium.org/2672313006/diff/1/media/base/android/java/src...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:242:
Log.w(TAG, "Codec released");
On 2017/02/07 02:45:03, watk wrote:
> This change was kind of random. Happy to revert if you want.

i like it.

https://codereview.chromium.org/2672313006/diff/1/media/base/android/media_co...
File media/base/android/media_codec_bridge.h (right):

https://codereview.chromium.org/2672313006/diff/1/media/base/android/media_co...
media/base/android/media_codec_bridge.h:147: // This constructor is only for
making this class mockable.
personally, i think that this should still be an interface.  it's okay if the
interface now includes java-specific types things that cause it not to compile
as android_anywhere.

however, i don't think that it adds any clarity merging the sdk impl and the
interface.  having an SdkMediaCodecBridge isn't by itself bad.

i believe that i misunderstood our offline discussion, and i apologize.  i
thought that removing ndk caused it to became convenient to move methods with
non-portable signatures (e.g. setOutputSurface) into MCB, breaking
android_anywhere.

https://codereview.chromium.org/2672313006/diff/1/media/base/android/media_co...
media/base/android/media_codec_bridge.h:181: class MEDIA_EXPORT AudioCodecBridge
: public MediaCodecBridge {
this and VideoCodecBridge should be in their own headers / impls.

they are wrong in ToT, too, in my opinion.  but if you're moving them, then
might as well split them up.

https://codereview.chromium.org/2672313006/diff/1/media/base/android/media_co...
media/base/android/media_codec_bridge.h:216: class MEDIA_EXPORT VideoCodecBridge
: public MediaCodecBridge {
i keep going back and forth on these.  on one hand, it's kind of nice to have a
VideoCodecBridge instead of a MediaCodecBridge that has some handy helper
functions.

however, it's also handy if they aren't concrete implementations.  how to get
both of those together is a good question.  :)

the constructors can eventually be moved into factories, as can the other
statics.  i.e., one could get a VideoCodecBridgeFactory that can answer (without
statics) IsKnownUnaccelerated.  probably not part of this cl, but as part of the
larger picture of enabling better testing.

one might also argue that, since the underlying MediaCodec object doesn't have
video and audio variants, then neither should we.  it seems kinda arbitrary that
we have VideoCodecBridge but not VideoDecoderBridge / VideoEncoderBridge.  one
could still keep the factories separate to control the arguments to
construction, which is really where most of the differences are.  the other
differences are just extra functions that are just as easy not to call if one
doesn't need them.

i'm sure that it's all out of scope for this CL, but i'm curious about your
thoughts on it.

https://codereview.chromium.org/2672313006/diff/1/media/base/android/media_co...
media/base/android/media_codec_bridge.h:223: static VideoCodecBridge*
CreateDecoder(
if you're aiming to make these testable, then these should be factories.  once
you remove the sdk / ndk "pick an include" method of inheritance, we don't need
these static creation functions either.

maybe it's outside the scope of this CL, but it's worth considering if you
haven't already.

[DaleCurtis, 2017-02-07 20:08:02]

I'm definitely in favor of either killing Audio/Video codec bridge or splitting
them into their own classes.

https://codereview.chromium.org/2672313006/diff/1/media/base/android/media_co...
File media/base/android/media_codec_bridge.h (right):

https://codereview.chromium.org/2672313006/diff/1/media/base/android/media_co...
media/base/android/media_codec_bridge.h:147: // This constructor is only for
making this class mockable.
On 2017/02/07 at 06:19:43, liberato wrote:
> personally, i think that this should still be an interface.  it's okay if the
interface now includes java-specific types things that cause it not to compile
as android_anywhere.
> 
> however, i don't think that it adds any clarity merging the sdk impl and the
interface.  having an SdkMediaCodecBridge isn't by itself bad.
> 
> i believe that i misunderstood our offline discussion, and i apologize.  i
thought that removing ndk caused it to became convenient to move methods with
non-portable signatures (e.g. setOutputSurface) into MCB, breaking
android_anywhere.

Chrome is taking an active stance to reduce layers these days (see onion soup),
interfaces that only serve one implementation are unnecessary overhead and
should be removed.

A better argument would be that keeping the interface allows us to mock out
badly behaving MediaCodec implementations to simulate their badness and thus
write better tests for problematic devices.

[liberato (no reviews please), 2017-02-07 20:37:30]

https://codereview.chromium.org/2672313006/diff/1/media/base/android/media_co...
File media/base/android/media_codec_bridge.h (right):

https://codereview.chromium.org/2672313006/diff/1/media/base/android/media_co...
media/base/android/media_codec_bridge.h:147: // This constructor is only for
making this class mockable.
On 2017/02/07 20:08:01, DaleCurtis wrote:
> On 2017/02/07 at 06:19:43, liberato wrote:
> > personally, i think that this should still be an interface.  it's okay if
the
> interface now includes java-specific types things that cause it not to compile
> as android_anywhere.
> > 
> > however, i don't think that it adds any clarity merging the sdk impl and the
> interface.  having an SdkMediaCodecBridge isn't by itself bad.
> > 
> > i believe that i misunderstood our offline discussion, and i apologize.  i
> thought that removing ndk caused it to became convenient to move methods with
> non-portable signatures (e.g. setOutputSurface) into MCB, breaking
> android_anywhere.
> 
> Chrome is taking an active stance to reduce layers these days (see onion
soup),
> interfaces that only serve one implementation are unnecessary overhead and
> should be removed.
> 
> A better argument would be that keeping the interface allows us to mock out
> badly behaving MediaCodec implementations to simulate their badness and thus
> write better tests for problematic devices.

won't we need virtual functions anyway to mock this thing (badly behaving codecs
or an (elusive!) correctly behaving codec)?  mocking is my primary concern,
though i didn't write that anywhere in the comment.  :)

[watk, 2017-02-08 02:53:58]

thanks! I put back the virtual interface

https://codereview.chromium.org/2672313006/diff/1/media/base/android/media_co...
File media/base/android/media_codec_bridge.h (right):

https://codereview.chromium.org/2672313006/diff/1/media/base/android/media_co...
media/base/android/media_codec_bridge.h:147: // This constructor is only for
making this class mockable.
On 2017/02/07 20:37:30, liberato wrote:
> On 2017/02/07 20:08:01, DaleCurtis wrote:
> > On 2017/02/07 at 06:19:43, liberato wrote:
> > > personally, i think that this should still be an interface.  it's okay if
> the
> > interface now includes java-specific types things that cause it not to
compile
> > as android_anywhere.
> > > 
> > > however, i don't think that it adds any clarity merging the sdk impl and
the
> > interface.  having an SdkMediaCodecBridge isn't by itself bad.
> > > 
> > > i believe that i misunderstood our offline discussion, and i apologize.  i
> > thought that removing ndk caused it to became convenient to move methods
with
> > non-portable signatures (e.g. setOutputSurface) into MCB, breaking
> > android_anywhere.
> > 
> > Chrome is taking an active stance to reduce layers these days (see onion
> soup),
> > interfaces that only serve one implementation are unnecessary overhead and
> > should be removed.
> > 
> > A better argument would be that keeping the interface allows us to mock out
> > badly behaving MediaCodec implementations to simulate their badness and thus
> > write better tests for problematic devices.
> 
> won't we need virtual functions anyway to mock this thing (badly behaving
codecs
> or an (elusive!) correctly behaving codec)?  mocking is my primary concern,
> though i didn't write that anywhere in the comment.  :)

I was intending to make it mockable either way (that's one, far removed,
motivation for this CL). Talked and researched offline and decided that a
separate pure virtual interface is preferable for mocking (as opposed to having
the mock inherit from the impl) so I've done that now.

The interface is called MediaCodecBridge and the impl is called
MediaCodecBridgeImpl. I don't want to call it SdkMCB because that's no longer an
interesting distinction and Impl is conventional for a single impl.

https://codereview.chromium.org/2672313006/diff/1/media/base/android/media_co...
media/base/android/media_codec_bridge.h:181: class MEDIA_EXPORT AudioCodecBridge
: public MediaCodecBridge {
On 2017/02/07 06:19:43, liberato wrote:
> this and VideoCodecBridge should be in their own headers / impls.
> 
> they are wrong in ToT, too, in my opinion.  but if you're moving them, then
> might as well split them up.

Will do in a follow up.

https://codereview.chromium.org/2672313006/diff/1/media/base/android/media_co...
media/base/android/media_codec_bridge.h:216: class MEDIA_EXPORT VideoCodecBridge
: public MediaCodecBridge {
On 2017/02/07 06:19:43, liberato wrote:
> i keep going back and forth on these.  on one hand, it's kind of nice to have
a
> VideoCodecBridge instead of a MediaCodecBridge that has some handy helper
> functions.
> 
> however, it's also handy if they aren't concrete implementations.  how to get
> both of those together is a good question.  :)
> 
> the constructors can eventually be moved into factories, as can the other
> statics.  i.e., one could get a VideoCodecBridgeFactory that can answer
(without
> statics) IsKnownUnaccelerated.  probably not part of this cl, but as part of
the
> larger picture of enabling better testing.
> 
> one might also argue that, since the underlying MediaCodec object doesn't have
> video and audio variants, then neither should we.  it seems kinda arbitrary
that
> we have VideoCodecBridge but not VideoDecoderBridge / VideoEncoderBridge.  one
> could still keep the factories separate to control the arguments to
> construction, which is really where most of the differences are.  the other
> differences are just extra functions that are just as easy not to call if one
> doesn't need them.
> 
> i'm sure that it's all out of scope for this CL, but i'm curious about your
> thoughts on it.

Yeah I don't like these. I have long felt that they don't serve a purpose and
are only awkward. Especially because they each require their own mock. I'll plan
to refactor in a follow up.

https://codereview.chromium.org/2672313006/diff/1/media/base/android/media_co...
media/base/android/media_codec_bridge.h:223: static VideoCodecBridge*
CreateDecoder(
On 2017/02/07 06:19:43, liberato wrote:
> if you're aiming to make these testable, then these should be factories.  once
> you remove the sdk / ndk "pick an include" method of inheritance, we don't
need
> these static creation functions either.
> 
> maybe it's outside the scope of this CL, but it's worth considering if you
> haven't already.

Sounds good. For my immediate testing needs, I think it's enough to think of
AVDACodecAllocator as the factory, but I'll rethink this when I remove the
video/audio bridges.

https://codereview.chromium.org/2672313006/diff/20001/media/base/android/mock...
File media/base/android/mock_media_codec_bridge.h (right):

https://codereview.chromium.org/2672313006/diff/20001/media/base/android/mock...
media/base/android/mock_media_codec_bridge.h:22: MOCK_METHOD1(GetOutputSize,
MediaCodecStatus(gfx::Size* size));
This file changed because I ran gmock_gen.py which adds the names.

[watk, 2017-02-08 20:17:04]

The CQ bit was checked by watk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 20:17:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-08 20:45:52]

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

[commit-bot: I haz the power, 2017-02-08 20:45:53]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[watk, 2017-02-08 20:56:06]

The CQ bit was checked by watk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 20:57:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-08 22:38:13]

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

[commit-bot: I haz the power, 2017-02-08 22:38:14]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[watk, 2017-02-08 23:01:24]

The CQ bit was checked by watk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 23:02:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 01:16:03]

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

[commit-bot: I haz the power, 2017-02-09 01:16:04]

Dry run: This issue passed the CQ dry run.

[watk, 2017-02-14 01:41:42]

ping. I addressed most of the requests in a follow up CL:
https://codereview.chromium.org/2697643003/

[liberato (no reviews please), 2017-02-14 05:32:40]

sorry for the delay.

lgtm.

-fl

[watk, 2017-02-14 19:54:19]

The CQ bit was checked by watk@chromium.org

[commit-bot: I haz the power, 2017-02-14 19:55:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 19:58:30]

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

[commit-bot: I haz the power, 2017-02-14 19:58:32]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[watk, 2017-02-14 20:04:53]

The CQ bit was checked by watk@chromium.org

[watk, 2017-02-14 20:04:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
liberato@chromium.org
Link to the patchset: https://codereview.chromium.org/2672313006/#ps80001
(title: "rebase past conflict")

[commit-bot: I haz the power, 2017-02-14 20:05:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 20:42:24]

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

[commit-bot: I haz the power, 2017-02-14 20:42:25]

Try jobs failed on following builders:
  win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)

[watk, 2017-02-14 23:34:21]

The CQ bit was checked by watk@chromium.org

[commit-bot: I haz the power, 2017-02-14 23:35:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 00:41:24]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487115261768080,
"parent_rev": "abcc1f5eb8bb22bd603342f951dbd076c8491d3b", "commit_rev":
"ddd685822dcdc6ec5e1fd32bb6ff3824d93c675c"}

[commit-bot: I haz the power, 2017-02-15 00:42:03]

Description was changed from

==========
media: Remove the unused NdkMediaCodecBridge

NdkMediaCodecBridge has been incomplete and unused since it was
introduced over a year ago. Since we don't have short term plans to
make it production ready, we'll remove it for now and bring it back if
we need it later.
Some rationale:
 * The NDK API is a second class citizen compared to the Java API, and
   is missing new APIs. For example, it's missing the callback API, and
   setOutputSurface(), which means we can't use it on Marshmallow.
 * It only works with 64 bit builds.
 * The benefit of removing JNI call overhead isn't expected to be very
   large anyway. JNI calls add 100s of nanoseconds to 10s of microseconds
   of overhead (according to internal mailing lists/docs). We have lower
   hanging fruit, like using MediaCodec's callback API instead of polling.

This change includes collapsing SdkMediaCodecBridge into
MediaCodecBridge. In doing that, MediaCodecLoop and its unit test no
longer build cross-platform, so this CL also removes the "anywhere"
targets. Alternatively we could have kept a platform independent
virtual MediaCodecBridge interface with a single impl.

BUG=560451
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
media: Remove the unused NdkMediaCodecBridge

NdkMediaCodecBridge has been incomplete and unused since it was
introduced over a year ago. Since we don't have short term plans to
make it production ready, we'll remove it for now and bring it back if
we need it later.
Some rationale:
 * The NDK API is a second class citizen compared to the Java API, and
   is missing new APIs. For example, it's missing the callback API, and
   setOutputSurface(), which means we can't use it on Marshmallow.
 * It only works with 64 bit builds.
 * The benefit of removing JNI call overhead isn't expected to be very
   large anyway. JNI calls add 100s of nanoseconds to 10s of microseconds
   of overhead (according to internal mailing lists/docs). We have lower
   hanging fruit, like using MediaCodec's callback API instead of polling.

This change includes collapsing SdkMediaCodecBridge into
MediaCodecBridge. In doing that, MediaCodecLoop and its unit test no
longer build cross-platform, so this CL also removes the "anywhere"
targets. Alternatively we could have kept a platform independent
virtual MediaCodecBridge interface with a single impl.

BUG=560451
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2672313006
Cr-Commit-Position: refs/heads/master@{#450529}
Committed:
https://chromium.googlesource.com/chromium/src/+/ddd685822dcdc6ec5e1fd32bb6ff...
==========

[commit-bot: I haz the power, 2017-02-15 00:42:04]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/ddd685822dcdc6ec5e1fd32bb6ff...