Make video codec support logic overridable via MediaClient

Make video codec support logic overridable via MediaClient

This will give content embedders (such as Chromecast) a way to override
supported video codecs logic.

BUG=477103
Committed: https://crrev.com/7318a94af3fe7cdca14f427339060de5ac6a487a
Cr-Commit-Position: refs/heads/master@{#407681}

[servolk, 2016-07-15 20:36:20]

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

[commit-bot: I haz the power, 2016-07-15 20:36:57]

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, 2016-07-15 21:50:40]

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

[commit-bot: I haz the power, 2016-07-15 21:50:41]

Dry run: This issue passed the CQ dry run.

[servolk, 2016-07-18 18:24:33]

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

[commit-bot: I haz the power, 2016-07-18 18:25:04]

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, 2016-07-18 19:47:49]

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

[commit-bot: I haz the power, 2016-07-18 19:47:50]

Dry run: This issue passed the CQ dry run.

[servolk, 2016-07-20 23:04:27]

Description was changed from

==========
Make video codec support logic overridable via MediaClient

This will give content embedders (such as Chromecast) a way to override
supported video codecs logic.

BUG=477103
==========

to

==========
Make video codec support logic overridable via MediaClient

This will give content embedders (such as Chromecast) a way to override
supported video codecs logic.

BUG=477103
==========

[servolk, 2016-07-20 23:04:27]

servolk@chromium.org changed reviewers:
+ dalecurtis@chromium.org, ddorwin@chromium.org, wolenetz@chromium.org

[servolk, 2016-07-20 23:08:36]

On 2016/07/20 23:04:27, servolk wrote:
> mailto:servolk@chromium.org changed reviewers:
> + mailto:dalecurtis@chromium.org, mailto:ddorwin@chromium.org,
mailto:wolenetz@chromium.org

Coming back to crbug.com/477103, let's make the codec support logic overridable
by content embedders on the MediaClient level. For now I've kept the old logic
for determining whether we return 'probably' or 'maybe' for VP9 and H.264
codecs, to minimize the chances of breaking anything. But going forward we might
want to review and change that logic so that we never return 'maybe' result for
codec ids which provide enough information for us to determine if they are
actually supported or not (H.264, HEVC and hopefully VP9 soon).

[ddorwin, 2016-07-20 23:26:24]

ddorwin@chromium.org changed reviewers:
+ chcunningham@chromium.org
- wolenetz@chromium.org

[ddorwin, 2016-07-20 23:26:24]

https://codereview.chromium.org/2153643002/diff/40001/content/browser/media/m...
File content/browser/media/media_canplaytype_browsertest.cc (right):

https://codereview.chromium.org/2153643002/diff/40001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:753: // HEVC is currently
not supported in Chromium.
Do these test pass on implementations that do have
BUILDFLAG(ENABLE_HEVC_DEMUXING) and a corresponding MediaClient?

(I'm also wondering how we'll run tests when we start detecting VP9 profiles,
etc.)

Regarding the text, IIRC we use "Chrome" when referring to the browser. Also,
"currently" can probably be dropped.

https://codereview.chromium.org/2153643002/diff/40001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:754: EXPECT_EQ(kNot,
CanPlay("'video/mp4; codecs=\"hev1.1.6.L93.B0\"'"));
What does encrypted_media_supported_types_browsertest.cc do? It appears
BUILDFLAG(ENABLE_HEVC_DEMUXING) would include HEVC codecs, but those should have
failed since is_ambiguous was true.

[servolk, 2016-07-20 23:50:41]

https://codereview.chromium.org/2153643002/diff/40001/content/browser/media/m...
File content/browser/media/media_canplaytype_browsertest.cc (right):

https://codereview.chromium.org/2153643002/diff/40001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:753: // HEVC is currently
not supported in Chromium.
On 2016/07/20 23:26:24, ddorwin wrote:
> Do these test pass on implementations that do have
> BUILDFLAG(ENABLE_HEVC_DEMUXING) and a corresponding MediaClient?
> 
> (I'm also wondering how we'll run tests when we start detecting VP9 profiles,
> etc.)
> 
> Regarding the text, IIRC we use "Chrome" when referring to the browser. Also,
> "currently" can probably be dropped.

There's no custom Chromecast MediaClient yet, I'm working on it in a separate CL
https://codereview.chromium.org/2156193003/ (although I wanted to inherit it
from the content::RenderMediaClient to avoid duplicating functions other than
IsSupportedVideoConfig and ran into some deps issues, looking into how to fix
that). But yes, you are right this would fail on platforms that do support HEVC
once we add the custom media client. I guess we could remove the HEVC tests from
this file completely and move it to Chromecast internal code, where we'll be
able to tell the actual expected outcome based on which platform we are running.

https://codereview.chromium.org/2153643002/diff/40001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:754: EXPECT_EQ(kNot,
CanPlay("'video/mp4; codecs=\"hev1.1.6.L93.B0\"'"));
On 2016/07/20 23:26:24, ddorwin wrote:
> What does encrypted_media_supported_types_browsertest.cc do? It appears
> BUILDFLAG(ENABLE_HEVC_DEMUXING) would include HEVC codecs, but those should
have
> failed since is_ambiguous was true.

Hmm, looks like somehow these don't get run at all on cast_shell trybots. At
least I can't find the 'EncryptedMediaSupportedTypes' substring anywhere in the
cast_shell trybot log
(https://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_lin...).
I wonder if we have custom encrypted media tests for Cast, I'll research more
and ask Luke.

[ddorwin, 2016-07-22 00:40:52]

https://codereview.chromium.org/2153643002/diff/40001/content/browser/media/m...
File content/browser/media/media_canplaytype_browsertest.cc (right):

https://codereview.chromium.org/2153643002/diff/40001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:753: // HEVC is currently
not supported in Chromium.
On 2016/07/20 23:50:41, servolk wrote:
> On 2016/07/20 23:26:24, ddorwin wrote:
> > Do these test pass on implementations that do have
> > BUILDFLAG(ENABLE_HEVC_DEMUXING) and a corresponding MediaClient?
> > 
> > (I'm also wondering how we'll run tests when we start detecting VP9
profiles,
> > etc.)
> > 
> > Regarding the text, IIRC we use "Chrome" when referring to the browser.
Also,
> > "currently" can probably be dropped.
> 
> There's no custom Chromecast MediaClient yet, I'm working on it in a separate
CL
> https://codereview.chromium.org/2156193003/ (although I wanted to inherit it
> from the content::RenderMediaClient to avoid duplicating functions other than
> IsSupportedVideoConfig and ran into some deps issues, looking into how to fix
> that). But yes, you are right this would fail on platforms that do support
HEVC
> once we add the custom media client. I guess we could remove the HEVC tests
from
> this file completely and move it to Chromecast internal code, where we'll be
> able to tell the actual expected outcome based on which platform we are
running.

No, we shouldn't remove these tests. They at least provide negative coverage for
Chrome. Also, you will eventually want them. We will need to figure out how to
specify the expectations for such tests or override the results somehow.

Until then, I think it's probably better to leave kHevcSupported, even if it =
kNot, and add a TODO to figure this out where it is defined.

[chcunningham, 2016-07-22 00:54:41]

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

https://codereview.chromium.org/2153643002/diff/40001/media/base/mime_util_in...
media/base/mime_util_internal.cc:683: return
GetMediaClient()->IsSupportedVideoConfig(kCodecH264, profile,
This method is called StringToCodec, and its header comment says: 

Returns true if this method was able to map |codec_id| with
|mime_type_lower_case| to a specific Codec enum value. 

Given the above, it feels weird to me for this function to now be the arbiter of
platform support ... I think these calls to media client would be more at home
in AreSupportedCodecs. Then you'd only have to call it once. You'll want to
surface the level and profile as out params though

[ddorwin, 2016-07-22 02:06:37]

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

https://codereview.chromium.org/2153643002/diff/40001/media/base/mime_util_in...
media/base/mime_util_internal.cc:683: return
GetMediaClient()->IsSupportedVideoConfig(kCodecH264, profile,
On 2016/07/22 00:54:41, chcunningham wrote:
> This method is called StringToCodec, and its header comment says: 
> 
> Returns true if this method was able to map |codec_id| with
> |mime_type_lower_case| to a specific Codec enum value. 
> 
> Given the above, it feels weird to me for this function to now be the arbiter
of
> platform support ... I think these calls to media client would be more at home
> in AreSupportedCodecs. Then you'd only have to call it once. You'll want to
> surface the level and profile as out params though

Agreed about the naming. There are other issues, such as, IIRC IsTypeSupported()
only returning a value that then gets checked for real support.

[servolk, 2016-07-22 02:26:09]

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

https://codereview.chromium.org/2153643002/diff/40001/media/base/mime_util_in...
media/base/mime_util_internal.cc:683: return
GetMediaClient()->IsSupportedVideoConfig(kCodecH264, profile,
On 2016/07/22 02:06:37, ddorwin wrote:
> On 2016/07/22 00:54:41, chcunningham wrote:
> > This method is called StringToCodec, and its header comment says: 
> > 
> > Returns true if this method was able to map |codec_id| with
> > |mime_type_lower_case| to a specific Codec enum value. 
> > 
> > Given the above, it feels weird to me for this function to now be the
arbiter
> of
> > platform support ... I think these calls to media client would be more at
home
> > in AreSupportedCodecs. Then you'd only have to call it once. You'll want to
> > surface the level and profile as out params though
> 
> Agreed about the naming. There are other issues, such as, IIRC
IsTypeSupported()
> only returning a value that then gets checked for real support.

David, do you mean MSE's IsTypeSupported? It actually relies on the checks being
done here to decide if the codec is actually supported by the platform or not.
It then does some additional checks, but it never returns true for a codec that
was rejected here. So I believe it should be fine.

https://codereview.chromium.org/2153643002/diff/40001/media/base/mime_util_in...
media/base/mime_util_internal.cc:683: return
GetMediaClient()->IsSupportedVideoConfig(kCodecH264, profile,
On 2016/07/22 00:54:41, chcunningham wrote:
> This method is called StringToCodec, and its header comment says: 
> 
> Returns true if this method was able to map |codec_id| with
> |mime_type_lower_case| to a specific Codec enum value. 
> 
> Given the above, it feels weird to me for this function to now be the arbiter
of
> platform support ... I think these calls to media client would be more at home
> in AreSupportedCodecs. Then you'd only have to call it once. You'll want to
> surface the level and profile as out params though

I agree with this, but actually performing that is much trickier than it might
seem at first. There's a couple of things that make it harder:
* StringToCodec returns a MimeUtil codec, while for a publicly visible API in
the MediaClient we'd prefer to use a public media::VideoCodec type.
* I'd prefer to keep the logic for handling various H264 and VP9 profiles for
now to minimize a chance of regressing something.
Having said that, it might still be possible to shuffle some code around to
invoke MediaClient::IsSupported from AreSupportedCodecs. I'll give it a try.

[servolk, 2016-07-22 02:46:53]

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

[commit-bot: I haz the power, 2016-07-22 02:47:27]

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

[servolk, 2016-07-22 03:06:28]

https://codereview.chromium.org/2153643002/diff/40001/content/browser/media/m...
File content/browser/media/media_canplaytype_browsertest.cc (right):

https://codereview.chromium.org/2153643002/diff/40001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:753: // HEVC is currently
not supported in Chromium.
On 2016/07/22 00:40:51, ddorwin wrote:
> On 2016/07/20 23:50:41, servolk wrote:
> > On 2016/07/20 23:26:24, ddorwin wrote:
> > > Do these test pass on implementations that do have
> > > BUILDFLAG(ENABLE_HEVC_DEMUXING) and a corresponding MediaClient?
> > > 
> > > (I'm also wondering how we'll run tests when we start detecting VP9
> profiles,
> > > etc.)
> > > 
> > > Regarding the text, IIRC we use "Chrome" when referring to the browser.
> Also,
> > > "currently" can probably be dropped.
> > 
> > There's no custom Chromecast MediaClient yet, I'm working on it in a
separate
> CL
> > https://codereview.chromium.org/2156193003/ (although I wanted to inherit it
> > from the content::RenderMediaClient to avoid duplicating functions other
than
> > IsSupportedVideoConfig and ran into some deps issues, looking into how to
fix
> > that). But yes, you are right this would fail on platforms that do support
> HEVC
> > once we add the custom media client. I guess we could remove the HEVC tests
> from
> > this file completely and move it to Chromecast internal code, where we'll be
> > able to tell the actual expected outcome based on which platform we are
> running.
> 
> No, we shouldn't remove these tests. They at least provide negative coverage
for
> Chrome. Also, you will eventually want them. We will need to figure out how to
> specify the expectations for such tests or override the results somehow.
> 
> Until then, I think it's probably better to leave kHevcSupported, even if it =
> kNot, and add a TODO to figure this out where it is defined.

Done (restored the kHevcSupported)

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

https://codereview.chromium.org/2153643002/diff/40001/media/base/mime_util_in...
media/base/mime_util_internal.cc:683: return
GetMediaClient()->IsSupportedVideoConfig(kCodecH264, profile,
On 2016/07/22 02:26:09, servolk wrote:
> On 2016/07/22 00:54:41, chcunningham wrote:
> > This method is called StringToCodec, and its header comment says: 
> > 
> > Returns true if this method was able to map |codec_id| with
> > |mime_type_lower_case| to a specific Codec enum value. 
> > 
> > Given the above, it feels weird to me for this function to now be the
arbiter
> of
> > platform support ... I think these calls to media client would be more at
home
> > in AreSupportedCodecs. Then you'd only have to call it once. You'll want to
> > surface the level and profile as out params though
> 
> I agree with this, but actually performing that is much trickier than it might
> seem at first. There's a couple of things that make it harder:
> * StringToCodec returns a MimeUtil codec, while for a publicly visible API in
> the MediaClient we'd prefer to use a public media::VideoCodec type.
> * I'd prefer to keep the logic for handling various H264 and VP9 profiles for
> now to minimize a chance of regressing something.
> Having said that, it might still be possible to shuffle some code around to
> invoke MediaClient::IsSupported from AreSupportedCodecs. I'll give it a try.

Done in patchset #4

[commit-bot: I haz the power, 2016-07-22 03:23:19]

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

[commit-bot: I haz the power, 2016-07-22 03:23:20]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[servolk, 2016-07-22 03:31:12]

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

[commit-bot: I haz the power, 2016-07-22 03:32:20]

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, 2016-07-22 04:45:03]

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

[commit-bot: I haz the power, 2016-07-22 04:45:04]

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

[chcunningham, 2016-07-22 20:11:26]

LGTM % nit about the is_ambiguous flag.

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

https://codereview.chromium.org/2153643002/diff/80001/media/base/mime_util_in...
media/base/mime_util_internal.cc:305: if (video_codec == kCodecH264) {
Did you mean to move this stuff out too? I feel like its pretty at home in the
StringToCodec function. Its fine with me if that method decides whats ambiguous
but still emits the profile/level as out params for us to use with
IsSupportedVideoConfig. Maybe I'm missing some reason why this had to come out

https://codereview.chromium.org/2153643002/diff/80001/media/base/mime_util_in...
media/base/mime_util_internal.cc:344: if (GetMediaClient() && video_codec !=
kUnknownVideoCodec &&
Nice

[miu, 2016-07-22 20:58:15]

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

[miu, 2016-07-22 20:58:16]

Thanks for doing this! :)

BTW--Here are some tentative interfaces we were thinking of using to pass the
remote device's supported codec/profile/etc. We'll probably need to adjust this:
https://codereview.chromium.org/2138013003/diff/80001/media/mojo/interfaces/r...

Comments on this CL:

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

https://codereview.chromium.org/2153643002/diff/80001/media/base/mime_util_in...
media/base/mime_util_internal.cc:305: if (video_codec == kCodecH264) {
On 2016/07/22 20:11:25, chcunningham wrote:
> Did you mean to move this stuff out too? I feel like its pretty at home in the
> StringToCodec function. Its fine with me if that method decides whats
ambiguous
> but still emits the profile/level as out params for us to use with
> IsSupportedVideoConfig. Maybe I'm missing some reason why this had to come out

Agreed. For example, Chromecast currently doesn't support 10-bit profiles, so
we'll want to be sure to return IsNotSupported in that case.

https://codereview.chromium.org/2153643002/diff/80001/media/base/mime_util_in...
media/base/mime_util_internal.cc:344: if (GetMediaClient() && video_codec !=
kUnknownVideoCodec &&
Is GetMediaClient() global for the whole render process? For media remoting,
it's possible only a subset of render frames within the process are candidates
for remoting (i.e., not including render frames for other browser tabs).

[servolk, 2016-07-23 01:10:35]

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

https://codereview.chromium.org/2153643002/diff/80001/media/base/mime_util_in...
media/base/mime_util_internal.cc:305: if (video_codec == kCodecH264) {
On 2016/07/22 20:58:16, miu wrote:
> On 2016/07/22 20:11:25, chcunningham wrote:
> > Did you mean to move this stuff out too? I feel like its pretty at home in
the
> > StringToCodec function. Its fine with me if that method decides whats
> ambiguous
> > but still emits the profile/level as out params for us to use with
> > IsSupportedVideoConfig. Maybe I'm missing some reason why this had to come
out
> 
> Agreed. For example, Chromecast currently doesn't support 10-bit profiles, so
> we'll want to be sure to return IsNotSupported in that case.

Yes, Yuri, this is exactly why I've created this change - we want to be able to
express different media capabilities of the different generations of chromecast
devices, but still use the same cast_shell binary on all of them. FYI the next
generation of chromecast is actually going to support 10-bit profiles.

https://codereview.chromium.org/2153643002/diff/80001/media/base/mime_util_in...
media/base/mime_util_internal.cc:305: if (video_codec == kCodecH264) {
On 2016/07/22 20:11:25, chcunningham wrote:
> Did you mean to move this stuff out too? I feel like its pretty at home in the
> StringToCodec function. Its fine with me if that method decides whats
ambiguous
> but still emits the profile/level as out params for us to use with
> IsSupportedVideoConfig. Maybe I'm missing some reason why this had to come out

Chris, yes, I moved it here intentionally, since strictly speaking this is also
logic for deciding whether a codec is supported or not, and has nothing to do
with parsing the codec id per se. I'd prefer all such logic to be concentrated
here is AreSupportedCodecs, rather than spread across different functions.
I hope to get rid of it completely and switch over to relying solely on the
MediaClient API. Since with the current state of things even if media client API
says that codec is supported (returns true), we don't touch the is_ambiguous
value, which means that, for example, the final outcome for VP9 profile 2 is
still going to be 'maybe' for now, instead of 'probably', even if the
MediaClient supports it. Perhaps we should move this logic that analyzes
profiles into the RenderMediaClient implementation? WDYT?

https://codereview.chromium.org/2153643002/diff/80001/media/base/mime_util_in...
media/base/mime_util_internal.cc:344: if (GetMediaClient() && video_codec !=
kUnknownVideoCodec &&
On 2016/07/22 20:11:25, chcunningham wrote:
> Nice

Acknowledged.

https://codereview.chromium.org/2153643002/diff/80001/media/base/mime_util_in...
media/base/mime_util_internal.cc:344: if (GetMediaClient() && video_codec !=
kUnknownVideoCodec &&
On 2016/07/22 20:58:16, miu wrote:
> Is GetMediaClient() global for the whole render process? For media remoting,
> it's possible only a subset of render frames within the process are candidates
> for remoting (i.e., not including render frames for other browser tabs).

Yes, GetMediaClient is global. But my understanding is that media capabilities
are also always global on any given platform (I realize that there might be
other constraints, e.g. chromecast hardware decoder pipeline supports decoding
only one stream at a time, so it's going to be problematic if you create
multiple  HTMLMediaElements/MediaPlayers, but we have other means to deal with
that). How would we need to change this design to accomodate remoting needs
better?

[chcunningham, 2016-07-25 17:56:16]

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

https://codereview.chromium.org/2153643002/diff/80001/media/base/mime_util_in...
media/base/mime_util_internal.cc:305: if (video_codec == kCodecH264) {
> Chris, yes, I moved it here intentionally, since strictly speaking this is
also
> logic for deciding whether a codec is supported or not, and has nothing to do
> with parsing the codec id per se. 

I don't follow. This is all about how to set the is_ambiguous flag, which is to
say "I parsed that codec string, and found it to be misssing or partially
invalid". IMHO this does not imply anything about platform support. (Talking
only about lines 303 - 342; I think 344 - 349 are rightly placed in
AreSupportedCodecs)

[servolk, 2016-07-25 18:42:53]

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

https://codereview.chromium.org/2153643002/diff/80001/media/base/mime_util_in...
media/base/mime_util_internal.cc:305: if (video_codec == kCodecH264) {
On 2016/07/25 17:56:16, chcunningham wrote:
> > Chris, yes, I moved it here intentionally, since strictly speaking this is
> also
> > logic for deciding whether a codec is supported or not, and has nothing to
do
> > with parsing the codec id per se. 
> 
> I don't follow. This is all about how to set the is_ambiguous flag, which is
to
> say "I parsed that codec string, and found it to be misssing or partially
> invalid". IMHO this does not imply anything about platform support. (Talking
> only about lines 303 - 342; I think 344 - 349 are rightly placed in
> AreSupportedCodecs)

This is true only to some extent. The current logic uses the is_ambiguous flag
in some cases as an indication that codec id doesn't have enough information (in
cases like 'avc1' or 'vp9'), but in other cases we do have a codec id that has
all the necessary information (video profile, level), and yet is_ambiguous is
still set to true so that the final outcome of MimeUtil::AreSupportedCodecs is
'maybe'. I think we shouldn't use the is_ambiguous flag in the latter case, and
instead should always return a definitive answer (either '' or 'probably', but
not 'maybe') based on whether the platform actually supports the given
codec/profile or not. But for the sake of moving this CL forward, I can move
this logic back into StringToCodec for now. And then we can change that later in
a separate CL.

[servolk, 2016-07-25 18:53:02]

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

[commit-bot: I haz the power, 2016-07-25 18:53:50]

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

[miu, 2016-07-25 19:29:55]

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

https://codereview.chromium.org/2153643002/diff/80001/media/base/mime_util_in...
media/base/mime_util_internal.cc:344: if (GetMediaClient() && video_codec !=
kUnknownVideoCodec &&
On 2016/07/23 01:10:35, servolk wrote:
> On 2016/07/22 20:58:16, miu wrote:
> > Is GetMediaClient() global for the whole render process? For media remoting,
> > it's possible only a subset of render frames within the process are
candidates
> > for remoting (i.e., not including render frames for other browser tabs).
> 
> Yes, GetMediaClient is global. But my understanding is that media capabilities
> are also always global on any given platform (I realize that there might be

Yes media capabilities are static (since you either have support compiled-in or
not). However, what I'm talking about here is the case of dynamically switching
out that set of media capabilities to reflect what is supported by the platform
that will actually be decoding/rendering the media. In the case of remoting, for
example, we might want to respond to canPlayType() in terms of what a Chromecast
supports, not Chrome; or, maybe we should respond using the intersection of the
capabilities of the two platforms so the video would work on either.

[servolk, 2016-07-25 20:28:17]

On 2016/07/25 19:29:55, miu wrote:
>
https://codereview.chromium.org/2153643002/diff/80001/media/base/mime_util_in...
> File media/base/mime_util_internal.cc (right):
> 
>
https://codereview.chromium.org/2153643002/diff/80001/media/base/mime_util_in...
> media/base/mime_util_internal.cc:344: if (GetMediaClient() && video_codec !=
> kUnknownVideoCodec &&
> On 2016/07/23 01:10:35, servolk wrote:
> > On 2016/07/22 20:58:16, miu wrote:
> > > Is GetMediaClient() global for the whole render process? For media
remoting,
> > > it's possible only a subset of render frames within the process are
> candidates
> > > for remoting (i.e., not including render frames for other browser tabs).
> > 
> > Yes, GetMediaClient is global. But my understanding is that media
capabilities
> > are also always global on any given platform (I realize that there might be
> 
> Yes media capabilities are static (since you either have support compiled-in
or
> not). However, what I'm talking about here is the case of dynamically
switching
> out that set of media capabilities to reflect what is supported by the
platform
> that will actually be decoding/rendering the media. In the case of remoting,
for
> example, we might want to respond to canPlayType() in terms of what a
Chromecast
> supports, not Chrome; or, maybe we should respond using the intersection of
the
> capabilities of the two platforms so the video would work on either.

Ah, ok, I see what you are talking about. I think you can achieve the
flexibility you need by having a custom MediaClient implementation which would
override the IsSupportedVideoConfig function respond according to actual
capabilities of the target platform (and the answers might change dynamically,
that should be fine). The default implementation that I've provided in
render_media_client.cc is just a starting point and for now it happens to be
statically determined at compile-time, but that's only because of how Chrome
currently works. AFAIK there's nothing that stops us from making things more
dynamic there.

[commit-bot: I haz the power, 2016-07-25 22:05:58]

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

[commit-bot: I haz the power, 2016-07-25 22:05:59]

Dry run: This issue passed the CQ dry run.

[miu, 2016-07-25 23:50:42]

lgtm

[servolk, 2016-07-26 01:52:16]

The CQ bit was checked by servolk@chromium.org

[servolk, 2016-07-26 01:52:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
chcunningham@chromium.org
Link to the patchset: https://codereview.chromium.org/2153643002/#ps100001
(title: "Move is_ambiguous flag handling back to StringToCodec")

[commit-bot: I haz the power, 2016-07-26 01:52: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, 2016-07-26 01:55:57]

Description was changed from

==========
Make video codec support logic overridable via MediaClient

This will give content embedders (such as Chromecast) a way to override
supported video codecs logic.

BUG=477103
==========

to

==========
Make video codec support logic overridable via MediaClient

This will give content embedders (such as Chromecast) a way to override
supported video codecs logic.

BUG=477103
==========

[commit-bot: I haz the power, 2016-07-26 01:55:58]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-26 01:57:33]

Description was changed from

==========
Make video codec support logic overridable via MediaClient

This will give content embedders (such as Chromecast) a way to override
supported video codecs logic.

BUG=477103
==========

to

==========
Make video codec support logic overridable via MediaClient

This will give content embedders (such as Chromecast) a way to override
supported video codecs logic.

BUG=477103

Committed: https://crrev.com/7318a94af3fe7cdca14f427339060de5ac6a487a
Cr-Commit-Position: refs/heads/master@{#407681}
==========

[commit-bot: I haz the power, 2016-07-26 01:57:34]

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