Use the MediaCodec flush workaround in more cases

Use the MediaCodec flush workaround in more cases

Courtesy of exoplayer, now we use the flush workaround in more cases
than before.

BUG=606110
Committed: https://crrev.com/5f69cdd991be34f090aa3888ccfb60c5a674e0da
Cr-Commit-Position: refs/heads/master@{#406942}

[watk, 2016-04-23 02:34:07]

Patchset #1 (id:1) has been deleted

[watk, 2016-04-23 02:36:50]

Patchset #1 (id:20001) has been deleted

[watk, 2016-04-23 02:37:13]

Patchset #1 (id:40001) has been deleted

[watk, 2016-04-23 02:40:20]

Patchset #1 (id:60001) has been deleted

[watk, 2016-04-23 02:41:15]

Description was changed from

==========
Use the MediaCodec flush workaround in more cases

Courtesy of exoplayer, now we use the flush workaround in more cases
than before.

BUG=606110
==========

to

==========
Use the MediaCodec flush workaround in more cases

Courtesy of exoplayer, now we use the flush workaround in more cases
than before.

BUG=606110
==========

[watk, 2016-04-23 02:41:15]

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

[DaleCurtis, 2016-04-23 19:06:21]

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

https://codereview.chromium.org/1918623002/diff/80001/media/base/android/medi...
media/base/android/media_codec_util.cc:93: std::string
MediaCodecUtil::GetDefaultCodecName(const std::string& mime_type,
Is this even necessary if our blacklist really only affects api >= 18?

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

https://codereview.chromium.org/1918623002/diff/80001/media/base/android/medi...
media/base/android/media_codec_util.h:45: // Returns the component name of the
decoder that will be created for the
I don't like exposing the codec name getter, since it requires outside knowledge
of the android mime types which are weird and not inline with chromium types.
min has a nice CL to switch these to an enum value that I think we should
surface instead if we go this route.

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

https://codereview.chromium.org/1918623002/diff/80001/media/base/android/ndk_...
media/base/android/ndk_media_codec_bridge.cc:242: // The NDK doesn't expose a
name getter, so use the default name as a proxy.
I'm wary of this, there is already some worry that the codec created doesn't
always have the right name. Is there no other way to get a name type token?
Could we intead modify creation to get a name or something?

[watk, 2016-04-25 21:32:49]

+qinmin.

One way to refactor this is to always create MediaCodecs by name. On Lollipop
that's the preferred way to do it (see the note here
http://developer.android.com/intl/vi/reference/android/media/MediaCodec.html#...
)

So for creating a MediaCodec we'd:
* search for a component with the above suggestion on lollipop, or the same
method as GetDefaultDecoder on other platforms,
* create a decoder createDecoderByName, and save the name,
* now we have a name getter for the NDK and all API levels.

Shall I go that route?

[watk, 2016-04-25 21:33:07]

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

[qinmin, 2016-04-25 22:44:58]

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

https://codereview.chromium.org/1918623002/diff/80001/media/base/android/medi...
media/base/android/media_codec_util.cc:243: "SM-G800",
base::CompareCase::INSENSITIVE_ASCII) &&
This feels strange, so the same decoder works on other devices with the same
android version, but just not on SM-G800?

[watk, 2016-04-25 22:53:53]

WDYT about always creating decoders by name qinmin@? Or would you prefer what we
have here?

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

https://codereview.chromium.org/1918623002/diff/80001/media/base/android/medi...
media/base/android/media_codec_util.cc:243: "SM-G800",
base::CompareCase::INSENSITIVE_ASCII) &&
On 2016/04/25 22:44:58, qinmin wrote:
> This feels strange, so the same decoder works on other devices with the same
> android version, but just not on SM-G800?

Yeah, perhaps it happens on other devices too, but I guess the only confirmed
device was this one. Exoplayer issue here:
https://github.com/google/ExoPlayer/issues/603

[qinmin, 2016-04-25 23:07:40]

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

https://codereview.chromium.org/1918623002/diff/80001/media/base/android/medi...
media/base/android/media_codec_util.cc:243: "SM-G800",
base::CompareCase::INSENSITIVE_ASCII) &&
On 2016/04/25 22:53:53, watk wrote:
> On 2016/04/25 22:44:58, qinmin wrote:
> > This feels strange, so the same decoder works on other devices with the same
> > android version, but just not on SM-G800?
> 
> Yeah, perhaps it happens on other devices too, but I guess the only confirmed
> device was this one. Exoplayer issue here:
> https://github.com/google/ExoPlayer/issues/603

Yes, we should blacklist for all other devices.

[watk, 2016-07-20 21:35:08]

PTAL. I forgot about this for a long time.. Since we don't even use the ndk
bridge, I'm punting implementing something more complicated in order to work
around it not having getName().

[watk, 2016-07-20 21:36:09]

watk@chromium.org changed required reviewers:
+ dalecurtis@chromium.org

[watk, 2016-07-20 21:36:09]

Also, qinmin and I chatted with exoplayer people offline and decided to leave
the device blacklist as is.

[DaleCurtis, 2016-07-20 22:30:27]

We need to benchmark using NDK vs SDK to see if it's actually a win and revert
the NDK/SDK split if it's not. It's adding a lot of overhead maintaining
something we're not even using.

[DaleCurtis, 2016-07-20 22:31:48]

lgtm

https://codereview.chromium.org/1918623002/diff/100001/media/base/android/jav...
File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
(right):

https://codereview.chromium.org/1918623002/diff/100001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:314:
private String getName() {
Only available on API level >= 18 right?

[qinmin, 2016-07-21 17:01:58]

lgtm

[watk, 2016-07-21 18:27:29]

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

[commit-bot: I haz the power, 2016-07-21 18:28:26]

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

[watk, 2016-07-21 18:59:38]

thanks

https://codereview.chromium.org/1918623002/diff/100001/media/base/android/jav...
File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
(right):

https://codereview.chromium.org/1918623002/diff/100001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:314:
private String getName() {
On 2016/07/20 22:31:47, DaleCurtis wrote:
> Only available on API level >= 18 right?

Yeah, added TargetApi annotation.

[watk, 2016-07-21 18:59:46]

The CQ bit was unchecked by watk@chromium.org

[watk, 2016-07-21 18:59:49]

The CQ bit was checked by watk@chromium.org

[watk, 2016-07-21 18:59:49]

The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org,
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1918623002/#ps120001
(title: "Add TargetApi annotation")

[commit-bot: I haz the power, 2016-07-21 19:00: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, 2016-07-21 21:16:15]

Description was changed from

==========
Use the MediaCodec flush workaround in more cases

Courtesy of exoplayer, now we use the flush workaround in more cases
than before.

BUG=606110
==========

to

==========
Use the MediaCodec flush workaround in more cases

Courtesy of exoplayer, now we use the flush workaround in more cases
than before.

BUG=606110
==========

[commit-bot: I haz the power, 2016-07-21 21:16:17]

Committed patchset #3 (id:120001)

[commit-bot: I haz the power, 2016-07-21 21:17:30]

Description was changed from

==========
Use the MediaCodec flush workaround in more cases

Courtesy of exoplayer, now we use the flush workaround in more cases
than before.

BUG=606110
==========

to

==========
Use the MediaCodec flush workaround in more cases

Courtesy of exoplayer, now we use the flush workaround in more cases
than before.

BUG=606110

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

[commit-bot: I haz the power, 2016-07-21 21:17:31]

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