Issue 1677133003: Implemented parsing of HEVC codec ids

servolk

Description was changed from ========== Implemented parsing of HEVC codec ids BUG=482761 ========== to ========== ...

4 years, 10 months ago (2016-02-18 00:54:26 UTC) #1

servolk

Description was changed from ========== Implemented parsing of HEVC codec ids BUG=482761 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ...

4 years, 10 months ago (2016-02-18 03:54:11 UTC) #2

servolk

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

4 years, 10 months ago (2016-02-18 03:54:11 UTC) #3

ddorwin

I only reviewed at a high level. https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codecs.h File media/base/video_codecs.h (right): https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codecs.h#newcode58 media/base/video_codecs.h:58: HEVCPROFILE_MAIN, All ...

4 years, 10 months ago (2016-02-18 17:30:03 UTC) #5

servolk

https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codecs.h File media/base/video_codecs.h (right): https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codecs.h#newcode58 media/base/video_codecs.h:58: HEVCPROFILE_MAIN, On 2016/02/18 17:30:02, ddorwin wrote: > All other ...

4 years, 10 months ago (2016-02-18 19:16:20 UTC) #6

https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codecs.h
File media/base/video_codecs.h (right):

https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codec...
media/base/video_codecs.h:58: HEVCPROFILE_MAIN,
On 2016/02/18 17:30:02, ddorwin wrote:
> All other codecs follow this format:
> MIN
> FOO=MIN
> BAR
> MAX=BAR

Done (I've also removed unnecessary explicit assignment of values for individual
enum members, as currently changing this enum is quite painful, I think we
should be ok with having static asserts to ensure the media::VideoCodecProfile
enum values match mojo and gpu enums).

https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codec...
media/base/video_codecs.h:61: HEVCPROFILE_MAIN_STILL_PICTURE,
On 2016/02/18 17:30:02, ddorwin wrote:
> This is just an image right? Do you have plans to support it? Would it be
> supported in <video> or <img>?

Yes, this is a single video frame, i.e. essentially just an image, only encoded
with the same compression techniques as video. According to the HEVC standard
the 'main still picture' profile is a subset of the main profile, so at least in
theory it should be automatically supported by the video element if it supports
the main HEVC video profile. There's no immediate plans to support that for
<img>. I'm mainly adding it here because 'main still picture' is one of the
three v1 general profiles described in HEVC standard
(https://www.itu.int/rec/T-REC-H.265) section A.3.1
There is also a bunch of format range extensions profiles described in  section
A.3.5, but I haven't yet looked into how to support those best, while this one
is straightforward.

https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codec...
media/base/video_codecs.h:81: MEDIA_EXPORT bool ParseHEVCCodecId(const
std::string& codec_id,
On 2016/02/18 17:30:03, ddorwin wrote:
> Why have these been moved from mime_util and exposed not just outside the
> internal.cc file but outside media.lib?

I've exposed them as media_export to make them accessible to third-party vendor
code in chromecast. Re moving from mime_util - this file seemed like a more
appropriate place for this, since it defines various video codec profiles and
also deals with video codecs/profiles and their string representations. But I
can move the parsing functions back to mime_util, if you think that would be a
better place for this code.

ddorwin

dalecurtis, please see the discussions in https://codereview.chromium.org/1677133003/patch/180001/190007 https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codecs.h File media/base/video_codecs.h (right): https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codecs.h#newcode58 media/base/video_codecs.h:58: HEVCPROFILE_MAIN, On ...

4 years, 10 months ago (2016-02-18 21:52:22 UTC) #7

dalecurtis, please see the discussions in
https://codereview.chromium.org/1677133003/patch/180001/190007

https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codecs.h
File media/base/video_codecs.h (right):

https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codec...
media/base/video_codecs.h:58: HEVCPROFILE_MAIN,
On 2016/02/18 19:16:20, servolk wrote:
> On 2016/02/18 17:30:02, ddorwin wrote:
> > All other codecs follow this format:
> > MIN
> > FOO=MIN
> > BAR
> > MAX=BAR
> 
> Done (I've also removed unnecessary explicit assignment of values for
individual
> enum members, as currently changing this enum is quite painful, I think we
> should be ok with having static asserts to ensure the media::VideoCodecProfile
> enum values match mojo and gpu enums).

+dalecurtis to confirm there are not any problems with this.

https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codec...
media/base/video_codecs.h:81: MEDIA_EXPORT bool ParseHEVCCodecId(const
std::string& codec_id,
On 2016/02/18 19:16:20, servolk wrote:
> On 2016/02/18 17:30:03, ddorwin wrote:
> > Why have these been moved from mime_util and exposed not just outside the
> > internal.cc file but outside media.lib?
> 
> I've exposed them as media_export to make them accessible to third-party
vendor
> code in chromecast. Re moving from mime_util - this file seemed like a more
> appropriate place for this, since it defines various video codec profiles and
> also deals with video codecs/profiles and their string representations. But I
> can move the parsing functions back to mime_util, if you think that would be a
> better place for this code.

I'd like to understand more about that usage - not necessarily here. As
previously discussed, having one parser is better than multiple, but my initial
reactions is that:
a) We shouldn't be passing such strings to third-party code, so it shouldn't be
necessary for that.
b) If it is for other reasons, it's strange that vendor code would need to do
this and that they would rely on our code. I would also be concerned that they
might then come to rely on bugs in our code, making it harder for us to fix them
for all other platforms.

DaleCurtis

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

4 years, 10 months ago (2016-02-18 22:04:54 UTC) #8

DaleCurtis

https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codecs.h File media/base/video_codecs.h (right): https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codecs.h#newcode58 media/base/video_codecs.h:58: HEVCPROFILE_MAIN, On 2016/02/18 21:52:22, ddorwin wrote: > On 2016/02/18 ...

4 years, 10 months ago (2016-02-18 22:04:55 UTC) #9

ddorwin

https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codecs.h File media/base/video_codecs.h (right): https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codecs.h#newcode58 media/base/video_codecs.h:58: HEVCPROFILE_MAIN, On 2016/02/18 22:04:55, DaleCurtis wrote: > On 2016/02/18 ...

4 years, 10 months ago (2016-02-18 22:21:41 UTC) #10

servolk

https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codecs.h File media/base/video_codecs.h (right): https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codecs.h#newcode58 media/base/video_codecs.h:58: HEVCPROFILE_MAIN, On 2016/02/18 22:04:55, DaleCurtis wrote: > On 2016/02/18 ...

4 years, 10 months ago (2016-02-18 22:28:51 UTC) #11

https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codecs.h
File media/base/video_codecs.h (right):

https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codec...
media/base/video_codecs.h:58: HEVCPROFILE_MAIN,
On 2016/02/18 22:04:55, DaleCurtis wrote:
> On 2016/02/18 21:52:22, ddorwin wrote:
> > On 2016/02/18 19:16:20, servolk wrote:
> > > On 2016/02/18 17:30:02, ddorwin wrote:
> > > > All other codecs follow this format:
> > > > MIN
> > > > FOO=MIN
> > > > BAR
> > > > MAX=BAR
> > > 
> > > Done (I've also removed unnecessary explicit assignment of values for
> > individual
> > > enum members, as currently changing this enum is quite painful, I think we
> > > should be ok with having static asserts to ensure the
> media::VideoCodecProfile
> > > enum values match mojo and gpu enums).
> > 
> > +dalecurtis to confirm there are not any problems with this.
> 
> You can't do this since these are logged to UMA, they must be put on the end.
> They should also be protected by the build flag.

Ok, I'll move the HEVC profiles after VP8/VP9, but are we absolutely sure that
there's never gonna be more than one vp8/vp9 profile? If they ever introduce the
concept of profiles, are we going to have them  interspersed in this enum?

https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codec...
media/base/video_codecs.h:81: MEDIA_EXPORT bool ParseHEVCCodecId(const
std::string& codec_id,
On 2016/02/18 21:52:22, ddorwin wrote:
> On 2016/02/18 19:16:20, servolk wrote:
> > On 2016/02/18 17:30:03, ddorwin wrote:
> > > Why have these been moved from mime_util and exposed not just outside the
> > > internal.cc file but outside media.lib?
> > 
> > I've exposed them as media_export to make them accessible to third-party
> vendor
> > code in chromecast. Re moving from mime_util - this file seemed like a more
> > appropriate place for this, since it defines various video codec profiles
and
> > also deals with video codecs/profiles and their string representations. But
I
> > can move the parsing functions back to mime_util, if you think that would be
a
> > better place for this code.
> 
> I'd like to understand more about that usage - not necessarily here. As
> previously discussed, having one parser is better than multiple, but my
initial
> reactions is that:
> a) We shouldn't be passing such strings to third-party code, so it shouldn't
be
> necessary for that.
> b) If it is for other reasons, it's strange that vendor code would need to do
> this and that they would rely on our code. I would also be concerned that they
> might then come to rely on bugs in our code, making it harder for us to fix
them
> for all other platforms.

Sure, we can probably move that discussion to a separate mail thread. But in
short: currently Chromecast codec checks are done via string codec ids since, as
I've explained before, the enums are simply not enough to express the richness
of the information encoded in codec id string (all the various
profiles/levels/tiers/etc). We are planning to keep working with you on getting
to a common upstreamable solution that satisfies everybody, but we needed those
codec checks to be done sooner rather than later, so I've created a couple of
downstream CLs to unblock Chromecast development.

DaleCurtis

https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codecs.h File media/base/video_codecs.h (right): https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codecs.h#newcode58 media/base/video_codecs.h:58: HEVCPROFILE_MAIN, On 2016/02/18 at 22:28:51, servolk wrote: > On ...

4 years, 10 months ago (2016-02-19 23:15:17 UTC) #12

servolk

https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codecs.h File media/base/video_codecs.h (right): https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codecs.h#newcode58 media/base/video_codecs.h:58: HEVCPROFILE_MAIN, On 2016/02/19 23:15:17, DaleCurtis wrote: > On 2016/02/18 ...

4 years, 10 months ago (2016-02-19 23:47:55 UTC) #13

https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codecs.h
File media/base/video_codecs.h (right):

https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codec...
media/base/video_codecs.h:58: HEVCPROFILE_MAIN,
On 2016/02/19 23:15:17, DaleCurtis wrote:
> On 2016/02/18 at 22:28:51, servolk wrote:
> > On 2016/02/18 22:04:55, DaleCurtis wrote:
> > > On 2016/02/18 21:52:22, ddorwin wrote:
> > > > On 2016/02/18 19:16:20, servolk wrote:
> > > > > On 2016/02/18 17:30:02, ddorwin wrote:
> > > > > > All other codecs follow this format:
> > > > > > MIN
> > > > > > FOO=MIN
> > > > > > BAR
> > > > > > MAX=BAR
> > > > > 
> > > > > Done (I've also removed unnecessary explicit assignment of values for
> > > > individual
> > > > > enum members, as currently changing this enum is quite painful, I
think
> we
> > > > > should be ok with having static asserts to ensure the
> > > media::VideoCodecProfile
> > > > > enum values match mojo and gpu enums).
> > > > 
> > > > +dalecurtis to confirm there are not any problems with this.
> > > 
> > > You can't do this since these are logged to UMA, they must be put on the
> end.
> > > They should also be protected by the build flag.
> > 
> > Ok, I'll move the HEVC profiles after VP8/VP9, but are we absolutely sure
that
> there's never gonna be more than one vp8/vp9 profile? If they ever introduce
the
> concept of profiles, are we going to have them  interspersed in this enum?
> 
> Doesn't really matter if we're sure or not; these values are already logged to
a
> histogram, so unless we want to create a new UMA stat, old values must remain
> the same.

Done (I've also restored the explicit value numbering in this enum, to ensure
values don't ever change).
Re guarding with a build flag - I think I've been told earlier that values in
the enum that are reported to UMA shouldn't be guarded (e.g. take a look at
VideoCodec::kCodecHEVC above - it's not protected by the buildflag).

servolk

https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codecs.h File media/base/video_codecs.h (right): https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codecs.h#newcode58 media/base/video_codecs.h:58: HEVCPROFILE_MAIN, On 2016/02/19 23:47:55, servolk wrote: > On 2016/02/19 ...

4 years, 10 months ago (2016-02-19 23:57:31 UTC) #14

https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codecs.h
File media/base/video_codecs.h (right):

https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codec...
media/base/video_codecs.h:58: HEVCPROFILE_MAIN,
On 2016/02/19 23:47:55, servolk wrote:
> On 2016/02/19 23:15:17, DaleCurtis wrote:
> > On 2016/02/18 at 22:28:51, servolk wrote:
> > > On 2016/02/18 22:04:55, DaleCurtis wrote:
> > > > On 2016/02/18 21:52:22, ddorwin wrote:
> > > > > On 2016/02/18 19:16:20, servolk wrote:
> > > > > > On 2016/02/18 17:30:02, ddorwin wrote:
> > > > > > > All other codecs follow this format:
> > > > > > > MIN
> > > > > > > FOO=MIN
> > > > > > > BAR
> > > > > > > MAX=BAR
> > > > > > 
> > > > > > Done (I've also removed unnecessary explicit assignment of values
for
> > > > > individual
> > > > > > enum members, as currently changing this enum is quite painful, I
> think
> > we
> > > > > > should be ok with having static asserts to ensure the
> > > > media::VideoCodecProfile
> > > > > > enum values match mojo and gpu enums).
> > > > > 
> > > > > +dalecurtis to confirm there are not any problems with this.
> > > > 
> > > > You can't do this since these are logged to UMA, they must be put on the
> > end.
> > > > They should also be protected by the build flag.
> > > 
> > > Ok, I'll move the HEVC profiles after VP8/VP9, but are we absolutely sure
> that
> > there's never gonna be more than one vp8/vp9 profile? If they ever introduce
> the
> > concept of profiles, are we going to have them  interspersed in this enum?
> > 
> > Doesn't really matter if we're sure or not; these values are already logged
to
> a
> > histogram, so unless we want to create a new UMA stat, old values must
remain
> > the same.
> 
> Done (I've also restored the explicit value numbering in this enum, to ensure
> values don't ever change).
> Re guarding with a build flag - I think I've been told earlier that values in
> the enum that are reported to UMA shouldn't be guarded (e.g. take a look at
> VideoCodec::kCodecHEVC above - it's not protected by the buildflag).

Btw, I've also just noticed that there's a mismatch between these values and
PPAPI values (despite the comments at lines 36-38 above).
https://bugs.chromium.org/p/chromium/issues/detail?id=588336

servolk

On 2016/02/19 23:57:31, servolk wrote: > https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codecs.h > File media/base/video_codecs.h (right): > > https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codecs.h#newcode58 > ...

4 years, 10 months ago (2016-02-26 18:40:38 UTC) #15

On 2016/02/19 23:57:31, servolk wrote:
>
https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codecs.h
> File media/base/video_codecs.h (right):
> 
>
https://codereview.chromium.org/1677133003/diff/180001/media/base/video_codec...
> media/base/video_codecs.h:58: HEVCPROFILE_MAIN,
> On 2016/02/19 23:47:55, servolk wrote:
> > On 2016/02/19 23:15:17, DaleCurtis wrote:
> > > On 2016/02/18 at 22:28:51, servolk wrote:
> > > > On 2016/02/18 22:04:55, DaleCurtis wrote:
> > > > > On 2016/02/18 21:52:22, ddorwin wrote:
> > > > > > On 2016/02/18 19:16:20, servolk wrote:
> > > > > > > On 2016/02/18 17:30:02, ddorwin wrote:
> > > > > > > > All other codecs follow this format:
> > > > > > > > MIN
> > > > > > > > FOO=MIN
> > > > > > > > BAR
> > > > > > > > MAX=BAR
> > > > > > > 
> > > > > > > Done (I've also removed unnecessary explicit assignment of values
> for
> > > > > > individual
> > > > > > > enum members, as currently changing this enum is quite painful, I
> > think
> > > we
> > > > > > > should be ok with having static asserts to ensure the
> > > > > media::VideoCodecProfile
> > > > > > > enum values match mojo and gpu enums).
> > > > > > 
> > > > > > +dalecurtis to confirm there are not any problems with this.
> > > > > 
> > > > > You can't do this since these are logged to UMA, they must be put on
the
> > > end.
> > > > > They should also be protected by the build flag.
> > > > 
> > > > Ok, I'll move the HEVC profiles after VP8/VP9, but are we absolutely
sure
> > that
> > > there's never gonna be more than one vp8/vp9 profile? If they ever
introduce
> > the
> > > concept of profiles, are we going to have them  interspersed in this enum?
> > > 
> > > Doesn't really matter if we're sure or not; these values are already
logged
> to
> > a
> > > histogram, so unless we want to create a new UMA stat, old values must
> remain
> > > the same.
> > 
> > Done (I've also restored the explicit value numbering in this enum, to
ensure
> > values don't ever change).
> > Re guarding with a build flag - I think I've been told earlier that values
in
> > the enum that are reported to UMA shouldn't be guarded (e.g. take a look at
> > VideoCodec::kCodecHEVC above - it's not protected by the buildflag).
> 
> Btw, I've also just noticed that there's a mismatch between these values and
> PPAPI values (despite the comments at lines 36-38 above).
> https://bugs.chromium.org/p/chromium/issues/detail?id=588336

ping

ddorwin

dalecurtis: Should we reserve UMA values for VP9 profiles and potentially other HEVC profiles? servolk: ...

4 years, 10 months ago (2016-02-26 18:54:17 UTC) #16

DaleCurtis

Do we really care about alphabetical order that much? Or are there pieces of code ...

4 years, 10 months ago (2016-02-26 19:29:18 UTC) #17

ddorwin

On 2016/02/26 19:29:18, DaleCurtis wrote: > Do we really care about alphabetical order that much? ...

4 years, 10 months ago (2016-02-26 19:48:08 UTC) #18

chromium-reviews

Yeah, for example for VP9 we have range checks in a couple of places: https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu/media/vaapi_video_decode_accelerator.cc&l=370&cl=GROK ...

4 years, 10 months ago (2016-02-26 19:50:00 UTC) #19

servolk

https://codereview.chromium.org/1677133003/diff/330001/gpu/config/gpu_info.h File gpu/config/gpu_info.h (right): https://codereview.chromium.org/1677133003/diff/330001/gpu/config/gpu_info.h#newcode43 gpu/config/gpu_info.h:43: H264PROFILE_MAIN, On 2016/02/26 18:54:17, ddorwin wrote: > If this ...

4 years, 10 months ago (2016-02-26 19:50:19 UTC) #20

https://codereview.chromium.org/1677133003/diff/330001/gpu/config/gpu_info.h
File gpu/config/gpu_info.h (right):

https://codereview.chromium.org/1677133003/diff/330001/gpu/config/gpu_info.h#...
gpu/config/gpu_info.h:43: H264PROFILE_MAIN,
On 2016/02/26 18:54:17, ddorwin wrote:
> If this must match, shouldn't we leave the literals?

I think that's redundant. Values in this enum must match media::VideoProfile,
but AFAIK there's no direct requirement for the values here to remain unchanged,
since they are not logged into UMA. So I think it's sufficient to have just
static asserts for ensuring values match the media::VideoProfile and have a
single source of truth in media::VideoProfile with explicitly specified values.

https://codereview.chromium.org/1677133003/diff/330001/media/base/video_codecs.h
File media/base/video_codecs.h (right):

https://codereview.chromium.org/1677133003/diff/330001/media/base/video_codec...
media/base/video_codecs.h:65: VP9PROFILE_ANY = VP9PROFILE_MIN,
On 2016/02/26 18:54:17, ddorwin wrote:
> Should we reserve 3 or 4 values for VP9 profiles?

Yeah, I think we probably should. According to wikipedia vp9 has 4 profiles
(although there's a full byte for encoding profile value in the bitstream, so at
least in theory there could be more added later, but I hope VP9 is pretty stable
now and not gonna change much anymore, new development is gonna happen in VP10).

https://codereview.chromium.org/1677133003/diff/330001/media/base/video_codec...
media/base/video_codecs.h:70: HEVCPROFILE_MAIN_STILL_PICTURE = 15,
On 2016/02/26 18:54:17, ddorwin wrote:
> If you plan to add other HEVC profiles, should we reserve some space?

Currently (and I don't expect that to change for a while) HEVC is the last codec
anyway, so we don't need to reserve any space, although we could, I guess.

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

https://codereview.chromium.org/1677133003/diff/330001/ppapi/api/dev/pp_video...
ppapi/api/dev/pp_video_dev.idl:37: PP_VIDEODECODER_HEVCPROFILE_MAIN = 14,
On 2016/02/26 18:54:17, ddorwin wrote:
> Does it matter that we have no plans to support certain codecs from Pepper
> (because Pepper is not supported on platforms that support them)?

I think that doesn't matter, as that can easily change in the future (we
actually have limited support for Pepper on Chromecast now), and I don't see any
downsides in having these values accessible in Pepper.

DaleCurtis

Ah, yeah I forgot about those, reserving a couple sgtm.

4 years, 10 months ago (2016-02-26 20:40:45 UTC) #21

servolk

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

4 years, 10 months ago (2016-02-26 22:24:05 UTC) #22

servolk

https://codereview.chromium.org/1677133003/diff/330001/media/base/video_codecs.h File media/base/video_codecs.h (right): https://codereview.chromium.org/1677133003/diff/330001/media/base/video_codecs.h#newcode65 media/base/video_codecs.h:65: VP9PROFILE_ANY = VP9PROFILE_MIN, On 2016/02/26 18:54:17, ddorwin wrote: > ...

4 years, 10 months ago (2016-02-26 22:24:26 UTC) #23

ddorwin

https://codereview.chromium.org/1677133003/diff/350001/gpu/config/gpu_info.h File gpu/config/gpu_info.h (right): https://codereview.chromium.org/1677133003/diff/350001/gpu/config/gpu_info.h#newcode56 gpu/config/gpu_info.h:56: HEVCPROFILE_MAIN = 16, This needs to be relative since ...

4 years, 10 months ago (2016-02-26 22:29:33 UTC) #24

servolk

https://codereview.chromium.org/1677133003/diff/350001/gpu/config/gpu_info.h File gpu/config/gpu_info.h (right): https://codereview.chromium.org/1677133003/diff/350001/gpu/config/gpu_info.h#newcode56 gpu/config/gpu_info.h:56: HEVCPROFILE_MAIN = 16, On 2016/02/26 22:29:32, ddorwin wrote: > ...

4 years, 10 months ago (2016-02-26 22:56:06 UTC) #25

servolk

On 2016/02/26 22:56:06, servolk wrote: > https://codereview.chromium.org/1677133003/diff/350001/gpu/config/gpu_info.h > File gpu/config/gpu_info.h (right): > > https://codereview.chromium.org/1677133003/diff/350001/gpu/config/gpu_info.h#newcode56 > ...

4 years, 9 months ago (2016-03-01 02:10:48 UTC) #26

servolk

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

4 years, 9 months ago (2016-03-01 03:38:00 UTC) #27

servolk

On 2016/03/01 02:10:48, servolk wrote: > On 2016/02/26 22:56:06, servolk wrote: > > https://codereview.chromium.org/1677133003/diff/350001/gpu/config/gpu_info.h > ...

4 years, 9 months ago (2016-03-01 03:41:22 UTC) #28

Alexei Svitkine (slow)

lgtm https://codereview.chromium.org/1677133003/diff/430001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1677133003/diff/430001/tools/metrics/histograms/histograms.xml#newcode82807 tools/metrics/histograms/histograms.xml:82807: +<!-- 4 profile values are reserved for VP9 ...

4 years, 9 months ago (2016-03-01 15:30:34 UTC) #29

servolk

https://codereview.chromium.org/1677133003/diff/430001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1677133003/diff/430001/tools/metrics/histograms/histograms.xml#newcode82807 tools/metrics/histograms/histograms.xml:82807: +<!-- 4 profile values are reserved for VP9 profiles, ...

4 years, 9 months ago (2016-03-01 18:00:45 UTC) #30

raymes

ppapi lgtm but please wait for the main reviewer for this CL (it's a bit ...

4 years, 9 months ago (2016-03-01 23:19:47 UTC) #31

ddorwin

https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h File gpu/config/gpu_info.h (right): https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h#newcode56 gpu/config/gpu_info.h:56: HEVCPROFILE_MAIN = VP9PROFILE_ANY + 4, //=16 Just remove the ...

4 years, 9 months ago (2016-03-02 01:36:38 UTC) #32

servolk

https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h File gpu/config/gpu_info.h (right): https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h#newcode56 gpu/config/gpu_info.h:56: HEVCPROFILE_MAIN = VP9PROFILE_ANY + 4, //=16 On 2016/03/02 01:36:38, ...

4 years, 9 months ago (2016-03-02 01:47:45 UTC) #33

https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h
File gpu/config/gpu_info.h (right):

https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h#...
gpu/config/gpu_info.h:56: HEVCPROFILE_MAIN = VP9PROFILE_ANY + 4, //=16
On 2016/03/02 01:36:38, ddorwin wrote:
> Just remove the 16 entirely. It could rot (although that is more unlikely in
> this case.

I'm starting to think that maybe it would be best to just add the 4 VP9 profiles
in various enum already, even if they are going to be just reserved, but unused
for now (kinda like I did in histograms.xml per asvitkine@ suggestion). WDYT?

https://codereview.chromium.org/1677133003/diff/450001/media/mojo/interfaces/...
File media/mojo/interfaces/media_types.mojom (right):

https://codereview.chromium.org/1677133003/diff/450001/media/mojo/interfaces/...
media/mojo/interfaces/media_types.mojom:151: H264PROFILE_MAIN,
On 2016/03/02 01:36:38, ddorwin wrote:
> Since this is a MOJO API definition, I wonder whether values are allowed to
> change. It seems we have a mix. I wonder why.

Is there a requirement that constants never change in Mojo API? I assumed Mojo
values might change, although of course that would make it necessary to rebuild
also clients that use this .mojom file.

https://codereview.chromium.org/1677133003/diff/450001/media/mojo/interfaces/...
media/mojo/interfaces/media_types.mojom:169: HEVCPROFILE_MIN = 16,
On 2016/03/02 01:36:38, ddorwin wrote:
> not done. There are no other literals here.

I think this is the best we can do. I've tried to have HEVCPROFILE_MIN =
VP9PROFILE_ANY + 4 here in patchset #20. But that caused a number of build
failures. Apparently mojom files don't allow expressions, so I've changed this
to an explicit value for now.

Here is the build error from PS #20:
FAILED: python ../../mojo/public/tools/bindings/mojom_bindings_generator.py
--use_bundled_pylibs generate ../../media/mojo/interfaces/renderer.mojom -d
../../ -I ../../ -o /b/build/slave/linux_clobber/build/src/out/Release/gen
--bytecode_path
/b/build/slave/linux_clobber/build/src/out/Release/gen/mojo/public/tools/bindings
-g c++,javascript,java
../../media/mojo/interfaces/media_types.mojom:169: Error: Unexpected '+':
  HEVCPROFILE_MIN = VP9PROFILE_ANY + 4, //=16

xhwang

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

4 years, 9 months ago (2016-03-02 04:17:54 UTC) #34

xhwang

https://codereview.chromium.org/1677133003/diff/450001/media/mojo/interfaces/media_types.mojom File media/mojo/interfaces/media_types.mojom (right): https://codereview.chromium.org/1677133003/diff/450001/media/mojo/interfaces/media_types.mojom#newcode151 media/mojo/interfaces/media_types.mojom:151: H264PROFILE_MAIN, On 2016/03/02 01:47:45, servolk wrote: > On 2016/03/02 ...

4 years, 9 months ago (2016-03-02 04:17:55 UTC) #35

ddorwin

sandersd and dalecurtis: What are your thoughts on whether VP9PROFILE_ANY would be the same as ...

4 years, 9 months ago (2016-03-03 22:00:21 UTC) #36

sandersd and dalecurtis: What are your thoughts on whether VP9PROFILE_ANY would
be the same as profile 0 and whether to define these values now?

https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h
File gpu/config/gpu_info.h (right):

https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h#...
gpu/config/gpu_info.h:56: HEVCPROFILE_MAIN = VP9PROFILE_ANY + 4, //=16
On 2016/03/02 01:47:45, servolk wrote:
> On 2016/03/02 01:36:38, ddorwin wrote:
> > Just remove the 16 entirely. It could rot (although that is more unlikely in
> > this case.
> 
> I'm starting to think that maybe it would be best to just add the 4 VP9
profiles
> in various enum already, even if they are going to be just reserved, but
unused
> for now (kinda like I did in histograms.xml per asvitkine@ suggestion). WDYT?

I'm hesitant to define things we don't use, though +4 is also weird. I also
wonder whether we would want ANY to be separate from PROFILE0. That might vary
based on the API.

Perhaps sandersd or dalecurtis@ have thoughts.

https://codereview.chromium.org/1677133003/diff/450001/media/mojo/interfaces/...
File media/mojo/interfaces/media_types.mojom (right):

https://codereview.chromium.org/1677133003/diff/450001/media/mojo/interfaces/...
media/mojo/interfaces/media_types.mojom:169: HEVCPROFILE_MIN = 16,
On 2016/03/02 01:47:45, servolk wrote:
> On 2016/03/02 01:36:38, ddorwin wrote:
> > not done. There are no other literals here.
> 
> I think this is the best we can do. I've tried to have HEVCPROFILE_MIN =
> VP9PROFILE_ANY + 4 here in patchset #20. But that caused a number of build
> failures. Apparently mojom files don't allow expressions, so I've changed this
> to an explicit value for now.
> 
> Here is the build error from PS #20:
> FAILED: python ../../mojo/public/tools/bindings/mojom_bindings_generator.py
> --use_bundled_pylibs generate ../../media/mojo/interfaces/renderer.mojom -d
> ../../ -I ../../ -o /b/build/slave/linux_clobber/build/src/out/Release/gen
> --bytecode_path
>
/b/build/slave/linux_clobber/build/src/out/Release/gen/mojo/public/tools/bindings
> -g c++,javascript,java
> ../../media/mojo/interfaces/media_types.mojom:169: Error: Unexpected '+':
>   HEVCPROFILE_MIN = VP9PROFILE_ANY + 4, //=16

Longshot - does it work if you use parentheses?

That looks like a tooling bug. I suggest filing a bug in the Internals>Mojo
component. For now, we can define RESERVED1-4 and add a comment referencing that
bug.

DaleCurtis

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

4 years, 9 months ago (2016-03-03 22:05:31 UTC) #37

DaleCurtis

https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h File gpu/config/gpu_info.h (right): https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h#newcode56 gpu/config/gpu_info.h:56: HEVCPROFILE_MAIN = VP9PROFILE_ANY + 4, //=16 On 2016/03/03 at ...

4 years, 9 months ago (2016-03-03 22:05:32 UTC) #38

servolk

On 2016/03/03 22:05:32, DaleCurtis wrote: > https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h > File gpu/config/gpu_info.h (right): > > https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h#newcode56 > ...

4 years, 9 months ago (2016-03-03 22:15:17 UTC) #39

DaleCurtis

https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h File gpu/config/gpu_info.h (right): https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h#newcode56 gpu/config/gpu_info.h:56: HEVCPROFILE_MAIN = VP9PROFILE_ANY + 4, //=16 On 2016/03/03 at ...

4 years, 9 months ago (2016-03-03 22:17:20 UTC) #40

ddorwin

https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h File gpu/config/gpu_info.h (right): https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h#newcode56 gpu/config/gpu_info.h:56: HEVCPROFILE_MAIN = VP9PROFILE_ANY + 4, //=16 On 2016/03/03 22:17:20, ...

4 years, 9 months ago (2016-03-03 22:21:17 UTC) #41

servolk

On 2016/03/03 22:17:20, DaleCurtis wrote: > https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h > File gpu/config/gpu_info.h (right): > > https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h#newcode56 > ...

4 years, 9 months ago (2016-03-03 22:23:22 UTC) #42

On 2016/03/03 22:17:20, DaleCurtis wrote:
> https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h
> File gpu/config/gpu_info.h (right):
> 
>
https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h#...
> gpu/config/gpu_info.h:56: HEVCPROFILE_MAIN = VP9PROFILE_ANY + 4, //=16
> On 2016/03/03 at 22:05:32, DaleCurtis wrote:
> > On 2016/03/03 at 22:00:21, ddorwin wrote:
> > > On 2016/03/02 01:47:45, servolk wrote:
> > > > On 2016/03/02 01:36:38, ddorwin wrote:
> > > > > Just remove the 16 entirely. It could rot (although that is more
> unlikely in
> > > > > this case.
> > > > 
> > > > I'm starting to think that maybe it would be best to just add the 4 VP9
> profiles
> > > > in various enum already, even if they are going to be just reserved, but
> unused
> > > > for now (kinda like I did in histograms.xml per asvitkine@ suggestion).
> WDYT?
> > > 
> > > I'm hesitant to define things we don't use, though +4 is also weird. I
also
> wonder whether we would want ANY to be separate from PROFILE0. That might vary
> based on the API.
> > > 
> > > Perhaps sandersd or dalecurtis@ have thoughts.
> > 
> > Sergey, you say "the 4 VP9 profiles" above. Do we know those are set in
stone?
> Or did you just make that value up? Does the vp9 team have any insight for us?
> > 
> > If those are legit, I'm fine with adding them here now and removing the _ANY
> designator in place of MIN/MAX variants if endpoints don't want to check the
> values explicitly.
> 
> Thanks for the link. Lets just include those here then.

Ok, just to confirm that I got it right: so I'll replace the vp9profile_any with
vp9profile_0..vp9profile_3 and will introduce the corresponding values in other
matching enums and ppapi, right? What are we going to do about UMA? Should we
just rename the vp9profile_any value in there into profile0 and just keep in
mind to ourselves the fact that previously we reported any vp9 profile as
profile0 (speaking pragmatically, that's probably ok for now, there's not a lot
of vp9 content yet, and I guess most of it is really in profile0). Or do we want
to point out the fact that previously reported values of vp9_any could include
other profiles, besides profile0?

servolk

On 2016/03/03 22:23:22, servolk wrote: > On 2016/03/03 22:17:20, DaleCurtis wrote: > > https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h > ...

4 years, 9 months ago (2016-03-04 19:48:05 UTC) #43

On 2016/03/03 22:23:22, servolk wrote:
> On 2016/03/03 22:17:20, DaleCurtis wrote:
> > https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h
> > File gpu/config/gpu_info.h (right):
> > 
> >
>
https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h#...
> > gpu/config/gpu_info.h:56: HEVCPROFILE_MAIN = VP9PROFILE_ANY + 4, //=16
> > On 2016/03/03 at 22:05:32, DaleCurtis wrote:
> > > On 2016/03/03 at 22:00:21, ddorwin wrote:
> > > > On 2016/03/02 01:47:45, servolk wrote:
> > > > > On 2016/03/02 01:36:38, ddorwin wrote:
> > > > > > Just remove the 16 entirely. It could rot (although that is more
> > unlikely in
> > > > > > this case.
> > > > > 
> > > > > I'm starting to think that maybe it would be best to just add the 4
VP9
> > profiles
> > > > > in various enum already, even if they are going to be just reserved,
but
> > unused
> > > > > for now (kinda like I did in histograms.xml per asvitkine@
suggestion).
> > WDYT?
> > > > 
> > > > I'm hesitant to define things we don't use, though +4 is also weird. I
> also
> > wonder whether we would want ANY to be separate from PROFILE0. That might
vary
> > based on the API.
> > > > 
> > > > Perhaps sandersd or dalecurtis@ have thoughts.
> > > 
> > > Sergey, you say "the 4 VP9 profiles" above. Do we know those are set in
> stone?
> > Or did you just make that value up? Does the vp9 team have any insight for
us?
> > > 
> > > If those are legit, I'm fine with adding them here now and removing the
_ANY
> > designator in place of MIN/MAX variants if endpoints don't want to check the
> > values explicitly.
> > 
> > Thanks for the link. Lets just include those here then.
> 
> Ok, just to confirm that I got it right: so I'll replace the vp9profile_any
with
> vp9profile_0..vp9profile_3 and will introduce the corresponding values in
other
> matching enums and ppapi, right? What are we going to do about UMA? Should we
> just rename the vp9profile_any value in there into profile0 and just keep in
> mind to ourselves the fact that previously we reported any vp9 profile as
> profile0 (speaking pragmatically, that's probably ok for now, there's not a
lot
> of vp9 content yet, and I guess most of it is really in profile0). Or do we
want
> to point out the fact that previously reported values of vp9_any could include
> other profiles, besides profile0?

I've created a separate bug and a separate CL for adding the 4 VP9 profiles:
https://codereview.chromium.org/1677133003
Let's keep this CL focused on HEVC and move the VP9 discussion there.

servolk

On 2016/03/04 19:48:05, servolk wrote: > On 2016/03/03 22:23:22, servolk wrote: > > On 2016/03/03 ...

4 years, 9 months ago (2016-03-10 01:00:34 UTC) #44

On 2016/03/04 19:48:05, servolk wrote:
> On 2016/03/03 22:23:22, servolk wrote:
> > On 2016/03/03 22:17:20, DaleCurtis wrote:
> > >
https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h
> > > File gpu/config/gpu_info.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h#...
> > > gpu/config/gpu_info.h:56: HEVCPROFILE_MAIN = VP9PROFILE_ANY + 4, //=16
> > > On 2016/03/03 at 22:05:32, DaleCurtis wrote:
> > > > On 2016/03/03 at 22:00:21, ddorwin wrote:
> > > > > On 2016/03/02 01:47:45, servolk wrote:
> > > > > > On 2016/03/02 01:36:38, ddorwin wrote:
> > > > > > > Just remove the 16 entirely. It could rot (although that is more
> > > unlikely in
> > > > > > > this case.
> > > > > > 
> > > > > > I'm starting to think that maybe it would be best to just add the 4
> VP9
> > > profiles
> > > > > > in various enum already, even if they are going to be just reserved,
> but
> > > unused
> > > > > > for now (kinda like I did in histograms.xml per asvitkine@
> suggestion).
> > > WDYT?
> > > > > 
> > > > > I'm hesitant to define things we don't use, though +4 is also weird. I
> > also
> > > wonder whether we would want ANY to be separate from PROFILE0. That might
> vary
> > > based on the API.
> > > > > 
> > > > > Perhaps sandersd or dalecurtis@ have thoughts.
> > > > 
> > > > Sergey, you say "the 4 VP9 profiles" above. Do we know those are set in
> > stone?
> > > Or did you just make that value up? Does the vp9 team have any insight for
> us?
> > > > 
> > > > If those are legit, I'm fine with adding them here now and removing the
> _ANY
> > > designator in place of MIN/MAX variants if endpoints don't want to check
the
> > > values explicitly.
> > > 
> > > Thanks for the link. Lets just include those here then.
> > 
> > Ok, just to confirm that I got it right: so I'll replace the vp9profile_any
> with
> > vp9profile_0..vp9profile_3 and will introduce the corresponding values in
> other
> > matching enums and ppapi, right? What are we going to do about UMA? Should
we
> > just rename the vp9profile_any value in there into profile0 and just keep in
> > mind to ourselves the fact that previously we reported any vp9 profile as
> > profile0 (speaking pragmatically, that's probably ok for now, there's not a
> lot
> > of vp9 content yet, and I guess most of it is really in profile0). Or do we
> want
> > to point out the fact that previously reported values of vp9_any could
include
> > other profiles, besides profile0?
> 
> I've created a separate bug and a separate CL for adding the 4 VP9 profiles:
> https://codereview.chromium.org/1677133003
> Let's keep this CL focused on HEVC and move the VP9 discussion there.

ping

servolk

On 2016/03/10 01:00:34, servolk wrote: > On 2016/03/04 19:48:05, servolk wrote: > > On 2016/03/03 ...

4 years, 9 months ago (2016-03-11 01:45:46 UTC) #45

On 2016/03/10 01:00:34, servolk wrote:
> On 2016/03/04 19:48:05, servolk wrote:
> > On 2016/03/03 22:23:22, servolk wrote:
> > > On 2016/03/03 22:17:20, DaleCurtis wrote:
> > > >
> https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h
> > > > File gpu/config/gpu_info.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h#...
> > > > gpu/config/gpu_info.h:56: HEVCPROFILE_MAIN = VP9PROFILE_ANY + 4, //=16
> > > > On 2016/03/03 at 22:05:32, DaleCurtis wrote:
> > > > > On 2016/03/03 at 22:00:21, ddorwin wrote:
> > > > > > On 2016/03/02 01:47:45, servolk wrote:
> > > > > > > On 2016/03/02 01:36:38, ddorwin wrote:
> > > > > > > > Just remove the 16 entirely. It could rot (although that is more
> > > > unlikely in
> > > > > > > > this case.
> > > > > > > 
> > > > > > > I'm starting to think that maybe it would be best to just add the
4
> > VP9
> > > > profiles
> > > > > > > in various enum already, even if they are going to be just
reserved,
> > but
> > > > unused
> > > > > > > for now (kinda like I did in histograms.xml per asvitkine@
> > suggestion).
> > > > WDYT?
> > > > > > 
> > > > > > I'm hesitant to define things we don't use, though +4 is also weird.
I
> > > also
> > > > wonder whether we would want ANY to be separate from PROFILE0. That
might
> > vary
> > > > based on the API.
> > > > > > 
> > > > > > Perhaps sandersd or dalecurtis@ have thoughts.
> > > > > 
> > > > > Sergey, you say "the 4 VP9 profiles" above. Do we know those are set
in
> > > stone?
> > > > Or did you just make that value up? Does the vp9 team have any insight
for
> > us?
> > > > > 
> > > > > If those are legit, I'm fine with adding them here now and removing
the
> > _ANY
> > > > designator in place of MIN/MAX variants if endpoints don't want to check
> the
> > > > values explicitly.
> > > > 
> > > > Thanks for the link. Lets just include those here then.
> > > 
> > > Ok, just to confirm that I got it right: so I'll replace the
vp9profile_any
> > with
> > > vp9profile_0..vp9profile_3 and will introduce the corresponding values in
> > other
> > > matching enums and ppapi, right? What are we going to do about UMA? Should
> we
> > > just rename the vp9profile_any value in there into profile0 and just keep
in
> > > mind to ourselves the fact that previously we reported any vp9 profile as
> > > profile0 (speaking pragmatically, that's probably ok for now, there's not
a
> > lot
> > > of vp9 content yet, and I guess most of it is really in profile0). Or do
we
> > want
> > > to point out the fact that previously reported values of vp9_any could
> include
> > > other profiles, besides profile0?
> > 
> > I've created a separate bug and a separate CL for adding the 4 VP9 profiles:
> > https://codereview.chromium.org/1677133003
> > Let's keep this CL focused on HEVC and move the VP9 discussion there.
> 
> ping

ping

ddorwin

On 2016/03/11 01:45:46, servolk wrote: > On 2016/03/10 01:00:34, servolk wrote: > > On 2016/03/04 ...

4 years, 9 months ago (2016-03-11 17:37:10 UTC) #46

On 2016/03/11 01:45:46, servolk wrote:
> On 2016/03/10 01:00:34, servolk wrote:
> > On 2016/03/04 19:48:05, servolk wrote:
> > > On 2016/03/03 22:23:22, servolk wrote:
> > > > On 2016/03/03 22:17:20, DaleCurtis wrote:
> > > > >
> > https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h
> > > > > File gpu/config/gpu_info.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h#...
> > > > > gpu/config/gpu_info.h:56: HEVCPROFILE_MAIN = VP9PROFILE_ANY + 4, //=16
> > > > > On 2016/03/03 at 22:05:32, DaleCurtis wrote:
> > > > > > On 2016/03/03 at 22:00:21, ddorwin wrote:
> > > > > > > On 2016/03/02 01:47:45, servolk wrote:
> > > > > > > > On 2016/03/02 01:36:38, ddorwin wrote:
> > > > > > > > > Just remove the 16 entirely. It could rot (although that is
more
> > > > > unlikely in
> > > > > > > > > this case.
> > > > > > > > 
> > > > > > > > I'm starting to think that maybe it would be best to just add
the
> 4
> > > VP9
> > > > > profiles
> > > > > > > > in various enum already, even if they are going to be just
> reserved,
> > > but
> > > > > unused
> > > > > > > > for now (kinda like I did in histograms.xml per asvitkine@
> > > suggestion).
> > > > > WDYT?
> > > > > > > 
> > > > > > > I'm hesitant to define things we don't use, though +4 is also
weird.
> I
> > > > also
> > > > > wonder whether we would want ANY to be separate from PROFILE0. That
> might
> > > vary
> > > > > based on the API.
> > > > > > > 
> > > > > > > Perhaps sandersd or dalecurtis@ have thoughts.
> > > > > > 
> > > > > > Sergey, you say "the 4 VP9 profiles" above. Do we know those are set
> in
> > > > stone?
> > > > > Or did you just make that value up? Does the vp9 team have any insight
> for
> > > us?
> > > > > > 
> > > > > > If those are legit, I'm fine with adding them here now and removing
> the
> > > _ANY
> > > > > designator in place of MIN/MAX variants if endpoints don't want to
check
> > the
> > > > > values explicitly.
> > > > > 
> > > > > Thanks for the link. Lets just include those here then.
> > > > 
> > > > Ok, just to confirm that I got it right: so I'll replace the
> vp9profile_any
> > > with
> > > > vp9profile_0..vp9profile_3 and will introduce the corresponding values
in
> > > other
> > > > matching enums and ppapi, right? What are we going to do about UMA?
Should
> > we
> > > > just rename the vp9profile_any value in there into profile0 and just
keep
> in
> > > > mind to ourselves the fact that previously we reported any vp9 profile
as
> > > > profile0 (speaking pragmatically, that's probably ok for now, there's
not
> a
> > > lot
> > > > of vp9 content yet, and I guess most of it is really in profile0). Or do
> we
> > > want
> > > > to point out the fact that previously reported values of vp9_any could
> > include
> > > > other profiles, besides profile0?
> > > 
> > > I've created a separate bug and a separate CL for adding the 4 VP9
profiles:
> > > https://codereview.chromium.org/1677133003
> > > Let's keep this CL focused on HEVC and move the VP9 discussion there.
> > 
> > ping
> 
> ping

Can you be more specific about what you are pinging for?
It seems that most of the latest discussion around VP9 has moved to
https://codereview.chromium.org/1769593002.
The previous suggestion was to implement the VP9 values first, which would mean
this is blocked on that CL. If you think otherwise, you should say so.

servolk

On 2016/03/11 17:37:10, ddorwin wrote: > On 2016/03/11 01:45:46, servolk wrote: > > On 2016/03/10 ...

4 years, 9 months ago (2016-03-11 20:04:51 UTC) #47

On 2016/03/11 17:37:10, ddorwin wrote:
> On 2016/03/11 01:45:46, servolk wrote:
> > On 2016/03/10 01:00:34, servolk wrote:
> > > On 2016/03/04 19:48:05, servolk wrote:
> > > > On 2016/03/03 22:23:22, servolk wrote:
> > > > > On 2016/03/03 22:17:20, DaleCurtis wrote:
> > > > > >
> > >
https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h
> > > > > > File gpu/config/gpu_info.h (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h#...
> > > > > > gpu/config/gpu_info.h:56: HEVCPROFILE_MAIN = VP9PROFILE_ANY + 4,
//=16
> > > > > > On 2016/03/03 at 22:05:32, DaleCurtis wrote:
> > > > > > > On 2016/03/03 at 22:00:21, ddorwin wrote:
> > > > > > > > On 2016/03/02 01:47:45, servolk wrote:
> > > > > > > > > On 2016/03/02 01:36:38, ddorwin wrote:
> > > > > > > > > > Just remove the 16 entirely. It could rot (although that is
> more
> > > > > > unlikely in
> > > > > > > > > > this case.
> > > > > > > > > 
> > > > > > > > > I'm starting to think that maybe it would be best to just add
> the
> > 4
> > > > VP9
> > > > > > profiles
> > > > > > > > > in various enum already, even if they are going to be just
> > reserved,
> > > > but
> > > > > > unused
> > > > > > > > > for now (kinda like I did in histograms.xml per asvitkine@
> > > > suggestion).
> > > > > > WDYT?
> > > > > > > > 
> > > > > > > > I'm hesitant to define things we don't use, though +4 is also
> weird.
> > I
> > > > > also
> > > > > > wonder whether we would want ANY to be separate from PROFILE0. That
> > might
> > > > vary
> > > > > > based on the API.
> > > > > > > > 
> > > > > > > > Perhaps sandersd or dalecurtis@ have thoughts.
> > > > > > > 
> > > > > > > Sergey, you say "the 4 VP9 profiles" above. Do we know those are
set
> > in
> > > > > stone?
> > > > > > Or did you just make that value up? Does the vp9 team have any
insight
> > for
> > > > us?
> > > > > > > 
> > > > > > > If those are legit, I'm fine with adding them here now and
removing
> > the
> > > > _ANY
> > > > > > designator in place of MIN/MAX variants if endpoints don't want to
> check
> > > the
> > > > > > values explicitly.
> > > > > > 
> > > > > > Thanks for the link. Lets just include those here then.
> > > > > 
> > > > > Ok, just to confirm that I got it right: so I'll replace the
> > vp9profile_any
> > > > with
> > > > > vp9profile_0..vp9profile_3 and will introduce the corresponding values
> in
> > > > other
> > > > > matching enums and ppapi, right? What are we going to do about UMA?
> Should
> > > we
> > > > > just rename the vp9profile_any value in there into profile0 and just
> keep
> > in
> > > > > mind to ourselves the fact that previously we reported any vp9 profile
> as
> > > > > profile0 (speaking pragmatically, that's probably ok for now, there's
> not
> > a
> > > > lot
> > > > > of vp9 content yet, and I guess most of it is really in profile0). Or
do
> > we
> > > > want
> > > > > to point out the fact that previously reported values of vp9_any could
> > > include
> > > > > other profiles, besides profile0?
> > > > 
> > > > I've created a separate bug and a separate CL for adding the 4 VP9
> profiles:
> > > > https://codereview.chromium.org/1677133003
> > > > Let's keep this CL focused on HEVC and move the VP9 discussion there.
> > > 
> > > ping
> > 
> > ping
> 
> Can you be more specific about what you are pinging for?
> It seems that most of the latest discussion around VP9 has moved to
> https://codereview.chromium.org/1769593002.
> The previous suggestion was to implement the VP9 values first, which would
mean
> this is blocked on that CL. If you think otherwise, you should say so.

I'm pinging to get approvals for content/, gpu/ and media/ changes. So far I
only got approvals for ppapi/ from raymes@ and tools/ from asvitkine@. content/
and gpu/ could be approved by sievers@. media/ could be any of the media folks
on the reviewer list. I've initially added sandersd@ and you for media/ and
content/browser/media/media_canplaytype_browsertest.cc changes, but I have just
noticed that somebody also added wolenetz@ and xhwang@ (I guess it was Dale?).
As I've explained in https://codereview.chromium.org/1769593002#msg7 currently
VP9 CL is based on HEVC CL. The HEVC CL (this one) is ready to land, I think,
and I don't know if we need to hold it to wait for VP9 CL to land first. I think
the order doesn't matter, as the end result will be the same anyway and it would
allow me to start using the new HEVC codec parsing functions for further
Chromecast work without waiting for VP9 CL to go in (which is still waiting for
VP9 folks to look at). Since nobody responded to my comments about VP9 CL being
based on HEVC CL at https://codereview.chromium.org/1769593002#msg7, I assumed
everybody agreed (or didn't care) about the ordering. If that's not the case,
please let me know, I can rebase.

ddorwin

On 2016/03/11 20:04:51, servolk wrote: > On 2016/03/11 17:37:10, ddorwin wrote: > > On 2016/03/11 ...

4 years, 9 months ago (2016-03-11 20:11:52 UTC) #48

On 2016/03/11 20:04:51, servolk wrote:
> On 2016/03/11 17:37:10, ddorwin wrote:
> > On 2016/03/11 01:45:46, servolk wrote:
> > > On 2016/03/10 01:00:34, servolk wrote:
> > > > On 2016/03/04 19:48:05, servolk wrote:
> > > > > On 2016/03/03 22:23:22, servolk wrote:
> > > > > > On 2016/03/03 22:17:20, DaleCurtis wrote:
> > > > > > >
> > > >
> https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h
> > > > > > > File gpu/config/gpu_info.h (right):
> > > > > > > 
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h#...
> > > > > > > gpu/config/gpu_info.h:56: HEVCPROFILE_MAIN = VP9PROFILE_ANY + 4,
> //=16
> > > > > > > On 2016/03/03 at 22:05:32, DaleCurtis wrote:
> > > > > > > > On 2016/03/03 at 22:00:21, ddorwin wrote:
> > > > > > > > > On 2016/03/02 01:47:45, servolk wrote:
> > > > > > > > > > On 2016/03/02 01:36:38, ddorwin wrote:
> > > > > > > > > > > Just remove the 16 entirely. It could rot (although that
is
> > more
> > > > > > > unlikely in
> > > > > > > > > > > this case.
> > > > > > > > > > 
> > > > > > > > > > I'm starting to think that maybe it would be best to just
add
> > the
> > > 4
> > > > > VP9
> > > > > > > profiles
> > > > > > > > > > in various enum already, even if they are going to be just
> > > reserved,
> > > > > but
> > > > > > > unused
> > > > > > > > > > for now (kinda like I did in histograms.xml per asvitkine@
> > > > > suggestion).
> > > > > > > WDYT?
> > > > > > > > > 
> > > > > > > > > I'm hesitant to define things we don't use, though +4 is also
> > weird.
> > > I
> > > > > > also
> > > > > > > wonder whether we would want ANY to be separate from PROFILE0.
That
> > > might
> > > > > vary
> > > > > > > based on the API.
> > > > > > > > > 
> > > > > > > > > Perhaps sandersd or dalecurtis@ have thoughts.
> > > > > > > > 
> > > > > > > > Sergey, you say "the 4 VP9 profiles" above. Do we know those are
> set
> > > in
> > > > > > stone?
> > > > > > > Or did you just make that value up? Does the vp9 team have any
> insight
> > > for
> > > > > us?
> > > > > > > > 
> > > > > > > > If those are legit, I'm fine with adding them here now and
> removing
> > > the
> > > > > _ANY
> > > > > > > designator in place of MIN/MAX variants if endpoints don't want to
> > check
> > > > the
> > > > > > > values explicitly.
> > > > > > > 
> > > > > > > Thanks for the link. Lets just include those here then.
> > > > > > 
> > > > > > Ok, just to confirm that I got it right: so I'll replace the
> > > vp9profile_any
> > > > > with
> > > > > > vp9profile_0..vp9profile_3 and will introduce the corresponding
values
> > in
> > > > > other
> > > > > > matching enums and ppapi, right? What are we going to do about UMA?
> > Should
> > > > we
> > > > > > just rename the vp9profile_any value in there into profile0 and just
> > keep
> > > in
> > > > > > mind to ourselves the fact that previously we reported any vp9
profile
> > as
> > > > > > profile0 (speaking pragmatically, that's probably ok for now,
there's
> > not
> > > a
> > > > > lot
> > > > > > of vp9 content yet, and I guess most of it is really in profile0).
Or
> do
> > > we
> > > > > want
> > > > > > to point out the fact that previously reported values of vp9_any
could
> > > > include
> > > > > > other profiles, besides profile0?
> > > > > 
> > > > > I've created a separate bug and a separate CL for adding the 4 VP9
> > profiles:
> > > > > https://codereview.chromium.org/1677133003
> > > > > Let's keep this CL focused on HEVC and move the VP9 discussion there.
> > > > 
> > > > ping
> > > 
> > > ping
> > 
> > Can you be more specific about what you are pinging for?
> > It seems that most of the latest discussion around VP9 has moved to
> > https://codereview.chromium.org/1769593002.
> > The previous suggestion was to implement the VP9 values first, which would
> mean
> > this is blocked on that CL. If you think otherwise, you should say so.
> 
> I'm pinging to get approvals for content/, gpu/ and media/ changes. So far I
> only got approvals for ppapi/ from raymes@ and tools/ from asvitkine@.
content/
> and gpu/ could be approved by sievers@. media/ could be any of the media folks
> on the reviewer list. I've initially added sandersd@ and you for media/ and
> content/browser/media/media_canplaytype_browsertest.cc changes, but I have
just
> noticed that somebody also added wolenetz@ and xhwang@ (I guess it was Dale?).
> As I've explained in https://codereview.chromium.org/1769593002#msg7 currently
> VP9 CL is based on HEVC CL. The HEVC CL (this one) is ready to land, I think,
> and I don't know if we need to hold it to wait for VP9 CL to land first. I
think
> the order doesn't matter, as the end result will be the same anyway and it
would
> allow me to start using the new HEVC codec parsing functions for further
> Chromecast work without waiting for VP9 CL to go in (which is still waiting
for
> VP9 folks to look at). Since nobody responded to my comments about VP9 CL
being
> based on HEVC CL at https://codereview.chromium.org/1769593002#msg7, I assumed
> everybody agreed (or didn't care) about the ordering. If that's not the case,
> please let me know, I can rebase.

Okay, if you have multiple reviewers, you're more likely to get the attention
you need if you include their ID.

As for the order, I think we should make sure that "+4" is the correct number
and that we don't need a separate ANY to deal with ambiguity in the container.
Thus, I think we should resolve the other CL first.

servolk

On 2016/03/11 20:11:52, ddorwin wrote: > On 2016/03/11 20:04:51, servolk wrote: > > On 2016/03/11 ...

4 years, 9 months ago (2016-03-11 23:12:35 UTC) #49

On 2016/03/11 20:11:52, ddorwin wrote:
> On 2016/03/11 20:04:51, servolk wrote:
> > On 2016/03/11 17:37:10, ddorwin wrote:
> > > On 2016/03/11 01:45:46, servolk wrote:
> > > > On 2016/03/10 01:00:34, servolk wrote:
> > > > > On 2016/03/04 19:48:05, servolk wrote:
> > > > > > On 2016/03/03 22:23:22, servolk wrote:
> > > > > > > On 2016/03/03 22:17:20, DaleCurtis wrote:
> > > > > > > >
> > > > >
> > https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h
> > > > > > > > File gpu/config/gpu_info.h (right):
> > > > > > > > 
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h#...
> > > > > > > > gpu/config/gpu_info.h:56: HEVCPROFILE_MAIN = VP9PROFILE_ANY + 4,
> > //=16
> > > > > > > > On 2016/03/03 at 22:05:32, DaleCurtis wrote:
> > > > > > > > > On 2016/03/03 at 22:00:21, ddorwin wrote:
> > > > > > > > > > On 2016/03/02 01:47:45, servolk wrote:
> > > > > > > > > > > On 2016/03/02 01:36:38, ddorwin wrote:
> > > > > > > > > > > > Just remove the 16 entirely. It could rot (although that
> is
> > > more
> > > > > > > > unlikely in
> > > > > > > > > > > > this case.
> > > > > > > > > > > 
> > > > > > > > > > > I'm starting to think that maybe it would be best to just
> add
> > > the
> > > > 4
> > > > > > VP9
> > > > > > > > profiles
> > > > > > > > > > > in various enum already, even if they are going to be just
> > > > reserved,
> > > > > > but
> > > > > > > > unused
> > > > > > > > > > > for now (kinda like I did in histograms.xml per asvitkine@
> > > > > > suggestion).
> > > > > > > > WDYT?
> > > > > > > > > > 
> > > > > > > > > > I'm hesitant to define things we don't use, though +4 is
also
> > > weird.
> > > > I
> > > > > > > also
> > > > > > > > wonder whether we would want ANY to be separate from PROFILE0.
> That
> > > > might
> > > > > > vary
> > > > > > > > based on the API.
> > > > > > > > > > 
> > > > > > > > > > Perhaps sandersd or dalecurtis@ have thoughts.
> > > > > > > > > 
> > > > > > > > > Sergey, you say "the 4 VP9 profiles" above. Do we know those
are
> > set
> > > > in
> > > > > > > stone?
> > > > > > > > Or did you just make that value up? Does the vp9 team have any
> > insight
> > > > for
> > > > > > us?
> > > > > > > > > 
> > > > > > > > > If those are legit, I'm fine with adding them here now and
> > removing
> > > > the
> > > > > > _ANY
> > > > > > > > designator in place of MIN/MAX variants if endpoints don't want
to
> > > check
> > > > > the
> > > > > > > > values explicitly.
> > > > > > > > 
> > > > > > > > Thanks for the link. Lets just include those here then.
> > > > > > > 
> > > > > > > Ok, just to confirm that I got it right: so I'll replace the
> > > > vp9profile_any
> > > > > > with
> > > > > > > vp9profile_0..vp9profile_3 and will introduce the corresponding
> values
> > > in
> > > > > > other
> > > > > > > matching enums and ppapi, right? What are we going to do about
UMA?
> > > Should
> > > > > we
> > > > > > > just rename the vp9profile_any value in there into profile0 and
just
> > > keep
> > > > in
> > > > > > > mind to ourselves the fact that previously we reported any vp9
> profile
> > > as
> > > > > > > profile0 (speaking pragmatically, that's probably ok for now,
> there's
> > > not
> > > > a
> > > > > > lot
> > > > > > > of vp9 content yet, and I guess most of it is really in profile0).
> Or
> > do
> > > > we
> > > > > > want
> > > > > > > to point out the fact that previously reported values of vp9_any
> could
> > > > > include
> > > > > > > other profiles, besides profile0?
> > > > > > 
> > > > > > I've created a separate bug and a separate CL for adding the 4 VP9
> > > profiles:
> > > > > > https://codereview.chromium.org/1677133003
> > > > > > Let's keep this CL focused on HEVC and move the VP9 discussion
there.
> > > > > 
> > > > > ping
> > > > 
> > > > ping
> > > 
> > > Can you be more specific about what you are pinging for?
> > > It seems that most of the latest discussion around VP9 has moved to
> > > https://codereview.chromium.org/1769593002.
> > > The previous suggestion was to implement the VP9 values first, which would
> > mean
> > > this is blocked on that CL. If you think otherwise, you should say so.
> > 
> > I'm pinging to get approvals for content/, gpu/ and media/ changes. So far I
> > only got approvals for ppapi/ from raymes@ and tools/ from asvitkine@.
> content/
> > and gpu/ could be approved by sievers@. media/ could be any of the media
folks
> > on the reviewer list. I've initially added sandersd@ and you for media/ and
> > content/browser/media/media_canplaytype_browsertest.cc changes, but I have
> just
> > noticed that somebody also added wolenetz@ and xhwang@ (I guess it was
Dale?).
> > As I've explained in https://codereview.chromium.org/1769593002#msg7
currently
> > VP9 CL is based on HEVC CL. The HEVC CL (this one) is ready to land, I
think,
> > and I don't know if we need to hold it to wait for VP9 CL to land first. I
> think
> > the order doesn't matter, as the end result will be the same anyway and it
> would
> > allow me to start using the new HEVC codec parsing functions for further
> > Chromecast work without waiting for VP9 CL to go in (which is still waiting
> for
> > VP9 folks to look at). Since nobody responded to my comments about VP9 CL
> being
> > based on HEVC CL at https://codereview.chromium.org/1769593002#msg7, I
assumed
> > everybody agreed (or didn't care) about the ordering. If that's not the
case,
> > please let me know, I can rebase.
> 
> Okay, if you have multiple reviewers, you're more likely to get the attention
> you need if you include their ID.
> 
> As for the order, I think we should make sure that "+4" is the correct number
> and that we don't need a separate ANY to deal with ambiguity in the container.
> Thus, I think we should resolve the other CL first.

Ok, I've rebased the VP9 CL on top of Chromium master branch.

servolk

On 2016/03/11 23:12:35, servolk wrote: > On 2016/03/11 20:11:52, ddorwin wrote: > > On 2016/03/11 ...

4 years, 8 months ago (2016-04-01 19:10:21 UTC) #50

On 2016/03/11 23:12:35, servolk wrote:
> On 2016/03/11 20:11:52, ddorwin wrote:
> > On 2016/03/11 20:04:51, servolk wrote:
> > > On 2016/03/11 17:37:10, ddorwin wrote:
> > > > On 2016/03/11 01:45:46, servolk wrote:
> > > > > On 2016/03/10 01:00:34, servolk wrote:
> > > > > > On 2016/03/04 19:48:05, servolk wrote:
> > > > > > > On 2016/03/03 22:23:22, servolk wrote:
> > > > > > > > On 2016/03/03 22:17:20, DaleCurtis wrote:
> > > > > > > > >
> > > > > >
> > >
https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h
> > > > > > > > > File gpu/config/gpu_info.h (right):
> > > > > > > > > 
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1677133003/diff/450001/gpu/config/gpu_info.h#...
> > > > > > > > > gpu/config/gpu_info.h:56: HEVCPROFILE_MAIN = VP9PROFILE_ANY +
4,
> > > //=16
> > > > > > > > > On 2016/03/03 at 22:05:32, DaleCurtis wrote:
> > > > > > > > > > On 2016/03/03 at 22:00:21, ddorwin wrote:
> > > > > > > > > > > On 2016/03/02 01:47:45, servolk wrote:
> > > > > > > > > > > > On 2016/03/02 01:36:38, ddorwin wrote:
> > > > > > > > > > > > > Just remove the 16 entirely. It could rot (although
that
> > is
> > > > more
> > > > > > > > > unlikely in
> > > > > > > > > > > > > this case.
> > > > > > > > > > > > 
> > > > > > > > > > > > I'm starting to think that maybe it would be best to
just
> > add
> > > > the
> > > > > 4
> > > > > > > VP9
> > > > > > > > > profiles
> > > > > > > > > > > > in various enum already, even if they are going to be
just
> > > > > reserved,
> > > > > > > but
> > > > > > > > > unused
> > > > > > > > > > > > for now (kinda like I did in histograms.xml per
asvitkine@
> > > > > > > suggestion).
> > > > > > > > > WDYT?
> > > > > > > > > > > 
> > > > > > > > > > > I'm hesitant to define things we don't use, though +4 is
> also
> > > > weird.
> > > > > I
> > > > > > > > also
> > > > > > > > > wonder whether we would want ANY to be separate from PROFILE0.
> > That
> > > > > might
> > > > > > > vary
> > > > > > > > > based on the API.
> > > > > > > > > > > 
> > > > > > > > > > > Perhaps sandersd or dalecurtis@ have thoughts.
> > > > > > > > > > 
> > > > > > > > > > Sergey, you say "the 4 VP9 profiles" above. Do we know those
> are
> > > set
> > > > > in
> > > > > > > > stone?
> > > > > > > > > Or did you just make that value up? Does the vp9 team have any
> > > insight
> > > > > for
> > > > > > > us?
> > > > > > > > > > 
> > > > > > > > > > If those are legit, I'm fine with adding them here now and
> > > removing
> > > > > the
> > > > > > > _ANY
> > > > > > > > > designator in place of MIN/MAX variants if endpoints don't
want
> to
> > > > check
> > > > > > the
> > > > > > > > > values explicitly.
> > > > > > > > > 
> > > > > > > > > Thanks for the link. Lets just include those here then.
> > > > > > > > 
> > > > > > > > Ok, just to confirm that I got it right: so I'll replace the
> > > > > vp9profile_any
> > > > > > > with
> > > > > > > > vp9profile_0..vp9profile_3 and will introduce the corresponding
> > values
> > > > in
> > > > > > > other
> > > > > > > > matching enums and ppapi, right? What are we going to do about
> UMA?
> > > > Should
> > > > > > we
> > > > > > > > just rename the vp9profile_any value in there into profile0 and
> just
> > > > keep
> > > > > in
> > > > > > > > mind to ourselves the fact that previously we reported any vp9
> > profile
> > > > as
> > > > > > > > profile0 (speaking pragmatically, that's probably ok for now,
> > there's
> > > > not
> > > > > a
> > > > > > > lot
> > > > > > > > of vp9 content yet, and I guess most of it is really in
profile0).
> > Or
> > > do
> > > > > we
> > > > > > > want
> > > > > > > > to point out the fact that previously reported values of vp9_any
> > could
> > > > > > include
> > > > > > > > other profiles, besides profile0?
> > > > > > > 
> > > > > > > I've created a separate bug and a separate CL for adding the 4 VP9
> > > > profiles:
> > > > > > > https://codereview.chromium.org/1677133003
> > > > > > > Let's keep this CL focused on HEVC and move the VP9 discussion
> there.
> > > > > > 
> > > > > > ping
> > > > > 
> > > > > ping
> > > > 
> > > > Can you be more specific about what you are pinging for?
> > > > It seems that most of the latest discussion around VP9 has moved to
> > > > https://codereview.chromium.org/1769593002.
> > > > The previous suggestion was to implement the VP9 values first, which
would
> > > mean
> > > > this is blocked on that CL. If you think otherwise, you should say so.
> > > 
> > > I'm pinging to get approvals for content/, gpu/ and media/ changes. So far
I
> > > only got approvals for ppapi/ from raymes@ and tools/ from asvitkine@.
> > content/
> > > and gpu/ could be approved by sievers@. media/ could be any of the media
> folks
> > > on the reviewer list. I've initially added sandersd@ and you for media/
and
> > > content/browser/media/media_canplaytype_browsertest.cc changes, but I have
> > just
> > > noticed that somebody also added wolenetz@ and xhwang@ (I guess it was
> Dale?).
> > > As I've explained in https://codereview.chromium.org/1769593002#msg7
> currently
> > > VP9 CL is based on HEVC CL. The HEVC CL (this one) is ready to land, I
> think,
> > > and I don't know if we need to hold it to wait for VP9 CL to land first. I
> > think
> > > the order doesn't matter, as the end result will be the same anyway and it
> > would
> > > allow me to start using the new HEVC codec parsing functions for further
> > > Chromecast work without waiting for VP9 CL to go in (which is still
waiting
> > for
> > > VP9 folks to look at). Since nobody responded to my comments about VP9 CL
> > being
> > > based on HEVC CL at https://codereview.chromium.org/1769593002#msg7, I
> assumed
> > > everybody agreed (or didn't care) about the ordering. If that's not the
> case,
> > > please let me know, I can rebase.
> > 
> > Okay, if you have multiple reviewers, you're more likely to get the
attention
> > you need if you include their ID.
> > 
> > As for the order, I think we should make sure that "+4" is the correct
number
> > and that we don't need a separate ANY to deal with ambiguity in the
container.
> > Thus, I think we should resolve the other CL first.
> 
> Ok, I've rebased the VP9 CL on top of Chromium master branch.

Now that VP9 profiles CL has landed, we can get back to this. I have rebased the
latest patchset + removed PPAPI changes like we did in VP9 CL. Please take a
look.
David, I know this will probably conflict with
https://codereview.chromium.org/1728193004/ (in
content/browser/media/media_canplaytype_browsertest.cc). I'll rebase this one as
soon as that one lands.

ddorwin

I did not review the HEVC details in: content/browser/media/media_canplaytype_browsertest.cc media/base/video_codecs.cc Please get someone to specifically ...

4 years, 8 months ago (2016-04-02 00:09:06 UTC) #51

servolk

https://codereview.chromium.org/1677133003/diff/490001/media/base/mime_util_internal.cc File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1677133003/diff/490001/media/base/mime_util_internal.cc#newcode517 media/base/mime_util_internal.cc:517: #if BUILDFLAG(ENABLE_HEVC_DEMUXING) On 2016/04/02 00:09:06, ddorwin wrote: > I ...

4 years, 8 months ago (2016-04-02 00:35:04 UTC) #52

ddorwin

https://codereview.chromium.org/1677133003/diff/490001/media/base/mime_util_internal.cc File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1677133003/diff/490001/media/base/mime_util_internal.cc#newcode502 media/base/mime_util_internal.cc:502: bool* is_ambiguous) const { As you mentioned below, this ...

4 years, 8 months ago (2016-04-02 00:56:09 UTC) #53

ddorwin

https://codereview.chromium.org/1677133003/diff/490001/media/base/mime_util_internal.cc File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1677133003/diff/490001/media/base/mime_util_internal.cc#newcode519 media/base/mime_util_internal.cc:519: *is_ambiguous = false; On 2016/04/02 00:56:09, ddorwin wrote: > ...

4 years, 8 months ago (2016-04-06 01:43:56 UTC) #54

servolk

https://codereview.chromium.org/1677133003/diff/490001/media/base/mime_util_internal.cc File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1677133003/diff/490001/media/base/mime_util_internal.cc#newcode502 media/base/mime_util_internal.cc:502: bool* is_ambiguous) const { On 2016/04/02 00:56:09, ddorwin wrote: ...

4 years, 8 months ago (2016-04-07 00:59:40 UTC) #55

https://codereview.chromium.org/1677133003/diff/490001/media/base/mime_util_i...
File media/base/mime_util_internal.cc (right):

https://codereview.chromium.org/1677133003/diff/490001/media/base/mime_util_i...
media/base/mime_util_internal.cc:502: bool* is_ambiguous) const {
On 2016/04/02 00:56:09, ddorwin wrote:
> As you mentioned below, this is confusing. Perhaps (in a separate CL) we
should
> change to is_unsure, is_maybe, or something like that. (It's the opposite of
"is
> known supported.)
> 
> The only case where anything is actually _ambiguous_ now is in
> kAmbiguousCodecStringMap.

Acknowledged.

https://codereview.chromium.org/1677133003/diff/490001/media/base/mime_util_i...
media/base/mime_util_internal.cc:519: *is_ambiguous = false;
On 2016/04/06 01:43:56, ddorwin wrote:
> On 2016/04/02 00:56:09, ddorwin wrote:
> > On 2016/04/02 00:35:04, servolk wrote:
> > > On 2016/04/02 00:09:06, ddorwin wrote:
> > > > This assumes that only supported profiles are returned (at least DCHECK
> > that)
> > > > AND that the level is valid. Is the latter a safe assumption?
> > > 
> > > Well, HEVC codec ids contain enough information to be unambiguous, so I've
> set
> > > it to false here. And yes, there are already a few CHECKs inside
> > > ParseHEVCCodecId function that ensure that returned values are within
valid
> > > ranges. The actual issue here is that this is_ambiguous flag is actually
> used
> > to
> > > decide whether we need to return 'maybe' or 'probably' as CanPlayType
> result,
> > > but the problem is that we  don't have enough information here to decide
> that.
> > > We'll need to ask actual platform/decoder whether this combination of
> > > profile/level is supported or not. That part is not implemented yet.
> > 
> > See above. Ambiguous no longer makes sense, BUT we should prefer "maybe" for
> > things we are unsure about. In this case, until the platform checking is
> added.
> > This should not affect most apps.
> > 
> > This will cause the DCHECK in AddContainerWithCodecs() to fire, but,
> especially
> > since we want to eliminate this call there (https://crbug.com/461009), you
> could
> > just add "|| codec==HEVC".
> 
> That bug is now fixed, so you don't need to worry about the DCHECK.
> https://codereview.chromium.org/1864743002/

Done.

ddorwin

https://codereview.chromium.org/1677133003/diff/590001/media/base/mime_util_internal.cc File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1677133003/diff/590001/media/base/mime_util_internal.cc#newcode576 media/base/mime_util_internal.cc:576: // tests expect. We'll need to add platform level ...

4 years, 8 months ago (2016-04-07 20:29:46 UTC) #56

servolk

https://codereview.chromium.org/1677133003/diff/590001/media/base/mime_util_internal.cc File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1677133003/diff/590001/media/base/mime_util_internal.cc#newcode576 media/base/mime_util_internal.cc:576: // tests expect. We'll need to add platform level ...

4 years, 8 months ago (2016-04-07 20:44:03 UTC) #57

ddorwin

https://codereview.chromium.org/1677133003/diff/590001/media/base/mime_util_internal.cc File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1677133003/diff/590001/media/base/mime_util_internal.cc#newcode576 media/base/mime_util_internal.cc:576: // tests expect. We'll need to add platform level ...

4 years, 8 months ago (2016-04-08 20:43:24 UTC) #58

servolk

https://codereview.chromium.org/1677133003/diff/590001/media/base/mime_util_internal.cc File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1677133003/diff/590001/media/base/mime_util_internal.cc#newcode576 media/base/mime_util_internal.cc:576: // tests expect. We'll need to add platform level ...

4 years, 8 months ago (2016-04-08 22:11:45 UTC) #59

servolk

On 2016/04/08 22:11:45, servolk wrote: > https://codereview.chromium.org/1677133003/diff/590001/media/base/mime_util_internal.cc > File media/base/mime_util_internal.cc (right): > > https://codereview.chromium.org/1677133003/diff/590001/media/base/mime_util_internal.cc#newcode576 > ...

4 years, 8 months ago (2016-04-11 19:56:39 UTC) #60

On 2016/04/08 22:11:45, servolk wrote:
>
https://codereview.chromium.org/1677133003/diff/590001/media/base/mime_util_i...
> File media/base/mime_util_internal.cc (right):
> 
>
https://codereview.chromium.org/1677133003/diff/590001/media/base/mime_util_i...
> media/base/mime_util_internal.cc:576: // tests expect. We'll need to add
> platform level checks to ensure that the
> On 2016/04/08 20:43:24, ddorwin wrote:
> > On 2016/04/07 20:44:03, servolk wrote:
> > > On 2016/04/07 20:29:46, ddorwin wrote:
> > > > What tests expect 'probably'? Can't we just update these?
> > > > The point is that we should err on the side of underpromising, which is
> > > 'maybe'.
> > > 
> > > The MediaCanPlayTypeTest.CodecSupportTest_HEVCVariants test that I've
added
> in
> > > content/browser/media/media_canplaytype_browsertest.cc in this CL (see
> > > cast_shell trybot failure in PS #30). I guess we can update that test to
> > expect
> > > 'maybe', but then it would fail on platforms that do actually support
HEVC.
> > > Since Chromecast is the only platform that enables HEVC I think it's ok to
> > > overpromise here for the time being, while we are working on better
> > > platform-level checks.
> > 
> > If there are profiles, level, etc. combinations that will always be
supported
> > when this build flag is enabled, you could report "probably" for those, but
> > currently this would make requests for 12-bit and whatever else return
> > "probably". If the results are meaningless, authors will learn to ignore
them.
> > 
> > As for the tests, you can change kHevcSupported to "maybe" with a TODO
noting
> > this should be "probably" for some, preferably with a crbug ID.
> > 
> > We are going to need to come up with a new solution for those tests when we
> add
> > platform querying anyway.
> 
> Done.

Ping. sanders@ could you please take a look at media/ changes? Or recommend
somebody else from media/OWNERS who could take a look at this?

servolk

On 2016/04/11 19:56:39, servolk wrote: > On 2016/04/08 22:11:45, servolk wrote: > > > https://codereview.chromium.org/1677133003/diff/590001/media/base/mime_util_internal.cc ...

4 years, 8 months ago (2016-04-15 17:19:39 UTC) #61

On 2016/04/11 19:56:39, servolk wrote:
> On 2016/04/08 22:11:45, servolk wrote:
> >
>
https://codereview.chromium.org/1677133003/diff/590001/media/base/mime_util_i...
> > File media/base/mime_util_internal.cc (right):
> > 
> >
>
https://codereview.chromium.org/1677133003/diff/590001/media/base/mime_util_i...
> > media/base/mime_util_internal.cc:576: // tests expect. We'll need to add
> > platform level checks to ensure that the
> > On 2016/04/08 20:43:24, ddorwin wrote:
> > > On 2016/04/07 20:44:03, servolk wrote:
> > > > On 2016/04/07 20:29:46, ddorwin wrote:
> > > > > What tests expect 'probably'? Can't we just update these?
> > > > > The point is that we should err on the side of underpromising, which
is
> > > > 'maybe'.
> > > > 
> > > > The MediaCanPlayTypeTest.CodecSupportTest_HEVCVariants test that I've
> added
> > in
> > > > content/browser/media/media_canplaytype_browsertest.cc in this CL (see
> > > > cast_shell trybot failure in PS #30). I guess we can update that test to
> > > expect
> > > > 'maybe', but then it would fail on platforms that do actually support
> HEVC.
> > > > Since Chromecast is the only platform that enables HEVC I think it's ok
to
> > > > overpromise here for the time being, while we are working on better
> > > > platform-level checks.
> > > 
> > > If there are profiles, level, etc. combinations that will always be
> supported
> > > when this build flag is enabled, you could report "probably" for those,
but
> > > currently this would make requests for 12-bit and whatever else return
> > > "probably". If the results are meaningless, authors will learn to ignore
> them.
> > > 
> > > As for the tests, you can change kHevcSupported to "maybe" with a TODO
> noting
> > > this should be "probably" for some, preferably with a crbug ID.
> > > 
> > > We are going to need to come up with a new solution for those tests when
we
> > add
> > > platform querying anyway.
> > 
> > Done.
> 
> Ping. sanders@ could you please take a look at media/ changes? Or recommend
> somebody else from media/OWNERS who could take a look at this?

Ping. sandersd@ could you take a look please?

sandersd (OOO until July 31)

Looking good, just some media/ style nits. https://codereview.chromium.org/1677133003/diff/650001/media/base/video_codecs.cc File media/base/video_codecs.cc (right): https://codereview.chromium.org/1677133003/diff/650001/media/base/video_codecs.cc#newcode212 media/base/video_codecs.cc:212: CHECK(general_profile_space >= ...

4 years, 8 months ago (2016-04-20 18:13:54 UTC) #62

servolk

https://codereview.chromium.org/1677133003/diff/650001/media/base/video_codecs.cc File media/base/video_codecs.cc (right): https://codereview.chromium.org/1677133003/diff/650001/media/base/video_codecs.cc#newcode212 media/base/video_codecs.cc:212: CHECK(general_profile_space >= 0 && general_profile_space <= 3); On 2016/04/20 ...

4 years, 8 months ago (2016-04-20 18:21:11 UTC) #63

sandersd (OOO until July 31)

LGTM % nit. https://codereview.chromium.org/1677133003/diff/670001/media/base/video_codecs.cc File media/base/video_codecs.cc (right): https://codereview.chromium.org/1677133003/diff/670001/media/base/video_codecs.cc#newcode250 media/base/video_codecs.cc:250: CHECK(general_tier_flag == 0 || general_tier_flag == ...

4 years, 8 months ago (2016-04-20 18:23:02 UTC) #64

servolk

https://codereview.chromium.org/1677133003/diff/670001/media/base/video_codecs.cc File media/base/video_codecs.cc (right): https://codereview.chromium.org/1677133003/diff/670001/media/base/video_codecs.cc#newcode250 media/base/video_codecs.cc:250: CHECK(general_tier_flag == 0 || general_tier_flag == 1); On 2016/04/20 ...

4 years, 8 months ago (2016-04-20 18:24:16 UTC) #65

servolk

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

4 years, 8 months ago (2016-04-20 18:27:27 UTC) #66

servolk

On 2016/04/20 18:24:16, servolk wrote: > https://codereview.chromium.org/1677133003/diff/670001/media/base/video_codecs.cc > File media/base/video_codecs.cc (right): > > https://codereview.chromium.org/1677133003/diff/670001/media/base/video_codecs.cc#newcode250 > ...

4 years, 8 months ago (2016-04-20 18:27:47 UTC) #67

ddorwin

https://codereview.chromium.org/1677133003/diff/690001/media/base/video_codecs.cc File media/base/video_codecs.cc (right): https://codereview.chromium.org/1677133003/diff/690001/media/base/video_codecs.cc#newcode208 media/base/video_codecs.cc:208: if (elem[1][0] == 'A' || elem[1][0] == 'B' || ...

4 years, 8 months ago (2016-04-20 21:10:56 UTC) #69

servolk

https://codereview.chromium.org/1677133003/diff/690001/media/base/video_codecs.cc File media/base/video_codecs.cc (right): https://codereview.chromium.org/1677133003/diff/690001/media/base/video_codecs.cc#newcode208 media/base/video_codecs.cc:208: if (elem[1][0] == 'A' || elem[1][0] == 'B' || ...

4 years, 8 months ago (2016-04-20 21:46:06 UTC) #70

ddorwin

https://codereview.chromium.org/1677133003/diff/710001/content/browser/media/media_canplaytype_browsertest.cc File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1677133003/diff/710001/content/browser/media/media_canplaytype_browsertest.cc#newcode1263 content/browser/media/media_canplaytype_browsertest.cc:1263: EXPECT_EQ(kNot, CanPlay("'video/mp4; codecs=hvc1.'")); Add this test without the extra ...

4 years, 8 months ago (2016-04-20 22:28:15 UTC) #71

servolk

https://codereview.chromium.org/1677133003/diff/710001/content/browser/media/media_canplaytype_browsertest.cc File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1677133003/diff/710001/content/browser/media/media_canplaytype_browsertest.cc#newcode1263 content/browser/media/media_canplaytype_browsertest.cc:1263: EXPECT_EQ(kNot, CanPlay("'video/mp4; codecs=hvc1.'")); On 2016/04/20 22:28:15, ddorwin wrote: > ...

4 years, 8 months ago (2016-04-20 23:14:46 UTC) #72

https://codereview.chromium.org/1677133003/diff/710001/content/browser/media/...
File content/browser/media/media_canplaytype_browsertest.cc (right):

https://codereview.chromium.org/1677133003/diff/710001/content/browser/media/...
content/browser/media/media_canplaytype_browsertest.cc:1263: EXPECT_EQ(kNot,
CanPlay("'video/mp4; codecs=hvc1.'"));
On 2016/04/20 22:28:15, ddorwin wrote:
> Add this test without the extra dot.
> Actually, if anything, having a trailing dot after the third element (l1265 +
> '.') makes the most sense.

Done.

https://codereview.chromium.org/1677133003/diff/710001/content/browser/media/...
content/browser/media/media_canplaytype_browsertest.cc:1266: 
On 2016/04/20 22:28:15, ddorwin wrote:
> Test consecutive periods (a lot). These are allowed by the VP9 in MP4
proposal,
> which makes me think they might be allowed here. Our parsing at least makes
this
> something we should test.
> 
> Examples:
> hvc1...
> hvc1....
> hvc1.1..
> hvc1.1.6..
> hvc1.1.6.L93.
> hvc1.1.6.L93..
> hvc1..6.L93
> hvc1...6.L93
> hvc1...L93
> hvc1....L93
> hvc1.1..L93
> hvc1.1...L93
> 
> Do similar for the constraint bytes:
> hvc1.1.6.L0.0.
> hvc1.1.6.L0.0..
> hvc1.1.6.L0.0..0
> hvc1.1.6.L0.0..0.0.0.0
> hvc1.1.6.L0.0.0.0.0.0.0.
> hvc1.1.6.L0......0
> hvc1.1.6.L0......

Done.

https://codereview.chromium.org/1677133003/diff/710001/media/base/video_codec...
File media/base/video_codecs.cc (right):

https://codereview.chromium.org/1677133003/diff/710001/media/base/video_codec...
media/base/video_codecs.cc:204: codec_id, ".", base::TRIM_WHITESPACE,
base::SPLIT_WANT_ALL);
On 2016/04/20 22:28:15, ddorwin wrote:
> What happens with consecutive periods? If it creates an empty element, is that
> handled correctly?
> 
> Let's add tests.

Yes, base::SPLIT_WANT_ALL means we get empty elements if there's nothing in
between two dots.

Done.

https://codereview.chromium.org/1677133003/diff/710001/media/base/video_codec...
media/base/video_codecs.cc:270: // Remove everything up to the first
constraint_flags byte
On 2016/04/20 22:28:15, ddorwin wrote:
> This seems a little strange when we could just change the starting position.
> Also, there is unnecessary modification of the data. Could we instead use an
> iterator in the for loop?

Done.

https://codereview.chromium.org/1677133003/diff/710001/media/base/video_codec...
media/base/video_codecs.cc:272: if (elem.size() > 6) {
On 2016/04/20 22:28:15, ddorwin wrote:
> In particular, it's not immediately clear that this is really allowing up to
10
> elements (nine periods). This is minor, though it does require a little more
> thought.

Since we no longer remove the first elements, the number 10 is more explicit
now.

ddorwin

Thanks. LGTM (for the files I reviewed*) with question. * As before, I did not ...

4 years, 8 months ago (2016-04-20 23:32:42 UTC) #73

servolk

Thanks for the reviews, everyone. https://codereview.chromium.org/1677133003/diff/700013/media/base/video_codecs.cc File media/base/video_codecs.cc (right): https://codereview.chromium.org/1677133003/diff/700013/media/base/video_codecs.cc#newcode218 media/base/video_codecs.cc:218: DCHECK(general_profile_space >= 0 && ...

4 years, 8 months ago (2016-04-20 23:57:48 UTC) #74

ddorwin

https://codereview.chromium.org/1677133003/diff/700013/media/base/video_codecs.cc File media/base/video_codecs.cc (right): https://codereview.chromium.org/1677133003/diff/700013/media/base/video_codecs.cc#newcode218 media/base/video_codecs.cc:218: DCHECK(general_profile_space >= 0 && general_profile_space <= 3); On 2016/04/20 ...

4 years, 8 months ago (2016-04-21 00:00:32 UTC) #75

servolk

https://codereview.chromium.org/1677133003/diff/700013/media/base/video_codecs.cc File media/base/video_codecs.cc (right): https://codereview.chromium.org/1677133003/diff/700013/media/base/video_codecs.cc#newcode218 media/base/video_codecs.cc:218: DCHECK(general_profile_space >= 0 && general_profile_space <= 3); On 2016/04/21 ...

4 years, 8 months ago (2016-04-21 00:13:27 UTC) #76

servolk

The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, raymes@chromium.org, sandersd@chromium.org, piman@chromium.org ...

4 years, 8 months ago (2016-04-21 02:39:58 UTC) #78