Allow WebView to force MediaPlayer for unsupported containers.

Allow WebView to force MediaPlayer for unsupported containers.

WebView needs to allow all of the OS supported codecs since it
is a system level feature. This forces all the containers that
Chrome does not support onto the MediaPlayer path.

Adds a non-exhaustive WebView test to ensure that 3gp media can be
played. I've run the test locally w/ and w/o the unified media
pipeline enabled.

BUG=none
TEST=new test.

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

[DaleCurtis, 2016-03-28 20:27:22]

Description was changed from

==========
Allow WebView to force MediaPlayer for unsupported containers.

WebView needs to allow all of the OS supported codecs since it
is a system level feature. This forces all the containers that
Chrome does not support onto the MediaPlayer path.

BUG=...
TEST=Files with these extensions play.
==========

to

==========
Allow WebView to force MediaPlayer for unsupported containers.

WebView needs to allow all of the OS supported codecs since it
is a system level feature. This forces all the containers that
Chrome does not support onto the MediaPlayer path.

Adds a non-exhaustive WebView test to ensure that 3gp media can be
played. I've run the test locally w/ and w/o the unified media
pipeline enabled.

BUG=none
TEST=new test.
==========

[DaleCurtis, 2016-03-28 20:27:23]

dalecurtis@chromium.org changed reviewers:
+ boliu@chromium.org, nick@chromium.org

[ncarter (slow), 2016-03-28 22:30:04]

https://codereview.chromium.org/1824143004/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1824143004/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:772: // Android WebView may allow codecs
and containers that Chrome does not.
A command line switch seems like an awkward way to implement AW-specific
treatment of a few extensions. My instinct is that the renderer process ought to
know that it's an AW renderer, so there ought to be a more direct way to query
the AW platform, than to plumb state in via a command line.

Could we add a bool-returning ContentRendererClient method for this instead,
with a default implementation that either just 'return true' or 'return false'?
With a name like ShouldForcePlatformDecoderForPath() or
ShouldDisableUnifiedMediaPipelineForPath()?

(If this command line flag has utility otherwise, feel free to push back on this
suggestion)

[ncarter (slow), 2016-03-28 22:50:07]

https://codereview.chromium.org/1824143004/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1824143004/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:772: // Android WebView may allow codecs
and containers that Chrome does not.
On 2016/03/28 22:30:03, ncarter wrote:
> A command line switch seems like an awkward way to implement AW-specific
> treatment of a few extensions. My instinct is that the renderer process ought
to
> know that it's an AW renderer, so there ought to be a more direct way to query
> the AW platform, than to plumb state in via a command line.
> 
> Could we add a bool-returning ContentRendererClient method for this instead,
> with a default implementation that either just 'return true' or 'return
false'?
> With a name like ShouldForcePlatformDecoderForPath() or
> ShouldDisableUnifiedMediaPipelineForPath()?
> 
> (If this command line flag has utility otherwise, feel free to push back on
this
> suggestion)

Also, I checked with Charlie, who thinks it's a good idea to make this
ContentRendererCient method be #ifdef OS_ANDROID, which gives you more
flexibility in the naming.

[ddorwin, 2016-03-28 22:50:51]

https://codereview.chromium.org/1824143004/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1824143004/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:772: // Android WebView may allow codecs
and containers that Chrome does not.
On 2016/03/28 22:30:03, ncarter wrote:
> A command line switch seems like an awkward way to implement AW-specific
> treatment of a few extensions. My instinct is that the renderer process ought
to
> know that it's an AW renderer, so there ought to be a more direct way to query
> the AW platform, than to plumb state in via a command line.
> 
> Could we add a bool-returning ContentRendererClient method for this instead,
> with a default implementation that either just 'return true' or 'return
false'?
> With a name like ShouldForcePlatformDecoderForPath() or
> ShouldDisableUnifiedMediaPipelineForPath()?
> 
> (If this command line flag has utility otherwise, feel free to push back on
this
> suggestion)

See, for example ShouldUseVideoOverlayForEmbeddedEncryptedVideo(). This ends up
using prefs, which can only be modified by WebView. (I don't think we want that
because all existing users of WebView would be broken, but you could certainly
hook up the WebView logic within RendererMediaPlayerManager or similar.

Long-term, it would be nice for WebView applications, especially browsers, to
not use MP or WMPA just like Chrome. We may want to talk to the WebView team
about exposing this capability to WebView applications (or making it the default
when building against newer versions of the WebView API).

[boliu, 2016-03-28 23:39:45]

test lg2m. And second not using switches for static webview-only settings; but I
don't have a preference between using ContentRendererClient vs
RendererPreferences.

[DaleCurtis, 2016-03-29 00:28:19]

https://codereview.chromium.org/1824143004/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1824143004/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:772: // Android WebView may allow codecs
and containers that Chrome does not.
On 2016/03/28 at 22:50:51, ddorwin wrote:
> On 2016/03/28 22:30:03, ncarter wrote:
> > A command line switch seems like an awkward way to implement AW-specific
> > treatment of a few extensions. My instinct is that the renderer process
ought to
> > know that it's an AW renderer, so there ought to be a more direct way to
query
> > the AW platform, than to plumb state in via a command line.
> > 
> > Could we add a bool-returning ContentRendererClient method for this instead,
> > with a default implementation that either just 'return true' or 'return
false'?
> > With a name like ShouldForcePlatformDecoderForPath() or
> > ShouldDisableUnifiedMediaPipelineForPath()?
> > 
> > (If this command line flag has utility otherwise, feel free to push back on
this
> > suggestion)
> 
> See, for example ShouldUseVideoOverlayForEmbeddedEncryptedVideo(). This ends
up using prefs, which can only be modified by WebView. (I don't think we want
that because all existing users of WebView would be broken, but you could
certainly hook up the WebView logic within RendererMediaPlayerManager or
similar.
> 
> Long-term, it would be nice for WebView applications, especially browsers, to
not use MP or WMPA just like Chrome. We may want to talk to the WebView team
about exposing this capability to WebView applications (or making it the default
when building against newer versions of the WebView API).

Done. Thanks for the suggestion, it's much cleaner. David: I started that long
term discussion on clank-webview@ and clank-security@ prior to publishing this.
I can't link to it from here though since they are private groups.

The short of it is: WebView doesn't have UMA yet, so we have no idea what the
compatibility risk looks like. It is coming though, so for now we'll forward
everything and then add some UMA to figure out how often these containers are
actually used.

[boliu, 2016-03-29 00:32:06]

a_w lgtm

[DaleCurtis, 2016-03-29 00:41:10]

Patchset #5 (id:80001) has been deleted

[Torne, 2016-03-29 12:49:27]

On 2016/03/28 22:30:04, ncarter wrote:
> A command line switch seems like an awkward way to implement AW-specific
> treatment of a few extensions. My instinct is that the renderer process ought
to
> know that it's an AW renderer, so there ought to be a more direct way to query
> the AW platform, than to plumb state in via a command line.

Using one of the content client interfaces as the current version of this CL
does is fine, but we have intentionally avoided there being a specific "are we
webview" flag anywhere, because we don't want people to hang random behaviour
off of it. We want to keep every place where WebView has to enable some special
behaviour in lower levels as single feature-specific configurations of one kind
or another. That doesn't mean it has to be a command line flag, any kind of pref
or content client delegate or similar is fine.

[ncarter (slow), 2016-03-29 16:15:24]

lgtm

[DaleCurtis, 2016-03-29 17:27:04]

Thanks torne@ for the context. Thanks Nick and Bo for review!

[DaleCurtis, 2016-03-29 17:27:08]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2016-03-29 17:27:09]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/1824143004/#ps100001
(title: "Fix comment typo. Remove unnecessary toLower.")

[commit-bot: I haz the power, 2016-03-29 17:27:32]

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

[commit-bot: I haz the power, 2016-03-29 17:43:26]

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

[commit-bot: I haz the power, 2016-03-29 17:43:27]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[DaleCurtis, 2016-03-29 17:49:02]

Failed on '"." + extension', not sure why that compiled locally but not on the
bots. Switched to embedding the period in extension list.

[DaleCurtis, 2016-03-29 17:57:22]

The answer of course is it didn't, I must have not had the file saved when I
tested. Retested and things look good.

[DaleCurtis, 2016-03-29 17:57:29]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2016-03-29 17:57:29]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/1824143004/#ps120001
(title: "Don't concat strings.")

[commit-bot: I haz the power, 2016-03-29 17:57:50]

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

[commit-bot: I haz the power, 2016-03-29 19:02:39]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-03-29 19:03:57]

Description was changed from

==========
Allow WebView to force MediaPlayer for unsupported containers.

WebView needs to allow all of the OS supported codecs since it
is a system level feature. This forces all the containers that
Chrome does not support onto the MediaPlayer path.

Adds a non-exhaustive WebView test to ensure that 3gp media can be
played. I've run the test locally w/ and w/o the unified media
pipeline enabled.

BUG=none
TEST=new test.
==========

to

==========
Allow WebView to force MediaPlayer for unsupported containers.

WebView needs to allow all of the OS supported codecs since it
is a system level feature. This forces all the containers that
Chrome does not support onto the MediaPlayer path.

Adds a non-exhaustive WebView test to ensure that 3gp media can be
played. I've run the test locally w/ and w/o the unified media
pipeline enabled.

BUG=none
TEST=new test.

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

[commit-bot: I haz the power, 2016-03-29 19:03:58]

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