Recognize codec id vp9.2 as VP9 profile 2

Recognize codec id vp9.2 as VP9 profile 2

YouTube HDR is now live and YouTube chose to use vp9.2 codec id to
indicate VP9 profile 2 streams in mime type. Chromecast already added
this codec id downstream, but I don't see any issues with moving this
change to upstream now that YT HDR is live.

BUG=671858

[servolk, 2016-12-07 00:56:44]

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

[servolk, 2016-12-07 00:57:06]

Description was changed from

==========
Recognize codec id vp9.2 as VP9 profile 2

YouTube HDR is now live and YouTube chose to use vp9.2 codec id to
indicate VP9 profile 2 streams in mime type. Chromecast already added
this codec id downstream, but I don't see any issues with moving this
change to upstream now that YT HDR is live.

BUG=671858
==========

to

==========
Recognize codec id vp9.2 as VP9 profile 2

YouTube HDR is now live and YouTube chose to use vp9.2 codec id to
indicate VP9 profile 2 streams in mime type. Chromecast already added
this codec id downstream, but I don't see any issues with moving this
change to upstream now that YT HDR is live.

BUG=671858
==========

[servolk, 2016-12-07 00:57:06]

servolk@chromium.org changed reviewers:
+ wolenetz@chromium.org

[commit-bot: I haz the power, 2016-12-07 00:58:01]

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-12-07 01:01:09]

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

[commit-bot: I haz the power, 2016-12-07 01:01:10]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[servolk, 2016-12-07 02:36:17]

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

[commit-bot: I haz the power, 2016-12-07 02:36:45]

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-12-07 03:14:57]

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

[commit-bot: I haz the power, 2016-12-07 03:14:58]

Dry run: 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...)

[wolenetz, 2016-12-07 21:52:33]

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

[wolenetz, 2016-12-07 21:52:33]

R+=ddorwin@, who reviewed similar recently for "new" style vp9-in-mp4.

A couple questions overall:
1) We should probably have a UMA for profile level, not just codec. Do we have
that? If not, please add.
2) Do our browser tests have cases that test vp9.2? (canPlayType and
isTypeSupported tests)
3) @ ddorwin: Are there any other upstream considerations related to this CL?

[ddorwin, 2016-12-07 22:07:44]

Not LGTM. vp9.x will almost certainly not be how other profiles are exposed.

We also need to know that profile 2 is supported before returning "probably".
https://codereview.chromium.org/2388793003/ was an attempt to do that, but we
need to ask the embedder regarding HW decoder support.

There are ongoing discussions on these topics.

[servolk, 2016-12-07 22:21:20]

servolk@chromium.org changed reviewers:
+ hubbe@chromium.org

[servolk, 2016-12-07 22:21:54]

On 2016/12/07 22:07:44, ddorwin wrote:
> Not LGTM. vp9.x will almost certainly not be how other profiles are exposed.
> 
> We also need to know that profile 2 is supported before returning "probably".
> https://codereview.chromium.org/2388793003/ was an attempt to do that, but we
> need to ask the embedder regarding HW decoder support.
> 
> There are ongoing discussions on these topics.

Yes, we know that vp9.2 is going to be replaced by a different id sometime in
the future, but YouTube HDR is already using this codec id and it got published
in YT technical requirements spec (see
https://docs.google.com/document/d/1Ay6o5Y7gmtG3NnqmH1U-MYImpfgzV1OvrQaMi0fgB...,
search for 'vp9.2'). So while it is somewhat unfortunate that the new extended
VP9 codec id format didn't get standardized in time for YT to use it, this means
that vp9.2 codec id is going to be encountered by users visiting YouTube and we
better support it.
Also, please note that with this CL Chrome and Chromium WILL NOT return
"probably" for vp9.2 due to the change made in
content/renderer/media/render_media_client.cc in the default RenderMediaClient
used by Chrome/Chromium. We do have a mechanism now to query actual video codec
capabilities from the platform via MediaClient, so let's use it!

[ddorwin, 2016-12-08 02:08:00]

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

[ddorwin, 2016-12-08 02:08:01]

On 2016/12/07 22:21:54, servolk wrote:
> On 2016/12/07 22:07:44, ddorwin wrote:
> > Not LGTM. vp9.x will almost certainly not be how other profiles are exposed.
> > 
> > We also need to know that profile 2 is supported before returning
"probably".
> > https://codereview.chromium.org/2388793003/ was an attempt to do that, but
we
> > need to ask the embedder regarding HW decoder support.
> > 
> > There are ongoing discussions on these topics.
> 
> Yes, we know that vp9.2 is going to be replaced by a different id sometime in
> the future, but YouTube HDR is already using this codec id and it got
published
> in YT technical requirements spec (see
>
https://docs.google.com/document/d/1Ay6o5Y7gmtG3NnqmH1U-MYImpfgzV1OvrQaMi0fgB...,
> search for 'vp9.2'). So while it is somewhat unfortunate that the new extended
> VP9 codec id format didn't get standardized in time for YT to use it, this
means
> that vp9.2 codec id is going to be encountered by users visiting YouTube and
we
> better support it.
> Also, please note that with this CL Chrome and Chromium WILL NOT return
> "probably" for vp9.2 due to the change made in
> content/renderer/media/render_media_client.cc in the default RenderMediaClient
> used by Chrome/Chromium. We do have a mechanism now to query actual video
codec
> capabilities from the platform via MediaClient, so let's use it!

Exposing 9.2 is a web-visible change, so we must be careful. For example, if we
expose it, sites will start depending on it and other UAs may implement it. It
will be much easier for YouTube to support two strings (and hopefully deprecate
the non-standard one). Since Chrome does not currently support HDR and won't
advertise 9.2 in this CL, it doesn't make sense to take that risk in mainline.

I filed and updated a few issues related to this work.

https://codereview.chromium.org/2550413003/diff/20001/content/renderer/media/...
File content/renderer/media/render_media_client.cc (right):

https://codereview.chromium.org/2550413003/diff/20001/content/renderer/media/...
content/renderer/media/render_media_client.cc:100: if (codec == media::kCodecVP9
&& profile == media::VP9PROFILE_PROFILE2)
This can be in the switch statement. We'll need to move line 105 out.

Also, Chrome doesn't support anything except profile 0.

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

https://codereview.chromium.org/2550413003/diff/20001/media/base/mime_util_in...
media/base/mime_util_internal.cc:590: bool MimeUtil::StringToCodec(const
std::string& mime_type_lower_case,
Checks for valid and support are spread out too much in this and other files. We
need to establish clear responsibilities and expectations.

For example, there should be a clear expectation that IsSupportedVideoConfig()
must be called after this function. (One could even argue that it should be
called from within this function if we are going to return "supported" and not
ambiguous.)

It might be better if we limit this function to actually extracting data from a
string and defer processing it to a separate function. Yes, we then need to
check the Codec in a switch statement, but the responsibilities will be much
clearer. This also means we'll need to add more output parameters to support
some of the longer codec strings, but we're going to need to do that to be able
to query the platform anyway.

We also have IsCodecSupported(), which calls IsCodecSupportedOnPlatform(). Thus,
we have multiple paths doing similar things.

I filed https://crbug.com/672327 to track this.

https://codereview.chromium.org/2550413003/diff/20001/media/base/mime_util_in...
media/base/mime_util_internal.cc:625: // We don't know if the underlying
platform supports these profiles.
This statement does not seem true anymore. IsSupportedVideoConfig() should
answer this. In that case, it would seem that we do not need the profile switch
statement here and (for now - until we support levels, etc.) VP9 is not
ambiguous. (The profile extraction code should reject anything that is not 0-3.)

This assumes that all IsSupportedVideoConfig() implementations are implemented
correctly (only accepting 0 and maybe 2), which should be the case. However,
given the proposed Chrome implementation, we should verify this.

See also https://crbug.com/672327.

https://codereview.chromium.org/2550413003/diff/20001/media/base/video_codecs.cc
File media/base/video_codecs.cc (right):

https://codereview.chromium.org/2550413003/diff/20001/media/base/video_codecs...
media/base/video_codecs.cc:169: if (codec_id == "vp9.2") {
We need to address https://crbug.com/672240.

https://codereview.chromium.org/2550413003/diff/20001/media/filters/stream_pa...
File media/filters/stream_parser_factory.cc (right):

https://codereview.chromium.org/2550413003/diff/20001/media/filters/stream_pa...
media/filters/stream_parser_factory.cc:86: static const CodecInfo kVP92CodecInfo
= {"vp9.2", CodecInfo::VIDEO, NULL,
I think we probably only need one entry for VP9. (I wonder if how "vp9.0" is
supported here.) On the other hand, we might need one for legacy and one for new
(https://crbug.com/672240).

[hubbe, 2017-03-16 17:45:47]

hubbe@chromium.org changed reviewers:
- hubbe@chromium.org

[chcunningham, 2017-03-22 01:03:39]

I think we can close this now. Any objection?

New multi-part vp9 string is checked in. Spec should be published any day now.

[servolk, 2017-03-22 01:24:44]

On 2017/03/22 01:03:39, chcunningham wrote:
> I think we can close this now. Any objection?
> 
> New multi-part vp9 string is checked in. Spec should be published any day now.

Sure, we can close this. For now we are still supporting vp9.2 codec id via a
local downstream Chromecast patch until YT updates their codec ids.