Wire up MediaCapabilities is_supported to MimeUtil

Wire up MediaCapabilities is_supported to media/

navigator.mediaCapabilities.decodingInfo(...) will now query
the media layer to parse the mime type and check platform support.

Some work remains to reject ambiguous mime strings. In some cases
we are also not deeply checking for platform support. Will get to
this soon.

BUG=695264
TEST=new browser tests

Review-Url: https://codereview.chromium.org/2805553004
Cr-Commit-Position: refs/heads/master@{#467184}
Committed: https://chromium.googlesource.com/chromium/src/+/20645b6c0f3fa1fe3d20742ca7fcd77058147ad8

[chcunningham, 2017-04-07 00:54:37]

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

[chcunningham, 2017-04-07 00:54:38]

Don't review yet - tests on the way. Just FYI in case you're wondering where
that github issue came from.

[mlamouri (slow - plz ping), 2017-04-10 12:31:50]

https://codereview.chromium.org/2805553004/diff/1/media/blink/webmediacapabil...
File media/blink/webmediacapabilitiesclient_impl.cc (right):

https://codereview.chromium.org/2805553004/diff/1/media/blink/webmediacapabil...
media/blink/webmediacapabilitiesclient_impl.cc:36:
DCHECK_EQ(codec_vector.size(), 1U);
Maybe we should do this check on the Blink side by exposing the method to
platform?

[chcunningham, 2017-04-20 21:48:10]

Ready to go :)

https://codereview.chromium.org/2805553004/diff/1/media/blink/webmediacapabil...
File media/blink/webmediacapabilitiesclient_impl.cc (right):

https://codereview.chromium.org/2805553004/diff/1/media/blink/webmediacapabil...
media/blink/webmediacapabilitiesclient_impl.cc:36:
DCHECK_EQ(codec_vector.size(), 1U);
On 2017/04/10 12:31:50, mlamouri wrote:
> Maybe we should do this check on the Blink side by exposing the method to
> platform?

I think we should do this check in blink, but we can probably just use a simpler
call to WTF::String.split(...).

SplitCodecsToVector is used a few places in media, so it would be harder to move
that entirely to blink. 

Will save this for a follow up.

[chcunningham, 2017-04-20 21:53:41]

Description was changed from

==========
Wire up MediaCapabilities is_supported to MimeUtil

BUG=695264
==========

to

==========
Wire up MediaCapabilities is_supported to media/

navigator.mediaCapabilities.decodingInfo(...) will now query
the media layer to parse the mime type and check platform support.

Some work remains to reject ambiguous mime strings. In some cases
we are also not deeply checking for platform support. Will get to
this soon.

BUG=695264
TEST=new browser tests
==========

[chcunningham, 2017-04-20 21:53:42]

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

[chcunningham, 2017-04-20 21:53:56]

+ jrummel@ for media/

[chcunningham, 2017-04-20 21:58:38]

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

[commit-bot: I haz the power, 2017-04-20 21:59:28]

Dry run: 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, 2017-04-20 22:26:59]

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

[commit-bot: I haz the power, 2017-04-20 22:27:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chcunningham, 2017-04-21 18:48:45]

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

[commit-bot: I haz the power, 2017-04-21 18:49:07]

Dry run: 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, 2017-04-21 18:53:13]

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

[commit-bot: I haz the power, 2017-04-21 18:53:14]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[chcunningham, 2017-04-21 19:33:54]

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

[chcunningham, 2017-04-21 19:33:54]

+ mcasas for media recorder

[chcunningham, 2017-04-21 19:34:23]

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

[commit-bot: I haz the power, 2017-04-21 19:34:41]

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

[mcasas, 2017-04-21 20:18:33]

encoding parts lgtm, thanks!

https://codereview.chromium.org/2805553004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/media_capabilities/MediaCapabilities.cpp
(right):

https://codereview.chromium.org/2805553004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_capabilities/MediaCapabilities.cpp:117:
DCHECK(configuration.hasType());
nit: I missed this one in my previous CL, if the dictionary
|configuration| requires |type|, this check is superfluous,
JS would throw and exception (and reject the Promise):

> navigator.mediaCapabilities.encodingInfo({});
Promise {[[PromiseStatus]]: "rejected", [[PromiseValue]]: TypeError: Failed to
execute 'encodingInfo' on 'MediaCapabilities': required member type is undefined
…}

[commit-bot: I haz the power, 2017-04-21 20:35:15]

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

[commit-bot: I haz the power, 2017-04-21 20:35:16]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jrummell, 2017-04-21 21:14:36]

media LGTM

https://codereview.chromium.org/2805553004/diff/60001/content/browser/media/m...
File content/browser/media/media_capabilities_browsertest.cc (right):

https://codereview.chromium.org/2805553004/diff/60001/content/browser/media/m...
content/browser/media/media_capabilities_browsertest.cc:117:
EXPECT_EQ(kUnsupported, CanDecodeVideo("'video/mp4; codecs=\"vp9\"'"));
How about testing audio codecs in the video field (and the opposite below)?

[chcunningham, 2017-04-24 22:42:01]

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

[commit-bot: I haz the power, 2017-04-24 22:43:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chcunningham, 2017-04-24 22:47:52]

Mounir, gentle ping

https://codereview.chromium.org/2805553004/diff/60001/content/browser/media/m...
File content/browser/media/media_capabilities_browsertest.cc (right):

https://codereview.chromium.org/2805553004/diff/60001/content/browser/media/m...
content/browser/media/media_capabilities_browsertest.cc:117:
EXPECT_EQ(kUnsupported, CanDecodeVideo("'video/mp4; codecs=\"vp9\"'"));
On 2017/04/21 21:14:36, jrummell wrote:
> How about testing audio codecs in the video field (and the opposite below)?

In the AudioDecodeTypes I do try 'audio/webm; codecs="vp8"'.

For video, I originally had some audio codecs tested video/webm, but I found
they all pass. Perhaps this is just an artifact of how the mime code is written,
but I also think its acceptable given how we often mention audio codecs
alongside video codecs for the top-level video/* type. The extra validation
required to fail video/* when only audio codecs are present is probably not
worth it.

https://codereview.chromium.org/2805553004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/media_capabilities/MediaCapabilities.cpp
(right):

https://codereview.chromium.org/2805553004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_capabilities/MediaCapabilities.cpp:37:
// DCHECK(parsed_content_type.IsValid());
FYI, this DCHECK (and same for ToVideoConfig) was causing test failures. DCHECK
is not what we want, so this comment just serves as a reminder to add the
exception later on.

[commit-bot: I haz the power, 2017-04-24 23:35:56]

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

[commit-bot: I haz the power, 2017-04-24 23:35:57]

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

[mlamouri (slow - plz ping), 2017-04-25 12:48:44]

lgtm. Sorry for the delay. I took Monday off-reviews :)

https://codereview.chromium.org/2805553004/diff/80001/content/browser/media/m...
File content/browser/media/media_capabilities_browsertest.cc (right):

https://codereview.chromium.org/2805553004/diff/80001/content/browser/media/m...
content/browser/media/media_capabilities_browsertest.cc:39:
MediaCapabilitiesTest() {}
nit: = default;

https://codereview.chromium.org/2805553004/diff/80001/media/blink/webmediacap...
File media/blink/webmediacapabilitiesclient_impl.cc (right):

https://codereview.chromium.org/2805553004/diff/80001/media/blink/webmediacap...
media/blink/webmediacapabilitiesclient_impl.cc:28: if
(configuration.audio_configuration.has_value()) {
nit: up to you but you can drop |has_value()| FWIW

https://codereview.chromium.org/2805553004/diff/80001/media/blink/webmediacap...
media/blink/webmediacapabilitiesclient_impl.cc:36:
DCHECK_LE(codec_vector.size(), 1U);
I think we should implement the parsing on Blink so exceptions wouldn't have to
happen from media/

https://codereview.chromium.org/2805553004/diff/80001/media/blink/webmediacap...
media/blink/webmediacapabilitiesclient_impl.cc:64: audio_support == IsSupported
&& video_support == IsSupported;
Should we expect code that will check if we can support them *together*?

[chcunningham, 2017-04-25 20:53:51]

Thanks everyone.

https://codereview.chromium.org/2805553004/diff/80001/content/browser/media/m...
File content/browser/media/media_capabilities_browsertest.cc (right):

https://codereview.chromium.org/2805553004/diff/80001/content/browser/media/m...
content/browser/media/media_capabilities_browsertest.cc:39:
MediaCapabilitiesTest() {}
On 2017/04/25 12:48:43, mlamouri wrote:
> nit: = default;

Done.

https://codereview.chromium.org/2805553004/diff/80001/media/blink/webmediacap...
File media/blink/webmediacapabilitiesclient_impl.cc (right):

https://codereview.chromium.org/2805553004/diff/80001/media/blink/webmediacap...
media/blink/webmediacapabilitiesclient_impl.cc:28: if
(configuration.audio_configuration.has_value()) {
On 2017/04/25 12:48:43, mlamouri wrote:
> nit: up to you but you can drop |has_value()| FWIW

Done.

https://codereview.chromium.org/2805553004/diff/80001/media/blink/webmediacap...
media/blink/webmediacapabilitiesclient_impl.cc:36:
DCHECK_LE(codec_vector.size(), 1U);
On 2017/04/25 12:48:43, mlamouri wrote:
> I think we should implement the parsing on Blink so exceptions wouldn't have
to
> happen from media/

ACK. Will follow up. See my earlier comment about move being non-trivial. Will
evaluate full move vs just doing some simple parsing in blink for purpose of
this exception.

https://codereview.chromium.org/2805553004/diff/80001/media/blink/webmediacap...
media/blink/webmediacapabilitiesclient_impl.cc:64: audio_support == IsSupported
&& video_support == IsSupported;
On 2017/04/25 12:48:43, mlamouri wrote:
> Should we expect code that will check if we can support them *together*?

Not for now. For the unencrypted MSE case, all codecs / containers that are
supported separately are also supported together (in separate MSE source
buffers). For non-MSE (src="foo.webm") this is also true, with the big caveat
that the file type's would have to match  (audio/webm and video/webm), because
different file types doesn't make sense if you're asking about a single file. We
could add logic to check that the file types match for type="file", but its not
urgent - there is no way for users to actually construct a frankenstein mixed
mp4/webm with any expectation that we could play it.

When we start to consider encryption, we may then need to check things together.
Even then, I don't currently have any concrete case where things could be played
separately but not together (I think these discussions were always a bit
hypothetical, but I may have forgotten some details).

[chcunningham, 2017-04-25 20:54:27]

The CQ bit was checked by chcunningham@chromium.org

[chcunningham, 2017-04-25 20:54:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
jrummell@chromium.org, mcasas@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2805553004/#ps100001
(title: "Feedback and test fix")

[commit-bot: I haz the power, 2017-04-25 20:55:27]

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, 2017-04-25 21:47:40]

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

[commit-bot: I haz the power, 2017-04-25 21:47:41]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[chcunningham, 2017-04-25 22:23:50]

The CQ bit was checked by chcunningham@chromium.org

[chcunningham, 2017-04-25 22:23:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
jrummell@chromium.org, mcasas@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2805553004/#ps120001
(title: "Remove test for theora - not supported on android")

[commit-bot: I haz the power, 2017-04-25 22:24:33]

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, 2017-04-26 00:53:08]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1493159030342290,
"parent_rev": "7924a4308a1956e0c4b065c483de4da44cb6c383", "commit_rev":
"20645b6c0f3fa1fe3d20742ca7fcd77058147ad8"}

[commit-bot: I haz the power, 2017-04-26 00:53:15]

Description was changed from

==========
Wire up MediaCapabilities is_supported to media/

navigator.mediaCapabilities.decodingInfo(...) will now query
the media layer to parse the mime type and check platform support.

Some work remains to reject ambiguous mime strings. In some cases
we are also not deeply checking for platform support. Will get to
this soon.

BUG=695264
TEST=new browser tests
==========

to

==========
Wire up MediaCapabilities is_supported to media/

navigator.mediaCapabilities.decodingInfo(...) will now query
the media layer to parse the mime type and check platform support.

Some work remains to reject ambiguous mime strings. In some cases
we are also not deeply checking for platform support. Will get to
this soon.

BUG=695264
TEST=new browser tests

Review-Url: https://codereview.chromium.org/2805553004
Cr-Commit-Position: refs/heads/master@{#467184}
Committed:
https://chromium.googlesource.com/chromium/src/+/20645b6c0f3fa1fe3d20742ca7fc...
==========

[commit-bot: I haz the power, 2017-04-26 00:53:16]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/20645b6c0f3fa1fe3d20742ca7fc...