Add MimeUtil::Parse{Audio|Video}CodecString

Add MimeUtil::Parse{Audio|Video}CodecString

Media Capabilities needs MimeUtil to turn strings into codecs before
it can query media::DecodeCapabilities APIs.

This also exposes when the parsed string is ambiguous so that Media
Capabilities can reject support and log some error.

BUG=695264
TEST=new media_unittest

Review-Url: https://codereview.chromium.org/2864593003
Cr-Commit-Position: refs/heads/master@{#472987}
Committed: https://chromium.googlesource.com/chromium/src/+/a4a66a6e3e01cf6594872f4d34208685a6eae5ea

[chcunningham, 2017-05-08 22:44:45]

Description was changed from

==========
Add MimeUtil::Parse{Audio|Video}CodecString

Media Capabilities needs MimeUtil to turn strings into codecs before
it can query media::DecodeCapabilities APIs. Unlike canPlayType,
Media Capabilities will query for capabilities directly rather than
deferring to MimeUtil to do table lookups.

This also exposes when the parsed string is ambiguous so that Media
Capabilities can reject support and log some error.

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

to

==========
Add MimeUtil::Parse{Audio|Video}CodecString

Media Capabilities needs MimeUtil to turn strings into codecs before
it can query media::DecodeCapabilities APIs.

This also exposes when the parsed string is ambiguous so that Media
Capabilities can reject support and log some error.

BUG=695264
TEST=new media_unittest
==========

[chcunningham, 2017-05-08 22:45:06]

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

[chcunningham, 2017-05-08 22:45:06]

Adding reviewers ;)

[tguilbert, 2017-05-08 23:19:57]

LGTM

The code looks fine to me. However, I have little experience with mime type
parsing, and therefore will defer judgment about 'is this the right thing to do'
to chcunningham@.

https://codereview.chromium.org/2864593003/diff/20001/media/base/mime_util_in...
File media/base/mime_util_internal.cc (right):

https://codereview.chromium.org/2864593003/diff/20001/media/base/mime_util_in...
media/base/mime_util_internal.cc:454: // Not part of audio parsing, but its
possible users will mistakenly put
Pure curiosity and not a CL comment: why do we handle when users add video
codecs in the audio config string, but not vice versa?

[chcunningham, 2017-05-10 20:39:38]

Thanks 

>
https://codereview.chromium.org/2864593003/diff/20001/media/base/mime_util_in...
> File media/base/mime_util_internal.cc (right):
> 
>
https://codereview.chromium.org/2864593003/diff/20001/media/base/mime_util_in...
> media/base/mime_util_internal.cc:454: // Not part of audio parsing, but its
> possible users will mistakenly put
> Pure curiosity and not a CL comment: why do we handle when users add video
> codecs in the audio config string, but not vice versa?

Both the Audio and Video APIs may encounter folks passing mismatched codecs. I
just only add this comment in the audio implementation because its weird to see
a bunch of video codec things for an audio query. But in either case, if the
codec type is not a match for the method signature, the
MimeUtilTo{Video|Audio}Codec(...) call will return kUnkownCodec.

[mlamouri (slow - plz ping), 2017-05-11 09:59:07]

lgtm with documentation in the header files

https://codereview.chromium.org/2864593003/diff/20001/media/base/mime_util.h
File media/base/mime_util.h (right):

https://codereview.chromium.org/2864593003/diff/20001/media/base/mime_util.h#...
media/base/mime_util.h:41: AudioCodec* out_codec);
Here and for the internal header too, it would be good to give an explanation of
what the methods are doing and what to expect. Especially given tha they are
taking out params and also have a return values. As a caller, should I expect
`out_level` to be set if `ParseVideoCodecString` returns false for example?

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

https://codereview.chromium.org/2864593003/diff/20001/media/blink/webmediacap...
media/blink/webmediacapabilitiesclient_impl.cc:51: AudioConfig audio_config =
{audio_codec};
style: should we have spaces around { } ?

https://codereview.chromium.org/2864593003/diff/20001/media/blink/webmediacap...
media/blink/webmediacapabilitiesclient_impl.cc:78: video_color_space};
ditto

[chcunningham, 2017-05-11 22:28:31]

On 2017/05/11 09:59:07, mlamouri wrote:
> lgtm with documentation in the header files
> 
> https://codereview.chromium.org/2864593003/diff/20001/media/base/mime_util.h
> File media/base/mime_util.h (right):
> 
>
https://codereview.chromium.org/2864593003/diff/20001/media/base/mime_util.h#...
> media/base/mime_util.h:41: AudioCodec* out_codec);
> Here and for the internal header too, it would be good to give an explanation
of
> what the methods are doing and what to expect. Especially given tha they are
> taking out params and also have a return values. As a caller, should I expect
> `out_level` to be set if `ParseVideoCodecString` returns false for example?
> 
>
https://codereview.chromium.org/2864593003/diff/20001/media/blink/webmediacap...
> File media/blink/webmediacapabilitiesclient_impl.cc (right):
> 
>
https://codereview.chromium.org/2864593003/diff/20001/media/blink/webmediacap...
> media/blink/webmediacapabilitiesclient_impl.cc:51: AudioConfig audio_config =
> {audio_codec};
> style: should we have spaces around { } ?
> 
>
https://codereview.chromium.org/2864593003/diff/20001/media/blink/webmediacap...
> media/blink/webmediacapabilitiesclient_impl.cc:78: video_color_space};
> ditto

FYI. I had to add a little method to media.cc/h to toggle whether the new vp9
string parsing is enabled. It was getting stuck as enabled between tests and
causing tests to flake. Will be enabled by default very soon, so all
flags/platform methods for "vp09.*" will go away.


On 2017/05/11 09:59:07, mlamouri wrote:
> lgtm with documentation in the header files
> 
> https://codereview.chromium.org/2864593003/diff/20001/media/base/mime_util.h
> File media/base/mime_util.h (right):
> 
>
https://codereview.chromium.org/2864593003/diff/20001/media/base/mime_util.h#...
> media/base/mime_util.h:41: AudioCodec* out_codec);
> Here and for the internal header too, it would be good to give an explanation
of
> what the methods are doing and what to expect. Especially given tha they are
> taking out params and also have a return values. As a caller, should I expect
> `out_level` to be set if `ParseVideoCodecString` returns false for example?

Done.

> 
>
https://codereview.chromium.org/2864593003/diff/20001/media/blink/webmediacap...
> File media/blink/webmediacapabilitiesclient_impl.cc (right):
> 
>
https://codereview.chromium.org/2864593003/diff/20001/media/blink/webmediacap...
> media/blink/webmediacapabilitiesclient_impl.cc:51: AudioConfig audio_config =
> {audio_codec};
> style: should we have spaces around { } ?
> 
>
https://codereview.chromium.org/2864593003/diff/20001/media/blink/webmediacap...
> media/blink/webmediacapabilitiesclient_impl.cc:78: video_color_space};
> ditto

Clang format prefers no spaces.

[chcunningham, 2017-05-11 22:29:54]

The CQ bit was checked by chcunningham@chromium.org

[chcunningham, 2017-05-11 22:29:54]

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

[commit-bot: I haz the power, 2017-05-11 22:31:01]

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-05-11 23:39:41]

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

[commit-bot: I haz the power, 2017-05-11 23:39:42]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[chcunningham, 2017-05-16 23:46:51]

tguilbert@ - I changed everything in media/. PTAL
mlamouri@ - all media caps stuff is untouched.

[chcunningham, 2017-05-17 00:10:54]

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

[commit-bot: I haz the power, 2017-05-17 00:12:19]

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-05-17 01:41:03]

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

[commit-bot: I haz the power, 2017-05-17 01:41:04]

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_...)

[mlamouri (slow - plz ping), 2017-05-17 13:31:01]

On 2017/05/16 at 23:46:51, chcunningham wrote:
> tguilbert@ - I changed everything in media/. PTAL
> mlamouri@ - all media caps stuff is untouched.

Based on the above, I assume you don't want me to take another look. Please, let
me know if that's incorrect.

[tguilbert, 2017-05-17 20:58:33]

still LGTM

https://codereview.chromium.org/2864593003/diff/80001/media/base/mime_util_un...
File media/base/mime_util_unittest.cc (right):

https://codereview.chromium.org/2864593003/diff/80001/media/base/mime_util_un...
media/base/mime_util_unittest.cc:280: EXPECT_EQ(VideoColorSpace::REC709(),
out_colorspace);
Can you add a UT for a out_colorspace that isn't REC709

[chcunningham, 2017-05-18 01:29:42]

Thanks for review!

On 2017/05/17 13:31:01, mlamouri wrote:
> Based on the above, I assume you don't want me to take another look. Please,
let me know if that's incorrect.

Nope, you're good!

https://codereview.chromium.org/2864593003/diff/80001/media/base/mime_util_un...
File media/base/mime_util_unittest.cc (right):

https://codereview.chromium.org/2864593003/diff/80001/media/base/mime_util_un...
media/base/mime_util_unittest.cc:280: EXPECT_EQ(VideoColorSpace::REC709(),
out_colorspace);
On 2017/05/17 20:58:33, tguilbert wrote:
> Can you add a UT for a out_colorspace that isn't REC709

Done.

[chcunningham, 2017-05-18 01:29:52]

The CQ bit was checked by chcunningham@chromium.org

[chcunningham, 2017-05-18 01:29:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org, tguilbert@chromium.org
Link to the patchset: https://codereview.chromium.org/2864593003/#ps100001
(title: "Feedback && Proprietary Codecs fix && Rebase")

[commit-bot: I haz the power, 2017-05-18 01:31:55]

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-05-18 05:57:40]

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

[commit-bot: I haz the power, 2017-05-18 05:57:41]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[chcunningham, 2017-05-18 18:05:32]

The CQ bit was checked by chcunningham@chromium.org

[commit-bot: I haz the power, 2017-05-18 18:06:18]

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-05-19 00:14:08]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495130732521680,
"parent_rev": "c1a9cc5af57748545858620aab0e9cd8e24dcc93", "commit_rev":
"a4a66a6e3e01cf6594872f4d34208685a6eae5ea"}

[commit-bot: I haz the power, 2017-05-19 00:14:22]

Description was changed from

==========
Add MimeUtil::Parse{Audio|Video}CodecString

Media Capabilities needs MimeUtil to turn strings into codecs before
it can query media::DecodeCapabilities APIs.

This also exposes when the parsed string is ambiguous so that Media
Capabilities can reject support and log some error.

BUG=695264
TEST=new media_unittest
==========

to

==========
Add MimeUtil::Parse{Audio|Video}CodecString

Media Capabilities needs MimeUtil to turn strings into codecs before
it can query media::DecodeCapabilities APIs.

This also exposes when the parsed string is ambiguous so that Media
Capabilities can reject support and log some error.

BUG=695264
TEST=new media_unittest

Review-Url: https://codereview.chromium.org/2864593003
Cr-Commit-Position: refs/heads/master@{#472987}
Committed:
https://chromium.googlesource.com/chromium/src/+/a4a66a6e3e01cf6594872f4d3420...
==========

[commit-bot: I haz the power, 2017-05-19 00:14:23]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/a4a66a6e3e01cf6594872f4d3420...