Remove Exynos from MediaCodec blacklist. Limit MediaTek blacklist.

Remove Exynos from MediaCodec blacklist. Limit MediaTek blacklist.

MediaCodec has a newer path for limiting the VP8 blacklist more
precisely that should be used instead of a blanket Exynos blacklist.

MediaTek nowadays also has a vp9 decoder, so restrict blacklist to
only vp8 MediaTek devices. Per glaznev@ the MediaTek vp8 decoder is
slower than software, so it kind of makes sense to include it on
the "known unaccelerated" list.

BUG=446974, 594814
TEST=hardware vp9 on galaxy s6 works with <video>.

Committed: https://crrev.com/fb7409de2920256bca4a678a030198d6a99f2709
Cr-Commit-Position: refs/heads/master@{#381322}

[DaleCurtis, 2016-03-15 00:17:43]

dalecurtis@chromium.org changed reviewers:
+ glaznev@chromium.org, timav@chromium.org

[DaleCurtis, 2016-03-15 01:46:00]

Description was changed from

==========
Remove Exynos from MediaCodec blacklist. Limit MediaTek blacklist.

MediaCodec has a newer path for limited the VP8 blacklist more
precisely that should be used instead of a blanket Exynos blacklist.

MediaTek nowadays also has a vp9 decoder, so restrict blacklist to
only vp8 MediaTek devices. Per glaznev@ the MediaTek vp8 decoder is
slower than software, so it kind of makes sense to include it on
the "known unaccelerated" list.

BUG=446974, 594814
TEST=hardware vp9 on galaxy s6 works with <video>.
==========

to

==========
Remove Exynos from MediaCodec blacklist. Limit MediaTek blacklist.

MediaCodec has a newer path for limiting the VP8 blacklist more
precisely that should be used instead of a blanket Exynos blacklist.

MediaTek nowadays also has a vp9 decoder, so restrict blacklist to
only vp8 MediaTek devices. Per glaznev@ the MediaTek vp8 decoder is
slower than software, so it kind of makes sense to include it on
the "known unaccelerated" list.

BUG=446974, 594814
TEST=hardware vp9 on galaxy s6 works with <video>.
==========

[Tima Vaisburd, 2016-03-15 03:26:42]

https://codereview.chromium.org/1796393002/diff/1/media/base/android/media_co...
File media/base/android/media_codec_util.cc (right):

https://codereview.chromium.org/1796393002/diff/1/media/base/android/media_co...
media/base/android/media_codec_util.cc:52: // MediaCodec::getName() is only
supported on newer Android versions.
It seems the whole body of this function belongs in Java side.

https://codereview.chromium.org/1796393002/diff/1/media/base/android/media_co...
media/base/android/media_codec_util.cc:168: mime_type == "video/x-vnd.on2.vp8");
Limiting the blacklisting to specific codec is great.
If I understand it correctly, this is the only change that alters the
functionality and is also the one that corresponds to the CL description. If
this is the case, indeed, can you keep here just this change and have the
refactoring part as a separate CL?

https://codereview.chromium.org/1796393002/diff/1/media/base/android/media_co...
media/base/android/media_codec_util.cc:198: bool
MediaCodecUtil::IsVp8CodecAvailable() {
How is this renaming justified?
We blacklist some decoders because they cannot work with a surface texture as
advertised by API, I'd expect that encoders won't have this problem / will have
different set of problems?

Do you think it would be reasonable to have separate blacklists for encoders and
decoders, at least at the interface level?

https://codereview.chromium.org/1796393002/diff/1/media/base/android/media_co...
media/base/android/media_codec_util.cc:198: bool
MediaCodecUtil::IsVp8CodecAvailable() {
A side remark unrelated to this CL: I'd expect something like
IsCodecAvailable(codec) or IsAudio/VideoCodecAvailable(codec). What is so
special about Vp8?

[DaleCurtis, 2016-03-15 17:30:37]

Just comments, will post when CL is updated.

https://codereview.chromium.org/1796393002/diff/1/media/base/android/media_co...
File media/base/android/media_codec_util.cc (right):

https://codereview.chromium.org/1796393002/diff/1/media/base/android/media_co...
media/base/android/media_codec_util.cc:52: // MediaCodec::getName() is only
supported on newer Android versions.
On 2016/03/15 at 03:26:42, Tima Vaisburd wrote:
> It seems the whole body of this function belongs in Java side.

You're right. I realize now my cleanup didn't go far enough :) I'll revert these
changes and leave only the functional change. I'll then send you a followup CL
to move this all into Java.

https://codereview.chromium.org/1796393002/diff/1/media/base/android/media_co...
media/base/android/media_codec_util.cc:168: mime_type == "video/x-vnd.on2.vp8");
On 2016/03/15 at 03:26:42, Tima Vaisburd wrote:
> Limiting the blacklisting to specific codec is great.
> If I understand it correctly, this is the only change that alters the
functionality and is also the one that corresponds to the CL description. If
this is the case, indeed, can you keep here just this change and have the
refactoring part as a separate CL?

Kind of. This CL also needs the changes in the AndroidVideo_Encode_Accelerator
to preserve the existing VEA blacklist. I'm happy to pull just those changes out
(since they will likely need merging too).

https://codereview.chromium.org/1796393002/diff/1/media/base/android/media_co...
media/base/android/media_codec_util.cc:198: bool
MediaCodecUtil::IsVp8CodecAvailable() {
On 2016/03/15 at 03:26:42, Tima Vaisburd wrote:
> How is this renaming justified?
> We blacklist some decoders because they cannot work with a surface texture as
advertised by API, I'd expect that encoders won't have this problem / will have
different set of problems?
> 
> Do you think it would be reasonable to have separate blacklists for encoders
and decoders, at least at the interface level?

This allows the code in AndroidVideo_Encode_Accelerator to continue to blacklist
VP8 on the devices it was previously blacklisted on. I didn't want to change the
limits of VEA without someone from WebRTC to verify.

When we have another codec which needs blacklisting I think we should change the
method, but prior to that it's generally preferred in Chromium to write the code
as simple as possible. E.g. if that maxim had been applied to
GetDefaultCodecName() we wouldn't have 3 different general purpose functions :)

[Tima Vaisburd, 2016-03-15 18:48:47]

On 2016/03/15 17:30:37, DaleCurtis wrote:
> Just comments, will post when CL is updated.
> 
>
https://codereview.chromium.org/1796393002/diff/1/media/base/android/media_co...
> File media/base/android/media_codec_util.cc (right):
> 
>
https://codereview.chromium.org/1796393002/diff/1/media/base/android/media_co...
> media/base/android/media_codec_util.cc:52: // MediaCodec::getName() is only
> supported on newer Android versions.
> On 2016/03/15 at 03:26:42, Tima Vaisburd wrote:
> > It seems the whole body of this function belongs in Java side.
> 
> You're right. I realize now my cleanup didn't go far enough :) I'll revert
these
> changes and leave only the functional change. I'll then send you a followup CL
> to move this all into Java.
> 
>
https://codereview.chromium.org/1796393002/diff/1/media/base/android/media_co...
> media/base/android/media_codec_util.cc:168: mime_type ==
"video/x-vnd.on2.vp8");
> On 2016/03/15 at 03:26:42, Tima Vaisburd wrote:
> > Limiting the blacklisting to specific codec is great.
> > If I understand it correctly, this is the only change that alters the
> functionality and is also the one that corresponds to the CL description. If
> this is the case, indeed, can you keep here just this change and have the
> refactoring part as a separate CL?
> 
> Kind of. This CL also needs the changes in the AndroidVideo_Encode_Accelerator
> to preserve the existing VEA blacklist. I'm happy to pull just those changes
out
> (since they will likely need merging too).
> 
>
https://codereview.chromium.org/1796393002/diff/1/media/base/android/media_co...
> media/base/android/media_codec_util.cc:198: bool
> MediaCodecUtil::IsVp8CodecAvailable() {
> On 2016/03/15 at 03:26:42, Tima Vaisburd wrote:
> > How is this renaming justified?
> > We blacklist some decoders because they cannot work with a surface texture
as
> advertised by API, I'd expect that encoders won't have this problem / will
have
> different set of problems?
> > 
> > Do you think it would be reasonable to have separate blacklists for encoders
> and decoders, at least at the interface level?
> 
> This allows the code in AndroidVideo_Encode_Accelerator to continue to
blacklist
> VP8 on the devices it was previously blacklisted on. I didn't want to change
the
> limits of VEA without someone from WebRTC to verify.
> 
> When we have another codec which needs blacklisting I think we should change
the
> method, but prior to that it's generally preferred in Chromium to write the
code
> as simple as possible. E.g. if that maxim had been applied to
> GetDefaultCodecName() we wouldn't have 3 different general purpose functions
:)

I see. One option would be to have
 bool MediaCodec::IsVp8EncoderAvailable() {
   return IsVp8DecoderAvailable(); // explanation why, is this temporary?
}
and use this in the encode accelerator. But I'm not sure whether it satisfies
the
criterion of simplicity and so it's up to you.

[DaleCurtis, 2016-03-15 18:50:29]

On 2016/03/15 at 18:48:47, timav wrote:
> On 2016/03/15 17:30:37, DaleCurtis wrote:
> > Just comments, will post when CL is updated.
> > 
> >
https://codereview.chromium.org/1796393002/diff/1/media/base/android/media_co...
> > File media/base/android/media_codec_util.cc (right):
> > 
> >
https://codereview.chromium.org/1796393002/diff/1/media/base/android/media_co...
> > media/base/android/media_codec_util.cc:52: // MediaCodec::getName() is only
> > supported on newer Android versions.
> > On 2016/03/15 at 03:26:42, Tima Vaisburd wrote:
> > > It seems the whole body of this function belongs in Java side.
> > 
> > You're right. I realize now my cleanup didn't go far enough :) I'll revert
these
> > changes and leave only the functional change. I'll then send you a followup
CL
> > to move this all into Java.
> > 
> >
https://codereview.chromium.org/1796393002/diff/1/media/base/android/media_co...
> > media/base/android/media_codec_util.cc:168: mime_type ==
"video/x-vnd.on2.vp8");
> > On 2016/03/15 at 03:26:42, Tima Vaisburd wrote:
> > > Limiting the blacklisting to specific codec is great.
> > > If I understand it correctly, this is the only change that alters the
> > functionality and is also the one that corresponds to the CL description. If
> > this is the case, indeed, can you keep here just this change and have the
> > refactoring part as a separate CL?
> > 
> > Kind of. This CL also needs the changes in the
AndroidVideo_Encode_Accelerator
> > to preserve the existing VEA blacklist. I'm happy to pull just those changes
out
> > (since they will likely need merging too).
> > 
> >
https://codereview.chromium.org/1796393002/diff/1/media/base/android/media_co...
> > media/base/android/media_codec_util.cc:198: bool
> > MediaCodecUtil::IsVp8CodecAvailable() {
> > On 2016/03/15 at 03:26:42, Tima Vaisburd wrote:
> > > How is this renaming justified?
> > > We blacklist some decoders because they cannot work with a surface texture
as
> > advertised by API, I'd expect that encoders won't have this problem / will
have
> > different set of problems?
> > > 
> > > Do you think it would be reasonable to have separate blacklists for
encoders
> > and decoders, at least at the interface level?
> > 
> > This allows the code in AndroidVideo_Encode_Accelerator to continue to
blacklist
> > VP8 on the devices it was previously blacklisted on. I didn't want to change
the
> > limits of VEA without someone from WebRTC to verify.
> > 
> > When we have another codec which needs blacklisting I think we should change
the
> > method, but prior to that it's generally preferred in Chromium to write the
code
> > as simple as possible. E.g. if that maxim had been applied to
> > GetDefaultCodecName() we wouldn't have 3 different general purpose functions
:)
> 
> I see. One option would be to have
>  bool MediaCodec::IsVp8EncoderAvailable() {
>    return IsVp8DecoderAvailable(); // explanation why, is this temporary?
> }
> and use this in the encode accelerator. But I'm not sure whether it satisfies
the
> criterion of simplicity and so it's up to you.

I think that's also fine and what I had originally, so I will revert to that.

[DaleCurtis, 2016-03-15 19:23:56]

PTAL. Simplified as discussed.

[Tima Vaisburd, 2016-03-15 19:57:20]

On 2016/03/15 19:23:56, DaleCurtis wrote:
> PTAL. Simplified as discussed.

lgtm

[DaleCurtis, 2016-03-15 20:07:26]

glaznev: Did you want to weigh in?

[AlexGlaznev, 2016-03-15 20:18:04]

lgtm

[DaleCurtis, 2016-03-15 20:18:59]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2016-03-15 20:19:28]

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

[commit-bot: I haz the power, 2016-03-15 21:32:18]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-15 21:34:11]

Description was changed from

==========
Remove Exynos from MediaCodec blacklist. Limit MediaTek blacklist.

MediaCodec has a newer path for limiting the VP8 blacklist more
precisely that should be used instead of a blanket Exynos blacklist.

MediaTek nowadays also has a vp9 decoder, so restrict blacklist to
only vp8 MediaTek devices. Per glaznev@ the MediaTek vp8 decoder is
slower than software, so it kind of makes sense to include it on
the "known unaccelerated" list.

BUG=446974, 594814
TEST=hardware vp9 on galaxy s6 works with <video>.
==========

to

==========
Remove Exynos from MediaCodec blacklist. Limit MediaTek blacklist.

MediaCodec has a newer path for limiting the VP8 blacklist more
precisely that should be used instead of a blanket Exynos blacklist.

MediaTek nowadays also has a vp9 decoder, so restrict blacklist to
only vp8 MediaTek devices. Per glaznev@ the MediaTek vp8 decoder is
slower than software, so it kind of makes sense to include it on
the "known unaccelerated" list.

BUG=446974, 594814
TEST=hardware vp9 on galaxy s6 works with <video>.

Committed: https://crrev.com/fb7409de2920256bca4a678a030198d6a99f2709
Cr-Commit-Position: refs/heads/master@{#381322}
==========

[commit-bot: I haz the power, 2016-03-15 21:34:12]

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