Add 4 VP9 profiles to the media::VideoCodecProfile enum.

Add 4 VP9 profiles to the media::VideoCodecProfile enum.

Currently media/ code doesn't handle any of the VP9 profiles, it just
ignores the existence of different VP9 bitstream profiles and bundles
them all together under VP9PROFILE_ANY.
We know that there are actually 4 different VP9 profiles (see enum
BITSTREAM_PROFILE definition in vp9_enums.h). So at the very least we
should provide 4 values in the media::VideoCodecProfile enum to be able
to distinguish different VP9 profiles.

BUG=592074
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

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

[servolk, 2016-03-04 19:46:20]

Description was changed from

==========
Add 4 VP9 profiles to the media::VideoCodecProfile enum.

Currently media/ code doesn't handle any of the VP9 profiles, it just
ignores the existence of different VP9 bitstream profiles and bundles
them all together under VP9PROFILE_ANY.
We know that there are actually 4 different VP9 profiles (see enum
BITSTREAM_PROFILE definition in vp9_enums.h). So at the very least we
should provide 4 values in the media::VideoCodecProfile enum to be able
to distinguish different VP9 profiles.

BUG=592074
==========

to

==========
Add 4 VP9 profiles to the media::VideoCodecProfile enum.

Currently media/ code doesn't handle any of the VP9 profiles, it just
ignores the existence of different VP9 bitstream profiles and bundles
them all together under VP9PROFILE_ANY.
We know that there are actually 4 different VP9 profiles (see enum
BITSTREAM_PROFILE definition in vp9_enums.h). So at the very least we
should provide 4 values in the media::VideoCodecProfile enum to be able
to distinguish different VP9 profiles.

BUG=592074
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[servolk, 2016-03-09 02:41:33]

Description was changed from

==========
Add 4 VP9 profiles to the media::VideoCodecProfile enum.

Currently media/ code doesn't handle any of the VP9 profiles, it just
ignores the existence of different VP9 bitstream profiles and bundles
them all together under VP9PROFILE_ANY.
We know that there are actually 4 different VP9 profiles (see enum
BITSTREAM_PROFILE definition in vp9_enums.h). So at the very least we
should provide 4 values in the media::VideoCodecProfile enum to be able
to distinguish different VP9 profiles.

BUG=592074
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Add 4 VP9 profiles to the media::VideoCodecProfile enum.

Currently media/ code doesn't handle any of the VP9 profiles, it just
ignores the existence of different VP9 bitstream profiles and bundles
them all together under VP9PROFILE_ANY.
We know that there are actually 4 different VP9 profiles (see enum
BITSTREAM_PROFILE definition in vp9_enums.h). So at the very least we
should provide 4 values in the media::VideoCodecProfile enum to be able
to distinguish different VP9 profiles.

BUG=592074
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[servolk, 2016-03-09 02:41:34]

servolk@chromium.org changed reviewers:
+ asvitkine@chromium.org, jam@chromium.org

[ddorwin, 2016-03-09 17:24:20]

ddorwin@chromium.org changed reviewers:
+ dalecurtis@chromium.org, fgalligan@chromium.org

[ddorwin, 2016-03-09 17:24:22]

This should be reviewed by the media team before adding other reviewers. We need
to make sure the ANY => 0 change doesn't cause any problems. Adding dalecurtis,
but feel free to reassign to someone more familiar with the code.

Also, this CL depends on https://codereview.chromium.org/1677133003/, but I
thought the plan was to populate VP9 profiles then add new profiles after it.

[servolk, 2016-03-09 17:40:55]

On 2016/03/09 17:24:22, ddorwin wrote:
> This should be reviewed by the media team before adding other reviewers. We
need
> to make sure the ANY => 0 change doesn't cause any problems. Adding
dalecurtis,
> but feel free to reassign to someone more familiar with the code.
> 
> Also, this CL depends on https://codereview.chromium.org/1677133003/, but I
> thought the plan was to populate VP9 profiles then add new profiles after it.

Sure. Most changes in this CL are actually splitting the old VP9_ANY profile
code path into 4 essentially identical code paths for handling the 4 different
profiles. There's just a couple of places (e.g.  media/ffmpeg/ffmpeg_common.cc
or media/formats/webm/webm_video_client.cc) where we don't know the actual
profile yet and have to pick just one of the profiles. But I believe that should
be ok for now, since all the other places treat any of the 4 VP9 profiles the
same, and so it makes no difference which profile value we pick in those places
where we do have to make a choice. Of course this might change in the future,
when different profiles are started being treated differently.

Re the dependency on HEVC CL. I thought HEVC CL might go in first (since it's
all done already, just waiting for reviewer approval), so I've based this CL on
the HEVC one to avoid conflicts later. But I can rebase it on top of the tip of
the master, if necessary, shouldn't be too hard to do.

[DaleCurtis, 2016-03-09 18:49:47]

https://codereview.chromium.org/1769593002/diff/180001/remoting/client/plugin...
File remoting/client/plugin/pepper_video_renderer_3d.cc (right):

https://codereview.chromium.org/1769593002/diff/180001/remoting/client/plugin...
remoting/client/plugin/pepper_video_renderer_3d.cc:163: video_profile =
PP_VIDEOPROFILE_VP9_PROFILE3;
Why?

[servolk, 2016-03-09 19:32:34]

https://codereview.chromium.org/1769593002/diff/180001/remoting/client/plugin...
File remoting/client/plugin/pepper_video_renderer_3d.cc (right):

https://codereview.chromium.org/1769593002/diff/180001/remoting/client/plugin...
remoting/client/plugin/pepper_video_renderer_3d.cc:163: video_profile =
PP_VIDEOPROFILE_VP9_PROFILE3;
On 2016/03/09 18:49:47, DaleCurtis wrote:
> Why?

Well, previously we used VP9_ANY here, which meant potentially you could get any
profile, even the highest profile 3. So to minimize a chance of regressions I've
chosen to use profile3 here (if we used profile0 to initialize the
video_decoder_, then potentially it could refuse to accept higher profiles later
on, but I think if we use profile3 to init the decoder, then using lower-profile
streams later on should be fine). I think I should actually do the same in
media/ffmpeg/ffmpeg_common.cc and media/formats/webm/webm_video_client.cc, i.e.
use the highest profile, instead of profile0).

[DaleCurtis, 2016-03-09 21:48:47]

https://codereview.chromium.org/1769593002/diff/180001/remoting/client/plugin...
File remoting/client/plugin/pepper_video_renderer_3d.cc (right):

https://codereview.chromium.org/1769593002/diff/180001/remoting/client/plugin...
remoting/client/plugin/pepper_video_renderer_3d.cc:163: video_profile =
PP_VIDEOPROFILE_VP9_PROFILE3;
On 2016/03/09 at 19:32:33, servolk wrote:
> On 2016/03/09 18:49:47, DaleCurtis wrote:
> > Why?
> 
> Well, previously we used VP9_ANY here, which meant potentially you could get
any profile, even the highest profile 3. So to minimize a chance of regressions
I've chosen to use profile3 here (if we used profile0 to initialize the
video_decoder_, then potentially it could refuse to accept higher profiles later
on, but I think if we use profile3 to init the decoder, then using lower-profile
streams later on should be fine). I think I should actually do the same in
media/ffmpeg/ffmpeg_common.cc and media/formats/webm/webm_video_client.cc, i.e.
use the highest profile, instead of profile0).

I think your reasoning is sound, but was calling out that this is inconsistent
with what you do elsewhere.

[servolk, 2016-03-09 21:52:40]

https://codereview.chromium.org/1769593002/diff/180001/remoting/client/plugin...
File remoting/client/plugin/pepper_video_renderer_3d.cc (right):

https://codereview.chromium.org/1769593002/diff/180001/remoting/client/plugin...
remoting/client/plugin/pepper_video_renderer_3d.cc:163: video_profile =
PP_VIDEOPROFILE_VP9_PROFILE3;
On 2016/03/09 21:48:47, DaleCurtis wrote:
> On 2016/03/09 at 19:32:33, servolk wrote:
> > On 2016/03/09 18:49:47, DaleCurtis wrote:
> > > Why?
> > 
> > Well, previously we used VP9_ANY here, which meant potentially you could get
> any profile, even the highest profile 3. So to minimize a chance of
regressions
> I've chosen to use profile3 here (if we used profile0 to initialize the
> video_decoder_, then potentially it could refuse to accept higher profiles
later
> on, but I think if we use profile3 to init the decoder, then using
lower-profile
> streams later on should be fine). I think I should actually do the same in
> media/ffmpeg/ffmpeg_common.cc and media/formats/webm/webm_video_client.cc,
i.e.
> use the highest profile, instead of profile0).
> 
> I think your reasoning is sound, but was calling out that this is inconsistent
> with what you do elsewhere.

Ok, I've made a change to use profile3 in ffmpeg_common and webm_video_client in
the latest patchset.

[Alexei Svitkine (slow), 2016-03-09 21:53:11]

lgtm for histograms.xml

[DaleCurtis, 2016-03-09 21:55:24]

fgalligan@ can you approve if you're happy with exposing these? I'll stamp after
you're happy.

[ddorwin, 2016-03-09 22:20:04]

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

[ddorwin, 2016-03-09 22:20:05]

https://codereview.chromium.org/1769593002/diff/180001/remoting/client/plugin...
File remoting/client/plugin/pepper_video_renderer_3d.cc (right):

https://codereview.chromium.org/1769593002/diff/180001/remoting/client/plugin...
remoting/client/plugin/pepper_video_renderer_3d.cc:163: video_profile =
PP_VIDEOPROFILE_VP9_PROFILE3;
On 2016/03/09 21:52:40, servolk wrote:
> On 2016/03/09 21:48:47, DaleCurtis wrote:
> > On 2016/03/09 at 19:32:33, servolk wrote:
> > > On 2016/03/09 18:49:47, DaleCurtis wrote:
> > > > Why?
> > > 
> > > Well, previously we used VP9_ANY here, which meant potentially you could
get
> > any profile, even the highest profile 3. So to minimize a chance of
> regressions
> > I've chosen to use profile3 here (if we used profile0 to initialize the
> > video_decoder_, then potentially it could refuse to accept higher profiles
> later
> > on, but I think if we use profile3 to init the decoder, then using
> lower-profile
> > streams later on should be fine). I think I should actually do the same in
> > media/ffmpeg/ffmpeg_common.cc and media/formats/webm/webm_video_client.cc,
> i.e.
> > use the highest profile, instead of profile0).
> > 
> > I think your reasoning is sound, but was calling out that this is
inconsistent
> > with what you do elsewhere.
> 
> Ok, I've made a change to use profile3 in ffmpeg_common and webm_video_client
in
> the latest patchset.

IIUC, there is not currently a way to specify other profiles in WebM. Thus, we
should assume in those other places that it is profile 0.

I don't have any thoughts either way on this file.

Another item to be aware of is that some of these are eventually reported as
UMAs. If we go with something other than PROFILE0, we'll see step functions in
the UMA stats. I'm not saying that's a good reason to pick PROFILE0, but it's
something to be aware of.

[servolk, 2016-03-09 22:27:42]

https://codereview.chromium.org/1769593002/diff/180001/remoting/client/plugin...
File remoting/client/plugin/pepper_video_renderer_3d.cc (right):

https://codereview.chromium.org/1769593002/diff/180001/remoting/client/plugin...
remoting/client/plugin/pepper_video_renderer_3d.cc:163: video_profile =
PP_VIDEOPROFILE_VP9_PROFILE3;
On 2016/03/09 22:20:05, ddorwin wrote:
> On 2016/03/09 21:52:40, servolk wrote:
> > On 2016/03/09 21:48:47, DaleCurtis wrote:
> > > On 2016/03/09 at 19:32:33, servolk wrote:
> > > > On 2016/03/09 18:49:47, DaleCurtis wrote:
> > > > > Why?
> > > > 
> > > > Well, previously we used VP9_ANY here, which meant potentially you could
> get
> > > any profile, even the highest profile 3. So to minimize a chance of
> > regressions
> > > I've chosen to use profile3 here (if we used profile0 to initialize the
> > > video_decoder_, then potentially it could refuse to accept higher profiles
> > later
> > > on, but I think if we use profile3 to init the decoder, then using
> > lower-profile
> > > streams later on should be fine). I think I should actually do the same in
> > > media/ffmpeg/ffmpeg_common.cc and media/formats/webm/webm_video_client.cc,
> > i.e.
> > > use the highest profile, instead of profile0).
> > > 
> > > I think your reasoning is sound, but was calling out that this is
> inconsistent
> > > with what you do elsewhere.
> > 
> > Ok, I've made a change to use profile3 in ffmpeg_common and
webm_video_client
> in
> > the latest patchset.
> 
> IIUC, there is not currently a way to specify other profiles in WebM. Thus, we
> should assume in those other places that it is profile 0.
> 
> I don't have any thoughts either way on this file.
> 
> Another item to be aware of is that some of these are eventually reported as
> UMAs. If we go with something other than PROFILE0, we'll see step functions in
> the UMA stats. I'm not saying that's a good reason to pick PROFILE0, but it's
> something to be aware of.

Yes, currently we won't use any other VP9 profiles other than those hardcoded in
this CL, but hopefully it's going to change soon, as I'm planning to look into
reading VP9 profile from the bitstream (or maybe we can just wait until VP9
profile is also provided in codec private data?
https://bugs.chromium.org/p/chromium/issues/detail?id=592074#c6).
And yes, good point about UMA. Currently all profiles get reported as a value
that is going to be used for just PROFILE0 after this CL. But I think this is
the best we can do, no way to adjust that data retroactively.

[ddorwin, 2016-03-09 23:30:09]

https://codereview.chromium.org/1769593002/diff/180001/remoting/client/plugin...
File remoting/client/plugin/pepper_video_renderer_3d.cc (right):

https://codereview.chromium.org/1769593002/diff/180001/remoting/client/plugin...
remoting/client/plugin/pepper_video_renderer_3d.cc:163: video_profile =
PP_VIDEOPROFILE_VP9_PROFILE3;
On 2016/03/09 22:27:42, servolk wrote:
> On 2016/03/09 22:20:05, ddorwin wrote:
> > On 2016/03/09 21:52:40, servolk wrote:
> > > On 2016/03/09 21:48:47, DaleCurtis wrote:
> > > > On 2016/03/09 at 19:32:33, servolk wrote:
> > > > > On 2016/03/09 18:49:47, DaleCurtis wrote:
> > > > > > Why?
> > > > > 
> > > > > Well, previously we used VP9_ANY here, which meant potentially you
could
> > get
> > > > any profile, even the highest profile 3. So to minimize a chance of
> > > regressions
> > > > I've chosen to use profile3 here (if we used profile0 to initialize the
> > > > video_decoder_, then potentially it could refuse to accept higher
profiles
> > > later
> > > > on, but I think if we use profile3 to init the decoder, then using
> > > lower-profile
> > > > streams later on should be fine). I think I should actually do the same
in
> > > > media/ffmpeg/ffmpeg_common.cc and
media/formats/webm/webm_video_client.cc,
> > > i.e.
> > > > use the highest profile, instead of profile0).
> > > > 
> > > > I think your reasoning is sound, but was calling out that this is
> > inconsistent
> > > > with what you do elsewhere.
> > > 
> > > Ok, I've made a change to use profile3 in ffmpeg_common and
> webm_video_client
> > in
> > > the latest patchset.
> > 
> > IIUC, there is not currently a way to specify other profiles in WebM. Thus,
we
> > should assume in those other places that it is profile 0.
> > 
> > I don't have any thoughts either way on this file.
> > 
> > Another item to be aware of is that some of these are eventually reported as
> > UMAs. If we go with something other than PROFILE0, we'll see step functions
in
> > the UMA stats. I'm not saying that's a good reason to pick PROFILE0, but
it's
> > something to be aware of.
> 
> Yes, currently we won't use any other VP9 profiles other than those hardcoded
in
> this CL, but hopefully it's going to change soon, as I'm planning to look into
> reading VP9 profile from the bitstream (or maybe we can just wait until VP9
> profile is also provided in codec private data?
> https://bugs.chromium.org/p/chromium/issues/detail?id=592074#c6).
> And yes, good point about UMA. Currently all profiles get reported as a value
> that is going to be used for just PROFILE0 after this CL. But I think this is
> the best we can do, no way to adjust that data retroactively.

My point is that any data that does not specify the profile in the private data
must assumed to be profile 0. Thus, PROFILE0 would be the safe choice until we
have such support. Also, all existing content is likely profile 0.

[servolk, 2016-03-09 23:42:10]

https://codereview.chromium.org/1769593002/diff/180001/remoting/client/plugin...
File remoting/client/plugin/pepper_video_renderer_3d.cc (right):

https://codereview.chromium.org/1769593002/diff/180001/remoting/client/plugin...
remoting/client/plugin/pepper_video_renderer_3d.cc:163: video_profile =
PP_VIDEOPROFILE_VP9_PROFILE3;
On 2016/03/09 23:30:09, ddorwin wrote:
> On 2016/03/09 22:27:42, servolk wrote:
> > On 2016/03/09 22:20:05, ddorwin wrote:
> > > On 2016/03/09 21:52:40, servolk wrote:
> > > > On 2016/03/09 21:48:47, DaleCurtis wrote:
> > > > > On 2016/03/09 at 19:32:33, servolk wrote:
> > > > > > On 2016/03/09 18:49:47, DaleCurtis wrote:
> > > > > > > Why?
> > > > > > 
> > > > > > Well, previously we used VP9_ANY here, which meant potentially you
> could
> > > get
> > > > > any profile, even the highest profile 3. So to minimize a chance of
> > > > regressions
> > > > > I've chosen to use profile3 here (if we used profile0 to initialize
the
> > > > > video_decoder_, then potentially it could refuse to accept higher
> profiles
> > > > later
> > > > > on, but I think if we use profile3 to init the decoder, then using
> > > > lower-profile
> > > > > streams later on should be fine). I think I should actually do the
same
> in
> > > > > media/ffmpeg/ffmpeg_common.cc and
> media/formats/webm/webm_video_client.cc,
> > > > i.e.
> > > > > use the highest profile, instead of profile0).
> > > > > 
> > > > > I think your reasoning is sound, but was calling out that this is
> > > inconsistent
> > > > > with what you do elsewhere.
> > > > 
> > > > Ok, I've made a change to use profile3 in ffmpeg_common and
> > webm_video_client
> > > in
> > > > the latest patchset.
> > > 
> > > IIUC, there is not currently a way to specify other profiles in WebM.
Thus,
> we
> > > should assume in those other places that it is profile 0.
> > > 
> > > I don't have any thoughts either way on this file.
> > > 
> > > Another item to be aware of is that some of these are eventually reported
as
> > > UMAs. If we go with something other than PROFILE0, we'll see step
functions
> in
> > > the UMA stats. I'm not saying that's a good reason to pick PROFILE0, but
> it's
> > > something to be aware of.
> > 
> > Yes, currently we won't use any other VP9 profiles other than those
hardcoded
> in
> > this CL, but hopefully it's going to change soon, as I'm planning to look
into
> > reading VP9 profile from the bitstream (or maybe we can just wait until VP9
> > profile is also provided in codec private data?
> > https://bugs.chromium.org/p/chromium/issues/detail?id=592074#c6).
> > And yes, good point about UMA. Currently all profiles get reported as a
value
> > that is going to be used for just PROFILE0 after this CL. But I think this
is
> > the best we can do, no way to adjust that data retroactively.
> 
> My point is that any data that does not specify the profile in the private
data
> must assumed to be profile 0. Thus, PROFILE0 would be the safe choice until we
> have such support. Also, all existing content is likely profile 0.

I agree that most existing content is likely profile 0. But I'm not sure if I
agree that we must assume profile0. All VP9 streams do specify the profile in
bitstream header, it's just that currently we don't read it and so we treat any
stream as being profile VP9_ANY. That wasn't ideal, but that allowed any profile
to work, even profiles 1,2,3. If we used profile0 here, then the video decoder
would be initialized with a set of parameters for profile0 and would most likely
fail to decode higher profiles. But if we use profile3 here, then the decoder
will be ready to decode profile3 and (I'm guessing here) should be also able to
decode lower profiles as well, since all lower profiles seem to be subsets of
higher profiles. So it should keep things working for all VP9 streams and it
shouldn't be any worse than the current situation.
The same reasoning applies to the other two places.

[jam, 2016-03-14 16:25:44]

which files do you want me to review?

[servolk, 2016-03-15 21:54:54]

On 2016/03/14 16:25:44, jam wrote:
> which files do you want me to review?

Hey jam@ I've initially added you on this CL per suggestion of 'git-cl owners'
(I guess that's because this CL touches a lot of files from different dirs). But
per ddorwin@ and dalecurtis@ comments above, we should have media/ and VP9 folks
to take a look at this first. We are still waiting to hear from fgalligan@, so
feel free to postpone reviewing this.

fgalligan@, could you take a look please?

[fgalligan1, 2016-03-23 18:13:48]

On 2016/03/15 21:54:54, servolk wrote:
> On 2016/03/14 16:25:44, jam wrote:
> > which files do you want me to review?
> 
> Hey jam@ I've initially added you on this CL per suggestion of 'git-cl owners'
> (I guess that's because this CL touches a lot of files from different dirs).
But
> per ddorwin@ and dalecurtis@ comments above, we should have media/ and VP9
folks
> to take a look at this first. We are still waiting to hear from fgalligan@, so
> feel free to postpone reviewing this.
> 
> fgalligan@, could you take a look please?

Sorry this fell through the cracks in a busy week.

[fgalligan1, 2016-03-23 18:14:09]

https://codereview.chromium.org/1769593002/diff/260001/content/renderer/peppe...
File content/renderer/pepper/ppb_video_decoder_impl.cc (right):

https://codereview.chromium.org/1769593002/diff/260001/content/renderer/peppe...
content/renderer/pepper/ppb_video_decoder_impl.cc:70: case
PP_VIDEODECODER_VP9PROFILE_PROFILE0:
Will this be fine? Currently this decoder does not have support for vp9.

https://codereview.chromium.org/1769593002/diff/260001/media/ffmpeg/ffmpeg_co...
File media/ffmpeg/ffmpeg_common.cc (right):

https://codereview.chromium.org/1769593002/diff/260001/media/ffmpeg/ffmpeg_co...
media/ffmpeg/ffmpeg_common.cc:465: profile = VP9PROFILE_PROFILE3;
I read the comments on why this should be profile 3 in another file. So this
will only be used to setup a decoder that says it can handle profile 3 and
below? If so then this should be fine.

But if you use this to say the stream is a profile 3 and then use the config to
see if it will play in a decoder, then I think you should make this profile 0.

[servolk, 2016-03-24 18:05:43]

https://codereview.chromium.org/1769593002/diff/260001/content/renderer/peppe...
File content/renderer/pepper/ppb_video_decoder_impl.cc (right):

https://codereview.chromium.org/1769593002/diff/260001/content/renderer/peppe...
content/renderer/pepper/ppb_video_decoder_impl.cc:70: case
PP_VIDEODECODER_VP9PROFILE_PROFILE0:
On 2016/03/23 18:14:08, fgalligan1 wrote:
> Will this be fine? Currently this decoder does not have support for vp9.

Yes, I believe this should be fine. This simply allows translation from
PP_VideoDecoder_Profile into media::VideoCodecProfile (note that all other
profile values are also not checked to be supported here and are merely
translated). The actual check whether this is a supported profile or not is
going to happen in one of the several implementations of
media::VideoDecodeAccelerator interface. E.g. inside
VaapiVideoDecodeAccelerator::Initialize for VAAPI.

https://codereview.chromium.org/1769593002/diff/260001/media/ffmpeg/ffmpeg_co...
File media/ffmpeg/ffmpeg_common.cc (right):

https://codereview.chromium.org/1769593002/diff/260001/media/ffmpeg/ffmpeg_co...
media/ffmpeg/ffmpeg_common.cc:465: profile = VP9PROFILE_PROFILE3;
On 2016/03/23 18:14:09, fgalligan1 wrote:
> I read the comments on why this should be profile 3 in another file. So this
> will only be used to setup a decoder that says it can handle profile 3 and
> below? If so then this should be fine.
> 
> But if you use this to say the stream is a profile 3 and then use the config
to
> see if it will play in a decoder, then I think you should make this profile 0.

I'm not sure I understand the distinction, this is going to be used  both for
deciding which decoder to use and for initializing the decoder later.
I just did a bit more research and unfortunately it looks like current code is
somewhat fragile here, since currently we just send this VP9PROFILE_ANY to all
decoders and leave it up to them to interpret what it means. This is problematic
because for example DXVAVideoDecodeAccelerator doesn't check which profiles are
actually supported
(https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...)
and VAAPI simply maps VP9PROFILE_ANY to VAProfileVP9Profile0
(https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...).
I guess this means that we might get unexpected VP9 playback failures on
DXVA/VAAPI platforms when trying to play VP9 streams with profiles higher than
0.
I've opened crbug.com/597693 for this issue.

Having said this, I now agree that profile0 is probably a better choice here,
since software libvpx decoder ignores profile value completely in
InitializeVpxContext
(https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/vpx_...),
and for accelerated decoders using profile0 is going to be most similar to the
current situation with VP9PROFILE_ANY and thus should minimize the chances of
worsening the current situation. Once this CL lands and we are able to
distinguish various VP9 profiles in Chromium media stack, we can start making
progress on fixing crbug.com/597693.

[servolk, 2016-03-24 18:09:52]

https://codereview.chromium.org/1769593002/diff/180001/remoting/client/plugin...
File remoting/client/plugin/pepper_video_renderer_3d.cc (right):

https://codereview.chromium.org/1769593002/diff/180001/remoting/client/plugin...
remoting/client/plugin/pepper_video_renderer_3d.cc:163: video_profile =
PP_VIDEOPROFILE_VP9_PROFILE3;
On 2016/03/09 23:42:10, servolk wrote:
> On 2016/03/09 23:30:09, ddorwin wrote:
> > On 2016/03/09 22:27:42, servolk wrote:
> > > On 2016/03/09 22:20:05, ddorwin wrote:
> > > > On 2016/03/09 21:52:40, servolk wrote:
> > > > > On 2016/03/09 21:48:47, DaleCurtis wrote:
> > > > > > On 2016/03/09 at 19:32:33, servolk wrote:
> > > > > > > On 2016/03/09 18:49:47, DaleCurtis wrote:
> > > > > > > > Why?
> > > > > > > 
> > > > > > > Well, previously we used VP9_ANY here, which meant potentially you
> > could
> > > > get
> > > > > > any profile, even the highest profile 3. So to minimize a chance of
> > > > > regressions
> > > > > > I've chosen to use profile3 here (if we used profile0 to initialize
> the
> > > > > > video_decoder_, then potentially it could refuse to accept higher
> > profiles
> > > > > later
> > > > > > on, but I think if we use profile3 to init the decoder, then using
> > > > > lower-profile
> > > > > > streams later on should be fine). I think I should actually do the
> same
> > in
> > > > > > media/ffmpeg/ffmpeg_common.cc and
> > media/formats/webm/webm_video_client.cc,
> > > > > i.e.
> > > > > > use the highest profile, instead of profile0).
> > > > > > 
> > > > > > I think your reasoning is sound, but was calling out that this is
> > > > inconsistent
> > > > > > with what you do elsewhere.
> > > > > 
> > > > > Ok, I've made a change to use profile3 in ffmpeg_common and
> > > webm_video_client
> > > > in
> > > > > the latest patchset.
> > > > 
> > > > IIUC, there is not currently a way to specify other profiles in WebM.
> Thus,
> > we
> > > > should assume in those other places that it is profile 0.
> > > > 
> > > > I don't have any thoughts either way on this file.
> > > > 
> > > > Another item to be aware of is that some of these are eventually
reported
> as
> > > > UMAs. If we go with something other than PROFILE0, we'll see step
> functions
> > in
> > > > the UMA stats. I'm not saying that's a good reason to pick PROFILE0, but
> > it's
> > > > something to be aware of.
> > > 
> > > Yes, currently we won't use any other VP9 profiles other than those
> hardcoded
> > in
> > > this CL, but hopefully it's going to change soon, as I'm planning to look
> into
> > > reading VP9 profile from the bitstream (or maybe we can just wait until
VP9
> > > profile is also provided in codec private data?
> > > https://bugs.chromium.org/p/chromium/issues/detail?id=592074#c6).
> > > And yes, good point about UMA. Currently all profiles get reported as a
> value
> > > that is going to be used for just PROFILE0 after this CL. But I think this
> is
> > > the best we can do, no way to adjust that data retroactively.
> > 
> > My point is that any data that does not specify the profile in the private
> data
> > must assumed to be profile 0. Thus, PROFILE0 would be the safe choice until
we
> > have such support. Also, all existing content is likely profile 0.
> 
> I agree that most existing content is likely profile 0. But I'm not sure if I
> agree that we must assume profile0. All VP9 streams do specify the profile in
> bitstream header, it's just that currently we don't read it and so we treat
any
> stream as being profile VP9_ANY. That wasn't ideal, but that allowed any
profile
> to work, even profiles 1,2,3. If we used profile0 here, then the video decoder
> would be initialized with a set of parameters for profile0 and would most
likely
> fail to decode higher profiles. But if we use profile3 here, then the decoder
> will be ready to decode profile3 and (I'm guessing here) should be also able
to
> decode lower profiles as well, since all lower profiles seem to be subsets of
> higher profiles. So it should keep things working for all VP9 streams and it
> shouldn't be any worse than the current situation.
> The same reasoning applies to the other two places.

I've changed this to profile0 now, see the explanations at
https://codereview.chromium.org/1769593002/#msg23

[fgalligan1, 2016-03-25 22:30:26]

lgtm

[fgalligan1, 2016-03-25 22:30:29]

lgtm

[ddorwin, 2016-03-25 22:51:22]

https://codereview.chromium.org/1769593002/diff/280001/content/common/gpu/med...
File content/common/gpu/media/vaapi_wrapper.cc (right):

https://codereview.chromium.org/1769593002/diff/280001/content/common/gpu/med...
content/common/gpu/media/vaapi_wrapper.cc:128: // TODO(servolk): Need to add VP9
profiles 1,2,3 here after rolling
Should we have a bug for this?

https://codereview.chromium.org/1769593002/diff/280001/content/renderer/peppe...
File content/renderer/pepper/video_encoder_shim.cc (right):

https://codereview.chromium.org/1769593002/diff/280001/content/renderer/peppe...
content/renderer/pepper/video_encoder_shim.cc:81: // TODO(servolk): These
numbers should be adjusted for different profiles.
Bug?

https://codereview.chromium.org/1769593002/diff/280001/media/ffmpeg/ffmpeg_co...
File media/ffmpeg/ffmpeg_common.cc (right):

https://codereview.chromium.org/1769593002/diff/280001/media/ffmpeg/ffmpeg_co...
media/ffmpeg/ffmpeg_common.cc:464: // TODO(servolk): Find a way to obtain actual
VP9 profile from FFmpeg.
Bug?

https://codereview.chromium.org/1769593002/diff/280001/media/formats/webm/web...
File media/formats/webm/webm_video_client.cc (right):

https://codereview.chromium.org/1769593002/diff/280001/media/formats/webm/web...
media/formats/webm/webm_video_client.cc:47: // TODO(servolk): Find a way to read
actual VP9 profile from WebM.
ditto

https://codereview.chromium.org/1769593002/diff/280001/ppapi/tests/test_video...
File ppapi/tests/test_video_encoder.cc (right):

https://codereview.chromium.org/1769593002/diff/280001/ppapi/tests/test_video...
ppapi/tests/test_video_encoder.cc:30: std::string
TestVideoEncoder::TestAvailableCodecs() {
Should this test have ifdef'd tests for H.264? (Not in this CL, but is this
something missing?)

https://codereview.chromium.org/1769593002/diff/280001/ppapi/tests/test_video...
ppapi/tests/test_video_encoder.cc:106: return
TestInitializeCodec(PP_VIDEOPROFILE_VP9_PROFILE2);
s/2/3/

[servolk, 2016-03-25 23:52:02]

https://codereview.chromium.org/1769593002/diff/280001/content/common/gpu/med...
File content/common/gpu/media/vaapi_wrapper.cc (right):

https://codereview.chromium.org/1769593002/diff/280001/content/common/gpu/med...
content/common/gpu/media/vaapi_wrapper.cc:128: // TODO(servolk): Need to add VP9
profiles 1,2,3 here after rolling
On 2016/03/25 22:51:21, ddorwin wrote:
> Should we have a bug for this?

Done.

https://codereview.chromium.org/1769593002/diff/280001/content/renderer/peppe...
File content/renderer/pepper/video_encoder_shim.cc (right):

https://codereview.chromium.org/1769593002/diff/280001/content/renderer/peppe...
content/renderer/pepper/video_encoder_shim.cc:81: // TODO(servolk): These
numbers should be adjusted for different profiles.
On 2016/03/25 22:51:21, ddorwin wrote:
> Bug?

Done.

https://codereview.chromium.org/1769593002/diff/280001/media/ffmpeg/ffmpeg_co...
File media/ffmpeg/ffmpeg_common.cc (right):

https://codereview.chromium.org/1769593002/diff/280001/media/ffmpeg/ffmpeg_co...
media/ffmpeg/ffmpeg_common.cc:464: // TODO(servolk): Find a way to obtain actual
VP9 profile from FFmpeg.
On 2016/03/25 22:51:22, ddorwin wrote:
> Bug?

I think we can reuse crbug.com/592074 for this, let me know if you'd prefer a
separate bug.

https://codereview.chromium.org/1769593002/diff/280001/media/formats/webm/web...
File media/formats/webm/webm_video_client.cc (right):

https://codereview.chromium.org/1769593002/diff/280001/media/formats/webm/web...
media/formats/webm/webm_video_client.cc:47: // TODO(servolk): Find a way to read
actual VP9 profile from WebM.
On 2016/03/25 22:51:22, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/1769593002/diff/280001/ppapi/tests/test_video...
File ppapi/tests/test_video_encoder.cc (right):

https://codereview.chromium.org/1769593002/diff/280001/ppapi/tests/test_video...
ppapi/tests/test_video_encoder.cc:30: std::string
TestVideoEncoder::TestAvailableCodecs() {
On 2016/03/25 22:51:22, ddorwin wrote:
> Should this test have ifdef'd tests for H.264? (Not in this CL, but is this
> something missing?)

The only H.264 encoder that I could find in Chrome is webrtc::H264Encoder and
that doesn't seem to work through PPAPI, so H.264 tests should be added only
after adding PPAPI-compatible H.264 encoder.

https://codereview.chromium.org/1769593002/diff/280001/ppapi/tests/test_video...
ppapi/tests/test_video_encoder.cc:106: return
TestInitializeCodec(PP_VIDEOPROFILE_VP9_PROFILE2);
On 2016/03/25 22:51:22, ddorwin wrote:
> s/2/3/

Done.

[ddorwin, 2016-03-26 00:45:08]

lgtm

Thanks.

https://codereview.chromium.org/1769593002/diff/280001/ppapi/tests/test_video...
File ppapi/tests/test_video_encoder.cc (right):

https://codereview.chromium.org/1769593002/diff/280001/ppapi/tests/test_video...
ppapi/tests/test_video_encoder.cc:30: std::string
TestVideoEncoder::TestAvailableCodecs() {
On 2016/03/25 23:52:01, servolk wrote:
> On 2016/03/25 22:51:22, ddorwin wrote:
> > Should this test have ifdef'd tests for H.264? (Not in this CL, but is this
> > something missing?)
> 
> The only H.264 encoder that I could find in Chrome is webrtc::H264Encoder and
> that doesn't seem to work through PPAPI, so H.264 tests should be added only
> after adding PPAPI-compatible H.264 encoder.

Thanks for checking.

[servolk, 2016-03-28 18:32:55]

servolk@chromium.org changed reviewers:
+ garykac@chromium.org, halliwell@chromium.org, raymes@chromium.org,
sievers@chromium.org

[servolk, 2016-03-28 18:33:16]

On 2016/03/26 00:45:08, ddorwin wrote:
> lgtm
> 
> Thanks.
> 
>
https://codereview.chromium.org/1769593002/diff/280001/ppapi/tests/test_video...
> File ppapi/tests/test_video_encoder.cc (right):
> 
>
https://codereview.chromium.org/1769593002/diff/280001/ppapi/tests/test_video...
> ppapi/tests/test_video_encoder.cc:30: std::string
> TestVideoEncoder::TestAvailableCodecs() {
> On 2016/03/25 23:52:01, servolk wrote:
> > On 2016/03/25 22:51:22, ddorwin wrote:
> > > Should this test have ifdef'd tests for H.264? (Not in this CL, but is
this
> > > something missing?)
> > 
> > The only H.264 encoder that I could find in Chrome is webrtc::H264Encoder
and
> > that doesn't seem to work through PPAPI, so H.264 tests should be added only
> > after adding PPAPI-compatible H.264 encoder.
> 
> Thanks for checking.

Thanks for review, David. Could you please also take a quick look at a separate
CL to add VP9 profiles to content_decryption_module.h?
https://codereview.chromium.org/1782483002/

+raymes@ for ppapi/
+sievers@ for gpu/ and content/
+halliwell@ for chromecast/
+garykac@ for remoting/

[halliwell, 2016-03-28 18:38:26]

On 2016/03/28 18:33:16, servolk wrote:
> On 2016/03/26 00:45:08, ddorwin wrote:
> > lgtm
> > 
> > Thanks.
> > 
> >
>
https://codereview.chromium.org/1769593002/diff/280001/ppapi/tests/test_video...
> > File ppapi/tests/test_video_encoder.cc (right):
> > 
> >
>
https://codereview.chromium.org/1769593002/diff/280001/ppapi/tests/test_video...
> > ppapi/tests/test_video_encoder.cc:30: std::string
> > TestVideoEncoder::TestAvailableCodecs() {
> > On 2016/03/25 23:52:01, servolk wrote:
> > > On 2016/03/25 22:51:22, ddorwin wrote:
> > > > Should this test have ifdef'd tests for H.264? (Not in this CL, but is
> this
> > > > something missing?)
> > > 
> > > The only H.264 encoder that I could find in Chrome is webrtc::H264Encoder
> and
> > > that doesn't seem to work through PPAPI, so H.264 tests should be added
only
> > > after adding PPAPI-compatible H.264 encoder.
> > 
> > Thanks for checking.
> 
> Thanks for review, David. Could you please also take a quick look at a
separate
> CL to add VP9 profiles to content_decryption_module.h?
> https://codereview.chromium.org/1782483002/
> 
> +raymes@ for ppapi/
> +sievers@ for gpu/ and content/
> +halliwell@ for chromecast/
> +garykac@ for remoting/

chromecast/ lgtm BUT the chromecast/public header enum changes *must* go into
1.19.  We are not able to change these headers in 1.20.  So either cherry-pick
this CL or make a smaller change that just makes 1.19 public headers the same as
this.

[ddorwin, 2016-03-28 18:45:44]

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

[ddorwin, 2016-03-28 18:45:46]

xhwang, please see comments below and review the two adapter files.

https://codereview.chromium.org/1769593002/diff/300001/media/cdm/ppapi/ppapi_...
File media/cdm/ppapi/ppapi_cdm_adapter.cc (right):

https://codereview.chromium.org/1769593002/diff/300001/media/cdm/ppapi/ppapi_...
media/cdm/ppapi/ppapi_cdm_adapter.cc:199: // TODO(servolk): See
crbug.com/592074. We'll need to update this code to
We might need to be smarter about how we implement this and changes in
content_decryption_module.h [1] to avoid causing problems for older CDMs.

This should probably be part of
https://bugs.chromium.org/p/chromium/issues/detail?id=570216.

[1] https://codereview.chromium.org/1782483002/

https://codereview.chromium.org/1769593002/diff/300001/media/cdm/ppapi/ppapi_...
media/cdm/ppapi/ppapi_cdm_adapter.cc:202: // For now return kProfileNotNeeded to
avoid breaking unit tests.
Which unit tests? How does this avoid breaking them? It changes the return value
for VP9 from kUnknownVideoCodecProfile to kProfileNotNeeded.

[servolk, 2016-03-28 19:03:02]

https://codereview.chromium.org/1769593002/diff/300001/media/cdm/ppapi/ppapi_...
File media/cdm/ppapi/ppapi_cdm_adapter.cc (right):

https://codereview.chromium.org/1769593002/diff/300001/media/cdm/ppapi/ppapi_...
media/cdm/ppapi/ppapi_cdm_adapter.cc:199: // TODO(servolk): See
crbug.com/592074. We'll need to update this code to
On 2016/03/28 18:45:46, ddorwin wrote:
> We might need to be smarter about how we implement this and changes in
> content_decryption_module.h [1] to avoid causing problems for older CDMs.
> 
> This should probably be part of
> https://bugs.chromium.org/p/chromium/issues/detail?id=570216.
> 
> [1] https://codereview.chromium.org/1782483002/

Acknowledged.

https://codereview.chromium.org/1769593002/diff/300001/media/cdm/ppapi/ppapi_...
media/cdm/ppapi/ppapi_cdm_adapter.cc:202: // For now return kProfileNotNeeded to
avoid breaking unit tests.
On 2016/03/28 18:45:45, ddorwin wrote:
> Which unit tests? How does this avoid breaking them? It changes the return
value
> for VP9 from kUnknownVideoCodecProfile to kProfileNotNeeded.

In earlier patchsets #7,#8 of this CL trybots indicated failures of
PPAPINaClPNaClTest.VideoEncoder, PPAPINaClNewlibTest.VideoEncoder and
OutOfProcessPPAPITest.VideoEncoder (It's been ~3 weeks ago, so not all trybot
results are available now, but for example take a look at
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_dbg...).
If I remember correctly the problem was that we used to return
kUnknownVideoCodecProfile here when trying to initialialize PPAPI VP9 decoders
and that failed. But returning kProfileNotNeeded is fine, since then the profile
value is just ignored in PPAPI. And later we'll replace it with proper VP9
profiles once we add them to CDM interface.

[raymes, 2016-03-29 01:51:49]

I defer to bbudge for ppapi who probably knows this code a bit better than me.

https://codereview.chromium.org/1769593002/diff/300001/ppapi/api/pp_codecs.idl
File ppapi/api/pp_codecs.idl (left):

https://codereview.chromium.org/1769593002/diff/300001/ppapi/api/pp_codecs.id...
ppapi/api/pp_codecs.idl:22: PP_VIDEOPROFILE_VP9_ANY = 12,
This looks like an API change. Is there potential for this to break existing
apps that use this?

[raymes, 2016-03-29 01:52:11]

raymes@chromium.org changed reviewers:
+ bbudge@chromium.org

[raymes, 2016-03-29 01:52:12]

I defer to bbudge for ppapi who probably knows this code a bit better than me.

[bbudge, 2016-03-29 21:42:13]

https://codereview.chromium.org/1769593002/diff/300001/ppapi/api/dev/pp_video...
File ppapi/api/dev/pp_video_dev.idl (right):

https://codereview.chromium.org/1769593002/diff/300001/ppapi/api/dev/pp_video...
ppapi/api/dev/pp_video_dev.idl:38: PP_VIDEODECODER_VP9PROFILE_PROFILE3 = 16
You might be able to get away with modifying the API here, since this is a 'Dev'
API, used only by Flash.

https://codereview.chromium.org/1769593002/diff/300001/ppapi/api/pp_codecs.idl
File ppapi/api/pp_codecs.idl (left):

https://codereview.chromium.org/1769593002/diff/300001/ppapi/api/pp_codecs.id...
ppapi/api/pp_codecs.idl:22: PP_VIDEOPROFILE_VP9_ANY = 12,
On 2016/03/29 01:51:49, raymes wrote:
> This looks like an API change. Is there potential for this to break existing
> apps that use this?

This is used by two Stable APIs (PPB_VideoDecoder, PPB_VideoEncoder), so in
general you have to create a new version of those APIs to modify any methods, or
any structs or enums used by those methods. You can see how new versions of
functions were added for those APIs in their IDL files (look for [version =
0.x]).

Probably the easiest thing for you to do is not modify Pepper. I don't know of
any plugin clients that are requesting more VP9 profiles.

https://codereview.chromium.org/1769593002/diff/300001/ppapi/api/private/pp_c...
File ppapi/api/private/pp_content_decryptor.idl (right):

https://codereview.chromium.org/1769593002/diff/300001/ppapi/api/private/pp_c...
ppapi/api/private/pp_content_decryptor.idl:357:
PP_VIDEOCODECPROFILE_VP9_PROFILE3 = 12
Since this is Private, you might be able to avoid versioning here too.

[servolk, 2016-03-30 23:24:17]

servolk@chromium.org changed reviewers:
- garykac@chromium.org

[servolk, 2016-03-30 23:28:14]

On 2016/03/29 21:42:13, bbudge wrote:
>
https://codereview.chromium.org/1769593002/diff/300001/ppapi/api/dev/pp_video...
> File ppapi/api/dev/pp_video_dev.idl (right):
> 
>
https://codereview.chromium.org/1769593002/diff/300001/ppapi/api/dev/pp_video...
> ppapi/api/dev/pp_video_dev.idl:38: PP_VIDEODECODER_VP9PROFILE_PROFILE3 = 16
> You might be able to get away with modifying the API here, since this is a
'Dev'
> API, used only by Flash.
> 
> https://codereview.chromium.org/1769593002/diff/300001/ppapi/api/pp_codecs.idl
> File ppapi/api/pp_codecs.idl (left):
> 
>
https://codereview.chromium.org/1769593002/diff/300001/ppapi/api/pp_codecs.id...
> ppapi/api/pp_codecs.idl:22: PP_VIDEOPROFILE_VP9_ANY = 12,
> On 2016/03/29 01:51:49, raymes wrote:
> > This looks like an API change. Is there potential for this to break existing
> > apps that use this?
> 
> This is used by two Stable APIs (PPB_VideoDecoder, PPB_VideoEncoder), so in
> general you have to create a new version of those APIs to modify any methods,
or
> any structs or enums used by those methods. You can see how new versions of
> functions were added for those APIs in their IDL files (look for [version =
> 0.x]).
> 
> Probably the easiest thing for you to do is not modify Pepper. I don't know of
> any plugin clients that are requesting more VP9 profiles.
> 
>
https://codereview.chromium.org/1769593002/diff/300001/ppapi/api/private/pp_c...
> File ppapi/api/private/pp_content_decryptor.idl (right):
> 
>
https://codereview.chromium.org/1769593002/diff/300001/ppapi/api/private/pp_c...
> ppapi/api/private/pp_content_decryptor.idl:357:
> PP_VIDEOCODECPROFILE_VP9_PROFILE3 = 12
> Since this is Private, you might be able to avoid versioning here too.

Ok, yes, I guess we can leave PPAPI unchanged for now to ensure perfect
backward-compatibility and update it later in a separate CL if any PPAPI client
ever needs to recognize a specific VP9 profile. So in the latest patchset there
are no longer any changes under PPAPI. I have updated the code under
content/renderer/pepper to map media::VideoCodecProfile::VP9 profiles to PPAPI
VP9_ANY and PPAPI VP9_ANY to media::VP9_profile0.

[servolk, 2016-03-31 17:04:59]

On 2016/03/30 23:28:14, servolk wrote:
> On 2016/03/29 21:42:13, bbudge wrote:
> >
>
https://codereview.chromium.org/1769593002/diff/300001/ppapi/api/dev/pp_video...
> > File ppapi/api/dev/pp_video_dev.idl (right):
> > 
> >
>
https://codereview.chromium.org/1769593002/diff/300001/ppapi/api/dev/pp_video...
> > ppapi/api/dev/pp_video_dev.idl:38: PP_VIDEODECODER_VP9PROFILE_PROFILE3 = 16
> > You might be able to get away with modifying the API here, since this is a
> 'Dev'
> > API, used only by Flash.
> > 
> >
https://codereview.chromium.org/1769593002/diff/300001/ppapi/api/pp_codecs.idl
> > File ppapi/api/pp_codecs.idl (left):
> > 
> >
>
https://codereview.chromium.org/1769593002/diff/300001/ppapi/api/pp_codecs.id...
> > ppapi/api/pp_codecs.idl:22: PP_VIDEOPROFILE_VP9_ANY = 12,
> > On 2016/03/29 01:51:49, raymes wrote:
> > > This looks like an API change. Is there potential for this to break
existing
> > > apps that use this?
> > 
> > This is used by two Stable APIs (PPB_VideoDecoder, PPB_VideoEncoder), so in
> > general you have to create a new version of those APIs to modify any
methods,
> or
> > any structs or enums used by those methods. You can see how new versions of
> > functions were added for those APIs in their IDL files (look for [version =
> > 0.x]).
> > 
> > Probably the easiest thing for you to do is not modify Pepper. I don't know
of
> > any plugin clients that are requesting more VP9 profiles.
> > 
> >
>
https://codereview.chromium.org/1769593002/diff/300001/ppapi/api/private/pp_c...
> > File ppapi/api/private/pp_content_decryptor.idl (right):
> > 
> >
>
https://codereview.chromium.org/1769593002/diff/300001/ppapi/api/private/pp_c...
> > ppapi/api/private/pp_content_decryptor.idl:357:
> > PP_VIDEOCODECPROFILE_VP9_PROFILE3 = 12
> > Since this is Private, you might be able to avoid versioning here too.
> 
> Ok, yes, I guess we can leave PPAPI unchanged for now to ensure perfect
> backward-compatibility and update it later in a separate CL if any PPAPI
client
> ever needs to recognize a specific VP9 profile. So in the latest patchset
there
> are no longer any changes under PPAPI. I have updated the code under
> content/renderer/pepper to map media::VideoCodecProfile::VP9 profiles to PPAPI
> VP9_ANY and PPAPI VP9_ANY to media::VP9_profile0.

Ping.
sievers@ could you please take a look at content/common/gpu/ and gpu/ ?
bbudge@ are you ok with these content/renderer/pepper/ changes?

[no sievers, 2016-03-31 17:20:42]

https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
File content/renderer/pepper/pepper_video_decoder_host.cc (right):

https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
content/renderer/pepper/pepper_video_decoder_host.cc:68: return
media::VP9PROFILE_PROFILE0;
hmm why is this right?

https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
File content/renderer/pepper/pepper_video_encoder_host.cc (right):

https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
content/renderer/pepper/pepper_video_encoder_host.cc:83: return
media::VP9PROFILE_PROFILE0;
and here

[bbudge, 2016-03-31 17:24:44]

https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
File content/renderer/pepper/pepper_video_encoder_host.cc (right):

https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
content/renderer/pepper/pepper_video_encoder_host.cc:118: case
media::VP9PROFILE_PROFILE3:
Don't accept profiles 1-3 here.

https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
content/renderer/pepper/pepper_video_encoder_host.cc:459: for
(media::VideoEncodeAccelerator::SupportedProfile profile : profiles) {
skip profiles 1-3 here.

https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
File content/renderer/pepper/video_encoder_shim.cc (right):

https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
content/renderer/pepper/video_encoder_shim.cc:78: case
media::VP9PROFILE_PROFILE1:
profiles 1-3 shouldn't be possible here, since the caller is the host, which
only accepts the pepper 'vp9 any' profile.

https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
content/renderer/pepper/video_encoder_shim.cc:205: output_profile ==
media::VP9PROFILE_PROFILE1 ||
ditto.

https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
content/renderer/pepper/video_encoder_shim.cc:235: output_profile ==
media::VP9PROFILE_PROFILE1 ||
ditto

https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
content/renderer/pepper/video_encoder_shim.cc:408: profile.profile =
media::VP9PROFILE_PROFILE1;
It's confusing to the client to return multiple vp9 profiles, when only profile
0 would be accessible to them.

[servolk, 2016-03-31 17:26:21]

https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
File content/renderer/pepper/pepper_video_decoder_host.cc (right):

https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
content/renderer/pepper/pepper_video_decoder_host.cc:68: return
media::VP9PROFILE_PROFILE0;
On 2016/03/31 17:20:42, sievers wrote:
> hmm why is this right?

This isn't 100% right, but that's essentially how we treat it today anyway, e.g.
look at
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu....
In the previous patchset I replaced PP_VIDEOPROFILE_VP9_ANY with 4 different
profile values, but then bbudge@ was concerned about changing PPAPI. So I've
decided to keep PPAPI unchanged for now (see
https://codereview.chromium.org/1769593002/#msg41). We can update it later if
necessary (if PPAPI decoders ever start caring about different VP9 profiles).

[no sievers, 2016-03-31 17:28:06]

lgtm

[servolk, 2016-03-31 17:42:05]

https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
File content/renderer/pepper/pepper_video_encoder_host.cc (right):

https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
content/renderer/pepper/pepper_video_encoder_host.cc:118: case
media::VP9PROFILE_PROFILE3:
On 2016/03/31 17:24:43, bbudge wrote:
> Don't accept profiles 1-3 here.

Done.

https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
content/renderer/pepper/pepper_video_encoder_host.cc:459: for
(media::VideoEncodeAccelerator::SupportedProfile profile : profiles) {
On 2016/03/31 17:24:44, bbudge wrote:
> skip profiles 1-3 here.

Done.

https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
File content/renderer/pepper/video_encoder_shim.cc (right):

https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
content/renderer/pepper/video_encoder_shim.cc:78: case
media::VP9PROFILE_PROFILE1:
On 2016/03/31 17:24:44, bbudge wrote:
> profiles 1-3 shouldn't be possible here, since the caller is the host, which
> only accepts the pepper 'vp9 any' profile.

Done.

https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
content/renderer/pepper/video_encoder_shim.cc:205: output_profile ==
media::VP9PROFILE_PROFILE1 ||
On 2016/03/31 17:24:44, bbudge wrote:
> ditto.

Done.

https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
content/renderer/pepper/video_encoder_shim.cc:235: output_profile ==
media::VP9PROFILE_PROFILE1 ||
On 2016/03/31 17:24:44, bbudge wrote:
> ditto

Done.

https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
content/renderer/pepper/video_encoder_shim.cc:408: profile.profile =
media::VP9PROFILE_PROFILE1;
On 2016/03/31 17:24:44, bbudge wrote:
> It's confusing to the client to return multiple vp9 profiles, when only
profile
> 0 would be accessible to them.

Done.

[servolk, 2016-03-31 17:44:22]

On 2016/03/31 17:24:44, bbudge wrote:
>
https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
> File content/renderer/pepper/pepper_video_encoder_host.cc (right):
> 
>
https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
> content/renderer/pepper/pepper_video_encoder_host.cc:118: case
> media::VP9PROFILE_PROFILE3:
> Don't accept profiles 1-3 here.
> 
>
https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
> content/renderer/pepper/pepper_video_encoder_host.cc:459: for
> (media::VideoEncodeAccelerator::SupportedProfile profile : profiles) {
> skip profiles 1-3 here.
> 
>
https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
> File content/renderer/pepper/video_encoder_shim.cc (right):
> 
>
https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
> content/renderer/pepper/video_encoder_shim.cc:78: case
> media::VP9PROFILE_PROFILE1:
> profiles 1-3 shouldn't be possible here, since the caller is the host, which
> only accepts the pepper 'vp9 any' profile.
> 
>
https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
> content/renderer/pepper/video_encoder_shim.cc:205: output_profile ==
> media::VP9PROFILE_PROFILE1 ||
> ditto.
> 
>
https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
> content/renderer/pepper/video_encoder_shim.cc:235: output_profile ==
> media::VP9PROFILE_PROFILE1 ||
> ditto
> 
>
https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
> content/renderer/pepper/video_encoder_shim.cc:408: profile.profile =
> media::VP9PROFILE_PROFILE1;
> It's confusing to the client to return multiple vp9 profiles, when only
profile
> 0 would be accessible to them.

Ok, after applying all these changes in the latest patchset, now essentially
PP_VIDEOPROFILE_VP9_ANY means media::VP9PROFILE_PROFILE0, so should we rename 
PP_VIDEOPROFILE_VP9_ANY into PP_VIDEOPROFILE_VP9_PROFILE0 to make it less
confusing?

[bbudge, 2016-03-31 17:46:26]

I'm assuming that Profile 0 is a baseline that is always available if vp9 is
supported, and that higher profiles are better but not always available. The
important thing is that always mapping to Profile 0 doesn't reduce the quality
or performance for current plugins.

There aren't a lot of uses of these APIs (via UMA stats).

LGTM

[bbudge, 2016-03-31 17:49:21]

On 2016/03/31 17:44:22, servolk wrote:
> On 2016/03/31 17:24:44, bbudge wrote:
> >
>
https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
> > File content/renderer/pepper/pepper_video_encoder_host.cc (right):
> > 
> >
>
https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
> > content/renderer/pepper/pepper_video_encoder_host.cc:118: case
> > media::VP9PROFILE_PROFILE3:
> > Don't accept profiles 1-3 here.
> > 
> >
>
https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
> > content/renderer/pepper/pepper_video_encoder_host.cc:459: for
> > (media::VideoEncodeAccelerator::SupportedProfile profile : profiles) {
> > skip profiles 1-3 here.
> > 
> >
>
https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
> > File content/renderer/pepper/video_encoder_shim.cc (right):
> > 
> >
>
https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
> > content/renderer/pepper/video_encoder_shim.cc:78: case
> > media::VP9PROFILE_PROFILE1:
> > profiles 1-3 shouldn't be possible here, since the caller is the host, which
> > only accepts the pepper 'vp9 any' profile.
> > 
> >
>
https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
> > content/renderer/pepper/video_encoder_shim.cc:205: output_profile ==
> > media::VP9PROFILE_PROFILE1 ||
> > ditto.
> > 
> >
>
https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
> > content/renderer/pepper/video_encoder_shim.cc:235: output_profile ==
> > media::VP9PROFILE_PROFILE1 ||
> > ditto
> > 
> >
>
https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
> > content/renderer/pepper/video_encoder_shim.cc:408: profile.profile =
> > media::VP9PROFILE_PROFILE1;
> > It's confusing to the client to return multiple vp9 profiles, when only
> profile
> > 0 would be accessible to them.
> 
> Ok, after applying all these changes in the latest patchset, now essentially
> PP_VIDEOPROFILE_VP9_ANY means media::VP9PROFILE_PROFILE0, so should we rename 
> PP_VIDEOPROFILE_VP9_ANY into PP_VIDEOPROFILE_VP9_PROFILE0 to make it less
> confusing?

Technically, that would be an API change that would require plugin writers to
update their source. I think it's OK to leave it as is, modulo my earlier
comment. I'm assuming that Profile 0 is roughly equivalent to what was available
before the other profiles changed.

[bbudge, 2016-03-31 17:50:10]

On 2016/03/31 17:49:21, bbudge wrote:
> On 2016/03/31 17:44:22, servolk wrote:
> > On 2016/03/31 17:24:44, bbudge wrote:
> > >
> >
>
https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
> > > File content/renderer/pepper/pepper_video_encoder_host.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
> > > content/renderer/pepper/pepper_video_encoder_host.cc:118: case
> > > media::VP9PROFILE_PROFILE3:
> > > Don't accept profiles 1-3 here.
> > > 
> > >
> >
>
https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
> > > content/renderer/pepper/pepper_video_encoder_host.cc:459: for
> > > (media::VideoEncodeAccelerator::SupportedProfile profile : profiles) {
> > > skip profiles 1-3 here.
> > > 
> > >
> >
>
https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
> > > File content/renderer/pepper/video_encoder_shim.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
> > > content/renderer/pepper/video_encoder_shim.cc:78: case
> > > media::VP9PROFILE_PROFILE1:
> > > profiles 1-3 shouldn't be possible here, since the caller is the host,
which
> > > only accepts the pepper 'vp9 any' profile.
> > > 
> > >
> >
>
https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
> > > content/renderer/pepper/video_encoder_shim.cc:205: output_profile ==
> > > media::VP9PROFILE_PROFILE1 ||
> > > ditto.
> > > 
> > >
> >
>
https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
> > > content/renderer/pepper/video_encoder_shim.cc:235: output_profile ==
> > > media::VP9PROFILE_PROFILE1 ||
> > > ditto
> > > 
> > >
> >
>
https://codereview.chromium.org/1769593002/diff/340001/content/renderer/peppe...
> > > content/renderer/pepper/video_encoder_shim.cc:408: profile.profile =
> > > media::VP9PROFILE_PROFILE1;
> > > It's confusing to the client to return multiple vp9 profiles, when only
> > profile
> > > 0 would be accessible to them.
> > 
> > Ok, after applying all these changes in the latest patchset, now essentially
> > PP_VIDEOPROFILE_VP9_ANY means media::VP9PROFILE_PROFILE0, so should we
rename 
> > PP_VIDEOPROFILE_VP9_ANY into PP_VIDEOPROFILE_VP9_PROFILE0 to make it less
> > confusing?
> 
> Technically, that would be an API change that would require plugin writers to
> update their source. I think it's OK to leave it as is, modulo my earlier
> comment. I'm assuming that Profile 0 is roughly equivalent to what was
available
> before the other profiles changed.

I mean before the other profiles were added.

[servolk, 2016-03-31 18:14:44]

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

[commit-bot: I haz the power, 2016-03-31 18:15:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1769593002/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1769593002/360001

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

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

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

Dry run: This issue passed the CQ dry run.

[ddorwin, 2016-03-31 21:09:51]

LGTM, though we might want to add some comments and/or asserts about the ANY=>0
mapping.

https://codereview.chromium.org/1769593002/diff/360001/content/renderer/peppe...
File content/renderer/pepper/video_encoder_shim.cc (right):

https://codereview.chromium.org/1769593002/diff/360001/content/renderer/peppe...
content/renderer/pepper/video_encoder_shim.cc:77: case
media::VP9PROFILE_PROFILE0:
I wonder if we should have a comment here about why other profiles are not
supported. Also, should they be NOTREACHED()?

https://codereview.chromium.org/1769593002/diff/360001/content/renderer/peppe...
content/renderer/pepper/video_encoder_shim.cc:202: config_.rc_end_usage =
VPX_CQ;
DCHECK(not the other three profiles)?

Similar below.

[servolk, 2016-03-31 21:26:39]

https://codereview.chromium.org/1769593002/diff/360001/content/renderer/peppe...
File content/renderer/pepper/video_encoder_shim.cc (right):

https://codereview.chromium.org/1769593002/diff/360001/content/renderer/peppe...
content/renderer/pepper/video_encoder_shim.cc:77: case
media::VP9PROFILE_PROFILE0:
On 2016/03/31 21:09:50, ddorwin wrote:
> I wonder if we should have a comment here about why other profiles are not
> supported. Also, should they be NOTREACHED()?

Probably not worth adding the profiles 1-3 here explicitly, since we'll fall
into default case, which already has NOTREACHED anyway.
I'll add a comment though.

https://codereview.chromium.org/1769593002/diff/360001/content/renderer/peppe...
content/renderer/pepper/video_encoder_shim.cc:202: config_.rc_end_usage =
VPX_CQ;
On 2016/03/31 21:09:50, ddorwin wrote:
> DCHECK(not the other three profiles)?
> 
> Similar below.

I've added some DCHECKs at the top of this method. There's no point in adding
DCHECKs here, since we'll reach NOTREACHED earlier when we call
GetVpxCodecParameters.

[servolk, 2016-03-31 21:28:04]

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

[commit-bot: I haz the power, 2016-03-31 21:28:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1769593002/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1769593002/380001

[commit-bot: I haz the power, 2016-03-31 21:42:25]

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

[commit-bot: I haz the power, 2016-03-31 21:42:27]

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

[servolk, 2016-03-31 23:05:29]

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

[commit-bot: I haz the power, 2016-03-31 23:05:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1769593002/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1769593002/380001

[commit-bot: I haz the power, 2016-04-01 01:23:26]

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

[commit-bot: I haz the power, 2016-04-01 01:23:28]

Dry run: This issue passed the CQ dry run.

[servolk, 2016-04-01 01:25:57]

The CQ bit was checked by servolk@chromium.org

[servolk, 2016-04-01 01:25:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, fgalligan@chromium.org, halliwell@chromium.org,
sievers@chromium.org, bbudge@chromium.org, ddorwin@chromium.org
Link to the patchset: https://codereview.chromium.org/1769593002/#ps380001
(title: "Added some comments and dchecks")

[commit-bot: I haz the power, 2016-04-01 01:26:29]

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

[commit-bot: I haz the power, 2016-04-01 01:33:32]

Description was changed from

==========
Add 4 VP9 profiles to the media::VideoCodecProfile enum.

Currently media/ code doesn't handle any of the VP9 profiles, it just
ignores the existence of different VP9 bitstream profiles and bundles
them all together under VP9PROFILE_ANY.
We know that there are actually 4 different VP9 profiles (see enum
BITSTREAM_PROFILE definition in vp9_enums.h). So at the very least we
should provide 4 values in the media::VideoCodecProfile enum to be able
to distinguish different VP9 profiles.

BUG=592074
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Add 4 VP9 profiles to the media::VideoCodecProfile enum.

Currently media/ code doesn't handle any of the VP9 profiles, it just
ignores the existence of different VP9 bitstream profiles and bundles
them all together under VP9PROFILE_ANY.
We know that there are actually 4 different VP9 profiles (see enum
BITSTREAM_PROFILE definition in vp9_enums.h). So at the very least we
should provide 4 values in the media::VideoCodecProfile enum to be able
to distinguish different VP9 profiles.

BUG=592074
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-04-01 01:33:35]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2016-04-01 01:35:02]

Description was changed from

==========
Add 4 VP9 profiles to the media::VideoCodecProfile enum.

Currently media/ code doesn't handle any of the VP9 profiles, it just
ignores the existence of different VP9 bitstream profiles and bundles
them all together under VP9PROFILE_ANY.
We know that there are actually 4 different VP9 profiles (see enum
BITSTREAM_PROFILE definition in vp9_enums.h). So at the very least we
should provide 4 values in the media::VideoCodecProfile enum to be able
to distinguish different VP9 profiles.

BUG=592074
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Add 4 VP9 profiles to the media::VideoCodecProfile enum.

Currently media/ code doesn't handle any of the VP9 profiles, it just
ignores the existence of different VP9 bitstream profiles and bundles
them all together under VP9PROFILE_ANY.
We know that there are actually 4 different VP9 profiles (see enum
BITSTREAM_PROFILE definition in vp9_enums.h). So at the very least we
should provide 4 values in the media::VideoCodecProfile enum to be able
to distinguish different VP9 profiles.

BUG=592074
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

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

[commit-bot: I haz the power, 2016-04-01 01:35:06]

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