Fix/Disable ffmpeg Media Tests on Android

Fix/Disable ffmpeg Media Tests on Android

VP8 decoders are not built into the ffmpeg decoders
by default on Android, even if building in ffmpeg is
enabled.

This change switches over several test files / configs
for Android to use supported decoders (h264) instead.

Fourteen tests will be fixed by this change.
Four tests have been disabled until proper test files
have been generated. (tracked via associated crbug)

BUG=720008

[kraush, 2017-05-09 18:15:34]

kraush@amazon.com changed reviewers:
+ ddorwin@chromium.org

[kraush, 2017-05-09 18:15:34]

Hi David,

Can you take a look at this change to fix media_unittests on Android in case
FFMPEG gets built in?

Thanks!
Holger

[ddorwin, 2017-05-09 18:44:26]

There are two issues:

1. Proprietary codecs are no longer supported by default in Chromium builds on
Android. Thus, these tests will not pass the bots.

2. We should not disable tests for the default configuration. There is a build
flag for this and maybe a precompiler define. If not, one could be defined
locally in the relevant GN file.

See also the bug comments.

[kraush, 2017-05-09 18:52:44]

On 2017/05/09 18:44:26, ddorwin wrote:
> There are two issues:
> 
> 1. Proprietary codecs are no longer supported by default in Chromium builds on
> Android. Thus, these tests will not pass the bots.
> 
> 2. We should not disable tests for the default configuration. There is a build
> flag for this and maybe a precompiler define. If not, one could be defined
> locally in the relevant GN file.
> 
> See also the bug comments.

Not sure about the first comment: To clarify, this is about builds using this GN
flag on Android:
disable_ffmpeg_video_decoders=false
Which will trigger ffmpeg_video_decoder_unittests.cc to be compiled in (see
respective GN file)

In regards to 2, just to clarify:
Do you want me to change the ifdefs to OS_ANDROID &&
!DISABLE_FFMPEG_VIDEO_DECODERS?
(If so, I'd be happy to do so - just wanted to double check. Functionally it
would be identical since the file does not get compiled in with
disable_ffmpeg_video_decoders set to true)


Your comments in 1. are very interesting though!
From my understanding, this was the set of flags used to build a Chrome-like
Chromium (in regards to codecs):
proprietary_codecs=true
ffmpeg_branding="Chrome"

Can you share on what the actual (new) correct flags would be?

Thanks! :)
Holger

[ddorwin, 2017-05-09 19:15:16]

On 2017/05/09 18:52:44, kraush wrote:
> Not sure about the first comment: To clarify, this is about builds using this
GN
> flag on Android:
> disable_ffmpeg_video_decoders=false
> Which will trigger ffmpeg_video_decoder_unittests.cc to be compiled in (see
> respective GN file)

Yes, the second file will not be compiled or run on the bots. However, the first
file is a helper that is (or available to be) used by other tests that do run.
See
https://cs.chromium.org/search/?q=TestVideoConfig&sq=package:chromium&type=cs. I
would expect those tests to fail for Chromium builds that include this change.

> In regards to 2, just to clarify:
> Do you want me to change the ifdefs to OS_ANDROID &&
> !DISABLE_FFMPEG_VIDEO_DECODERS?
> (If so, I'd be happy to do so - just wanted to double check. Functionally it
> would be identical since the file does not get compiled in with
> disable_ffmpeg_video_decoders set to true)

You are correct, such a check would redundant and unnecessary. However, this
test will if proprietary_codecs=false is used with
disable_ffmpeg_video_decoders=false, and we should probably consider that in
some way as it is just as valid as the case you are trying to fix.


> Your comments in 1. are very interesting though!
> From my understanding, this was the set of flags used to build a Chrome-like
> Chromium (in regards to codecs):
> proprietary_codecs=true
> ffmpeg_branding="Chrome"

Yes, but the public bots generally use the values for Chromium, which are
`false` and `"Chromium"`, respectively.

[kraush, 2017-05-09 19:22:48]

On 2017/05/09 19:15:16, ddorwin wrote:
> On 2017/05/09 18:52:44, kraush wrote:
> > Not sure about the first comment: To clarify, this is about builds using
this
> GN
> > flag on Android:
> > disable_ffmpeg_video_decoders=false
> > Which will trigger ffmpeg_video_decoder_unittests.cc to be compiled in (see
> > respective GN file)
> 
> Yes, the second file will not be compiled or run on the bots. However, the
first
> file is a helper that is (or available to be) used by other tests that do run.
> See
> https://cs.chromium.org/search/?q=TestVideoConfig&sq=package:chromium&type=cs.
I
> would expect those tests to fail for Chromium builds that include this change.
> 
> > In regards to 2, just to clarify:
> > Do you want me to change the ifdefs to OS_ANDROID &&
> > !DISABLE_FFMPEG_VIDEO_DECODERS?
> > (If so, I'd be happy to do so - just wanted to double check. Functionally it
> > would be identical since the file does not get compiled in with
> > disable_ffmpeg_video_decoders set to true)
> 
> You are correct, such a check would redundant and unnecessary. However, this
> test will if proprietary_codecs=false is used with
> disable_ffmpeg_video_decoders=false, and we should probably consider that in
> some way as it is just as valid as the case you are trying to fix.
> 
> 
> > Your comments in 1. are very interesting though!
> > From my understanding, this was the set of flags used to build a Chrome-like
> > Chromium (in regards to codecs):
> > proprietary_codecs=true
> > ffmpeg_branding="Chrome"
> 
> Yes, but the public bots generally use the values for Chromium, which are
> `false` and `"Chromium"`, respectively.

Makes sense, thanks for explaining!
I had looked through the other files using this helper, and it's currently only
being used by some fake_decoder, but that could of course change in the future,
so I can totally see how harder checks would make sense.

Would it be an acceptable solution then if I change all of my
#if defined(OS_ANDROID) to
#if defined(OS_ANDROID) && !defined(DISABLE_FFMPEG_VIDEO_DECODERS) &&
BUILDFLAG(USE_PROPRIETARY_CODECS)
I know it's long, but I think it should be functionally correct and cover all
the scenarios! :)


I realize that this test file will not be compiled in on your bots (as you
mentioned above) - this change is mainly meant for embedders who use the media
directory and use ffmpeg decoders on Android.

[watk, 2017-05-09 22:08:23]

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

[watk, 2017-05-09 22:08:23]

https://codereview.chromium.org/2874433002/diff/1/media/base/test_helpers.cc
File media/base/test_helpers.cc (right):

https://codereview.chromium.org/2874433002/diff/1/media/base/test_helpers.cc#...
media/base/test_helpers.cc:158: VideoDecoderConfig TestVideoConfig::Normal() {
Instead of this kDefaultCodec, I think it would be better if these had a codec
parameter which is defaulted to vp8.

https://codereview.chromium.org/2874433002/diff/1/media/filters/ffmpeg_video_...
File media/filters/ffmpeg_video_decoder_unittest.cc (right):

https://codereview.chromium.org/2874433002/diff/1/media/filters/ffmpeg_video_...
media/filters/ffmpeg_video_decoder_unittest.cc:77: #if defined(OS_ANDROID)
I would prefer if you defined something like USE_VP8 or USE_H264 at the top of
this file. Based on OS_ANDROID, PROPRIETARY_CODECS,
DISABLE_FFMPEG_VIDEO_DECODERS. And put an #error pragma for the case where
neither are available.

[kraush, 2017-05-10 15:17:57]

On 2017/05/09 22:08:23, watk wrote:
> https://codereview.chromium.org/2874433002/diff/1/media/base/test_helpers.cc
> File media/base/test_helpers.cc (right):
> 
>
https://codereview.chromium.org/2874433002/diff/1/media/base/test_helpers.cc#...
> media/base/test_helpers.cc:158: VideoDecoderConfig TestVideoConfig::Normal() {
> Instead of this kDefaultCodec, I think it would be better if these had a codec
> parameter which is defaulted to vp8.
> 
>
https://codereview.chromium.org/2874433002/diff/1/media/filters/ffmpeg_video_...
> File media/filters/ffmpeg_video_decoder_unittest.cc (right):
> 
>
https://codereview.chromium.org/2874433002/diff/1/media/filters/ffmpeg_video_...
> media/filters/ffmpeg_video_decoder_unittest.cc:77: #if defined(OS_ANDROID)
> I would prefer if you defined something like USE_VP8 or USE_H264 at the top of
> this file. Based on OS_ANDROID, PROPRIETARY_CODECS,
> DISABLE_FFMPEG_VIDEO_DECODERS. And put an #error pragma for the case where
> neither are available.

Makes sense, thanks for the suggestion!
I'll upload a revision with a default decoder which can be changed based on
#USE_VP8 / #USE_H264

[kraush, 2017-05-10 15:20:34]

Hi David, Hi watk@,

I've implemented the changes in the way suggested by watk@.
Let me know if this is not the way you imagined (e.g. I wasn't sure if you want
the #error in every place or just once on top)

Let me know what you think!

Thanks,
Holger

[watk, 2017-05-10 18:23:28]

After thinking about this some more, I don't think we want to have this change
upstream because it's for a non-standard ffmpeg configuration. We expect that if
FFmpeg video decoders are enabled that we'll have the codecs common to every
platform, like VP8, theora.

Sorry for asking you for changes before considering whether the change made
sense first :)

[kraush, 2017-05-10 18:29:30]

On 2017/05/10 18:23:28, watk wrote:
> After thinking about this some more, I don't think we want to have this change
> upstream because it's for a non-standard ffmpeg configuration. We expect that
if
> FFmpeg video decoders are enabled that we'll have the codecs common to every
> platform, like VP8, theora.
> 
> Sorry for asking you for changes before considering whether the change made
> sense first :)

Thanks, makes sense!

Can you briefly elaborate on that though?
From what I understand:
1. Chromium provides a GN flag to explicitly build ffmpeg into Android
2. Chromium's ffmpeg code defines VP8 and Theora as available on all platforms
except Android (this was not changed by us as an embedder)

Or do you mean that tests are not meant to support scenarios that are outside
the default gn flags (which I would totally get from a maintenance perspective -
just want to make sure I fully understand the reasoning)

[watk, 2017-05-10 19:29:48]

On 2017/05/10 18:29:30, kraush wrote:
> On 2017/05/10 18:23:28, watk wrote:
> > After thinking about this some more, I don't think we want to have this
change
> > upstream because it's for a non-standard ffmpeg configuration. We expect
that
> if
> > FFmpeg video decoders are enabled that we'll have the codecs common to every
> > platform, like VP8, theora.
> > 
> > Sorry for asking you for changes before considering whether the change made
> > sense first :)
> 
> Thanks, makes sense!
> 
> Can you briefly elaborate on that though?

Yep, np.

> From what I understand:
> 1. Chromium provides a GN flag to explicitly build ffmpeg into Android

Yes, we include ffmpeg for demuxing on Android. And we allow overriding
disable_ffmpeg_video_decoders to include video decoders as well, presumably
because it has little to no impact on the rest of the code to allow this
configuration: it's supported by the rest of the platforms. But there's no way
to create a build for android with ffmpeg video decoders today.

> 2. Chromium's ffmpeg code defines VP8 and Theora as available on all platforms
> except Android (this was not changed by us as an embedder)

That's the case because none of the ffmpeg video decoders are available on
Android.

> 
> Or do you mean that tests are not meant to support scenarios that are outside
> the default gn flags (which I would totally get from a maintenance perspective
-
> just want to make sure I fully understand the reasoning)

I wouldn't say that exactly. What you have seems reasonable if it were a
supported configuration. But in this case we're adding code to test a
configuration (android, ffmpeg with h264 but not vp8) which can't be built. It
requires us to add an assumption that if you have ffmpeg video decoders on
Android you'll probably have H264 but not vp8. Which is specific to your project
really.

Does that make sense?

[kraush, 2017-05-10 19:35:41]

On 2017/05/10 19:29:48, watk wrote:
> I wouldn't say that exactly. What you have seems reasonable if it were a
> supported configuration. But in this case we're adding code to test a
> configuration (android, ffmpeg with h264 but not vp8) which can't be built. It
> requires us to add an assumption that if you have ffmpeg video decoders on
> Android you'll probably have H264 but not vp8. Which is specific to your
project
> really.
> 
> Does that make sense?

Gotcha, thanks for explaining! :)
So just to make sure I understood it correctly:
While ffmpeg can be built into Chromium for Android and it works (embedders like
Amazon are doing it), the fact that h264 would be available but vp8 would not is
an embedder specific assumption (which is true, since embedders can alter the
ffmpeg config to bake in any decoders they want!)
--> Since there are no guarantees about which decoders people would bake in, we
shouldn't adjust the tests to expect any specific video decoder on Android.

Did I get that right?

[watk, 2017-05-10 19:56:04]

On 2017/05/10 19:35:41, kraush wrote:
> On 2017/05/10 19:29:48, watk wrote:
> > I wouldn't say that exactly. What you have seems reasonable if it were a
> > supported configuration. But in this case we're adding code to test a
> > configuration (android, ffmpeg with h264 but not vp8) which can't be built.
It
> > requires us to add an assumption that if you have ffmpeg video decoders on
> > Android you'll probably have H264 but not vp8. Which is specific to your
> project
> > really.
> > 
> > Does that make sense?
> 
> Gotcha, thanks for explaining! :)
> So just to make sure I understood it correctly:
> While ffmpeg can be built into Chromium for Android and it works (embedders
like
> Amazon are doing it), the fact that h264 would be available but vp8 would not
is
> an embedder specific assumption (which is true, since embedders can alter the
> ffmpeg config to bake in any decoders they want!)
> --> Since there are no guarantees about which decoders people would bake in,
we
> shouldn't adjust the tests to expect any specific video decoder on Android.
> 
> Did I get that right?

That's it, yep. I don't know if there are official recommendations for what
should be upstreamed, but my general recommendation is that you upstream changes
that either make sense for chromium in isolation, or that are very low impact
and orthogonal. e.g., we allow disable_ffmpeg_video_decoders to be overridden on
Android even though we will never override it because there is essentially no
impact on maintenance or clarity; it just switches the build to another
supported configuration.

[kraush, 2017-05-10 19:58:16]

On 2017/05/10 19:56:04, watk wrote:
> 
> That's it, yep. I don't know if there are official recommendations for what
> should be upstreamed, but my general recommendation is that you upstream
changes
> that either make sense for chromium in isolation, or that are very low impact
> and orthogonal. e.g., we allow disable_ffmpeg_video_decoders to be overridden
on
> Android even though we will never override it because there is essentially no
> impact on maintenance or clarity; it just switches the build to another
> supported configuration.

Gotcha, thanks for the support and clarifications! :)
I'll go ahead and discard this CL.

[kraush, 2017-05-10 19:58:30]

Description was changed from

==========
Fix/Disable ffmpeg Media Tests on Android

VP8 decoders are not built into the ffmpeg decoders
by default on Android, even if building in ffmpeg is
enabled.

This change switches over several test files / configs
for Android to use supported decoders (h264) instead.

Fourteen tests will be fixed by this change.
Four tests have been disabled until proper test files
have been generated. (tracked via associated crbug)

BUG=720008
==========

to

==========
Fix/Disable ffmpeg Media Tests on Android

VP8 decoders are not built into the ffmpeg decoders
by default on Android, even if building in ffmpeg is
enabled.

This change switches over several test files / configs
for Android to use supported decoders (h264) instead.

Fourteen tests will be fixed by this change.
Four tests have been disabled until proper test files
have been generated. (tracked via associated crbug)

BUG=720008
==========