Initial support for the desktop media pipeline on Android.

Initial support for the desktop media pipeline on Android.

Add experimental support for the desktop src= media pipeline on Android
behind a compile-time variable. To compile with it
enabled, set the gyp variable media_use_ffmpeg=1.

Currently only H/W accelerated VP8 is supported because that is all that
the existing AndroidVideoDecodeAccelerator supports.

BUG=507834
Committed: https://crrev.com/45c3063debd193f953d5c4aa55902d185469f4bc
Cr-Commit-Position: refs/heads/master@{#339498}

[watk, 2015-07-10 00:57:58]

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

[watk, 2015-07-10 00:57:58]

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

[watk, 2015-07-10 01:02:04]

This is basically a minimal working patch. Full screen isn't working yet.

Note: this won't compile yet, until chcunningham@ lands the latest ffmpeg roll
with the Android ffmpeg changes (or you build it manually).

I was thinking you could review, liberato@, and dalecurtis@ could give owners
approval.

[watk, 2015-07-10 01:03:37]

I forgot to mention, I haven't done the GN changes yet. Was going to wait for
agreement on the gyp approach.

[liberato (no reviews please), 2015-07-10 17:15:09]

so far so good.  only two meaningful comments.

https://codereview.chromium.org/1230793009/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1230793009/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:1978: #if defined(OS_ANDROID) &&
defined(ENABLE_MEDIA_PIPELINE_ON_ANDROID)
I think that this can simplify slightly to:

#if defined(OS_ANDROID) && !defined(ENABLE)
return WMPA
#else

#if defined(OS_ANDROID)
if (!flag)  return WMPA
#endif

return WMPI
#endif

keeps the cases disjoint, and maybe a little easier to read.

https://codereview.chromium.org/1230793009/diff/1/media/ffmpeg/ffmpeg_common.cc
File media/ffmpeg/ffmpeg_common.cc (left):

https://codereview.chromium.org/1230793009/diff/1/media/ffmpeg/ffmpeg_common....
media/ffmpeg/ffmpeg_common.cc:411: // TODO(tomfinegan): libavcodec doesn't know
about VP9.
maybe keep the TODO.

https://codereview.chromium.org/1230793009/diff/1/media/ffmpeg/ffmpeg_common.cc
File media/ffmpeg/ffmpeg_common.cc (right):

https://codereview.chromium.org/1230793009/diff/1/media/ffmpeg/ffmpeg_common....
media/ffmpeg/ffmpeg_common.cc:412: if (format == VideoFrame::UNKNOWN)
setting any unknown format to YV12 seems like it could go pretty wrong.  could
mess up UMA stats if nothing else.

at least, it needs to be protected for the android case.  i'm a little surprised
that it's okay for VP9.  doesn't VP9 support alpha?

how much does this setting actually affect?  it looks like there are a fair
number of consumers.

https://codereview.chromium.org/1230793009/diff/1/media/ffmpeg/ffmpeg_common....
media/ffmpeg/ffmpeg_common.cc:414: if (coded_size == gfx::Size(0, 0))
also seems scary to do for any unknown size.

[xhwang, 2015-07-10 17:27:51]

Drive-by comments:

Great work!

I didn't review the whole CL. Just some comments as I saw them.

Also, you need to update the CL description. "build_ffmpegsumo" doesn't apply
now.

https://codereview.chromium.org/1230793009/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1230793009/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:1978: #if defined(OS_ANDROID) &&
defined(ENABLE_MEDIA_PIPELINE_ON_ANDROID)
On 2015/07/10 17:15:09, liberato wrote:
> I think that this can simplify slightly to:
> 
> #if defined(OS_ANDROID) && !defined(ENABLE)
> return WMPA
> #else
> 
> #if defined(OS_ANDROID)
> if (!flag)  return WMPA
> #endif
> 
> return WMPI
> #endif
> 
> keeps the cases disjoint, and maybe a little easier to read.

Hmm, it's sad to see "return WMPA" twice.

Actually, why do we have both a gyp variable and a command line flag? Can we
just use one, e.g. the gyp variable, for prototyping/experiment?

https://codereview.chromium.org/1230793009/diff/1/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/1230793009/diff/1/media/media.gyp#newcode718
media/media.gyp:718: 'DISABLE_FFMPEG_VIDEO_DECODERS',
Add a comment why disable FFMPEG video decoders.

[liberato (no reviews please), 2015-07-10 17:32:07]

https://codereview.chromium.org/1230793009/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1230793009/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:1978: #if defined(OS_ANDROID) &&
defined(ENABLE_MEDIA_PIPELINE_ON_ANDROID)
On 2015/07/10 17:27:50, xhwang wrote:
> On 2015/07/10 17:15:09, liberato wrote:
> > I think that this can simplify slightly to:
> > 
> > #if defined(OS_ANDROID) && !defined(ENABLE)
> > return WMPA
> > #else
> > 
> > #if defined(OS_ANDROID)
> > if (!flag)  return WMPA
> > #endif
> > 
> > return WMPI
> > #endif
> > 
> > keeps the cases disjoint, and maybe a little easier to read.
> 
> Hmm, it's sad to see "return WMPA" twice.
> 
> Actually, why do we have both a gyp variable and a command line flag? Can we
> just use one, e.g. the gyp variable, for prototyping/experiment?
> 

i agree about the sad.  i tried to restructure this a few times, but couldn't do
it without sometimes returning null if the flag was set without WMPI compiled. 
if we're okay with that...

[liberato (no reviews please), 2015-07-10 17:38:36]

On 2015/07/10 17:32:07, liberato wrote:
>
https://codereview.chromium.org/1230793009/diff/1/content/renderer/render_fra...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/1230793009/diff/1/content/renderer/render_fra...
> content/renderer/render_frame_impl.cc:1978: #if defined(OS_ANDROID) &&
> defined(ENABLE_MEDIA_PIPELINE_ON_ANDROID)
> On 2015/07/10 17:27:50, xhwang wrote:
> > On 2015/07/10 17:15:09, liberato wrote:
> > > I think that this can simplify slightly to:
> > > 
> > > #if defined(OS_ANDROID) && !defined(ENABLE)
> > > return WMPA
> > > #else
> > > 
> > > #if defined(OS_ANDROID)
> > > if (!flag)  return WMPA
> > > #endif
> > > 
> > > return WMPI
> > > #endif
> > > 
> > > keeps the cases disjoint, and maybe a little easier to read.
> > 
> > Hmm, it's sad to see "return WMPA" twice.
> > 
> > Actually, why do we have both a gyp variable and a command line flag? Can we
> > just use one, e.g. the gyp variable, for prototyping/experiment?
> > 
> 
> i agree about the sad.  i tried to restructure this a few times, but couldn't
do
> it without sometimes returning null if the flag was set without WMPI compiled.

> if we're okay with that...

the more i think about it, i agree with my fellow non-anonymous reviewer.  the
runtime flag probably isn't helpful at this stage.  it'll just make it harder to
test during development when we accidentally test WMPA :)

[watk, 2015-07-10 18:51:04]

On 2015/07/10 17:38:36, liberato wrote:
> On 2015/07/10 17:32:07, liberato wrote:
> >
>
https://codereview.chromium.org/1230793009/diff/1/content/renderer/render_fra...
> > File content/renderer/render_frame_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1230793009/diff/1/content/renderer/render_fra...
> > content/renderer/render_frame_impl.cc:1978: #if defined(OS_ANDROID) &&
> > defined(ENABLE_MEDIA_PIPELINE_ON_ANDROID)
> > On 2015/07/10 17:27:50, xhwang wrote:
> > > On 2015/07/10 17:15:09, liberato wrote:
> > > > I think that this can simplify slightly to:
> > > > 
> > > > #if defined(OS_ANDROID) && !defined(ENABLE)
> > > > return WMPA
> > > > #else
> > > > 
> > > > #if defined(OS_ANDROID)
> > > > if (!flag)  return WMPA
> > > > #endif
> > > > 
> > > > return WMPI
> > > > #endif
> > > > 
> > > > keeps the cases disjoint, and maybe a little easier to read.
> > > 
> > > Hmm, it's sad to see "return WMPA" twice.
> > > 
> > > Actually, why do we have both a gyp variable and a command line flag? Can
we
> > > just use one, e.g. the gyp variable, for prototyping/experiment?
> > > 
> > 
> > i agree about the sad.  i tried to restructure this a few times, but
couldn't
> do
> > it without sometimes returning null if the flag was set without WMPI
compiled.
> 
> > if we're okay with that...
> 
> the more i think about it, i agree with my fellow non-anonymous reviewer.  the
> runtime flag probably isn't helpful at this stage.  it'll just make it harder
to
> test during development when we accidentally test WMPA :)

Sounds like you guys haven't done enough Android A/B testing to appreciate the
magic of the flag, haha. But now that we're done A/B testing for a while, I'm
fine with getting rid of this. I'll keep the commit handy to apply locally if I
want it :)

[watk, 2015-07-10 18:51:26]

https://codereview.chromium.org/1230793009/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1230793009/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:1978: #if defined(OS_ANDROID) &&
defined(ENABLE_MEDIA_PIPELINE_ON_ANDROID)
On 2015/07/10 17:32:07, liberato wrote:
> On 2015/07/10 17:27:50, xhwang wrote:
> > On 2015/07/10 17:15:09, liberato wrote:
> > > I think that this can simplify slightly to:
> > > 
> > > #if defined(OS_ANDROID) && !defined(ENABLE)
> > > return WMPA
> > > #else
> > > 
> > > #if defined(OS_ANDROID)
> > > if (!flag)  return WMPA
> > > #endif
> > > 
> > > return WMPI
> > > #endif
> > > 
> > > keeps the cases disjoint, and maybe a little easier to read.
> > 
> > Hmm, it's sad to see "return WMPA" twice.
> > 
> > Actually, why do we have both a gyp variable and a command line flag? Can we
> > just use one, e.g. the gyp variable, for prototyping/experiment?
> > 
> 
> i agree about the sad.  i tried to restructure this a few times, but couldn't
do
> it without sometimes returning null if the flag was set without WMPI compiled.

> if we're okay with that...

Removed

https://codereview.chromium.org/1230793009/diff/1/media/ffmpeg/ffmpeg_common.cc
File media/ffmpeg/ffmpeg_common.cc (right):

https://codereview.chromium.org/1230793009/diff/1/media/ffmpeg/ffmpeg_common....
media/ffmpeg/ffmpeg_common.cc:412: if (format == VideoFrame::UNKNOWN)
On 2015/07/10 17:15:09, liberato wrote:
> setting any unknown format to YV12 seems like it could go pretty wrong.  could
> mess up UMA stats if nothing else.
> 
> at least, it needs to be protected for the android case.  i'm a little
surprised
> that it's okay for VP9.  doesn't VP9 support alpha?
> 
> how much does this setting actually affect?  it looks like there are a fair
> number of consumers.

AFAIK this is ok, but I'm not sure enough, so I'll make it Android only. I think
the right longer term fix is to allow VideoDecoderConfigs to contain unknowns,
but that's a larger change which requires auditing the consumers.

https://codereview.chromium.org/1230793009/diff/1/media/ffmpeg/ffmpeg_common....
media/ffmpeg/ffmpeg_common.cc:414: if (coded_size == gfx::Size(0, 0))
On 2015/07/10 17:15:09, liberato wrote:
> also seems scary to do for any unknown size.

Done.

https://codereview.chromium.org/1230793009/diff/1/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/1230793009/diff/1/media/media.gyp#newcode718
media/media.gyp:718: 'DISABLE_FFMPEG_VIDEO_DECODERS',
On 2015/07/10 17:27:50, xhwang wrote:
> Add a comment why disable FFMPEG video decoders.

Done.

[liberato (no reviews please), 2015-07-10 18:55:19]

lgtm

[watk, 2015-07-10 22:40:09]

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

[watk, 2015-07-10 22:53:31]

xhwang@: would you mind being the owner reviewer?

[watk, 2015-07-10 22:56:44]

On 2015/07/10 17:27:51, xhwang wrote:
> ...
> Also, you need to update the CL description. "build_ffmpegsumo" doesn't apply
> now.

It still applies, see
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg...

Unless I misunderstood?

[xhwang, 2015-07-10 23:11:05]

On 2015/07/10 22:56:44, watk wrote:
> On 2015/07/10 17:27:51, xhwang wrote:
> > ...
> > Also, you need to update the CL description. "build_ffmpegsumo" doesn't
apply
> > now.
> 
> It still applies, see
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg...
> 
> Unless I misunderstood?

You are right. Can you play with the gyp files so that when media_use_ffmpeg=1
on Android, build_ffmpegsumo will be "1" by default? Having to set two gyp
variables for one feature doesn't look good.

[xhwang, 2015-07-10 23:13:18]

https://codereview.chromium.org/1230793009/diff/40001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/1230793009/diff/40001/media/media.gyp#newcode733
media/media.gyp:733: 'renderers/default_renderer_factory.h',
You need to rebase on this. Now we do need default_renderer_factory.h on
Android. See https://codereview.chromium.org/1209363004/

https://codereview.chromium.org/1230793009/diff/40001/media/media_cdm.gypi
File media/media_cdm.gypi (right):

https://codereview.chromium.org/1230793009/diff/40001/media/media_cdm.gypi#ne...
media/media_cdm.gypi:18: ['enable_pepper_cdms==1', {
The following are all under this "enable_pepper_cdms==1" condition, which is
false on Android. Then do we need the android specific checks below?

https://codereview.chromium.org/1230793009/diff/40001/media/media_cdm.gypi#ne...
media/media_cdm.gypi:39: 'defines': ['CLEAR_KEY_CDM_media_use_ffmpeg_DECODER'],
Wrong search-and-replace?

[watk, 2015-07-14 18:52:24]

Patchset #4 (id:60001) has been deleted

[watk, 2015-07-14 20:15:26]

Patchset #5 (id:100001) has been deleted

[watk, 2015-07-14 21:57:16]

Patchset #4 (id:80001) has been deleted

[watk, 2015-07-14 21:57:21]

Patchset #5 (id:110011) has been deleted

[watk, 2015-07-14 21:57:29]

Patchset #4 (id:120001) has been deleted

[watk, 2015-07-14 23:33:57]

Thanks xhwang! Now you only need to set build_ffmpegsumo=1 to compile for
android.

https://codereview.chromium.org/1230793009/diff/40001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/1230793009/diff/40001/media/media.gyp#newcode733
media/media.gyp:733: 'renderers/default_renderer_factory.h',
On 2015/07/10 23:13:17, xhwang wrote:
> You need to rebase on this. Now we do need default_renderer_factory.h on
> Android. See https://codereview.chromium.org/1209363004/

Done.

https://codereview.chromium.org/1230793009/diff/40001/media/media_cdm.gypi
File media/media_cdm.gypi (right):

https://codereview.chromium.org/1230793009/diff/40001/media/media_cdm.gypi#ne...
media/media_cdm.gypi:18: ['enable_pepper_cdms==1', {
On 2015/07/10 23:13:17, xhwang wrote:
> The following are all under this "enable_pepper_cdms==1" condition, which is
> false on Android. Then do we need the android specific checks below?

Oops, didn't realize. Removed them.

https://codereview.chromium.org/1230793009/diff/40001/media/media_cdm.gypi#ne...
media/media_cdm.gypi:39: 'defines': ['CLEAR_KEY_CDM_media_use_ffmpeg_DECODER'],
On 2015/07/10 23:13:17, xhwang wrote:
> Wrong search-and-replace?

Done.

[xhwang, 2015-07-15 00:14:21]

Looking pretty good now! Just a few comments for discussion.

https://codereview.chromium.org/1230793009/diff/170001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/1230793009/diff/170001/media/media.gyp#newcod...
media/media.gyp:720: # support hardware decoding.
s/hardware/hardware video/

https://codereview.chromium.org/1230793009/diff/170001/media/media_variables....
File media/media_variables.gypi (right):

https://codereview.chromium.org/1230793009/diff/170001/media/media_variables....
media/media_variables.gypi:10: # Android and iOS don't use FFmpeg or libvpx by
default.
What's gonna happen when I set build_ffmpegsumo=1 on an iOS build?

https://codereview.chromium.org/1230793009/diff/170001/media/media_variables....
media/media_variables.gypi:18: 'media_use_ffmpeg%': '<(build_ffmpegsumo)',
I found "media_use_ffmpeg" more intuitive than "build_ffmpegsumo". Can we use
media_use_ffmpeg=1 to enable this feature instead of using build_ffmpegsumo=1?

[watk, 2015-07-16 18:51:33]

> I found "media_use_ffmpeg" more intuitive than "build_ffmpegsumo". Can we use
> media_use_ffmpeg=1 to enable this feature instead of using build_ffmpegsumo=1?

This will require moving media_use_ffmpeg to common.gypi. WDYT?

[xhwang, 2015-07-16 19:10:24]

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

[xhwang, 2015-07-16 19:10:25]

+dalecurtis

Actually I don't fully understand the "build_ffmpegsumo" flag. Can we replace
"build_ffmpegsumo" in common.gypi with the easier-to-understand flag
"media_use_ffmpeg"? But that sounds well beyond the scope of this CL. We can
probably iterate on that later.

lgtm with the current approach.

[DaleCurtis, 2015-07-16 19:26:50]

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

[DaleCurtis, 2015-07-16 19:26:51]

+chcunningham, my recollection is we need both flags because some external
vendors want media_use_ffmpeg=true, but build_ffmpegsumo=0 so that the system
ffmpeg is used for building.

[watk, 2015-07-16 20:55:13]

watk@chromium.org changed reviewers:
+ henrika@chromium.org, nick@chromium.org

[watk, 2015-07-16 20:55:14]

nick@, could you please take a look at render_frame_impl.cc?
henrika@, could you please take a look at opus.gyp?

https://codereview.chromium.org/1230793009/diff/170001/media/media_variables....
File media/media_variables.gypi (right):

https://codereview.chromium.org/1230793009/diff/170001/media/media_variables....
media/media_variables.gypi:10: # Android and iOS don't use FFmpeg or libvpx by
default.
On 2015/07/15 00:14:20, xhwang wrote:
> What's gonna happen when I set build_ffmpegsumo=1 on an iOS build?

Your build won't work :) But even if we weren't setting this here it would break
anyway because ffmpeg wouldn't compile.

[ncarter (slow), 2015-07-16 21:08:42]

render_frame_host.cc lgtm

[ncarter (slow), 2015-07-16 21:09:30]

On 2015/07/16 21:08:42, ncarter wrote:
> render_frame_host.cc lgtm

I mean _impl.cc. The file you edited. That's the one that looks good to me.

[chcunningham, 2015-07-16 22:16:17]

On 2015/07/16 19:26:51, DaleCurtis wrote:
> +chcunningham, my recollection is we need both flags because some external
> vendors want media_use_ffmpeg=true, but build_ffmpegsumo=0 so that the system
> ffmpeg is used for building.

+phajdan to confirm what follows:

watk and I discussed. I think build_ffmpegsumo may once have helped linux folks
pull system libraries, but AFAIK they no longer do this (though there is still
interest in finding ways to use system ffmpeg). I think they either use the
ffmpeg we provide or the replace with their own ffmpeg.so to satisfy
licensing/patent requirements for their distro. 

If no one is using, I think we might just remove the build_ffmpegsumo var
entirely and just avoid building ffmpeg by not taking a dependency on it for
platforms that don't build it.

[henrika (OOO until Aug 14), 2015-07-17 07:46:57]

LGTM for opus.gyp

Adding tlegrand to CC:list so she can take a closer look as well when she is
back from vacation.

[watk, 2015-07-17 22:01:30]

Patchset #7 (id:200001) has been deleted

[watk, 2015-07-20 18:43:29]

The CQ bit was checked by watk@chromium.org

[watk, 2015-07-20 18:43:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
liberato@chromium.org, xhwang@chromium.org, nick@chromium.org,
henrika@chromium.org
Link to the patchset: https://codereview.chromium.org/1230793009/#ps240001
(title: "build_ffmpegsumo is gone! yay")

[commit-bot: I haz the power, 2015-07-20 18:44:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230793009/240001

[DaleCurtis, 2015-07-20 18:48:38]

lgtm

[commit-bot: I haz the power, 2015-07-20 20:16:23]

Committed patchset #8 (id:240001)

[commit-bot: I haz the power, 2015-07-20 20:17:28]

Patchset 8 (id:??) landed as
https://crrev.com/45c3063debd193f953d5c4aa55902d185469f4bc
Cr-Commit-Position: refs/heads/master@{#339498}