Implemented HEVC video demuxing and parsing

Implemented HEVC video demuxing and parsing

Chromecast team is interested in adding HEVC/H265 video codec support. Since we are using hardware decoders on our platforms, we don't need to add software decoder, but we still need to be able to demux and parse HEVC in mp4 containers. HEVC is very similar to H264 in many aspects, so we can reuse a lot of the existing H264 functionality.

BUG=454948
Committed: https://crrev.com/4585056bcf1491d19262aaef0577542840f54f19
Cr-Commit-Position: refs/heads/master@{#347814}

[servolk, 2015-01-16 01:56:57]

servolk@chromium.org changed reviewers:
+ gunsch@chromium.org, lcwu@chromium.org

[servolk, 2015-02-06 01:53:08]

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

[DaleCurtis, 2015-02-06 02:19:17]

Haven't looked much, but you'll need to protect all the files under some sort of
gyp/gn variable to ensure we don't compile these files on desktop platforms.

[servolk, 2015-02-06 02:37:50]

On 2015/02/06 02:19:17, DaleCurtis wrote:
> Haven't looked much, but you'll need to protect all the files under some sort
of
> gyp/gn variable to ensure we don't compile these files on desktop platforms.

Yep, I'm planning to do something like that, now that this CL is almost
complete.

[DaleCurtis, 2015-02-06 19:08:29]

You should also use it to gate include of the hevc source files.

https://codereview.chromium.org/816353010/diff/320001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/816353010/diff/320001/build/common.gypi#newco...
build/common.gypi:1475: # HEVC/H265 demuxing support. Uses FFmpeg demuxer, so
for this to work
This isn't allowed, you need to limit your option to media/.

[servolk, 2015-02-06 23:42:16]

On 2015/02/06 19:08:29, DaleCurtis wrote:
> You should also use it to gate include of the hevc source files.

I already did this in the latest patch. Both inclusion of hevc source files in
the .gyp file and #include of hevc/h265 headers are gated by
enable_hevc_demuxing. Or did I miss something?

> https://codereview.chromium.org/816353010/diff/320001/build/common.gypi
> File build/common.gypi (right):
> 
>
https://codereview.chromium.org/816353010/diff/320001/build/common.gypi#newco...
> build/common.gypi:1475: # HEVC/H265 demuxing support. Uses FFmpeg demuxer, so
> for this to work
> This isn't allowed, you need to limit your option to media/.

Why is this not allowed? How is this different from enable_mpeg2ts_stream_parser
in build/common.gypi?
I understand that ideally we might want this flag to be scoped to only media/,
but unfortunately mime type checks still live in net/ so we need to use bigger
scope here. I've started some work on trying to move media mime type checks out
of net/ into media/ (https://codereview.chromium.org/877323009/), but given net/
folks requests to move all (and not just media) mime type checks out of net/,
and given my unfamiliarity with the rest of the code, I'm not sure how long it
might take.

[DaleCurtis, 2015-02-06 23:48:34]

On 2015/02/06 23:42:16, servolk wrote:
> On 2015/02/06 19:08:29, DaleCurtis wrote:
> > You should also use it to gate include of the hevc source files.
> 
> I already did this in the latest patch. Both inclusion of hevc source files in
> the .gyp file and #include of hevc/h265 headers are gated by
> enable_hevc_demuxing. Or did I miss something?
> 
> > https://codereview.chromium.org/816353010/diff/320001/build/common.gypi
> > File build/common.gypi (right):
> > 
> >
>
https://codereview.chromium.org/816353010/diff/320001/build/common.gypi#newco...
> > build/common.gypi:1475: # HEVC/H265 demuxing support. Uses FFmpeg demuxer,
so
> > for this to work
> > This isn't allowed, you need to limit your option to media/.
> 
> Why is this not allowed? How is this different from
enable_mpeg2ts_stream_parser
> in build/common.gypi?
> I understand that ideally we might want this flag to be scoped to only media/,
> but unfortunately mime type checks still live in net/ so we need to use bigger
> scope here. I've started some work on trying to move media mime type checks
out
> of net/ into media/ (https://codereview.chromium.org/877323009/), but given
net/
> folks requests to move all (and not just media) mime type checks out of net/,
> and given my unfamiliarity with the rest of the code, I'm not sure how long it
> might take.

You won't be able to get a new flag here past the build/ reviewer since it
results in the define being added to all 17000+ chrome compilation targets, see
sandersd@'s recent attempt: https://codereview.chromium.org/772043004/ -- As a
temporary stop gap the net/ folk might allow a copy of this flag in net/, but I
think we might be at a stand still until the net/ code can be extracted. Sorry
for the pain.

[ddorwin, 2015-02-21 02:09:20]

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

[ddorwin, 2015-02-21 02:09:22]

Comments on mime_util.cc / canPlayType().

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:530: #if defined(ENABLE_HEVC_DEMUXING)
"DEMUXING" is not really accurate. This enables support in general.

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:531: "hev1,hvc1,"
See below.

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:567: {"hev1", MimeUtil::HEVC_MAIN},
I don't think we should support these extension-less versions. It was a mistake
to support "avc1", which is not spec-compliant, and now we are stuck supporting
it forever.

Are there really two separate variants? Why? Can we pick one and lead the
industry in that direction?

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:1023: if (StartsWithASCII(codec_id, "hev1.", true) ||
Once the other issues are addressed, you'll need to update
media_canplaytype_browsertest.cc. (Wait for
https://codereview.chromium.org/918463002/.)

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:1025: // TODO(servolk): Implement parsing of hevc codec
ids as described in
Our avc1 string handling is a mess. Similar to my comments above, we should be
accurate and restrictive from the beginning to avoid similar problems.

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:1029: *is_ambiguous = false;
This should be true since you do no verification.

[ddorwin, 2015-02-21 02:44:40]

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:567: {"hev1", MimeUtil::HEVC_MAIN},
On 2015/02/21 02:09:21, ddorwin wrote:
> I don't think we should support these extension-less versions. It was a
mistake
> to support "avc1", which is not spec-compliant, and now we are stuck
supporting
> it forever.
To clarify, this may not have been so much a mistake as needing to be compatible
with other browsers that supported it. But as the first (?) UA to support HEVC,
please try to avoid setting such a precedent.
> 
> Are there really two separate variants? Why? Can we pick one and lead the
> industry in that direction?

[servolk, 2015-02-23 19:16:17]

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:530: #if defined(ENABLE_HEVC_DEMUXING)
On 2015/02/21 02:09:21, ddorwin wrote:
> "DEMUXING" is not really accurate. This enables support in general.

No, I believe this is accurate. What exactly do you mean by "support in
general"? This change doesn't contain anything to support h265 decoding (which
would require us to deal with h265 licensing), it only enables demuxing and
parsing (which doesn't require any licenses) and this will allow us to support
hevc/h265 on platforms which have "external" h265 decoder, e.g. hardware h265
decoder, for which license costs are going to be paid by hardware manufacturer,
or OS-provided h265 decoder, for which license costs are going to be paid by OS
vendor. So, again, I believe the term "demuxing" is correct in this context, and
in fact we should be very careful about this, since changing it to something
more generic, like "enable_hevc" could get us in trouble by not being clear
enough that all we do is demuxing. Please let me know if you disagree and we can
discuss further.

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:567: {"hev1", MimeUtil::HEVC_MAIN},
On 2015/02/21 02:09:21, ddorwin wrote:
> I don't think we should support these extension-less versions. It was a
mistake
> to support "avc1", which is not spec-compliant, and now we are stuck
supporting
> it forever.
> 
> Are there really two separate variants? Why? Can we pick one and lead the
> industry in that direction?

As far as I know the difference between hev1 and hvc1 is analogous to that
between avc1 and avc3. It's defined in ISO/IEC 14496-15 (for avc3/hev1 decoder
params PPS/SPS/VPS are embedded in elementary stream, whereas avc1/hvc1 contain
all params in avcC/hevC record). Chrome actually supports both avc1 and avc3, so
I think we should do the same with hev1/hvc1 - i.e. we should support both, they
are both standard but cover slightly different use cases, which gives some
flexibility to encoders. My understanding is that people do really need to have
both, one is typically used e.g. in live streaming scenarios, where it's
essential to be able to change decoder parameters on the fly to accommodate
bandwidth and content bitrate changes. Whereas another one is typically used by
offline encoders, which can afford to preprocess the whole media stream
beforehand, collect all the necessary parameter sets and write them down in the
decoder config box of the MP4 file. It doesn't really matter if we are the first
UA to support HEVC (I doubt it), we don't get to pick and choose. There is an
international standard ISO/IEC 14496-15 and we need to try and implement/support
as much of it as possible. I don't see a good reason to go against the standard
and support only one type, when the standard clearly says there's two and
clearly specifies the difference between the two.
Btw, other media stacks/libraries are also going to support both, for example
take a look at FFmpeg's libavformat/isom.c - they also support both hev1 and
hvc1.

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:1023: if (StartsWithASCII(codec_id, "hev1.", true) ||
On 2015/02/21 02:09:21, ddorwin wrote:
> Once the other issues are addressed, you'll need to update
> media_canplaytype_browsertest.cc. (Wait for
> https://codereview.chromium.org/918463002/.)

Ok, yeah, once we have more clarity here we'll try to add more unit tests,
including mime type tests.

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:1025: // TODO(servolk): Implement parsing of hevc codec
ids as described in
On 2015/02/21 02:09:21, ddorwin wrote:
> Our avc1 string handling is a mess. Similar to my comments above, we should be
> accurate and restrictive from the beginning to avoid similar problems.

I understand your concerns, but we are in the early stages of implementation
here. We just don't know yet which profiles our hardware partners are going to
need/support. So for now we'd prefer to allow them all, to facilitate testing.

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:1029: *is_ambiguous = false;
yep, that's a good point, I'll change this.

[ddorwin, 2015-02-23 20:00:50]

https://codereview.chromium.org/816353010/diff/360001/media/filters/ffmpeg_de...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/816353010/diff/360001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer.cc:467: } else if (stream_->codec->codec_id ==
AV_CODEC_ID_HEVC) {
Do you expect FFmpeg to be built with such support?

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights
reserved.
If you intend to support MSE, there may be other code that needs to be updated
to support these specific codecs.

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:530: #if defined(ENABLE_HEVC_DEMUXING)
On 2015/02/23 19:16:16, servolk wrote:
> On 2015/02/21 02:09:21, ddorwin wrote:
> > "DEMUXING" is not really accurate. This enables support in general.
> 
> No, I believe this is accurate. What exactly do you mean by "support in
> general"? This change doesn't contain anything to support h265 decoding (which
> would require us to deal with h265 licensing), it only enables demuxing and
> parsing (which doesn't require any licenses) and this will allow us to support
> hevc/h265 on platforms which have "external" h265 decoder, e.g. hardware h265
> decoder, for which license costs are going to be paid by hardware
manufacturer,
> or OS-provided h265 decoder, for which license costs are going to be paid by
OS
> vendor. So, again, I believe the term "demuxing" is correct in this context,
and
> in fact we should be very careful about this, since changing it to something
> more generic, like "enable_hevc" could get us in trouble by not being clear
> enough that all we do is demuxing. Please let me know if you disagree and we
can
> discuss further.

The primary "support" it enables is advertising support via canPlayType().

Regardless of what this is called, we're clearly not enabling a software
decoder, but it seems reasonable to reuse this value if one needed to use it to
enable code around their decoder.

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:531: "hev1,hvc1,"
On 2015/02/21 02:09:21, ddorwin wrote:
> See below.

See some of the comments about how avc1 is handled in
https://codereview.chromium.org/952673002/. We should only have a single
unambiguous HEVC_MAIN string here.

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:567: {"hev1", MimeUtil::HEVC_MAIN},
On 2015/02/23 19:16:16, servolk wrote:
> On 2015/02/21 02:09:21, ddorwin wrote:
> > I don't think we should support these extension-less versions. It was a
> mistake
> > to support "avc1", which is not spec-compliant, and now we are stuck
> supporting
> > it forever.
> > 
> > Are there really two separate variants? Why? Can we pick one and lead the
> > industry in that direction?
> 
> As far as I know the difference between hev1 and hvc1 is analogous to that
> between avc1 and avc3. It's defined in ISO/IEC 14496-15 (for avc3/hev1 decoder
> params PPS/SPS/VPS are embedded in elementary stream, whereas avc1/hvc1
contain
> all params in avcC/hevC record). Chrome actually supports both avc1 and avc3,
so
> I think we should do the same with hev1/hvc1 - i.e. we should support both,
they
> are both standard but cover slightly different use cases, which gives some
> flexibility to encoders. My understanding is that people do really need to
have
> both, one is typically used e.g. in live streaming scenarios, where it's
> essential to be able to change decoder parameters on the fly to accommodate
> bandwidth and content bitrate changes. Whereas another one is typically used
by
> offline encoders, which can afford to preprocess the whole media stream
> beforehand, collect all the necessary parameter sets and write them down in
the
> decoder config box of the MP4 file. It doesn't really matter if we are the
first
> UA to support HEVC (I doubt it), we don't get to pick and choose. There is an
> international standard ISO/IEC 14496-15 and we need to try and
implement/support
> as much of it as possible. I don't see a good reason to go against the
standard
> and support only one type, when the standard clearly says there's two and
> clearly specifies the difference between the two.
> Btw, other media stacks/libraries are also going to support both, for example
> take a look at FFmpeg's libavformat/isom.c - they also support both hev1 and
> hvc1.

Thanks for the exmplanation of "hev" vs. "hvc". I thought maybe there was no
clear decision in this area (vs. being defined to be different by a spec). I
agree that supporting both makes sense if it is like avc1 vs. avc3.

However, I still don't think we should support the extensionless strings (unless
that is also specified to be valid by some spec). (For avc1, it is not
spec-compliant.) This should be in the Ambiguous array like avc1/avc3.

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:1022: #if defined(ENABLE_HEVC_DEMUXING)
Move this code to ParseHEVCCodecID or something like that.
We should try to keep this function as simple as possible.

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:1023: if (StartsWithASCII(codec_id, "hev1.", true) ||
On 2015/02/23 19:16:16, servolk wrote:
> On 2015/02/21 02:09:21, ddorwin wrote:
> > Once the other issues are addressed, you'll need to update
> > media_canplaytype_browsertest.cc. (Wait for
> > https://codereview.chromium.org/918463002/.)
> 
> Ok, yeah, once we have more clarity here we'll try to add more unit tests,
> including mime type tests.

You should add at least some basic tests soon. This code is complex and
difficult to get right.

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:1025: // TODO(servolk): Implement parsing of hevc codec
ids as described in
On 2015/02/23 19:16:16, servolk wrote:
> On 2015/02/21 02:09:21, ddorwin wrote:
> > Our avc1 string handling is a mess. Similar to my comments above, we should
be
> > accurate and restrictive from the beginning to avoid similar problems.
> 
> I understand your concerns, but we are in the early stages of implementation
> here. We just don't know yet which profiles our hardware partners are going to
> need/support. So for now we'd prefer to allow them all, to facilitate testing.

Can we at least check for valid values, similar to IsValidH264Level()?

[ddorwin, 2015-02-23 20:12:43]

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:531: "hev1,hvc1,"
On 2015/02/23 20:00:49, ddorwin wrote:
> On 2015/02/21 02:09:21, ddorwin wrote:
> > See below.
> 
> See some of the comments about how avc1 is handled in
> https://codereview.chromium.org/952673002/. We should only have a single
> unambiguous HEVC_MAIN string here.

Note that you must have an unambiguous string or you'll hit a DCHECK when
format_codec_mappings is processed.

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:567: {"hev1", MimeUtil::HEVC_MAIN},
On 2015/02/23 20:00:50, ddorwin wrote:
>  This should be in the Ambiguous array like avc1/avc3.

Oops, strike that. Because we do not want to support the extensionless strings,
this should not appear in either list. ParseHEVCCodecID will handle this like we
do for avc1.

[servolk, 2015-02-24 02:15:13]

https://codereview.chromium.org/816353010/diff/360001/media/filters/ffmpeg_de...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/816353010/diff/360001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer.cc:467: } else if (stream_->codec->codec_id ==
AV_CODEC_ID_HEVC) {
On 2015/02/23 20:00:49, ddorwin wrote:
> Do you expect FFmpeg to be built with such support?

Possibly. At the moment none of the ffmpeg_brandings support HEVC demuxing, but
we might introduce a new ffmpeg_branding=Chromecast with HEVC support. But
that's going to be a separate CL.

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights
reserved.
On 2015/02/23 20:00:49, ddorwin wrote:
> If you intend to support MSE, there may be other code that needs to be updated
> to support these specific codecs.

There is also some changes in stream_parser_factory.cc, if that's what you mean.
In fact I've already tested playing some HEVC content through MSE with private
Chromium builds with this CL. I've been able to play videos both through
software FFmpeg decoder and on a hardware board that supports hevc.

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:530: #if defined(ENABLE_HEVC_DEMUXING)
On 2015/02/23 20:00:49, ddorwin wrote:
> On 2015/02/23 19:16:16, servolk wrote:
> > On 2015/02/21 02:09:21, ddorwin wrote:
> > > "DEMUXING" is not really accurate. This enables support in general.
> > 
> > No, I believe this is accurate. What exactly do you mean by "support in
> > general"? This change doesn't contain anything to support h265 decoding
(which
> > would require us to deal with h265 licensing), it only enables demuxing and
> > parsing (which doesn't require any licenses) and this will allow us to
support
> > hevc/h265 on platforms which have "external" h265 decoder, e.g. hardware
h265
> > decoder, for which license costs are going to be paid by hardware
> manufacturer,
> > or OS-provided h265 decoder, for which license costs are going to be paid by
> OS
> > vendor. So, again, I believe the term "demuxing" is correct in this context,
> and
> > in fact we should be very careful about this, since changing it to something
> > more generic, like "enable_hevc" could get us in trouble by not being clear
> > enough that all we do is demuxing. Please let me know if you disagree and we
> can
> > discuss further.
> 
> The primary "support" it enables is advertising support via canPlayType().
> 
> Regardless of what this is called, we're clearly not enabling a software
> decoder, but it seems reasonable to reuse this value if one needed to use it
to
> enable code around their decoder.

Yes, this does enable hevc support via canPlayType, but my understanding was
that only platforms that actually support hevc would explicitly set
enable_hevc_demuxing, so it's probably ok to gate mime type checks with this
flag as well.

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:531: "hev1,hvc1,"
On 2015/02/23 20:00:49, ddorwin wrote:
> On 2015/02/21 02:09:21, ddorwin wrote:
> > See below.
> 
> See some of the comments about how avc1 is handled in
> https://codereview.chromium.org/952673002/. We should only have a single
> unambiguous HEVC_MAIN string here.

Wait, I don't understand - even in that CL we have both avc1 and avc3 - so
there's two strings for hevc as well. Are you saying that there should be only
avc3+hvc1?

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:1023: if (StartsWithASCII(codec_id, "hev1.", true) ||
On 2015/02/23 20:00:49, ddorwin wrote:
> On 2015/02/23 19:16:16, servolk wrote:
> > On 2015/02/21 02:09:21, ddorwin wrote:
> > > Once the other issues are addressed, you'll need to update
> > > media_canplaytype_browsertest.cc. (Wait for
> > > https://codereview.chromium.org/918463002/.)
> > 
> > Ok, yeah, once we have more clarity here we'll try to add more unit tests,
> > including mime type tests.
> 
> You should add at least some basic tests soon. This code is complex and
> difficult to get right.

Sure, I will add some tests. In fact I've already added some unit tests for
ffmpeg and chunk demuxer. Do we have any end-to-end MSE tests for Chromium?

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:1025: // TODO(servolk): Implement parsing of hevc codec
ids as described in
On 2015/02/23 20:00:49, ddorwin wrote:
> On 2015/02/23 19:16:16, servolk wrote:
> > On 2015/02/21 02:09:21, ddorwin wrote:
> > > Our avc1 string handling is a mess. Similar to my comments above, we
should
> be
> > > accurate and restrictive from the beginning to avoid similar problems.
> > 
> > I understand your concerns, but we are in the early stages of implementation
> > here. We just don't know yet which profiles our hardware partners are going
to
> > need/support. So for now we'd prefer to allow them all, to facilitate
testing.
> 
> Can we at least check for valid values, similar to IsValidH264Level()?

Yep, will do

[ddorwin, 2015-02-24 18:07:33]

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

https://codereview.chromium.org/816353010/diff/360001/media/filters/stream_pa...
media/filters/stream_parser_factory.cc:149: static const CodecInfo
kHEVCHEV1CodecInfo = { "hev1", CodecInfo::VIDEO, NULL,
Please don't support the extensionless versions. We don't even support that for
avc1, for which we do support it in canPlayType().

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights
reserved.
On 2015/02/24 02:15:13, servolk wrote:
> On 2015/02/23 20:00:49, ddorwin wrote:
> > If you intend to support MSE, there may be other code that needs to be
updated
> > to support these specific codecs.
> 
> There is also some changes in stream_parser_factory.cc, if that's what you
mean.
> In fact I've already tested playing some HEVC content through MSE with private
> Chromium builds with this CL. I've been able to play videos both through
> software FFmpeg decoder and on a hardware board that supports hevc.

Acknowledged.

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:530: #if defined(ENABLE_HEVC_DEMUXING)
On 2015/02/24 02:15:13, servolk wrote:
> On 2015/02/23 20:00:49, ddorwin wrote:
> > On 2015/02/23 19:16:16, servolk wrote:
> > > On 2015/02/21 02:09:21, ddorwin wrote:
> > > > "DEMUXING" is not really accurate. This enables support in general.
> > > 
> > > No, I believe this is accurate. What exactly do you mean by "support in
> > > general"? This change doesn't contain anything to support h265 decoding
> (which
> > > would require us to deal with h265 licensing), it only enables demuxing
and
> > > parsing (which doesn't require any licenses) and this will allow us to
> support
> > > hevc/h265 on platforms which have "external" h265 decoder, e.g. hardware
> h265
> > > decoder, for which license costs are going to be paid by hardware
> > manufacturer,
> > > or OS-provided h265 decoder, for which license costs are going to be paid
by
> > OS
> > > vendor. So, again, I believe the term "demuxing" is correct in this
context,
> > and
> > > in fact we should be very careful about this, since changing it to
something
> > > more generic, like "enable_hevc" could get us in trouble by not being
clear
> > > enough that all we do is demuxing. Please let me know if you disagree and
we
> > can
> > > discuss further.
> > 
> > The primary "support" it enables is advertising support via canPlayType().
> > 
> > Regardless of what this is called, we're clearly not enabling a software
> > decoder, but it seems reasonable to reuse this value if one needed to use it
> to
> > enable code around their decoder.
> 
> Yes, this does enable hevc support via canPlayType, but my understanding was
> that only platforms that actually support hevc would explicitly set
> enable_hevc_demuxing, so it's probably ok to gate mime type checks with this
> flag as well.

Agreed. My point is that this flag enables HEVC, not just HEVC deumuxing. Since
it would only be anbled on platforms that support HEVC, I don't think we need to
worry about the other issues you mentioned and can just call it ENABLE_HEVC.

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:531: "hev1,hvc1,"
On 2015/02/24 02:15:13, servolk wrote:
> On 2015/02/23 20:00:49, ddorwin wrote:
> > On 2015/02/21 02:09:21, ddorwin wrote:
> > > See below.
> > 
> > See some of the comments about how avc1 is handled in
> > https://codereview.chromium.org/952673002/. We should only have a single
> > unambiguous HEVC_MAIN string here.
> 
> Wait, I don't understand - even in that CL we have both avc1 and avc3 - so
> there's two strings for hevc as well. Are you saying that there should be only
> avc3+hvc1?

I didn't change anything in that CL, I just explained why things are the way
they are. (One of the things we need to do is replace these strings with
H264_BASELINE, etc.)
Look at line 529 here. We only use "avc1..." (not "avc3...). The whole point of
this is to add H264_BASELINE, etc. to a map.

In addition, these strings are unambiguous. You need an actual "hev1.xyzabc" or
you will hit a DCHECK.

https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
net/base/mime_util.cc:1023: if (StartsWithASCII(codec_id, "hev1.", true) ||
On 2015/02/24 02:15:13, servolk wrote:
> On 2015/02/23 20:00:49, ddorwin wrote:
> > On 2015/02/23 19:16:16, servolk wrote:
> > > On 2015/02/21 02:09:21, ddorwin wrote:
> > > > Once the other issues are addressed, you'll need to update
> > > > media_canplaytype_browsertest.cc. (Wait for
> > > > https://codereview.chromium.org/918463002/.)
> > > 
> > > Ok, yeah, once we have more clarity here we'll try to add more unit tests,
> > > including mime type tests.
> > 
> > You should add at least some basic tests soon. This code is complex and
> > difficult to get right.
> 
> Sure, I will add some tests. In fact I've already added some unit tests for
> ffmpeg and chunk demuxer. Do we have any end-to-end MSE tests for Chromium?

I think we have pipeline tests and content and chrome browser tests, both for
clear and encrypted content.

I was specifically referring to canPlayType() tests, though. The CL I referenced
has landed, so if you sync, you'll see examples of the detailed coverage that is
necessary for these codec strings.

[servolk, 2015-03-12 02:06:34]

On 2015/02/24 18:07:33, ddorwin wrote:
>
https://codereview.chromium.org/816353010/diff/360001/media/filters/stream_pa...
> File media/filters/stream_parser_factory.cc (right):
> 
>
https://codereview.chromium.org/816353010/diff/360001/media/filters/stream_pa...
> media/filters/stream_parser_factory.cc:149: static const CodecInfo
> kHEVCHEV1CodecInfo = { "hev1", CodecInfo::VIDEO, NULL,
> Please don't support the extensionless versions. We don't even support that
for
> avc1, for which we do support it in canPlayType().
> 
> https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc
> File net/base/mime_util.cc (right):
> 
>
https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
> net/base/mime_util.cc:1: // Copyright (c) 2012 The Chromium Authors. All
rights
> reserved.
> On 2015/02/24 02:15:13, servolk wrote:
> > On 2015/02/23 20:00:49, ddorwin wrote:
> > > If you intend to support MSE, there may be other code that needs to be
> updated
> > > to support these specific codecs.
> > 
> > There is also some changes in stream_parser_factory.cc, if that's what you
> mean.
> > In fact I've already tested playing some HEVC content through MSE with
private
> > Chromium builds with this CL. I've been able to play videos both through
> > software FFmpeg decoder and on a hardware board that supports hevc.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
> net/base/mime_util.cc:530: #if defined(ENABLE_HEVC_DEMUXING)
> On 2015/02/24 02:15:13, servolk wrote:
> > On 2015/02/23 20:00:49, ddorwin wrote:
> > > On 2015/02/23 19:16:16, servolk wrote:
> > > > On 2015/02/21 02:09:21, ddorwin wrote:
> > > > > "DEMUXING" is not really accurate. This enables support in general.
> > > > 
> > > > No, I believe this is accurate. What exactly do you mean by "support in
> > > > general"? This change doesn't contain anything to support h265 decoding
> > (which
> > > > would require us to deal with h265 licensing), it only enables demuxing
> and
> > > > parsing (which doesn't require any licenses) and this will allow us to
> > support
> > > > hevc/h265 on platforms which have "external" h265 decoder, e.g. hardware
> > h265
> > > > decoder, for which license costs are going to be paid by hardware
> > > manufacturer,
> > > > or OS-provided h265 decoder, for which license costs are going to be
paid
> by
> > > OS
> > > > vendor. So, again, I believe the term "demuxing" is correct in this
> context,
> > > and
> > > > in fact we should be very careful about this, since changing it to
> something
> > > > more generic, like "enable_hevc" could get us in trouble by not being
> clear
> > > > enough that all we do is demuxing. Please let me know if you disagree
and
> we
> > > can
> > > > discuss further.
> > > 
> > > The primary "support" it enables is advertising support via canPlayType().
> > > 
> > > Regardless of what this is called, we're clearly not enabling a software
> > > decoder, but it seems reasonable to reuse this value if one needed to use
it
> > to
> > > enable code around their decoder.
> > 
> > Yes, this does enable hevc support via canPlayType, but my understanding was
> > that only platforms that actually support hevc would explicitly set
> > enable_hevc_demuxing, so it's probably ok to gate mime type checks with this
> > flag as well.
> 
> Agreed. My point is that this flag enables HEVC, not just HEVC deumuxing.
Since
> it would only be anbled on platforms that support HEVC, I don't think we need
to
> worry about the other issues you mentioned and can just call it ENABLE_HEVC.
> 
>
https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
> net/base/mime_util.cc:531: "hev1,hvc1,"
> On 2015/02/24 02:15:13, servolk wrote:
> > On 2015/02/23 20:00:49, ddorwin wrote:
> > > On 2015/02/21 02:09:21, ddorwin wrote:
> > > > See below.
> > > 
> > > See some of the comments about how avc1 is handled in
> > > https://codereview.chromium.org/952673002/. We should only have a single
> > > unambiguous HEVC_MAIN string here.
> > 
> > Wait, I don't understand - even in that CL we have both avc1 and avc3 - so
> > there's two strings for hevc as well. Are you saying that there should be
only
> > avc3+hvc1?
> 
> I didn't change anything in that CL, I just explained why things are the way
> they are. (One of the things we need to do is replace these strings with
> H264_BASELINE, etc.)
> Look at line 529 here. We only use "avc1..." (not "avc3...). The whole point
of
> this is to add H264_BASELINE, etc. to a map.
> 
> In addition, these strings are unambiguous. You need an actual "hev1.xyzabc"
or
> you will hit a DCHECK.
> 
>
https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
> net/base/mime_util.cc:1023: if (StartsWithASCII(codec_id, "hev1.", true) ||
> On 2015/02/24 02:15:13, servolk wrote:
> > On 2015/02/23 20:00:49, ddorwin wrote:
> > > On 2015/02/23 19:16:16, servolk wrote:
> > > > On 2015/02/21 02:09:21, ddorwin wrote:
> > > > > Once the other issues are addressed, you'll need to update
> > > > > media_canplaytype_browsertest.cc. (Wait for
> > > > > https://codereview.chromium.org/918463002/.)
> > > > 
> > > > Ok, yeah, once we have more clarity here we'll try to add more unit
tests,
> > > > including mime type tests.
> > > 
> > > You should add at least some basic tests soon. This code is complex and
> > > difficult to get right.
> > 
> > Sure, I will add some tests. In fact I've already added some unit tests for
> > ffmpeg and chunk demuxer. Do we have any end-to-end MSE tests for Chromium?
> 
> I think we have pipeline tests and content and chrome browser tests, both for
> clear and encrypted content.
> 
> I was specifically referring to canPlayType() tests, though. The CL I
referenced
> has landed, so if you sync, you'll see examples of the detailed coverage that
is
> necessary for these codec strings.

Ok, sorry for the delay with this CL, I have landed it in the internal
Chromecast source tree for now, to unblock our partners and other work that
depends on this. But now that this is done (and I've incorporated some CR
feedback from that into this change), I'm planning to continue working on
landing this upstream.
I've made some changes after discussion with rsleevi@ and brettw@ - they advised
that we should use a global C++ preprocessor define, instead of introducing a
new global build flag into build/common.gypi. I've also addressed feedback given
by ddorwin@ in previous comment and added a few basic canPlayType unit tests for
hevc codec strings. PTAL

Re this:
> Agreed. My point is that this flag enables HEVC, not just HEVC deumuxing.
Since
> it would only be anbled on platforms that support HEVC, I don't think we need
to
> worry about the other issues you mentioned and can just call it ENABLE_HEVC.

I'd like to reiterate why I believe ENABLE_HEVC_DEMUXING is a better name than
just ENABLE_HEVC:
1. This change does not enable HEVC decoding. With it we can only support HEVC
if using an external decoder that supports HEVC (such as Chromecast HW decoder
via CmaRenderer). So again, this flag enables just that - HEVC demuxing. If
somebody who is not very familiar with the current state of things around HEVC
sees ENABLE_HEVC flag in Chromium code, they might get confused, e.g. they might
expect to just flip that flag and be able to build Chromium with HEVC playback
support. That's not the case. Having more explicit name hopefully will allow to
avoid that confusion.
2. We don't want any potential legal complications, so we want to be very clear
that this code only provides HEVC demuxing and has nothing to do with
parsing/decoding HEVC. Again, if we used just ENABLE_HEVC flag here, people
could reasonably assume that implied HEVC decoding support, which would require
having HEVC license. But demuxing doesn't require any licensing, so let's
emphasize that code only provides demuxing by having it right in the flag name.

[servolk, 2015-03-12 19:44:15]

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

https://codereview.chromium.org/816353010/diff/360001/media/filters/stream_pa...
media/filters/stream_parser_factory.cc:149: static const CodecInfo
kHEVCHEV1CodecInfo = { "hev1", CodecInfo::VIDEO, NULL,
On 2015/02/24 18:07:32, ddorwin wrote:
> Please don't support the extensionless versions. We don't even support that
for
> avc1, for which we do support it in canPlayType().

Done.

[servolk, 2015-03-18 20:33:32]

On 2015/03/12 19:44:15, servolk wrote:
>
https://codereview.chromium.org/816353010/diff/360001/media/filters/stream_pa...
> File media/filters/stream_parser_factory.cc (right):
> 
>
https://codereview.chromium.org/816353010/diff/360001/media/filters/stream_pa...
> media/filters/stream_parser_factory.cc:149: static const CodecInfo
> kHEVCHEV1CodecInfo = { "hev1", CodecInfo::VIDEO, NULL,
> On 2015/02/24 18:07:32, ddorwin wrote:
> > Please don't support the extensionless versions. We don't even support that
> for
> > avc1, for which we do support it in canPlayType().
> 
> Done.

Ping. Please take a look, I believe this code is ready for upstreaming.

[servolk, 2015-03-23 17:23:25]

On 2015/03/18 20:33:32, servolk wrote:
> On 2015/03/12 19:44:15, servolk wrote:
> >
>
https://codereview.chromium.org/816353010/diff/360001/media/filters/stream_pa...
> > File media/filters/stream_parser_factory.cc (right):
> > 
> >
>
https://codereview.chromium.org/816353010/diff/360001/media/filters/stream_pa...
> > media/filters/stream_parser_factory.cc:149: static const CodecInfo
> > kHEVCHEV1CodecInfo = { "hev1", CodecInfo::VIDEO, NULL,
> > On 2015/02/24 18:07:32, ddorwin wrote:
> > > Please don't support the extensionless versions. We don't even support
that
> > for
> > > avc1, for which we do support it in canPlayType().
> > 
> > Done.
> 
> Ping. Please take a look, I believe this code is ready for upstreaming.

Ping again

[gunsch, 2015-03-25 00:29:06]

On 2015/03/23 17:23:25, servolk wrote:
> On 2015/03/18 20:33:32, servolk wrote:
> > On 2015/03/12 19:44:15, servolk wrote:
> > >
> >
>
https://codereview.chromium.org/816353010/diff/360001/media/filters/stream_pa...
> > > File media/filters/stream_parser_factory.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/816353010/diff/360001/media/filters/stream_pa...
> > > media/filters/stream_parser_factory.cc:149: static const CodecInfo
> > > kHEVCHEV1CodecInfo = { "hev1", CodecInfo::VIDEO, NULL,
> > > On 2015/02/24 18:07:32, ddorwin wrote:
> > > > Please don't support the extensionless versions. We don't even support
> that
> > > for
> > > > avc1, for which we do support it in canPlayType().
> > > 
> > > Done.
> > 
> > Ping. Please take a look, I believe this code is ready for upstreaming.
> 
> Ping again

One comment: while we're adding this, can we add an entry in
media/base/android/media_codec_bridge.cc's VideoCodecToAndroidMimeType? The
Android mime type is "video/hevc" (see
http://developer.android.com/reference/android/media/MediaCodec.html#createDe...
).

[servolk, 2015-03-25 00:54:03]

On 2015/03/25 00:29:06, gunsch wrote:
> On 2015/03/23 17:23:25, servolk wrote:
> > On 2015/03/18 20:33:32, servolk wrote:
> > > On 2015/03/12 19:44:15, servolk wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/816353010/diff/360001/media/filters/stream_pa...
> > > > File media/filters/stream_parser_factory.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/816353010/diff/360001/media/filters/stream_pa...
> > > > media/filters/stream_parser_factory.cc:149: static const CodecInfo
> > > > kHEVCHEV1CodecInfo = { "hev1", CodecInfo::VIDEO, NULL,
> > > > On 2015/02/24 18:07:32, ddorwin wrote:
> > > > > Please don't support the extensionless versions. We don't even support
> > that
> > > > for
> > > > > avc1, for which we do support it in canPlayType().
> > > > 
> > > > Done.
> > > 
> > > Ping. Please take a look, I believe this code is ready for upstreaming.
> > 
> > Ping again
> 
> One comment: while we're adding this, can we add an entry in
> media/base/android/media_codec_bridge.cc's VideoCodecToAndroidMimeType? The
> Android mime type is "video/hevc" (see
>
http://developer.android.com/reference/android/media/MediaCodec.html#createDe...
> ).

Done.

[ddorwin, 2015-03-25 01:22:00]

https://codereview.chromium.org/816353010/diff/620001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/816353010/diff/620001/build/common.gypi#newco...
build/common.gypi:1846: # HEVC/H265 demuxing support. Note that toggling this
flag will enable
Where is the equivalent GN change?

https://codereview.chromium.org/816353010/diff/620001/build/common.gypi#newco...
build/common.gypi:1849: # licensing). Chromecast has hardware HEVC decoder on
some platforms.
Do you have codec detection yet? If not, you cannot enable this by default
because it will cause all platforms to advertise support.

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

https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:177: // Don't allow
incomplete/ambiguous codec ids for HEVC for now.
Remove " for now".

https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:180: EXPECT_EQ(kNot,
CanPlay("'" + mime + "; codecs=\"hvc1\"'"));
Also test these with a trailing dot.
And valid complete codecs with a trailing dot.

https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:587: kHevcSupported =
kPropProbably;
Why not just #if/#else?

https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:589: // TODO(servolk):
Add more unit test coverage once we have more info about
Detailed tests of the codec strings probably belong in an HEVC-specific test
like CodecSupportTest_Avc1Variants. So, you'd have:
CodecSupportTest_Hev1Variants
CodecSupportTest_Hvc1Variants

Something like CodecSupportTest_HevcLevels might also be appropriate.

https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:591:
EXPECT_EQ(kHevcSupported, CanPlay("'video/mp4; codecs=\"hvc1.1.L0.0\"'"));
You are testing video/mp4 far away from the other video/mp4 tests. Also, you
should test in all four MP4 containers + two HLS variants like the other codecs.

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

https://codereview.chromium.org/816353010/diff/620001/media/filters/stream_pa...
media/filters/stream_parser_factory.cc:144: static const CodecInfo
kH264AVC1CodecInfo = { "avc1.*", CodecInfo::VIDEO, NULL,
wolenetz: Do we check these against mime_util to make sure it is a valid
supported codec?

https://codereview.chromium.org/816353010/diff/620001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/816353010/diff/620001/net/base/mime_util.cc#n...
net/base/mime_util.cc:471: // HEVC/H.265 is supported only in Lollipop+ (API
Level 21). According to
#if defined(ENABLE_HEVC_DEMUXING)
...
#else
  return false;
#endif

https://codereview.chromium.org/816353010/diff/620001/net/base/mime_util.cc#n...
net/base/mime_util.cc:1052: // ones that can't be added to the
|string_to_codec_map_| and require parsing.
nit: "can't be"? are not?

https://codereview.chromium.org/816353010/diff/620001/net/base/mime_util.cc#n...
net/base/mime_util.cc:1054: #if defined(ENABLE_HEVC_DEMUXING)
Move this code to ParseHEVCCodecID or something like that.
We should try to keep this function as simple as possible.

https://codereview.chromium.org/816353010/diff/620001/net/base/mime_util.cc#n...
net/base/mime_util.cc:1061: *is_ambiguous = false;
This should be true since you do no verification.

[ddorwin, 2015-03-25 01:22:35]

On 2015/03/12 02:06:34, servolk wrote:
> On 2015/02/24 18:07:33, ddorwin wrote:
> >
>
https://codereview.chromium.org/816353010/diff/360001/media/filters/stream_pa...
> > File media/filters/stream_parser_factory.cc (right):
> > 
> >
>
https://codereview.chromium.org/816353010/diff/360001/media/filters/stream_pa...
> > media/filters/stream_parser_factory.cc:149: static const CodecInfo
> > kHEVCHEV1CodecInfo = { "hev1", CodecInfo::VIDEO, NULL,
> > Please don't support the extensionless versions. We don't even support that
> for
> > avc1, for which we do support it in canPlayType().
> > 
> > https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc
> > File net/base/mime_util.cc (right):
> > 
> >
>
https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
> > net/base/mime_util.cc:1: // Copyright (c) 2012 The Chromium Authors. All
> rights
> > reserved.
> > On 2015/02/24 02:15:13, servolk wrote:
> > > On 2015/02/23 20:00:49, ddorwin wrote:
> > > > If you intend to support MSE, there may be other code that needs to be
> > updated
> > > > to support these specific codecs.
> > > 
> > > There is also some changes in stream_parser_factory.cc, if that's what you
> > mean.
> > > In fact I've already tested playing some HEVC content through MSE with
> private
> > > Chromium builds with this CL. I've been able to play videos both through
> > > software FFmpeg decoder and on a hardware board that supports hevc.
> > 
> > Acknowledged.
> > 
> >
>
https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
> > net/base/mime_util.cc:530: #if defined(ENABLE_HEVC_DEMUXING)
> > On 2015/02/24 02:15:13, servolk wrote:
> > > On 2015/02/23 20:00:49, ddorwin wrote:
> > > > On 2015/02/23 19:16:16, servolk wrote:
> > > > > On 2015/02/21 02:09:21, ddorwin wrote:
> > > > > > "DEMUXING" is not really accurate. This enables support in general.
> > > > > 
> > > > > No, I believe this is accurate. What exactly do you mean by "support
in
> > > > > general"? This change doesn't contain anything to support h265
decoding
> > > (which
> > > > > would require us to deal with h265 licensing), it only enables
demuxing
> > and
> > > > > parsing (which doesn't require any licenses) and this will allow us to
> > > support
> > > > > hevc/h265 on platforms which have "external" h265 decoder, e.g.
hardware
> > > h265
> > > > > decoder, for which license costs are going to be paid by hardware
> > > > manufacturer,
> > > > > or OS-provided h265 decoder, for which license costs are going to be
> paid
> > by
> > > > OS
> > > > > vendor. So, again, I believe the term "demuxing" is correct in this
> > context,
> > > > and
> > > > > in fact we should be very careful about this, since changing it to
> > something
> > > > > more generic, like "enable_hevc" could get us in trouble by not being
> > clear
> > > > > enough that all we do is demuxing. Please let me know if you disagree
> and
> > we
> > > > can
> > > > > discuss further.
> > > > 
> > > > The primary "support" it enables is advertising support via
canPlayType().
> > > > 
> > > > Regardless of what this is called, we're clearly not enabling a software
> > > > decoder, but it seems reasonable to reuse this value if one needed to
use
> it
> > > to
> > > > enable code around their decoder.
> > > 
> > > Yes, this does enable hevc support via canPlayType, but my understanding
was
> > > that only platforms that actually support hevc would explicitly set
> > > enable_hevc_demuxing, so it's probably ok to gate mime type checks with
this
> > > flag as well.
> > 
> > Agreed. My point is that this flag enables HEVC, not just HEVC deumuxing.
> Since
> > it would only be anbled on platforms that support HEVC, I don't think we
need
> to
> > worry about the other issues you mentioned and can just call it ENABLE_HEVC.
> > 
> >
>
https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
> > net/base/mime_util.cc:531: "hev1,hvc1,"
> > On 2015/02/24 02:15:13, servolk wrote:
> > > On 2015/02/23 20:00:49, ddorwin wrote:
> > > > On 2015/02/21 02:09:21, ddorwin wrote:
> > > > > See below.
> > > > 
> > > > See some of the comments about how avc1 is handled in
> > > > https://codereview.chromium.org/952673002/. We should only have a single
> > > > unambiguous HEVC_MAIN string here.
> > > 
> > > Wait, I don't understand - even in that CL we have both avc1 and avc3 - so
> > > there's two strings for hevc as well. Are you saying that there should be
> only
> > > avc3+hvc1?
> > 
> > I didn't change anything in that CL, I just explained why things are the way
> > they are. (One of the things we need to do is replace these strings with
> > H264_BASELINE, etc.)
> > Look at line 529 here. We only use "avc1..." (not "avc3...). The whole point
> of
> > this is to add H264_BASELINE, etc. to a map.
> > 
> > In addition, these strings are unambiguous. You need an actual "hev1.xyzabc"
> or
> > you will hit a DCHECK.
> > 
> >
>
https://codereview.chromium.org/816353010/diff/360001/net/base/mime_util.cc#n...
> > net/base/mime_util.cc:1023: if (StartsWithASCII(codec_id, "hev1.", true) ||
> > On 2015/02/24 02:15:13, servolk wrote:
> > > On 2015/02/23 20:00:49, ddorwin wrote:
> > > > On 2015/02/23 19:16:16, servolk wrote:
> > > > > On 2015/02/21 02:09:21, ddorwin wrote:
> > > > > > Once the other issues are addressed, you'll need to update
> > > > > > media_canplaytype_browsertest.cc. (Wait for
> > > > > > https://codereview.chromium.org/918463002/.)
> > > > > 
> > > > > Ok, yeah, once we have more clarity here we'll try to add more unit
> tests,
> > > > > including mime type tests.
> > > > 
> > > > You should add at least some basic tests soon. This code is complex and
> > > > difficult to get right.
> > > 
> > > Sure, I will add some tests. In fact I've already added some unit tests
for
> > > ffmpeg and chunk demuxer. Do we have any end-to-end MSE tests for
Chromium?
> > 
> > I think we have pipeline tests and content and chrome browser tests, both
for
> > clear and encrypted content.
> > 
> > I was specifically referring to canPlayType() tests, though. The CL I
> referenced
> > has landed, so if you sync, you'll see examples of the detailed coverage
that
> is
> > necessary for these codec strings.
> 
> Ok, sorry for the delay with this CL, I have landed it in the internal
> Chromecast source tree for now, to unblock our partners and other work that
> depends on this. But now that this is done (and I've incorporated some CR
> feedback from that into this change), I'm planning to continue working on
> landing this upstream.
> I've made some changes after discussion with rsleevi@ and brettw@ - they
advised
> that we should use a global C++ preprocessor define, instead of introducing a
> new global build flag into build/common.gypi. I've also addressed feedback
given
> by ddorwin@ in previous comment and added a few basic canPlayType unit tests
for
> hevc codec strings. PTAL
> 
> Re this:
> > Agreed. My point is that this flag enables HEVC, not just HEVC deumuxing.
> Since
> > it would only be anbled on platforms that support HEVC, I don't think we
need
> to
> > worry about the other issues you mentioned and can just call it ENABLE_HEVC.
> 
> I'd like to reiterate why I believe ENABLE_HEVC_DEMUXING is a better name than
> just ENABLE_HEVC:
> 1. This change does not enable HEVC decoding. With it we can only support HEVC
> if using an external decoder that supports HEVC (such as Chromecast HW decoder
> via CmaRenderer). So again, this flag enables just that - HEVC demuxing. If
> somebody who is not very familiar with the current state of things around HEVC
> sees ENABLE_HEVC flag in Chromium code, they might get confused, e.g. they
might
> expect to just flip that flag and be able to build Chromium with HEVC playback
> support. That's not the case. Having more explicit name hopefully will allow
to
> avoid that confusion.
> 2. We don't want any potential legal complications, so we want to be very
clear
> that this code only provides HEVC demuxing and has nothing to do with
> parsing/decoding HEVC. Again, if we used just ENABLE_HEVC flag here, people
> could reasonably assume that implied HEVC decoding support, which would
require
> having HEVC license. But demuxing doesn't require any licensing, so let's
> emphasize that code only provides demuxing by having it right in the flag
name.

dalecurtis and I discussed. He was concerned that just ENABLE_HEVC might also
imply encoding support. I believe we agree that DEMUXING is not really accurate,
but we didn't have significantly better ideas. PLAYBACK was mentioned.

[servolk, 2015-03-25 02:20:21]

https://codereview.chromium.org/816353010/diff/620001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/816353010/diff/620001/build/common.gypi#newco...
build/common.gypi:1846: # HEVC/H265 demuxing support. Note that toggling this
flag will enable
On 2015/03/25 01:21:59, ddorwin wrote:
> Where is the equivalent GN change?

This chunk is inside a conditions section guarded by chromecast==1. Chromecast
doesn't use GN builds and no chromecast specific sections in .gn/.gni files, so
there's no corresponding GN change. I assume if/when chromecast migrates to use
GN builds, we'll copy all the variable definitions from here into some .gn file.

https://codereview.chromium.org/816353010/diff/620001/build/common.gypi#newco...
build/common.gypi:1849: # licensing). Chromecast has hardware HEVC decoder on
some platforms.
On 2015/03/25 01:21:59, ddorwin wrote:
> Do you have codec detection yet? If not, you cannot enable this by default
> because it will cause all platforms to advertise support.

No codec detection yet, but again, this is inside chromecast==1 section, so I
believe it should be fine, we are not enabling hevc by default.

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

https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:177: // Don't allow
incomplete/ambiguous codec ids for HEVC for now.
On 2015/03/25 01:21:59, ddorwin wrote:
> Remove " for now".

Done.

https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:180: EXPECT_EQ(kNot,
CanPlay("'" + mime + "; codecs=\"hvc1\"'"));
On 2015/03/25 01:21:59, ddorwin wrote:
> Also test these with a trailing dot.
> And valid complete codecs with a trailing dot.

Done.

https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:587: kHevcSupported =
kPropProbably;
On 2015/03/25 01:21:59, ddorwin wrote:
> Why not just #if/#else?

Something like:
#if defined(ENABLE_HEVC_DEMUXING)
  const char* kHevcSupported = kPropProbably;
#else
  const char* kHevcSupported = kPropNot;
#endif
?
That would be 5 lines instead of 4 and would have the same effect. So if we can
use just 4, why go for the longer one? I think this one is just as readable.

https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:589: // TODO(servolk):
Add more unit test coverage once we have more info about
On 2015/03/25 01:21:59, ddorwin wrote:
> Detailed tests of the codec strings probably belong in an HEVC-specific test
> like CodecSupportTest_Avc1Variants. So, you'd have:
> CodecSupportTest_Hev1Variants
> CodecSupportTest_Hvc1Variants
> 
> Something like CodecSupportTest_HevcLevels might also be appropriate.

Yes, but that's going to be done when we actually add handling of various
levels/profiles for HEVC.

https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:591:
EXPECT_EQ(kHevcSupported, CanPlay("'video/mp4; codecs=\"hvc1.1.L0.0\"'"));
On 2015/03/25 01:21:59, ddorwin wrote:
> You are testing video/mp4 far away from the other video/mp4 tests. Also, you
> should test in all four MP4 containers + two HLS variants like the other
codecs.

Done (except HLS; HEVC is not supported via HLS at the moment)

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

https://codereview.chromium.org/816353010/diff/620001/media/filters/stream_pa...
media/filters/stream_parser_factory.cc:144: static const CodecInfo
kH264AVC1CodecInfo = { "avc1.*", CodecInfo::VIDEO, NULL,
On 2015/03/25 01:21:59, ddorwin wrote:
> wolenetz: Do we check these against mime_util to make sure it is a valid
> supported codec?

Unfortunately we don't at the moment. I think we need to unify codec checking,
but first we have to extract media mime type checking from net/.

https://codereview.chromium.org/816353010/diff/620001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/816353010/diff/620001/net/base/mime_util.cc#n...
net/base/mime_util.cc:471: // HEVC/H.265 is supported only in Lollipop+ (API
Level 21). According to
On 2015/03/25 01:21:59, ddorwin wrote:
> #if defined(ENABLE_HEVC_DEMUXING)
> ...
> #else
>   return false;
> #endif

Done.

https://codereview.chromium.org/816353010/diff/620001/net/base/mime_util.cc#n...
net/base/mime_util.cc:1052: // ones that can't be added to the
|string_to_codec_map_| and require parsing.
On 2015/03/25 01:21:59, ddorwin wrote:
> nit: "can't be"? are not?

Done.

https://codereview.chromium.org/816353010/diff/620001/net/base/mime_util.cc#n...
net/base/mime_util.cc:1054: #if defined(ENABLE_HEVC_DEMUXING)
On 2015/03/25 01:21:59, ddorwin wrote:
> Move this code to ParseHEVCCodecID or something like that.
> We should try to keep this function as simple as possible.

Done.

https://codereview.chromium.org/816353010/diff/620001/net/base/mime_util.cc#n...
net/base/mime_util.cc:1061: *is_ambiguous = false;
On 2015/03/25 01:21:59, ddorwin wrote:
> This should be true since you do no verification.

Done.

[ddorwin, 2015-03-25 18:02:59]

There are 4 comments are in PS32.

https://codereview.chromium.org/816353010/diff/620001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/816353010/diff/620001/build/common.gypi#newco...
build/common.gypi:1849: # licensing). Chromecast has hardware HEVC decoder on
some platforms.
On 2015/03/25 02:20:20, servolk wrote:
> On 2015/03/25 01:21:59, ddorwin wrote:
> > Do you have codec detection yet? If not, you cannot enable this by default
> > because it will cause all platforms to advertise support.
> 
> No codec detection yet, but again, this is inside chromecast==1 section, so I
> believe it should be fine, we are not enabling hevc by default.

But you ARE enabling it by default for all Chromecast devices. I think that is
wrong. You should remove this line until you have a way to support existing
devices. Also, it probably shouldn't be enabled by default until the profile
strings are worked out.

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

https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:587: kHevcSupported =
kPropProbably;
On 2015/03/25 02:20:20, servolk wrote:
> On 2015/03/25 01:21:59, ddorwin wrote:
> > Why not just #if/#else?
> 
> Something like:
> #if defined(ENABLE_HEVC_DEMUXING)
>   const char* kHevcSupported = kPropProbably;
> #else
>   const char* kHevcSupported = kPropNot;
> #endif
> ?
> That would be 5 lines instead of 4 and would have the same effect. So if we
can
> use just 4, why go for the longer one? I think this one is just as readable.

Because immediately changing the value looks weird. Also, it can't be used at
the file scope, which you may need.

https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:589: // TODO(servolk):
Add more unit test coverage once we have more info about
On 2015/03/25 02:20:20, servolk wrote:
> On 2015/03/25 01:21:59, ddorwin wrote:
> > Detailed tests of the codec strings probably belong in an HEVC-specific test
> > like CodecSupportTest_Avc1Variants. So, you'd have:
> > CodecSupportTest_Hev1Variants
> > CodecSupportTest_Hvc1Variants
> > 
> > Something like CodecSupportTest_HevcLevels might also be appropriate.
> 
> Yes, but that's going to be done when we actually add handling of various
> levels/profiles for HEVC.

I was just saying that the tests wouldn't be at the site of the TODO. You might
leave yourself more notes like those in my comments or just "in separate
test(s)."

https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:591:
EXPECT_EQ(kHevcSupported, CanPlay("'video/mp4; codecs=\"hvc1.1.L0.0\"'"));
On 2015/03/25 02:20:20, servolk wrote:
> On 2015/03/25 01:21:59, ddorwin wrote:
> > You are testing video/mp4 far away from the other video/mp4 tests. Also, you
> > should test in all four MP4 containers + two HLS variants like the other
> codecs.
> 
> Done (except HLS; HEVC is not supported via HLS at the moment)

Then you should have negative tests.

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

https://codereview.chromium.org/816353010/diff/640001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:509: const char*
kHevcSupported = kNot;
It's odd to define a constant like this in the middle of a function,e specially
when it is used throughout the function. It should at least be at the top, but
around line 41 is probably better.

https://codereview.chromium.org/816353010/diff/640001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/816353010/diff/640001/net/base/mime_util.cc#n...
net/base/mime_util.cc:1049: // TODO(servolk): Implement parsing of hevc codec
ids as described in
Is there a bug for this?

https://codereview.chromium.org/816353010/diff/640001/net/base/mime_util.cc#n...
net/base/mime_util.cc:1053: *is_ambiguous = true;
IIRC, this will cause a DCHECK because of line 536. You might need to special
case "hvc1.1.L0.0" if you are hitting the DCHECK.

https://codereview.chromium.org/816353010/diff/640001/net/base/mime_util.cc#n...
net/base/mime_util.cc:1056: return false;
nit: empty line

[servolk, 2015-04-29 23:50:33]

https://codereview.chromium.org/816353010/diff/620001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/816353010/diff/620001/build/common.gypi#newco...
build/common.gypi:1849: # licensing). Chromecast has hardware HEVC decoder on
some platforms.
On 2015/03/25 18:02:57, ddorwin wrote:
> On 2015/03/25 02:20:20, servolk wrote:
> > On 2015/03/25 01:21:59, ddorwin wrote:
> > > Do you have codec detection yet? If not, you cannot enable this by default
> > > because it will cause all platforms to advertise support.
> > 
> > No codec detection yet, but again, this is inside chromecast==1 section, so
I
> > believe it should be fine, we are not enabling hevc by default.
> 
> But you ARE enabling it by default for all Chromecast devices. I think that is
> wrong. You should remove this line until you have a way to support existing
> devices. Also, it probably shouldn't be enabled by default until the profile
> strings are worked out.

Yes, but that was because the current mechanism of checking which codec is
supported on each platform isn't extensible. Codec choices are essentially
hard-coded in net/base/mime_util.cc and media/filters/stream_parser_factory.cc.
I've created a separate CL to make the mechanism for codec checks extensible by
various embedders, including Clank and Chromecast, see
https://codereview.chromium.org/1054943004/.
We have already merged that CL in Chromecast source tree and I've made sure HEVC
is only enabled only on platforms that actually support it (I can send you links
to internal CLs if you are interested to see how it's going to be used).

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

https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:587: kHevcSupported =
kPropProbably;
On 2015/03/25 18:02:58, ddorwin wrote:
> On 2015/03/25 02:20:20, servolk wrote:
> > On 2015/03/25 01:21:59, ddorwin wrote:
> > > Why not just #if/#else?
> > 
> > Something like:
> > #if defined(ENABLE_HEVC_DEMUXING)
> >   const char* kHevcSupported = kPropProbably;
> > #else
> >   const char* kHevcSupported = kPropNot;
> > #endif
> > ?
> > That would be 5 lines instead of 4 and would have the same effect. So if we
> can
> > use just 4, why go for the longer one? I think this one is just as readable.
> 
> Because immediately changing the value looks weird. Also, it can't be used at
> the file scope, which you may need.

Done.

https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:589: // TODO(servolk):
Add more unit test coverage once we have more info about
On 2015/03/25 18:02:58, ddorwin wrote:
> On 2015/03/25 02:20:20, servolk wrote:
> > On 2015/03/25 01:21:59, ddorwin wrote:
> > > Detailed tests of the codec strings probably belong in an HEVC-specific
test
> > > like CodecSupportTest_Avc1Variants. So, you'd have:
> > > CodecSupportTest_Hev1Variants
> > > CodecSupportTest_Hvc1Variants
> > > 
> > > Something like CodecSupportTest_HevcLevels might also be appropriate.
> > 
> > Yes, but that's going to be done when we actually add handling of various
> > levels/profiles for HEVC.
> 
> I was just saying that the tests wouldn't be at the site of the TODO. You
might
> leave yourself more notes like those in my comments or just "in separate
> test(s)."

I think this whole test could use some refactoring. There's a lot of duplication
here.

https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:591:
EXPECT_EQ(kHevcSupported, CanPlay("'video/mp4; codecs=\"hvc1.1.L0.0\"'"));
On 2015/03/25 18:02:58, ddorwin wrote:
> On 2015/03/25 02:20:20, servolk wrote:
> > On 2015/03/25 01:21:59, ddorwin wrote:
> > > You are testing video/mp4 far away from the other video/mp4 tests. Also,
you
> > > should test in all four MP4 containers + two HLS variants like the other
> > codecs.
> > 
> > Done (except HLS; HEVC is not supported via HLS at the moment)
> 
> Then you should have negative tests.

Done.

https://codereview.chromium.org/816353010/diff/640001/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/816353010/diff/640001/net/base/mime_util.cc#n...
net/base/mime_util.cc:1049: // TODO(servolk): Implement parsing of hevc codec
ids as described in
On 2015/03/25 18:02:58, ddorwin wrote:
> Is there a bug for this?

Created crbug.com/482761

https://codereview.chromium.org/816353010/diff/640001/net/base/mime_util.cc#n...
net/base/mime_util.cc:1053: *is_ambiguous = true;
On 2015/03/25 18:02:58, ddorwin wrote:
> IIRC, this will cause a DCHECK because of line 536. You might need to special
> case "hvc1.1.L0.0" if you are hitting the DCHECK.

Yep, that was causing dchecks, so I've changed is_ambiguous back to false for
now.

https://codereview.chromium.org/816353010/diff/640001/net/base/mime_util.cc#n...
net/base/mime_util.cc:1056: return false;
On 2015/03/25 18:02:58, ddorwin wrote:
> nit: empty line

Done.

[servolk, 2015-05-30 00:42:44]

On 2015/04/29 23:50:33, servolk wrote:
> https://codereview.chromium.org/816353010/diff/620001/build/common.gypi
> File build/common.gypi (right):
> 
>
https://codereview.chromium.org/816353010/diff/620001/build/common.gypi#newco...
> build/common.gypi:1849: # licensing). Chromecast has hardware HEVC decoder on
> some platforms.
> On 2015/03/25 18:02:57, ddorwin wrote:
> > On 2015/03/25 02:20:20, servolk wrote:
> > > On 2015/03/25 01:21:59, ddorwin wrote:
> > > > Do you have codec detection yet? If not, you cannot enable this by
default
> > > > because it will cause all platforms to advertise support.
> > > 
> > > No codec detection yet, but again, this is inside chromecast==1 section,
so
> I
> > > believe it should be fine, we are not enabling hevc by default.
> > 
> > But you ARE enabling it by default for all Chromecast devices. I think that
is
> > wrong. You should remove this line until you have a way to support existing
> > devices. Also, it probably shouldn't be enabled by default until the profile
> > strings are worked out.
> 
> Yes, but that was because the current mechanism of checking which codec is
> supported on each platform isn't extensible. Codec choices are essentially
> hard-coded in net/base/mime_util.cc and
media/filters/stream_parser_factory.cc.
> I've created a separate CL to make the mechanism for codec checks extensible
by
> various embedders, including Clank and Chromecast, see
> https://codereview.chromium.org/1054943004/.
> We have already merged that CL in Chromecast source tree and I've made sure
HEVC
> is only enabled only on platforms that actually support it (I can send you
links
> to internal CLs if you are interested to see how it's going to be used).
> 
>
https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
> File content/browser/media/media_canplaytype_browsertest.cc (right):
> 
>
https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
> content/browser/media/media_canplaytype_browsertest.cc:587: kHevcSupported =
> kPropProbably;
> On 2015/03/25 18:02:58, ddorwin wrote:
> > On 2015/03/25 02:20:20, servolk wrote:
> > > On 2015/03/25 01:21:59, ddorwin wrote:
> > > > Why not just #if/#else?
> > > 
> > > Something like:
> > > #if defined(ENABLE_HEVC_DEMUXING)
> > >   const char* kHevcSupported = kPropProbably;
> > > #else
> > >   const char* kHevcSupported = kPropNot;
> > > #endif
> > > ?
> > > That would be 5 lines instead of 4 and would have the same effect. So if
we
> > can
> > > use just 4, why go for the longer one? I think this one is just as
readable.
> > 
> > Because immediately changing the value looks weird. Also, it can't be used
at
> > the file scope, which you may need.
> 
> Done.
> 
>
https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
> content/browser/media/media_canplaytype_browsertest.cc:589: // TODO(servolk):
> Add more unit test coverage once we have more info about
> On 2015/03/25 18:02:58, ddorwin wrote:
> > On 2015/03/25 02:20:20, servolk wrote:
> > > On 2015/03/25 01:21:59, ddorwin wrote:
> > > > Detailed tests of the codec strings probably belong in an HEVC-specific
> test
> > > > like CodecSupportTest_Avc1Variants. So, you'd have:
> > > > CodecSupportTest_Hev1Variants
> > > > CodecSupportTest_Hvc1Variants
> > > > 
> > > > Something like CodecSupportTest_HevcLevels might also be appropriate.
> > > 
> > > Yes, but that's going to be done when we actually add handling of various
> > > levels/profiles for HEVC.
> > 
> > I was just saying that the tests wouldn't be at the site of the TODO. You
> might
> > leave yourself more notes like those in my comments or just "in separate
> > test(s)."
> 
> I think this whole test could use some refactoring. There's a lot of
duplication
> here.
> 
>
https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
> content/browser/media/media_canplaytype_browsertest.cc:591:
> EXPECT_EQ(kHevcSupported, CanPlay("'video/mp4; codecs=\"hvc1.1.L0.0\"'"));
> On 2015/03/25 18:02:58, ddorwin wrote:
> > On 2015/03/25 02:20:20, servolk wrote:
> > > On 2015/03/25 01:21:59, ddorwin wrote:
> > > > You are testing video/mp4 far away from the other video/mp4 tests. Also,
> you
> > > > should test in all four MP4 containers + two HLS variants like the other
> > > codecs.
> > > 
> > > Done (except HLS; HEVC is not supported via HLS at the moment)
> > 
> > Then you should have negative tests.
> 
> Done.
> 
> https://codereview.chromium.org/816353010/diff/640001/net/base/mime_util.cc
> File net/base/mime_util.cc (right):
> 
>
https://codereview.chromium.org/816353010/diff/640001/net/base/mime_util.cc#n...
> net/base/mime_util.cc:1049: // TODO(servolk): Implement parsing of hevc codec
> ids as described in
> On 2015/03/25 18:02:58, ddorwin wrote:
> > Is there a bug for this?
> 
> Created crbug.com/482761
> 
>
https://codereview.chromium.org/816353010/diff/640001/net/base/mime_util.cc#n...
> net/base/mime_util.cc:1053: *is_ambiguous = true;
> On 2015/03/25 18:02:58, ddorwin wrote:
> > IIRC, this will cause a DCHECK because of line 536. You might need to
special
> > case "hvc1.1.L0.0" if you are hitting the DCHECK.
> 
> Yep, that was causing dchecks, so I've changed is_ambiguous back to false for
> now.
> 
>
https://codereview.chromium.org/816353010/diff/640001/net/base/mime_util.cc#n...
> net/base/mime_util.cc:1056: return false;
> On 2015/03/25 18:02:58, ddorwin wrote:
> > nit: empty line
> 
> Done.

ping

[servolk, 2015-06-10 00:54:15]

On 2015/05/30 00:42:44, servolk wrote:
> On 2015/04/29 23:50:33, servolk wrote:
> > https://codereview.chromium.org/816353010/diff/620001/build/common.gypi
> > File build/common.gypi (right):
> > 
> >
>
https://codereview.chromium.org/816353010/diff/620001/build/common.gypi#newco...
> > build/common.gypi:1849: # licensing). Chromecast has hardware HEVC decoder
on
> > some platforms.
> > On 2015/03/25 18:02:57, ddorwin wrote:
> > > On 2015/03/25 02:20:20, servolk wrote:
> > > > On 2015/03/25 01:21:59, ddorwin wrote:
> > > > > Do you have codec detection yet? If not, you cannot enable this by
> default
> > > > > because it will cause all platforms to advertise support.
> > > > 
> > > > No codec detection yet, but again, this is inside chromecast==1 section,
> so
> > I
> > > > believe it should be fine, we are not enabling hevc by default.
> > > 
> > > But you ARE enabling it by default for all Chromecast devices. I think
that
> is
> > > wrong. You should remove this line until you have a way to support
existing
> > > devices. Also, it probably shouldn't be enabled by default until the
profile
> > > strings are worked out.
> > 
> > Yes, but that was because the current mechanism of checking which codec is
> > supported on each platform isn't extensible. Codec choices are essentially
> > hard-coded in net/base/mime_util.cc and
> media/filters/stream_parser_factory.cc.
> > I've created a separate CL to make the mechanism for codec checks extensible
> by
> > various embedders, including Clank and Chromecast, see
> > https://codereview.chromium.org/1054943004/.
> > We have already merged that CL in Chromecast source tree and I've made sure
> HEVC
> > is only enabled only on platforms that actually support it (I can send you
> links
> > to internal CLs if you are interested to see how it's going to be used).
> > 
> >
>
https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
> > File content/browser/media/media_canplaytype_browsertest.cc (right):
> > 
> >
>
https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
> > content/browser/media/media_canplaytype_browsertest.cc:587: kHevcSupported =
> > kPropProbably;
> > On 2015/03/25 18:02:58, ddorwin wrote:
> > > On 2015/03/25 02:20:20, servolk wrote:
> > > > On 2015/03/25 01:21:59, ddorwin wrote:
> > > > > Why not just #if/#else?
> > > > 
> > > > Something like:
> > > > #if defined(ENABLE_HEVC_DEMUXING)
> > > >   const char* kHevcSupported = kPropProbably;
> > > > #else
> > > >   const char* kHevcSupported = kPropNot;
> > > > #endif
> > > > ?
> > > > That would be 5 lines instead of 4 and would have the same effect. So if
> we
> > > can
> > > > use just 4, why go for the longer one? I think this one is just as
> readable.
> > > 
> > > Because immediately changing the value looks weird. Also, it can't be used
> at
> > > the file scope, which you may need.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
> > content/browser/media/media_canplaytype_browsertest.cc:589: //
TODO(servolk):
> > Add more unit test coverage once we have more info about
> > On 2015/03/25 18:02:58, ddorwin wrote:
> > > On 2015/03/25 02:20:20, servolk wrote:
> > > > On 2015/03/25 01:21:59, ddorwin wrote:
> > > > > Detailed tests of the codec strings probably belong in an
HEVC-specific
> > test
> > > > > like CodecSupportTest_Avc1Variants. So, you'd have:
> > > > > CodecSupportTest_Hev1Variants
> > > > > CodecSupportTest_Hvc1Variants
> > > > > 
> > > > > Something like CodecSupportTest_HevcLevels might also be appropriate.
> > > > 
> > > > Yes, but that's going to be done when we actually add handling of
various
> > > > levels/profiles for HEVC.
> > > 
> > > I was just saying that the tests wouldn't be at the site of the TODO. You
> > might
> > > leave yourself more notes like those in my comments or just "in separate
> > > test(s)."
> > 
> > I think this whole test could use some refactoring. There's a lot of
> duplication
> > here.
> > 
> >
>
https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
> > content/browser/media/media_canplaytype_browsertest.cc:591:
> > EXPECT_EQ(kHevcSupported, CanPlay("'video/mp4; codecs=\"hvc1.1.L0.0\"'"));
> > On 2015/03/25 18:02:58, ddorwin wrote:
> > > On 2015/03/25 02:20:20, servolk wrote:
> > > > On 2015/03/25 01:21:59, ddorwin wrote:
> > > > > You are testing video/mp4 far away from the other video/mp4 tests.
Also,
> > you
> > > > > should test in all four MP4 containers + two HLS variants like the
other
> > > > codecs.
> > > > 
> > > > Done (except HLS; HEVC is not supported via HLS at the moment)
> > > 
> > > Then you should have negative tests.
> > 
> > Done.
> > 
> > https://codereview.chromium.org/816353010/diff/640001/net/base/mime_util.cc
> > File net/base/mime_util.cc (right):
> > 
> >
>
https://codereview.chromium.org/816353010/diff/640001/net/base/mime_util.cc#n...
> > net/base/mime_util.cc:1049: // TODO(servolk): Implement parsing of hevc
codec
> > ids as described in
> > On 2015/03/25 18:02:58, ddorwin wrote:
> > > Is there a bug for this?
> > 
> > Created crbug.com/482761
> > 
> >
>
https://codereview.chromium.org/816353010/diff/640001/net/base/mime_util.cc#n...
> > net/base/mime_util.cc:1053: *is_ambiguous = true;
> > On 2015/03/25 18:02:58, ddorwin wrote:
> > > IIRC, this will cause a DCHECK because of line 536. You might need to
> special
> > > case "hvc1.1.L0.0" if you are hitting the DCHECK.
> > 
> > Yep, that was causing dchecks, so I've changed is_ambiguous back to false
for
> > now.
> > 
> >
>
https://codereview.chromium.org/816353010/diff/640001/net/base/mime_util.cc#n...
> > net/base/mime_util.cc:1056: return false;
> > On 2015/03/25 18:02:58, ddorwin wrote:
> > > nit: empty line
> > 
> > Done.
> 
> ping

ping

[wolenetz, 2015-06-12 00:02:13]

On 2015/06/10 00:54:15, servolk wrote:
> On 2015/05/30 00:42:44, servolk wrote:
> > On 2015/04/29 23:50:33, servolk wrote:
> > > https://codereview.chromium.org/816353010/diff/620001/build/common.gypi
> > > File build/common.gypi (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/816353010/diff/620001/build/common.gypi#newco...
> > > build/common.gypi:1849: # licensing). Chromecast has hardware HEVC decoder
> on
> > > some platforms.
> > > On 2015/03/25 18:02:57, ddorwin wrote:
> > > > On 2015/03/25 02:20:20, servolk wrote:
> > > > > On 2015/03/25 01:21:59, ddorwin wrote:
> > > > > > Do you have codec detection yet? If not, you cannot enable this by
> > default
> > > > > > because it will cause all platforms to advertise support.
> > > > > 
> > > > > No codec detection yet, but again, this is inside chromecast==1
section,
> > so
> > > I
> > > > > believe it should be fine, we are not enabling hevc by default.
> > > > 
> > > > But you ARE enabling it by default for all Chromecast devices. I think
> that
> > is
> > > > wrong. You should remove this line until you have a way to support
> existing
> > > > devices. Also, it probably shouldn't be enabled by default until the
> profile
> > > > strings are worked out.
> > > 
> > > Yes, but that was because the current mechanism of checking which codec is
> > > supported on each platform isn't extensible. Codec choices are essentially
> > > hard-coded in net/base/mime_util.cc and
> > media/filters/stream_parser_factory.cc.
> > > I've created a separate CL to make the mechanism for codec checks
extensible
> > by
> > > various embedders, including Clank and Chromecast, see
> > > https://codereview.chromium.org/1054943004/.
> > > We have already merged that CL in Chromecast source tree and I've made
sure
> > HEVC
> > > is only enabled only on platforms that actually support it (I can send you
> > links
> > > to internal CLs if you are interested to see how it's going to be used).
> > > 
> > >
> >
>
https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
> > > File content/browser/media/media_canplaytype_browsertest.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
> > > content/browser/media/media_canplaytype_browsertest.cc:587: kHevcSupported
=
> > > kPropProbably;
> > > On 2015/03/25 18:02:58, ddorwin wrote:
> > > > On 2015/03/25 02:20:20, servolk wrote:
> > > > > On 2015/03/25 01:21:59, ddorwin wrote:
> > > > > > Why not just #if/#else?
> > > > > 
> > > > > Something like:
> > > > > #if defined(ENABLE_HEVC_DEMUXING)
> > > > >   const char* kHevcSupported = kPropProbably;
> > > > > #else
> > > > >   const char* kHevcSupported = kPropNot;
> > > > > #endif
> > > > > ?
> > > > > That would be 5 lines instead of 4 and would have the same effect. So
if
> > we
> > > > can
> > > > > use just 4, why go for the longer one? I think this one is just as
> > readable.
> > > > 
> > > > Because immediately changing the value looks weird. Also, it can't be
used
> > at
> > > > the file scope, which you may need.
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
> > > content/browser/media/media_canplaytype_browsertest.cc:589: //
> TODO(servolk):
> > > Add more unit test coverage once we have more info about
> > > On 2015/03/25 18:02:58, ddorwin wrote:
> > > > On 2015/03/25 02:20:20, servolk wrote:
> > > > > On 2015/03/25 01:21:59, ddorwin wrote:
> > > > > > Detailed tests of the codec strings probably belong in an
> HEVC-specific
> > > test
> > > > > > like CodecSupportTest_Avc1Variants. So, you'd have:
> > > > > > CodecSupportTest_Hev1Variants
> > > > > > CodecSupportTest_Hvc1Variants
> > > > > > 
> > > > > > Something like CodecSupportTest_HevcLevels might also be
appropriate.
> > > > > 
> > > > > Yes, but that's going to be done when we actually add handling of
> various
> > > > > levels/profiles for HEVC.
> > > > 
> > > > I was just saying that the tests wouldn't be at the site of the TODO.
You
> > > might
> > > > leave yourself more notes like those in my comments or just "in separate
> > > > test(s)."
> > > 
> > > I think this whole test could use some refactoring. There's a lot of
> > duplication
> > > here.
> > > 
> > >
> >
>
https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
> > > content/browser/media/media_canplaytype_browsertest.cc:591:
> > > EXPECT_EQ(kHevcSupported, CanPlay("'video/mp4; codecs=\"hvc1.1.L0.0\"'"));
> > > On 2015/03/25 18:02:58, ddorwin wrote:
> > > > On 2015/03/25 02:20:20, servolk wrote:
> > > > > On 2015/03/25 01:21:59, ddorwin wrote:
> > > > > > You are testing video/mp4 far away from the other video/mp4 tests.
> Also,
> > > you
> > > > > > should test in all four MP4 containers + two HLS variants like the
> other
> > > > > codecs.
> > > > > 
> > > > > Done (except HLS; HEVC is not supported via HLS at the moment)
> > > > 
> > > > Then you should have negative tests.
> > > 
> > > Done.
> > > 
> > >
https://codereview.chromium.org/816353010/diff/640001/net/base/mime_util.cc
> > > File net/base/mime_util.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/816353010/diff/640001/net/base/mime_util.cc#n...
> > > net/base/mime_util.cc:1049: // TODO(servolk): Implement parsing of hevc
> codec
> > > ids as described in
> > > On 2015/03/25 18:02:58, ddorwin wrote:
> > > > Is there a bug for this?
> > > 
> > > Created crbug.com/482761
> > > 
> > >
> >
>
https://codereview.chromium.org/816353010/diff/640001/net/base/mime_util.cc#n...
> > > net/base/mime_util.cc:1053: *is_ambiguous = true;
> > > On 2015/03/25 18:02:58, ddorwin wrote:
> > > > IIRC, this will cause a DCHECK because of line 536. You might need to
> > special
> > > > case "hvc1.1.L0.0" if you are hitting the DCHECK.
> > > 
> > > Yep, that was causing dchecks, so I've changed is_ambiguous back to false
> for
> > > now.
> > > 
> > >
> >
>
https://codereview.chromium.org/816353010/diff/640001/net/base/mime_util.cc#n...
> > > net/base/mime_util.cc:1056: return false;
> > > On 2015/03/25 18:02:58, ddorwin wrote:
> > > > nit: empty line
> > > 
> > > Done.
> > 
> > ping
> 
> ping

ddorwin@ and/or myself will CR this soon (if me, tomorrow). Apologies for my
delay on this if you've been blocked on me.

[ddorwin, 2015-06-12 00:52:23]

ddorwin@chromium.org changed reviewers:
- scherkus@chromium.org

[ddorwin, 2015-06-12 00:52:27]

I reviewed and commented in PS38. I reviewed:
* changes to content/browser/media/media_canplaytype_browsertest.cc
* media/base/mime_util.cc, which is entirely new due to the move from net/ in
ToT.
* a few other files where I have left comments.

There are also 3 new replies in PS32 for media_canplaytype_browsertest.cc, but I
carried those forward so we don't need to refer back to them.

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

https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:587: kHevcSupported =
kPropProbably;
On 2015/04/29 23:50:32, servolk wrote:
> On 2015/03/25 18:02:58, ddorwin wrote:
> > On 2015/03/25 02:20:20, servolk wrote:
> > > On 2015/03/25 01:21:59, ddorwin wrote:
> > > > Why not just #if/#else?
> > > 
> > > Something like:
> > > #if defined(ENABLE_HEVC_DEMUXING)
> > >   const char* kHevcSupported = kPropProbably;
> > > #else
> > >   const char* kHevcSupported = kPropNot;
> > > #endif
> > > ?
> > > That would be 5 lines instead of 4 and would have the same effect. So if
we
> > can
> > > use just 4, why go for the longer one? I think this one is just as
readable.
> > 
> > Because immediately changing the value looks weird. Also, it can't be used
at
> > the file scope, which you may need.
> 
> Done.

It doesn't look like this was fully addressed. Added comments in PS38.

https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:589: // TODO(servolk):
Add more unit test coverage once we have more info about
On 2015/04/29 23:50:32, servolk wrote:
> On 2015/03/25 18:02:58, ddorwin wrote:
> > On 2015/03/25 02:20:20, servolk wrote:
> > > On 2015/03/25 01:21:59, ddorwin wrote:
> > > > Detailed tests of the codec strings probably belong in an HEVC-specific
> test
> > > > like CodecSupportTest_Avc1Variants. So, you'd have:
> > > > CodecSupportTest_Hev1Variants
> > > > CodecSupportTest_Hvc1Variants
> > > > 
> > > > Something like CodecSupportTest_HevcLevels might also be appropriate.
> > > 
> > > Yes, but that's going to be done when we actually add handling of various
> > > levels/profiles for HEVC.
> > 
> > I was just saying that the tests wouldn't be at the site of the TODO. You
> might
> > leave yourself more notes like those in my comments or just "in separate
> > test(s)."
> 
> I think this whole test could use some refactoring. There's a lot of
duplication
> here.

It is verbose, and that is intentional. We have a lot of test code to ensure we
do the right thing. Previously, we were not doing the right thing and didn't
know it. The logic that drives this is complex, so it's important to test
thoroughly.

It looks like you removed the TODO. I updated
https://code.google.com/p/chromium/issues/detail?id=482761 to capture this.

https://codereview.chromium.org/816353010/diff/620001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:591:
EXPECT_EQ(kHevcSupported, CanPlay("'video/mp4; codecs=\"hvc1.1.L0.0\"'"));
On 2015/04/29 23:50:32, servolk wrote:
> On 2015/03/25 18:02:58, ddorwin wrote:
> > On 2015/03/25 02:20:20, servolk wrote:
> > > On 2015/03/25 01:21:59, ddorwin wrote:
> > > > You are testing video/mp4 far away from the other video/mp4 tests. Also,
> you
> > > > should test in all four MP4 containers + two HLS variants like the other
> > > codecs.
> > > 
> > > Done (except HLS; HEVC is not supported via HLS at the moment)
> > 
> > Then you should have negative tests.
> 
> Done.

It doesn't look like this was done. Added comments in PS38.

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

https://codereview.chromium.org/816353010/diff/740001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:41: 
Move kHevcSupported up here.

https://codereview.chromium.org/816353010/diff/740001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:215: EXPECT_EQ(kNot,
CanPlay("'" + mime +"; codecs=\"avc3, opus\"'"));
Add HEVC

https://codereview.chromium.org/816353010/diff/740001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:256: EXPECT_EQ(kNot,
CanPlay("'" + mime +"; codecs=\"avc3, opus\"'"));
Add HEVC

https://codereview.chromium.org/816353010/diff/740001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:509: const char*
kHevcSupported = kNot;
From PS33:
It's odd to define a constant like this in the middle of a function, especially
when it is used throughout the function. It should at least be at the top, but
around line 41 is probably better.

https://codereview.chromium.org/816353010/diff/740001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:591:
EXPECT_EQ(kHevcSupported, CanPlay("'audio/mp4; codecs=\"hvc1.1.L0.0\"'"));
audio/* cannot contain video codecs.

https://codereview.chromium.org/816353010/diff/740001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:616:
EXPECT_EQ(kHevcSupported, CanPlay("'audio/x-m4a; codecs=\"hvc1.1.L0.0\"'"));
ditto

https://codereview.chromium.org/816353010/diff/740001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:1005:
CanPlay("'application/x-mpegurl; codecs=\"avc1.42F01E\"'"));
Add HEVC (kNot)

https://codereview.chromium.org/816353010/diff/740001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:1066:
CanPlay("'application/vnd.apple.mpegurl; codecs=\"avc1.42F01E\"'"));
ditto

https://codereview.chromium.org/816353010/diff/740001/media/base/mime_util.cc
File media/base/mime_util.cc (right):

https://codereview.chromium.org/816353010/diff/740001/media/base/mime_util.cc...
media/base/mime_util.cc:39: HEVC_MAIN,
Are there HEVC profiles other than main? Is that the only flavor that Cast
supports?

https://codereview.chromium.org/816353010/diff/740001/media/base/mime_util.cc...
media/base/mime_util.cc:39: HEVC_MAIN,
Is there a reason that "HEVC" is used instead of "H265", which would match
"H264"? It's certainly easier to distinguish, but that's not a good reason.
Should "H264" be "AVC"?

https://codereview.chromium.org/816353010/diff/740001/media/base/mime_util.cc...
media/base/mime_util.cc:205: // HEVC/H.265 is supported only in Lollipop+ (API
Level 21). According to
nit: remove "only"
s/. According/ according/

https://codereview.chromium.org/816353010/diff/740001/media/base/mime_util.cc...
media/base/mime_util.cc:405: DCHECK(!is_ambiguous);
Perhaps we can temporarily add (|| codec == HEVC_MAIN) with a reference to the
bug.

https://codereview.chromium.org/816353010/diff/740001/media/base/mime_util.cc...
media/base/mime_util.cc:591: *is_ambiguous = false;
This isn't correct. I know it prevents the DCHECK, but this is not an acceptable
semi-permanent solution because it will result in "probably" when we don't
really know ("maybe") per the comment above. Is there a string we know is always
supported? Otherwise, see my comment above about modifying the DCHECK.

https://codereview.chromium.org/816353010/diff/740001/media/base/mime_util.cc...
media/base/mime_util.cc:611: // either H.264 or HEVC/H.265 codec ID because
currently those are the only
ditto on mixing synonyms for the codecs. Also, why does "HEVC/H.265" appear
throughout the CL? Can we pick one? It seems anyone looking at this code would
understand.

https://codereview.chromium.org/816353010/diff/740001/media/base/mime_util.cc...
media/base/mime_util.cc:614: if (ParseHEVCCodecID(codec_id, codec,
is_ambiguous)) {
Note: It seems wrong to check the less common codec first. The enum order is
also the inverse. However, this is simpler to read, especially since the
majority of platforms that do not enable this code.

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

https://codereview.chromium.org/816353010/diff/740001/media/filters/stream_pa...
media/filters/stream_parser_factory.cc:235: // TODO(servolk): implement ES
parser for HEVC in mpeg2ts container
Remove until we need it. It doesn't sound like we have near-term plans to do
this, and commented-out code is discouraged.

https://codereview.chromium.org/816353010/diff/740001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/816353010/diff/740001/media/media.gyp#newcode647
media/media.gyp:647: 'filters/ffmpeg_h265_to_annex_b_bitstream_converter.cc',
Should we block these on a separate hevc variable that is only set when
'chromecast==1'?

[servolk, 2015-06-12 21:57:31]

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

https://codereview.chromium.org/816353010/diff/740001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:41: 
On 2015/06/12 00:52:26, ddorwin wrote:
> Move kHevcSupported up here.

Done.

https://codereview.chromium.org/816353010/diff/740001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:215: EXPECT_EQ(kNot,
CanPlay("'" + mime +"; codecs=\"avc3, opus\"'"));
On 2015/06/12 00:52:26, ddorwin wrote:
> Add HEVC

Done.

https://codereview.chromium.org/816353010/diff/740001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:509: const char*
kHevcSupported = kNot;
On 2015/06/12 00:52:26, ddorwin wrote:
> From PS33:
> It's odd to define a constant like this in the middle of a function,
especially
> when it is used throughout the function. It should at least be at the top, but
> around line 41 is probably better.

Done.

https://codereview.chromium.org/816353010/diff/740001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:591:
EXPECT_EQ(kHevcSupported, CanPlay("'audio/mp4; codecs=\"hvc1.1.L0.0\"'"));
On 2015/06/12 00:52:26, ddorwin wrote:
> audio/* cannot contain video codecs.

Done.

https://codereview.chromium.org/816353010/diff/740001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:616:
EXPECT_EQ(kHevcSupported, CanPlay("'audio/x-m4a; codecs=\"hvc1.1.L0.0\"'"));
On 2015/06/12 00:52:26, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/816353010/diff/740001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:1005:
CanPlay("'application/x-mpegurl; codecs=\"avc1.42F01E\"'"));
On 2015/06/12 00:52:26, ddorwin wrote:
> Add HEVC (kNot)

Done.

https://codereview.chromium.org/816353010/diff/740001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:1066:
CanPlay("'application/vnd.apple.mpegurl; codecs=\"avc1.42F01E\"'"));
On 2015/06/12 00:52:26, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/816353010/diff/740001/media/base/mime_util.cc
File media/base/mime_util.cc (right):

https://codereview.chromium.org/816353010/diff/740001/media/base/mime_util.cc...
media/base/mime_util.cc:39: HEVC_MAIN,
On 2015/06/12 00:52:26, ddorwin wrote:
> Are there HEVC profiles other than main? Is that the only flavor that Cast
> supports?

There are 3 HEVC v1 profiles: main, main10 and main_still_picture, but at the
moment we'll support only the main profile, and other enum values will be added
as part of work on crbug.com/482761.

https://codereview.chromium.org/816353010/diff/740001/media/base/mime_util.cc...
media/base/mime_util.cc:39: HEVC_MAIN,
On 2015/06/12 00:52:26, ddorwin wrote:
> Is there a reason that "HEVC" is used instead of "H265", which would match
> "H264"? It's certainly easier to distinguish, but that's not a good reason.
> Should "H264" be "AVC"?

Re this and your other comments on HEVC/H265. As far as I understand HEVC and
H.265 are pretty much synonyms, H.265 may have been used earlier in the codec
development process, but now that it's finalized and standardized more people
are starting to refer to it as HEVC. But  I can see a lot of places and articles
on the internet that refer to it as "H.265/HEVC" (much like "H.264/AVC"). I have
mentioned both intentionally, to make it easier for people to find in source
code, e.g. somebody might be searching for "265", others for "hevc". But if you
insist, I can get rid of that and pick just one (which should probably be HEVC).
Btw, out of curiosity, I can see that there are also ~44 occurences of string
"264" under media/ and ~40 occurences of "avc1" - does that need to be fixed as
well?

https://codereview.chromium.org/816353010/diff/740001/media/base/mime_util.cc...
media/base/mime_util.cc:205: // HEVC/H.265 is supported only in Lollipop+ (API
Level 21). According to
On 2015/06/12 00:52:26, ddorwin wrote:
> nit: remove "only"
> s/. According/ according/

Done.

https://codereview.chromium.org/816353010/diff/740001/media/base/mime_util.cc...
media/base/mime_util.cc:591: *is_ambiguous = false;
On 2015/06/12 00:52:26, ddorwin wrote:
> This isn't correct. I know it prevents the DCHECK, but this is not an
acceptable
> semi-permanent solution because it will result in "probably" when we don't
> really know ("maybe") per the comment above. Is there a string we know is
always
> supported? Otherwise, see my comment above about modifying the DCHECK.

Ok, I've added a special case in this method, to ensure we have at least one
unambigous HEVC codec id: "hev1.1.6.L93.B0", which means HEVC main profile,
level 3.1. That is likely to be supported by any HEVC-capable decoder and this
exact string is given as an example in ISO/IEC standard and in DASH-IF docs,
e.g. http://dashif.org/w/2015/04/DASH-IF-IOP-v3.0.pdf. So now we'll use that
special case string in kMP4VideoCodecsExpression without hitting DCHECKs and any
other HEVC codec id is going to be considered ambiguous until complete HEVC
codec id parsing is implemented.

PS: Also added "hvc1.1.6.L93.B0", since that's what I'm using in unit tests.

https://codereview.chromium.org/816353010/diff/740001/media/base/mime_util.cc...
media/base/mime_util.cc:611: // either H.264 or HEVC/H.265 codec ID because
currently those are the only
On 2015/06/12 00:52:26, ddorwin wrote:
> ditto on mixing synonyms for the codecs. Also, why does "HEVC/H.265" appear
> throughout the CL? Can we pick one? It seems anyone looking at this code would
> understand.

See above - yes, people would understand, but mentioning both is useful for
finding pieces of code that deal with H.265/HEVC. Plus we seem to be mixing
H.264 and avc1 also.

https://codereview.chromium.org/816353010/diff/740001/media/base/mime_util.cc...
media/base/mime_util.cc:614: if (ParseHEVCCodecID(codec_id, codec,
is_ambiguous)) {
On 2015/06/12 00:52:26, ddorwin wrote:
> Note: It seems wrong to check the less common codec first. The enum order is
> also the inverse. However, this is simpler to read, especially since the
> majority of platforms that do not enable this code.

Acknowledged.

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

https://codereview.chromium.org/816353010/diff/740001/media/filters/stream_pa...
media/filters/stream_parser_factory.cc:235: // TODO(servolk): implement ES
parser for HEVC in mpeg2ts container
On 2015/06/12 00:52:27, ddorwin wrote:
> Remove until we need it. It doesn't sound like we have near-term plans to do
> this, and commented-out code is discouraged.

Done.

https://codereview.chromium.org/816353010/diff/740001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/816353010/diff/740001/media/media.gyp#newcode647
media/media.gyp:647: 'filters/ffmpeg_h265_to_annex_b_bitstream_converter.cc',
On 2015/06/12 00:52:27, ddorwin wrote:
> Should we block these on a separate hevc variable that is only set when
> 'chromecast==1'?

We had a separate lengthy discussion with rsleevi@ and brettw@ about this.
Earlier patchsets of this CL used to add a new global gyp flag in
build/common.gypi for toggling HEVC support (e.g. see patchset #17). But
rsleevi@ and brettw@ we against introducing any new global gyp flags and
suggested that instead we should use the '#if defined(ENABLE_HEVC_DEMUXING)'
approach everywhere. So if you look, for example at
fmpeg_h265_to_annex_b_bitstream_converter.cc, it has #if at the very top of the
file, to ensure no HEVC code is included into any builds that didn't enable it
explicitly. I think that's the best we can do without global gyp flag.

[wolenetz, 2015-06-18 22:36:28]

I'll get to CR of this tomorrow. One quick note is to please update the
description such that the first line is a summary. my_activity.py, for instance,
currently shows a paragraph for this review ;)

[servolk, 2015-06-18 22:41:17]

done

[ddorwin, 2015-06-19 21:40:15]

https://codereview.chromium.org/816353010/diff/740001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/816353010/diff/740001/media/media.gyp#newcode647
media/media.gyp:647: 'filters/ffmpeg_h265_to_annex_b_bitstream_converter.cc',
On 2015/06/12 21:57:31, servolk wrote:
> On 2015/06/12 00:52:27, ddorwin wrote:
> > Should we block these on a separate hevc variable that is only set when
> > 'chromecast==1'?
> 
> We had a separate lengthy discussion with rsleevi@ and brettw@ about this.
> Earlier patchsets of this CL used to add a new global gyp flag in
> build/common.gypi for toggling HEVC support (e.g. see patchset #17). But
> rsleevi@ and brettw@ we against introducing any new global gyp flags and
> suggested that instead we should use the '#if defined(ENABLE_HEVC_DEMUXING)'
> approach everywhere. So if you look, for example at
> fmpeg_h265_to_annex_b_bitstream_converter.cc, it has #if at the very top of
the
> file, to ensure no HEVC code is included into any builds that didn't enable it
> explicitly. I think that's the best we can do without global gyp flag.

They didn't want *global* GYP flags. What I'm proposing is a GYP flag local to
this GYP file, which I believe is entirely acceptable. It's essentially a way of
defining a local variable to avoid duplicating logic.

We could #ifdef every Android-specific file, but we don't - we use GYP flags.

If you want to be extra careful, you could #error if ENABLE_HEVC_DEMUXING is not
defined in those files.

Speaking of that discussion, were they okay with ENABLE_HEVC_DEMUXING in
common.gypi? That has a similar cost to the entire project. See my comments
there.

https://codereview.chromium.org/816353010/diff/850001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/816353010/diff/850001/build/common.gypi#newco...
build/common.gypi:1834: # Enable HEVC/H265 demuxing. Actual decoding must be
provided by the
wolenetz and I were talking, and this should probably only be enabled for
devices that support it. IOW, a "chromecast=1" and "enable_hevc=1" would need to
be set in GYP_DEFINES.

https://codereview.chromium.org/816353010/diff/850001/build/common.gypi#newco...
build/common.gypi:1836: 'defines': ['ENABLE_HEVC_DEMUXING'],
I'm not sure that defining global defines like this is acceptable (just like GYP
variables).

Since all uses of this define are in media/ (except a test), you should be able
to define this in media.gyp. To satisfy the test, you would also define it
(again) in content_tests.gypi using the same logic.

This is the "right" way to do things and works in this case (unlike things like
USE_PROPRIETARY_CODECS, which are used more widely in other directories).

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

https://codereview.chromium.org/816353010/diff/850001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:193: // "hev0" is
invalid, should be "hev1"
nit: These four comments can probably be collapsed to one (something about "not
'1'").

https://codereview.chromium.org/816353010/diff/850001/media/base/mime_util.cc
File media/base/mime_util.cc (right):

https://codereview.chromium.org/816353010/diff/850001/media/base/mime_util.cc...
media/base/mime_util.cc:588: if (codec_id == "hev1.1.6.L93.B0" || codec_id ==
"hvc1.1.6.L93.B0") {
Move this check to 601. You can even make this a single statement: *is_ambiguous
= !(<condition above>)

That ensures you execute the same path for all codec IDs and avoids a couple
lines of duplicate code.

https://codereview.chromium.org/816353010/diff/850001/media/base/mime_util.cc...
media/base/mime_util.cc:594: if (base::StartsWithASCII(codec_id, "hev1.", true)
||
Please add a comment, possibly above the function, explaining what hev1 and hvc1
are. I think the answer is in one of the comments in this review, but we should
document it clearly.

[servolk, 2015-06-20 00:05:09]

Hi David,
We had this discussion about global GYP flag vs global define a few months ago.
Back then media codec checks lived in net/, so rsleevi@ suggested that we should
use global define (e.g. see the discussion in
https://codereview.chromium.org/877323009/). Now that we need HEVC flag in only
one place outside of media/ and that place is in the unit test, perhaps we
should indeed revive the idea of introducing a local flag in media.gyp.
Just let me see if I understand your suggestion correctly:
1. Introduce enable_hevc_demuxing local flag in media.gyp (and probably also in
media/BUILD.gn?)
2. Use that flag to control which files get built (e.g. if
enable_hevc_demuxing=0, then formats/mp4/hevc.cc, filters/h265_parser.cc, etc
should be excluded from build.
3. If enable_hevc_demuxing gyp flag is 1, then define ENABLE_HEVC_DEMUXING C++
macro for use in files shared with H.264 code paths.
4. In content/content_tests.gypi conditionally define ENABLE_HEVC_DEMUXING C++
macro if chromecast=1, for use in unit tests.
Is that correct?

https://codereview.chromium.org/816353010/diff/850001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/816353010/diff/850001/build/common.gypi#newco...
build/common.gypi:1834: # Enable HEVC/H265 demuxing. Actual decoding must be
provided by the
On 2015/06/19 21:40:14, ddorwin wrote:
> wolenetz and I were talking, and this should probably only be enabled for
> devices that support it. IOW, a "chromecast=1" and "enable_hevc=1" would need
to
> be set in GYP_DEFINES.

Note that this define is added inside chromecast=1 conditional section, so yes,
this is only enabled when explicitly setting chromecast=1 in GYP_DEFINES. Re
enable_hevc flag, see my reply.

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

https://codereview.chromium.org/816353010/diff/850001/content/browser/media/m...
content/browser/media/media_canplaytype_browsertest.cc:193: // "hev0" is
invalid, should be "hev1"
On 2015/06/19 21:40:14, ddorwin wrote:
> nit: These four comments can probably be collapsed to one (something about
"not
> '1'").

Done + added "hevc.xxx" and upper-case variants.

https://codereview.chromium.org/816353010/diff/850001/media/base/mime_util.cc
File media/base/mime_util.cc (right):

https://codereview.chromium.org/816353010/diff/850001/media/base/mime_util.cc...
media/base/mime_util.cc:588: if (codec_id == "hev1.1.6.L93.B0" || codec_id ==
"hvc1.1.6.L93.B0") {
On 2015/06/19 21:40:15, ddorwin wrote:
> Move this check to 601. You can even make this a single statement:
*is_ambiguous
> = !(<condition above>)
> 
> That ensures you execute the same path for all codec IDs and avoids a couple
> lines of duplicate code.

Done. I've kept it a separate block of code to preserve the TODO comment and
make it more visually distinctive/noticeable.

https://codereview.chromium.org/816353010/diff/850001/media/base/mime_util.cc...
media/base/mime_util.cc:594: if (base::StartsWithASCII(codec_id, "hev1.", true)
||
On 2015/06/19 21:40:15, ddorwin wrote:
> Please add a comment, possibly above the function, explaining what hev1 and
hvc1
> are. I think the answer is in one of the comments in this review, but we
should
> document it clearly.

Done (I've expanded comment from line 584 and moved it above the function).

[ddorwin, 2015-06-20 00:28:21]

On 2015/06/20 00:05:09, servolk wrote:
> Hi David,
> We had this discussion about global GYP flag vs global define a few months
ago.
> Back then media codec checks lived in net/, so rsleevi@ suggested that we
should
> use global define (e.g. see the discussion in
> https://codereview.chromium.org/877323009/). Now that we need HEVC flag in
only
> one place outside of media/ and that place is in the unit test, perhaps we
> should indeed revive the idea of introducing a local flag in media.gyp.
> Just let me see if I understand your suggestion correctly:
> 1. Introduce enable_hevc_demuxing local flag in media.gyp (and probably also
in
> media/BUILD.gn?)
> 2. Use that flag to control which files get built (e.g. if
> enable_hevc_demuxing=0, then formats/mp4/hevc.cc, filters/h265_parser.cc, etc
> should be excluded from build.
> 3. If enable_hevc_demuxing gyp flag is 1, then define ENABLE_HEVC_DEMUXING C++
> macro for use in files shared with H.264 code paths.
> 4. In content/content_tests.gypi conditionally define ENABLE_HEVC_DEMUXING C++
> macro if chromecast=1, for use in unit tests.
> Is that correct?

Yes, except #4. The logic for #4 needs to be the same as #3 -
enable_hevc_demuxing, etc.
The same logic for getting enable_hevc_demuxing from GYP_DEFINES would be used
in both .gyp[i] files. See the logic for other values in common.gypi, but it
should work in individual files too I think.

https://codereview.chromium.org/816353010/diff/850001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/816353010/diff/850001/build/common.gypi#newco...
build/common.gypi:1834: # Enable HEVC/H265 demuxing. Actual decoding must be
provided by the
On 2015/06/20 00:05:07, servolk wrote:
> On 2015/06/19 21:40:14, ddorwin wrote:
> > wolenetz and I were talking, and this should probably only be enabled for
> > devices that support it. IOW, a "chromecast=1" and "enable_hevc=1" would
need
> to
> > be set in GYP_DEFINES.
> 
> Note that this define is added inside chromecast=1 conditional section, so
yes,
> this is only enabled when explicitly setting chromecast=1 in GYP_DEFINES. Re
> enable_hevc flag, see my reply.

The point is that enable_hevc should not be set when compiling for platforms
that we *know* do not support HEVC. For example, v1 hardware. We should discuss
the build config goals offline.

https://codereview.chromium.org/816353010/diff/870001/media/base/mime_util.cc
File media/base/mime_util.cc (right):

https://codereview.chromium.org/816353010/diff/870001/media/base/mime_util.cc...
media/base/mime_util.cc:581: // HEVC/H.265 is a new video codec, evolution of
the H.264/AVC standard. ISO/IEC
I don't think the first sentence is relevant.

https://codereview.chromium.org/816353010/diff/870001/media/base/mime_util.cc...
media/base/mime_util.cc:594: *is_ambiguous = true;
This line should be below the TODO, since it would be affected.

Also, you could just do the following at 601.
*is_ambiguous = codec_id != "hev1.1.6.L93.B0" && codec_id != "hvc1.1.6.L93.B0".

If you really want to keep the two values separate, you might break the TODO up
into two separate comments - one that this is false until we implement it and
another that is the workaround.

[servolk, 2015-06-25 23:02:10]

Re switching to using a local GYP flag in media.gyp instead of global define
ENABLE_HEVC_DEMUXING in common.gypi - I've tried that locally and that's
possible to do theoretically, but looks like it would be actually more
cumbersome to maintain and would lead to tricky issues, so I've kept the global
define for now (I'll explain in more details in a separate follow up email
shortly).

https://codereview.chromium.org/816353010/diff/870001/media/base/mime_util.cc
File media/base/mime_util.cc (right):

https://codereview.chromium.org/816353010/diff/870001/media/base/mime_util.cc...
media/base/mime_util.cc:581: // HEVC/H.265 is a new video codec, evolution of
the H.264/AVC standard. ISO/IEC
On 2015/06/20 00:28:21, ddorwin wrote:
> I don't think the first sentence is relevant.

Done.

https://codereview.chromium.org/816353010/diff/870001/media/base/mime_util.cc...
media/base/mime_util.cc:594: *is_ambiguous = true;
On 2015/06/20 00:28:21, ddorwin wrote:
> This line should be below the TODO, since it would be affected.
> 
> Also, you could just do the following at 601.
> *is_ambiguous = codec_id != "hev1.1.6.L93.B0" && codec_id !=
"hvc1.1.6.L93.B0".
> 
> If you really want to keep the two values separate, you might break the TODO
up
> into two separate comments - one that this is false until we implement it and
> another that is the workaround.

Done.

[ddorwin, 2015-06-25 23:51:20]

LG for the files I reviewed other than the GYP/#define issue.

https://codereview.chromium.org/816353010/diff/930001/media/base/mime_util.cc
File media/base/mime_util.cc (right):

https://codereview.chromium.org/816353010/diff/930001/media/base/mime_util.cc...
media/base/mime_util.cc:591: // also use these in unit tests (see
content_browsertests).
nit: That is a binary, not a filename. The latter is probably more relevant.

[servolk, 2015-06-26 20:57:42]

https://codereview.chromium.org/816353010/diff/930001/media/base/mime_util.cc
File media/base/mime_util.cc (right):

https://codereview.chromium.org/816353010/diff/930001/media/base/mime_util.cc...
media/base/mime_util.cc:591: // also use these in unit tests (see
content_browsertests).
On 2015/06/25 23:51:20, ddorwin wrote:
> nit: That is a binary, not a filename. The latter is probably more relevant.

Done.

[servolk, 2015-06-30 21:40:46]

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

[servolk, 2015-06-30 21:42:51]

On 2015/06/26 20:57:42, servolk wrote:
> https://codereview.chromium.org/816353010/diff/930001/media/base/mime_util.cc
> File media/base/mime_util.cc (right):
> 
>
https://codereview.chromium.org/816353010/diff/930001/media/base/mime_util.cc...
> media/base/mime_util.cc:591: // also use these in unit tests (see
> content_browsertests).
> On 2015/06/25 23:51:20, ddorwin wrote:
> > nit: That is a binary, not a filename. The latter is probably more relevant.
> 
> Done.

+brettw@ to confirm that using global ENABLE_HEVC_DEMUXING define in
build/common.gypi is ok.
Brett, could you please take a look at build/common.gypi change in this CL? We
had a discussion with rsleevi@ and you about this in March, and AFAIR the
recommendation was for us to use global define.

[ddorwin, 2015-06-30 22:17:00]

On 2015/06/30 21:42:51, servolk wrote:
> On 2015/06/26 20:57:42, servolk wrote:
> >
https://codereview.chromium.org/816353010/diff/930001/media/base/mime_util.cc
> > File media/base/mime_util.cc (right):
> > 
> >
>
https://codereview.chromium.org/816353010/diff/930001/media/base/mime_util.cc...
> > media/base/mime_util.cc:591: // also use these in unit tests (see
> > content_browsertests).
> > On 2015/06/25 23:51:20, ddorwin wrote:
> > > nit: That is a binary, not a filename. The latter is probably more
relevant.
> > 
> > Done.
> 
> +brettw@ to confirm that using global ENABLE_HEVC_DEMUXING define in
> build/common.gypi is ok.
> Brett, could you please take a look at build/common.gypi change in this CL? We
> had a discussion with rsleevi@ and you about this in March, and AFAIR the
> recommendation was for us to use global define.

This question is missing context. Relying on a global define in this case has
other implications and is IMO worse than the alternatives. We need to have a
discussion about all the issues and alternatives at once.

[brettw, 2015-06-30 22:28:05]

The GN build doesn't match which is suspicious to me.

In general I'd like to move to a local model but ifdef/ifndef doesn't work well
because you're never sure if it applies to you. But we lack a way to do this in
general so havent' been moving much in this direction. This define does seem
weirdly specific to one thing, however.

Have you considered a local define like
  HEVC_DEMUXING_ENABLE_STATE
and setting it to 0 or 1 depending on the build flags? Then the build will fail
if you try to use it if you use it from the wrong target or don't have the
correct dependent settings, etc.

[sandersd (OOO until July 31), 2015-06-30 22:45:40]

On 2015/06/30 22:28:05, brettw wrote:
> The GN build doesn't match which is suspicious to me.
> 
> In general I'd like to move to a local model but ifdef/ifndef doesn't work
well
> because you're never sure if it applies to you. But we lack a way to do this
in
> general so havent' been moving much in this direction. This define does seem
> weirdly specific to one thing, however.
> 
> Have you considered a local define like
>   HEVC_DEMUXING_ENABLE_STATE
> and setting it to 0 or 1 depending on the build flags? Then the build will
fail
> if you try to use it if you use it from the wrong target or don't have the
> correct dependent settings, etc.

This isn't quite correct, and comes with sharp edges. Specifically: it won't
fail the build unless the define is used by actual code (preprocessor directives
just treat undefined like 0), and the previously common practice '#if
defined(FOO_ENABLED)' structures become always enabled. The first issue can be
solved by #ifdef ... #error, but it's onerous to do that at every site.

[brettw, 2015-06-30 23:18:24]

On 2015/06/30 22:45:40, sandersd wrote:
> This isn't quite correct, and comes with sharp edges. Specifically: it won't
> fail the build unless the define is used by actual code (preprocessor
directives
> just treat undefined like 0), and the previously common practice '#if
> defined(FOO_ENABLED)' structures become always enabled. The first issue can be
> solved by #ifdef ... #error, but it's onerous to do that at every site.

Interesting, I didn't know (or forgot) that behavior.

This define sounds so feature-specific that I'm really not a fan of making it
global. I don't think this is respectful to the global namespace. What's the
downside of making it specific to media and adding a direct_dependent setting
for it?

[servolk, 2015-08-28 20:28:17]

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

[servolk, 2015-08-28 20:33:56]

On 2015/06/30 23:18:24, brettw wrote:
> On 2015/06/30 22:45:40, sandersd wrote:
> > This isn't quite correct, and comes with sharp edges. Specifically: it won't
> > fail the build unless the define is used by actual code (preprocessor
> directives
> > just treat undefined like 0), and the previously common practice '#if
> > defined(FOO_ENABLED)' structures become always enabled. The first issue can
be
> > solved by #ifdef ... #error, but it's onerous to do that at every site.
> 
> Interesting, I didn't know (or forgot) that behavior.
> 
> This define sounds so feature-specific that I'm really not a fan of making it
> global. I don't think this is respectful to the global namespace. What's the
> downside of making it specific to media and adding a direct_dependent setting
> for it?

I've reworked this CL as we discussed on Wednesday. Patchset #54 now uses the
new bitstream converter interfaces (and that allows us to get rid of
HEVC-specific code in header files). Patchset #55 switches to using local gyp/gn
flag in media and that flag is duplicated in content gyp/gn files for the
media_canplaytype_browsertest.cc unit test. PTAL.

[servolk, 2015-09-02 18:46:04]

On 2015/08/28 20:33:56, servolk wrote:
> On 2015/06/30 23:18:24, brettw wrote:
> > On 2015/06/30 22:45:40, sandersd wrote:
> > > This isn't quite correct, and comes with sharp edges. Specifically: it
won't
> > > fail the build unless the define is used by actual code (preprocessor
> > directives
> > > just treat undefined like 0), and the previously common practice '#if
> > > defined(FOO_ENABLED)' structures become always enabled. The first issue
can
> be
> > > solved by #ifdef ... #error, but it's onerous to do that at every site.
> > 
> > Interesting, I didn't know (or forgot) that behavior.
> > 
> > This define sounds so feature-specific that I'm really not a fan of making
it
> > global. I don't think this is respectful to the global namespace. What's the
> > downside of making it specific to media and adding a direct_dependent
setting
> > for it?
> 
> I've reworked this CL as we discussed on Wednesday. Patchset #54 now uses the
> new bitstream converter interfaces (and that allows us to get rid of
> HEVC-specific code in header files). Patchset #55 switches to using local
gyp/gn
> flag in media and that flag is duplicated in content gyp/gn files for the
> media_canplaytype_browsertest.cc unit test. PTAL.

ping, PTAL

[wolenetz, 2015-09-02 20:43:19]

This is looking pretty good. Our discussion last week helped. Mostly nits, my
own noob-ness around GN, and a question for ddorwin. As I mentioned yesterday,
I'll be OoO from roughly now until the beginning of September 8th, so assuming
my nits are satisfied well enough, I am now rubber-stamping ddorwin@'s possible
l*tm that may occur while I'm OoO.

rslgtm

https://codereview.chromium.org/816353010/diff/1090001/content/content_tests....
File content/content_tests.gypi (right):

https://codereview.chromium.org/816353010/diff/1090001/content/content_tests....
content/content_tests.gypi:1515: ['chromecast==1', {
Double-checking: Is it the case that all chromecast devices should have
ENABLE_HEVC_DEMUXING defined for the tests here and the media code, too?

https://codereview.chromium.org/816353010/diff/1090001/content/test/BUILD.gn
File content/test/BUILD.gn (right):

https://codereview.chromium.org/816353010/diff/1090001/content/test/BUILD.gn#...
content/test/BUILD.gn:315: enable_hevc_demuxing = false
I don't know GN well enough: does this 'enable_hevc_demuxing' alias or
hide/conflict with the same identifier in the media/...gn files, or is it
different?

https://codereview.chromium.org/816353010/diff/1090001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/816353010/diff/1090001/media/BUILD.gn#newcode35
media/BUILD.gn:35: if (proprietary_codecs && enable_hevc_demuxing) {
ditto my content/...gn question about 'enable_hevc_demuxing'. Pardon my GN
noob-ness. I get that 'defines' aren't for exporting, but is the underlying
'enable_hevc_demuxing' the same entity as in content/...gn?

https://codereview.chromium.org/816353010/diff/1090001/media/base/mime_util.cc
File media/base/mime_util.cc (right):

https://codereview.chromium.org/816353010/diff/1090001/media/base/mime_util.c...
media/base/mime_util.cc:608: #if defined(ENABLE_HEVC_DEMUXING)
nit: move the condition start prior to the comment start for consistency.

https://codereview.chromium.org/816353010/diff/1090001/media/filters/ffmpeg_h...
File media/filters/ffmpeg_h265_to_annex_b_bitstream_converter.cc (right):

https://codereview.chromium.org/816353010/diff/1090001/media/filters/ffmpeg_h...
media/filters/ffmpeg_h265_to_annex_b_bitstream_converter.cc:5: #include
"media/filters/ffmpeg_h265_to_annex_b_bitstream_converter.h"
ddorwin: do we want to have extra sanity check by conditioning the code in this
impl file internally on ENABLE_HEVC_DEMUXING being defined, or is the gn/gyp
inclusion/exclusion of this file and its header sufficient sanity?

https://codereview.chromium.org/816353010/diff/1090001/media/filters/h265_par...
File media/filters/h265_parser_unittest.cc (right):

https://codereview.chromium.org/816353010/diff/1090001/media/filters/h265_par...
media/filters/h265_parser_unittest.cc:16: int num_nalus = 35;
nit: const

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/box_...
File media/formats/mp4/box_definitions.cc (right):

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/box_...
media/formats/mp4/box_definitions.cc:14: #if defined(USE_PROPRIETARY_CODECS) &&
defined(ENABLE_HEVC_DEMUXING)
nit: why sometimes condition on both proprietary+hevc_demuxing, and other times
(like in header for this file), just condition on the latter?

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/box_...
media/formats/mp4/box_definitions.cc:496:
RCHECK(reader->Read1(&configurationVersion) && configurationVersion == 1 &&
nit: now would be a great time to MEDIA_LOG why RCHECK might return false for
this new code. Without such detail, developers will be left wondering why decode
error resulted for (in this case, bad content). Please strongly consider adding
"MEDIA_LOG(ERROR, media_log) << ...;" describing the failure conditions for this
method here and in other current RCHECKs in this method.

Likewise, some basic sanity in the unit test, using a StrictMock<MockMediaLog>
for both a successful parse and at least one of the error conditions is a strong
recommendation.

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/box_...
media/formats/mp4/box_definitions.cc:644: MEDIA_LOG(ERROR, reader->media_log())
<< __FUNCTION__
nit: We haven't been including __FUNCTION__ in most of our current MEDIA_LOG()
in ToT, rather more user-friendly but still precise text. I don't have a strong
feeling here, but consider this message will likely eventually be plumbed into
devtools console output, which similarly doesn't usually show Chrome code
function names explicitly (IIRC).

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/hevc.cc
File media/formats/mp4/hevc.cc (right):

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/hevc...
media/formats/mp4/hevc.cc:103: // TODO(servolk): Implement this
nit: reference a bug?

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/mp4_...
File media/formats/mp4/mp4_stream_parser.cc (right):

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/mp4_...
media/formats/mp4/mp4_stream_parser.cc:307:
video_config.Initialize(entry.video_codec, entry.video_codec_profile,
nit: if we don't already DVLOG(1) entry.video_codec, this CL seems like a good
time to do that.

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/mp4_...
File media/formats/mp4/mp4_stream_parser_unittest.cc (right):

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/mp4_...
media/formats/mp4/mp4_stream_parser_unittest.cc:301:
EXPECT_TRUE(AppendDataInPieces(buffer->data(), buffer->data_size(), 512));
ditto nit: I would think my recent changes would now make this test fail without
some EXPECT_MEDIA_LOG(). Also, please add some parse failure (and associated
EXPECT_MEDIA_LOGs()) case(s).

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

https://codereview.chromium.org/816353010/diff/1090001/media/mojo/interfaces/...
media/mojo/interfaces/media_types.mojom:123: // Kept in sync with
media::VideoCodec via static_asserts.
Please update media_type_converters.cc's VideoCodec static asserts to include
HEVC.

[wolenetz, 2015-09-02 20:44:22]

On 2015/09/02 20:43:19, wolenetz_OoO_Sep3_through_Sep7 wrote:
> This is looking pretty good. Our discussion last week helped. Mostly nits, my
> own noob-ness around GN, and a question for ddorwin. As I mentioned yesterday,
> I'll be OoO from roughly now until the beginning of September 8th, so assuming
> my nits are satisfied well enough, I am now rubber-stamping ddorwin@'s
possible
> l*tm that may occur while I'm OoO.
> 
> rslgtm
> 
>
https://codereview.chromium.org/816353010/diff/1090001/content/content_tests....
> File content/content_tests.gypi (right):
> 
>
https://codereview.chromium.org/816353010/diff/1090001/content/content_tests....
> content/content_tests.gypi:1515: ['chromecast==1', {
> Double-checking: Is it the case that all chromecast devices should have
> ENABLE_HEVC_DEMUXING defined for the tests here and the media code, too?
> 
> https://codereview.chromium.org/816353010/diff/1090001/content/test/BUILD.gn
> File content/test/BUILD.gn (right):
> 
>
https://codereview.chromium.org/816353010/diff/1090001/content/test/BUILD.gn#...
> content/test/BUILD.gn:315: enable_hevc_demuxing = false
> I don't know GN well enough: does this 'enable_hevc_demuxing' alias or
> hide/conflict with the same identifier in the media/...gn files, or is it
> different?
> 
> https://codereview.chromium.org/816353010/diff/1090001/media/BUILD.gn
> File media/BUILD.gn (right):
> 
>
https://codereview.chromium.org/816353010/diff/1090001/media/BUILD.gn#newcode35
> media/BUILD.gn:35: if (proprietary_codecs && enable_hevc_demuxing) {
> ditto my content/...gn question about 'enable_hevc_demuxing'. Pardon my GN
> noob-ness. I get that 'defines' aren't for exporting, but is the underlying
> 'enable_hevc_demuxing' the same entity as in content/...gn?
> 
> https://codereview.chromium.org/816353010/diff/1090001/media/base/mime_util.cc
> File media/base/mime_util.cc (right):
> 
>
https://codereview.chromium.org/816353010/diff/1090001/media/base/mime_util.c...
> media/base/mime_util.cc:608: #if defined(ENABLE_HEVC_DEMUXING)
> nit: move the condition start prior to the comment start for consistency.
> 
>
https://codereview.chromium.org/816353010/diff/1090001/media/filters/ffmpeg_h...
> File media/filters/ffmpeg_h265_to_annex_b_bitstream_converter.cc (right):
> 
>
https://codereview.chromium.org/816353010/diff/1090001/media/filters/ffmpeg_h...
> media/filters/ffmpeg_h265_to_annex_b_bitstream_converter.cc:5: #include
> "media/filters/ffmpeg_h265_to_annex_b_bitstream_converter.h"
> ddorwin: do we want to have extra sanity check by conditioning the code in
this
> impl file internally on ENABLE_HEVC_DEMUXING being defined, or is the gn/gyp
> inclusion/exclusion of this file and its header sufficient sanity?
> 
>
https://codereview.chromium.org/816353010/diff/1090001/media/filters/h265_par...
> File media/filters/h265_parser_unittest.cc (right):
> 
>
https://codereview.chromium.org/816353010/diff/1090001/media/filters/h265_par...
> media/filters/h265_parser_unittest.cc:16: int num_nalus = 35;
> nit: const
> 
>
https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/box_...
> File media/formats/mp4/box_definitions.cc (right):
> 
>
https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/box_...
> media/formats/mp4/box_definitions.cc:14: #if defined(USE_PROPRIETARY_CODECS)
&&
> defined(ENABLE_HEVC_DEMUXING)
> nit: why sometimes condition on both proprietary+hevc_demuxing, and other
times
> (like in header for this file), just condition on the latter?
> 
>
https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/box_...
> media/formats/mp4/box_definitions.cc:496:
> RCHECK(reader->Read1(&configurationVersion) && configurationVersion == 1 &&
> nit: now would be a great time to MEDIA_LOG why RCHECK might return false for
> this new code. Without such detail, developers will be left wondering why
decode
> error resulted for (in this case, bad content). Please strongly consider
adding
> "MEDIA_LOG(ERROR, media_log) << ...;" describing the failure conditions for
this
> method here and in other current RCHECKs in this method.
> 
> Likewise, some basic sanity in the unit test, using a StrictMock<MockMediaLog>
> for both a successful parse and at least one of the error conditions is a
strong
> recommendation.
> 
>
https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/box_...
> media/formats/mp4/box_definitions.cc:644: MEDIA_LOG(ERROR,
reader->media_log())
> << __FUNCTION__
> nit: We haven't been including __FUNCTION__ in most of our current MEDIA_LOG()
> in ToT, rather more user-friendly but still precise text. I don't have a
strong
> feeling here, but consider this message will likely eventually be plumbed into
> devtools console output, which similarly doesn't usually show Chrome code
> function names explicitly (IIRC).
> 
>
https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/hevc.cc
> File media/formats/mp4/hevc.cc (right):
> 
>
https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/hevc...
> media/formats/mp4/hevc.cc:103: // TODO(servolk): Implement this
> nit: reference a bug?
> 
>
https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/mp4_...
> File media/formats/mp4/mp4_stream_parser.cc (right):
> 
>
https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/mp4_...
> media/formats/mp4/mp4_stream_parser.cc:307:
> video_config.Initialize(entry.video_codec, entry.video_codec_profile,
> nit: if we don't already DVLOG(1) entry.video_codec, this CL seems like a good
> time to do that.
> 
>
https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/mp4_...
> File media/formats/mp4/mp4_stream_parser_unittest.cc (right):
> 
>
https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/mp4_...
> media/formats/mp4/mp4_stream_parser_unittest.cc:301:
> EXPECT_TRUE(AppendDataInPieces(buffer->data(), buffer->data_size(), 512));
> ditto nit: I would think my recent changes would now make this test fail
without
> some EXPECT_MEDIA_LOG(). Also, please add some parse failure (and associated
> EXPECT_MEDIA_LOGs()) case(s).
> 
>
https://codereview.chromium.org/816353010/diff/1090001/media/mojo/interfaces/...
> File media/mojo/interfaces/media_types.mojom (right):
> 
>
https://codereview.chromium.org/816353010/diff/1090001/media/mojo/interfaces/...
> media/mojo/interfaces/media_types.mojom:123: // Kept in sync with
> media::VideoCodec via static_asserts.
> Please update media_type_converters.cc's VideoCodec static asserts to include
> HEVC.

It seems Rietveld doesn't understand rslgtm.
So, explicitly: rubber-stamp lgtm % my nits and ddorwin@'s l*tm.

[ddorwin, 2015-09-02 20:50:06]

I'll try to re-review the build changes this week.

https://codereview.chromium.org/816353010/diff/1090001/media/filters/ffmpeg_h...
File media/filters/ffmpeg_h265_to_annex_b_bitstream_converter.cc (right):

https://codereview.chromium.org/816353010/diff/1090001/media/filters/ffmpeg_h...
media/filters/ffmpeg_h265_to_annex_b_bitstream_converter.cc:5: #include
"media/filters/ffmpeg_h265_to_annex_b_bitstream_converter.h"
On 2015/09/02 20:43:18, wolenetz_OoO_Sep3_through_Sep7 wrote:
> ddorwin: do we want to have extra sanity check by conditioning the code in
this
> impl file internally on ENABLE_HEVC_DEMUXING being defined, or is the gn/gyp
> inclusion/exclusion of this file and its header sufficient sanity?

I don't think that's necessary or that we generally do this (think
platform-specific files).

[ddorwin, 2015-09-02 20:50:07]

I'll try to re-review the build changes this week.

[wolenetz, 2015-09-02 20:51:58]

From chat with ddorwin@, he'll look at the gyp/gn stuff, but not everything.

So, lgtm % my earlier nits and % ddorwin CR'ing the gyp/gn piece.

https://codereview.chromium.org/816353010/diff/1090001/media/filters/ffmpeg_h...
File media/filters/ffmpeg_h265_to_annex_b_bitstream_converter.cc (right):

https://codereview.chromium.org/816353010/diff/1090001/media/filters/ffmpeg_h...
media/filters/ffmpeg_h265_to_annex_b_bitstream_converter.cc:5: #include
"media/filters/ffmpeg_h265_to_annex_b_bitstream_converter.h"
On 2015/09/02 20:50:06, ddorwin wrote:
> On 2015/09/02 20:43:18, wolenetz_OoO_Sep3_through_Sep7 wrote:
> > ddorwin: do we want to have extra sanity check by conditioning the code in
> this
> > impl file internally on ENABLE_HEVC_DEMUXING being defined, or is the gn/gyp
> > inclusion/exclusion of this file and its header sufficient sanity?
> 
> I don't think that's necessary or that we generally do this (think
> platform-specific files).

Acknowledged.

[DaleCurtis, 2015-09-02 21:20:41]

https://codereview.chromium.org/816353010/diff/1090001/content/test/BUILD.gn
File content/test/BUILD.gn (right):

https://codereview.chromium.org/816353010/diff/1090001/content/test/BUILD.gn#...
content/test/BUILD.gn:315: enable_hevc_demuxing = false
On 2015/09/02 20:43:18, wolenetz_OoO_Sep3_through_Sep7 wrote:
> I don't know GN well enough: does this 'enable_hevc_demuxing' alias or
> hide/conflict with the same identifier in the media/...gn files, or is it
> different?

This file should include the media_options.gni file via import instead of
redefining it here.

https://codereview.chromium.org/816353010/diff/1090001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/816353010/diff/1090001/media/BUILD.gn#newcode35
media/BUILD.gn:35: if (proprietary_codecs && enable_hevc_demuxing) {
On 2015/09/02 20:43:18, wolenetz_OoO_Sep3_through_Sep7 wrote:
> ditto my content/...gn question about 'enable_hevc_demuxing'. Pardon my GN
> noob-ness. I get that 'defines' aren't for exporting, but is the underlying
> 'enable_hevc_demuxing' the same entity as in content/...gn?

This file imports the media_options.gni file so it's all good.

[wolenetz, 2015-09-02 21:22:37]

On 2015/09/02 21:20:41, DaleCurtis wrote:
> https://codereview.chromium.org/816353010/diff/1090001/content/test/BUILD.gn
> File content/test/BUILD.gn (right):
> 
>
https://codereview.chromium.org/816353010/diff/1090001/content/test/BUILD.gn#...
> content/test/BUILD.gn:315: enable_hevc_demuxing = false
> On 2015/09/02 20:43:18, wolenetz_OoO_Sep3_through_Sep7 wrote:
> > I don't know GN well enough: does this 'enable_hevc_demuxing' alias or
> > hide/conflict with the same identifier in the media/...gn files, or is it
> > different?
> 
> This file should include the media_options.gni file via import instead of
> redefining it here.
> 
> https://codereview.chromium.org/816353010/diff/1090001/media/BUILD.gn
> File media/BUILD.gn (right):
> 
>
https://codereview.chromium.org/816353010/diff/1090001/media/BUILD.gn#newcode35
> media/BUILD.gn:35: if (proprietary_codecs && enable_hevc_demuxing) {
> On 2015/09/02 20:43:18, wolenetz_OoO_Sep3_through_Sep7 wrote:
> > ditto my content/...gn question about 'enable_hevc_demuxing'. Pardon my GN
> > noob-ness. I get that 'defines' aren't for exporting, but is the underlying
> > 'enable_hevc_demuxing' the same entity as in content/...gn?
> 
> This file imports the media_options.gni file so it's all good.

Thank you for providing clarity on this, Dale.

[wolenetz, 2015-09-02 21:56:59]

Additional nit/detail on existing nit:

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/mp4_...
File media/formats/mp4/mp4_stream_parser_unittest.cc (right):

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/mp4_...
media/formats/mp4/mp4_stream_parser_unittest.cc:298: TEST_F(MP4StreamParserTest,
HEVC_in_MP4_container) {
cast_shell_linux try bot doesn't seem to have run this test.
Please fix.

[wolenetz, 2015-09-02 22:09:25]

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/mp4_...
File media/formats/mp4/mp4_stream_parser_unittest.cc (right):

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/mp4_...
media/formats/mp4/mp4_stream_parser_unittest.cc:298: TEST_F(MP4StreamParserTest,
HEVC_in_MP4_container) {
On 2015/09/02 21:56:59, wolenetz_OoO_Sep3_through_Sep7 wrote:
> cast_shell_linux try bot doesn't seem to have run this test.
> Please fix.

Hmm. More complicated than I initially thought. ENABLE_HEVC_DEMUXING is
dependent on both chromecast==1 and proprietary_codecs==1
(https://codereview.chromium.org/816353010/diff/1090001/media/media.gyp). The
latter isn't defined for chromium trybots, just for official release builds. So
this test will not be exercised in Chromium waterfall and may rot. Please ensure
this passes on Chromecast downstream at least :)

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/mp4_...
media/formats/mp4/mp4_stream_parser_unittest.cc:301:
EXPECT_TRUE(AppendDataInPieces(buffer->data(), buffer->data_size(), 512));
On 2015/09/02 20:43:18, wolenetz_OoO_Sep3_through_Sep7 wrote:
> ditto nit: I would think my recent changes would now make this test fail
without
> some EXPECT_MEDIA_LOG(). Also, please add some parse failure (and associated
> EXPECT_MEDIA_LOGs()) case(s).

See also my comment earlier in this test case.

[servolk, 2015-09-03 00:17:51]

https://codereview.chromium.org/816353010/diff/1090001/content/content_tests....
File content/content_tests.gypi (right):

https://codereview.chromium.org/816353010/diff/1090001/content/content_tests....
content/content_tests.gypi:1515: ['chromecast==1', {
On 2015/09/02 20:43:18, wolenetz_OoO_Sep3_through_Sep7 wrote:
> Double-checking: Is it the case that all chromecast devices should have
> ENABLE_HEVC_DEMUXING defined for the tests here and the media code, too?

Yes, as LeChun explained we want a single cast_shell binary for all
Chromecast-supported platforms. That means cast_shell will always support HEVC
demuxing, but it will check actual platform capabilities to decide if HEVC
playback should be enabled or not on the current platform (i.e. the HEVC flag
will be always enabled for Chromecast/cast_shell builds, but cast_shell will
check platform capabilities before deciding what should be the answer to
HTMLMediaElement.canPlayType/MediaSource.isTypeSupported('hevc')

https://codereview.chromium.org/816353010/diff/1090001/content/test/BUILD.gn
File content/test/BUILD.gn (right):

https://codereview.chromium.org/816353010/diff/1090001/content/test/BUILD.gn#...
content/test/BUILD.gn:315: enable_hevc_demuxing = false
On 2015/09/02 21:20:41, DaleCurtis wrote:
> On 2015/09/02 20:43:18, wolenetz_OoO_Sep3_through_Sep7 wrote:
> > I don't know GN well enough: does this 'enable_hevc_demuxing' alias or
> > hide/conflict with the same identifier in the media/...gn files, or is it
> > different?
> 
> This file should include the media_options.gni file via import instead of
> redefining it here.

Done.

https://codereview.chromium.org/816353010/diff/1090001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/816353010/diff/1090001/media/BUILD.gn#newcode35
media/BUILD.gn:35: if (proprietary_codecs && enable_hevc_demuxing) {
On 2015/09/02 21:20:41, DaleCurtis wrote:
> On 2015/09/02 20:43:18, wolenetz_OoO_Sep3_through_Sep7 wrote:
> > ditto my content/...gn question about 'enable_hevc_demuxing'. Pardon my GN
> > noob-ness. I get that 'defines' aren't for exporting, but is the underlying
> > 'enable_hevc_demuxing' the same entity as in content/...gn?
> 
> This file imports the media_options.gni file so it's all good.

Yes, in this case we are importing enable_hevc_demuxing flag from
//media/media_options.gni, and if that flag is set, we'll define the
ENABLE_HEVC_DEMUXING C++ macro for the code to use.

https://codereview.chromium.org/816353010/diff/1090001/media/base/mime_util.cc
File media/base/mime_util.cc (right):

https://codereview.chromium.org/816353010/diff/1090001/media/base/mime_util.c...
media/base/mime_util.cc:608: #if defined(ENABLE_HEVC_DEMUXING)
On 2015/09/02 20:43:18, wolenetz_OoO_Sep3_through_Sep7 wrote:
> nit: move the condition start prior to the comment start for consistency.

But this comment talks about both H.264 and HEVC. We probably shouldn't move it
inside the #if HEVC section. Or did I misunderstand something?

https://codereview.chromium.org/816353010/diff/1090001/media/filters/ffmpeg_h...
File media/filters/ffmpeg_h265_to_annex_b_bitstream_converter.cc (right):

https://codereview.chromium.org/816353010/diff/1090001/media/filters/ffmpeg_h...
media/filters/ffmpeg_h265_to_annex_b_bitstream_converter.cc:5: #include
"media/filters/ffmpeg_h265_to_annex_b_bitstream_converter.h"
On 2015/09/02 20:51:58, wolenetz_OoO_Sep3_through_Sep7 wrote:
> On 2015/09/02 20:50:06, ddorwin wrote:
> > On 2015/09/02 20:43:18, wolenetz_OoO_Sep3_through_Sep7 wrote:
> > > ddorwin: do we want to have extra sanity check by conditioning the code in
> > this
> > > impl file internally on ENABLE_HEVC_DEMUXING being defined, or is the
gn/gyp
> > > inclusion/exclusion of this file and its header sufficient sanity?
> > 
> > I don't think that's necessary or that we generally do this (think
> > platform-specific files).
> 
> Acknowledged.

Acknowledged.

https://codereview.chromium.org/816353010/diff/1090001/media/filters/h265_par...
File media/filters/h265_parser_unittest.cc (right):

https://codereview.chromium.org/816353010/diff/1090001/media/filters/h265_par...
media/filters/h265_parser_unittest.cc:16: int num_nalus = 35;
On 2015/09/02 20:43:18, wolenetz_OoO_Sep3_through_Sep7 wrote:
> nit: const

Done.

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/box_...
File media/formats/mp4/box_definitions.cc (right):

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/box_...
media/formats/mp4/box_definitions.cc:14: #if defined(USE_PROPRIETARY_CODECS) &&
defined(ENABLE_HEVC_DEMUXING)
On 2015/09/02 20:43:18, wolenetz_OoO_Sep3_through_Sep7 wrote:
> nit: why sometimes condition on both proprietary+hevc_demuxing, and other
times
> (like in header for this file), just condition on the latter?

Oh, these were remains of the way we had to do things when using global defined
in common.gypi. Now that ENABLE_HEVC_DEMUXING is defined in media.gyp/BUILD.gn
we can just ensure right in the build config files that HEVC is enabled only
when proprietary codecs are supported, so I'll update these #ifdef conditions
throughout the CL now.

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/box_...
media/formats/mp4/box_definitions.cc:496:
RCHECK(reader->Read1(&configurationVersion) && configurationVersion == 1 &&
On 2015/09/02 20:43:18, wolenetz_OoO_Sep3_through_Sep7 wrote:
> nit: now would be a great time to MEDIA_LOG why RCHECK might return false for
> this new code. Without such detail, developers will be left wondering why
decode
> error resulted for (in this case, bad content). Please strongly consider
adding
> "MEDIA_LOG(ERROR, media_log) << ...;" describing the failure conditions for
this
> method here and in other current RCHECKs in this method.
> 
> Likewise, some basic sanity in the unit test, using a StrictMock<MockMediaLog>
> for both a successful parse and at least one of the error conditions is a
strong
> recommendation

On one hand I understand your concerns, but on the other hand I was trying to be
consistent with H.264/AVC implementation here and the code there looks exactly
the same (see lines 408-411 above), only avcC header is smaller and has fewer
parameters. We could try to improve logging here to make diagnosing issues
easier, but note that all we are doing here is reading data from hvcC box (it's
a bunch of read* + one configVersion check), so if something fails right here
there are only two possible causes - either configVersion is unsupported, or the
hvcC header that we are trying to read was too short. If there's any other issue
with the header, then we'll get failure at one of the RCHECKs below and not at
this one. So while there's definitely room for improvement, we can make that
later as a separate CL if it ever becomes a real concern. For now it's probably
fine, I've never seen a failure here, except on some very early patchsets of
this CL.

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/hevc.cc
File media/formats/mp4/hevc.cc (right):

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/hevc...
media/formats/mp4/hevc.cc:103: // TODO(servolk): Implement this
On 2015/09/02 20:43:18, wolenetz_OoO_Sep3_through_Sep7 wrote:
> nit: reference a bug?

Done, crbug.com/527595

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/mp4_...
File media/formats/mp4/mp4_stream_parser.cc (right):

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/mp4_...
media/formats/mp4/mp4_stream_parser.cc:307:
video_config.Initialize(entry.video_codec, entry.video_codec_profile,
On 2015/09/02 20:43:18, wolenetz_OoO_Sep3_through_Sep7 wrote:
> nit: if we don't already DVLOG(1) entry.video_codec, this CL seems like a good
> time to do that.

We already log codec info into media_log when parsing avcC/hvcC boxes, that
should probably sufficient (and we have more detailed information there, so if
we want more details we should expand logging there, instead of adding it here).

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/mp4_...
File media/formats/mp4/mp4_stream_parser_unittest.cc (right):

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/mp4_...
media/formats/mp4/mp4_stream_parser_unittest.cc:298: TEST_F(MP4StreamParserTest,
HEVC_in_MP4_container) {
On 2015/09/02 22:09:24, wolenetz_OoO_Sep3_through_Sep7 wrote:
> On 2015/09/02 21:56:59, wolenetz_OoO_Sep3_through_Sep7 wrote:
> > cast_shell_linux try bot doesn't seem to have run this test.
> > Please fix.
> 
> Hmm. More complicated than I initially thought. ENABLE_HEVC_DEMUXING is
> dependent on both chromecast==1 and proprietary_codecs==1
> (https://codereview.chromium.org/816353010/diff/1090001/media/media.gyp). The
> latter isn't defined for chromium trybots, just for official release builds.
So
> this test will not be exercised in Chromium waterfall and may rot. Please
ensure
> this passes on Chromecast downstream at least :)

As we've discussed offline we are in the same situation as MP4 here.
But we do have internal Chromecast tests with actual video stream playback
(including HEVC), so we got this covered.

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/mp4_...
media/formats/mp4/mp4_stream_parser_unittest.cc:301:
EXPECT_TRUE(AppendDataInPieces(buffer->data(), buffer->data_size(), 512));
On 2015/09/02 20:43:18, wolenetz_OoO_Sep3_through_Sep7 wrote:
> ditto nit: I would think my recent changes would now make this test fail
without
> some EXPECT_MEDIA_LOG(). Also, please add some parse failure (and associated
> EXPECT_MEDIA_LOGs()) case(s).

Indeed, the test wasn't running, because ENABLE_HEVC_DEMUXING wasn't defined for
the test target. I've updated the .gyp file (BUILD.gn already handles this by
adding the define into media_config) and seen test failures. Added
EXPECT_MEDIA_LOG to fix the test.

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

https://codereview.chromium.org/816353010/diff/1090001/media/mojo/interfaces/...
media/mojo/interfaces/media_types.mojom:123: // Kept in sync with
media::VideoCodec via static_asserts.
On 2015/09/02 20:43:19, wolenetz_OoO_Sep3_through_Sep7 wrote:
> Please update media_type_converters.cc's VideoCodec static asserts to include
> HEVC.

Done.

[DaleCurtis, 2015-09-03 01:41:28]

gyp/gn lgtm

[servolk, 2015-09-08 18:35:55]

On 2015/09/03 01:41:28, DaleCurtis wrote:
> gyp/gn lgtm

Ping. David, could you take a look please?

[ddorwin, 2015-09-08 18:55:44]

LG except some build cleanup stuff. Thanks.

https://codereview.chromium.org/816353010/diff/1110001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/816353010/diff/1110001/media/BUILD.gn#newcode681
media/BUILD.gn:681: "filters/h265_parser_unittest.cc",
Requires enable_hevc_demuxing

https://codereview.chromium.org/816353010/diff/1110001/media/formats/mp4/box_...
File media/formats/mp4/box_definitions.h (right):

https://codereview.chromium.org/816353010/diff/1110001/media/formats/mp4/box_...
media/formats/mp4/box_definitions.h:15: #include
"media/base/video_decoder_config.h"
Is this necessary?

https://codereview.chromium.org/816353010/diff/1110001/media/formats/mp4/box_...
media/formats/mp4/box_definitions.h:194: struct MEDIA_EXPORT
HEVCDecoderConfigurationRecord : Box {
Can we put this in an HEVC-specific file? It's not actually used here.

Can it just be in the .cc file? As we have discussed, not using the #ifdef in
header files is preferred.

https://codereview.chromium.org/816353010/diff/1110001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/816353010/diff/1110001/media/media.gyp#newcod...
media/media.gyp:673: 'filters/ffmpeg_h265_to_annex_b_bitstream_converter.cc',
Remove here?

https://codereview.chromium.org/816353010/diff/1110001/media/media.gyp#newcod...
media/media.gyp:1113: 'filters/ffmpeg_h265_to_annex_b_bitstream_converter.cc',
Then probably need to use a media_use_ffmpeg==1 check here.

[servolk, 2015-09-08 19:22:02]

https://codereview.chromium.org/816353010/diff/1110001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/816353010/diff/1110001/media/BUILD.gn#newcode681
media/BUILD.gn:681: "filters/h265_parser_unittest.cc",
On 2015/09/08 18:55:44, ddorwin wrote:
> Requires enable_hevc_demuxing

Done.

https://codereview.chromium.org/816353010/diff/1110001/media/formats/mp4/box_...
File media/formats/mp4/box_definitions.h (right):

https://codereview.chromium.org/816353010/diff/1110001/media/formats/mp4/box_...
media/formats/mp4/box_definitions.h:15: #include
"media/base/video_decoder_config.h"
On 2015/09/08 18:55:44, ddorwin wrote:
> Is this necessary?

Yes, this is necessary, since media::VideoCodec and media::VideoCodecProfile
enums are defined in that header and we now have those in VideoSampleEntry box.

https://codereview.chromium.org/816353010/diff/1110001/media/formats/mp4/box_...
media/formats/mp4/box_definitions.h:194: struct MEDIA_EXPORT
HEVCDecoderConfigurationRecord : Box {
On 2015/09/08 18:55:44, ddorwin wrote:
> Can we put this in an HEVC-specific file? It's not actually used here.
> 
> Can it just be in the .cc file? As we have discussed, not using the #ifdef in
> header files is preferred.

Yeah, I was pondering which path to take - put this in here, to ensure all
supported mp4 boxes are defined in the same .h/.cc files, or put it into
hevc.h/.cc to ensure all HEVC-related code is grouped together. I've put it into
hevc.h/.cc files now, in the latest patchset. I'll rerun the trybots to make
sure this works everywhere.

https://codereview.chromium.org/816353010/diff/1110001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/816353010/diff/1110001/media/media.gyp#newcod...
media/media.gyp:673: 'filters/ffmpeg_h265_to_annex_b_bitstream_converter.cc',
On 2015/09/08 18:55:44, ddorwin wrote:
> Remove here?

Done.

https://codereview.chromium.org/816353010/diff/1110001/media/media.gyp#newcod...
media/media.gyp:1113: 'filters/ffmpeg_h265_to_annex_b_bitstream_converter.cc',
On 2015/09/08 18:55:44, ddorwin wrote:
> Then probably need to use a media_use_ffmpeg==1 check here.

Done.

[wolenetz, 2015-09-08 19:53:11]

re-lgtm % another tiny nit.

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/box_...
File media/formats/mp4/box_definitions.cc (right):

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/box_...
media/formats/mp4/box_definitions.cc:14: #if defined(USE_PROPRIETARY_CODECS) &&
defined(ENABLE_HEVC_DEMUXING)
On 2015/09/03 00:17:51, servolk wrote:
> On 2015/09/02 20:43:18, wolenetz_OoO_Sep3_through_Sep7 wrote:
> > nit: why sometimes condition on both proprietary+hevc_demuxing, and other
> times
> > (like in header for this file), just condition on the latter?
> 
> Oh, these were remains of the way we had to do things when using global
defined
> in common.gypi. Now that ENABLE_HEVC_DEMUXING is defined in media.gyp/BUILD.gn
> we can just ensure right in the build config files that HEVC is enabled only
> when proprietary codecs are supported, so I'll update these #ifdef conditions
> throughout the CL now.

Acknowledged.

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/box_...
media/formats/mp4/box_definitions.cc:496:
RCHECK(reader->Read1(&configurationVersion) && configurationVersion == 1 &&
On 2015/09/03 00:17:51, servolk wrote:
> On 2015/09/02 20:43:18, wolenetz_OoO_Sep3_through_Sep7 wrote:
> > nit: now would be a great time to MEDIA_LOG why RCHECK might return false
for
> > this new code. Without such detail, developers will be left wondering why
> decode
> > error resulted for (in this case, bad content). Please strongly consider
> adding
> > "MEDIA_LOG(ERROR, media_log) << ...;" describing the failure conditions for
> this
> > method here and in other current RCHECKs in this method.
> > 
> > Likewise, some basic sanity in the unit test, using a
StrictMock<MockMediaLog>
> > for both a successful parse and at least one of the error conditions is a
> strong
> > recommendation
> 
> On one hand I understand your concerns, but on the other hand I was trying to
be
> consistent with H.264/AVC implementation here and the code there looks exactly
> the same (see lines 408-411 above), only avcC header is smaller and has fewer
> parameters. We could try to improve logging here to make diagnosing issues
> easier, but note that all we are doing here is reading data from hvcC box
(it's
> a bunch of read* + one configVersion check), so if something fails right here
> there are only two possible causes - either configVersion is unsupported, or
the
> hvcC header that we are trying to read was too short. If there's any other
issue
> with the header, then we'll get failure at one of the RCHECKs below and not at
> this one. So while there's definitely room for improvement, we can make that
> later as a separate CL if it ever becomes a real concern. For now it's
probably
> fine, I've never seen a failure here, except on some very early patchsets of
> this CL.

That's fine, thank you for providing more detail in the review. In general, if
there's something that's likely to fail parse in a non-trivial portion of real
media, I would like to have the reason why the parse failed more easily
discovered by developers without requiring debug builds, dvlogs, etc on a
potentially-hard-to-repro scenario. In this case, let's land this first and
we'll/I'll continue improving media log coverage overall. ISTM like handling a
false retval from this method (and other box parsers) could use a generic "parse
of xxxx box failed" media log message; such would not be in scope of this CL.

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/mp4_...
File media/formats/mp4/mp4_stream_parser.cc (right):

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/mp4_...
media/formats/mp4/mp4_stream_parser.cc:307:
video_config.Initialize(entry.video_codec, entry.video_codec_profile,
On 2015/09/03 00:17:51, servolk wrote:
> On 2015/09/02 20:43:18, wolenetz_OoO_Sep3_through_Sep7 wrote:
> > nit: if we don't already DVLOG(1) entry.video_codec, this CL seems like a
good
> > time to do that.
> 
> We already log codec info into media_log when parsing avcC/hvcC boxes, that
> should probably sufficient (and we have more detailed information there, so if
> we want more details we should expand logging there, instead of adding it
here).

Acknowledged.

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/mp4_...
File media/formats/mp4/mp4_stream_parser_unittest.cc (right):

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/mp4_...
media/formats/mp4/mp4_stream_parser_unittest.cc:298: TEST_F(MP4StreamParserTest,
HEVC_in_MP4_container) {
On 2015/09/03 00:17:51, servolk wrote:
> On 2015/09/02 22:09:24, wolenetz_OoO_Sep3_through_Sep7 wrote:
> > On 2015/09/02 21:56:59, wolenetz_OoO_Sep3_through_Sep7 wrote:
> > > cast_shell_linux try bot doesn't seem to have run this test.
> > > Please fix.
> > 
> > Hmm. More complicated than I initially thought. ENABLE_HEVC_DEMUXING is
> > dependent on both chromecast==1 and proprietary_codecs==1
> > (https://codereview.chromium.org/816353010/diff/1090001/media/media.gyp).
The
> > latter isn't defined for chromium trybots, just for official release builds.
> So
> > this test will not be exercised in Chromium waterfall and may rot. Please
> ensure
> > this passes on Chromecast downstream at least :)
> 
> As we've discussed offline we are in the same situation as MP4 here.
> But we do have internal Chromecast tests with actual video stream playback
> (including HEVC), so we got this covered.

Acknowledged.

https://codereview.chromium.org/816353010/diff/1090001/media/formats/mp4/mp4_...
media/formats/mp4/mp4_stream_parser_unittest.cc:301:
EXPECT_TRUE(AppendDataInPieces(buffer->data(), buffer->data_size(), 512));
On 2015/09/03 00:17:51, servolk wrote:
> On 2015/09/02 20:43:18, wolenetz_OoO_Sep3_through_Sep7 wrote:
> > ditto nit: I would think my recent changes would now make this test fail
> without
> > some EXPECT_MEDIA_LOG(). Also, please add some parse failure (and associated
> > EXPECT_MEDIA_LOGs()) case(s).
> 
> Indeed, the test wasn't running, because ENABLE_HEVC_DEMUXING wasn't defined
for
> the test target. I've updated the .gyp file (BUILD.gn already handles this by
> adding the define into media_config) and seen test failures. Added
> EXPECT_MEDIA_LOG to fix the test.

Acknowledged. Thanks for fixing.

https://codereview.chromium.org/816353010/diff/1130001/media/formats/mp4/hevc.cc
File media/formats/mp4/hevc.cc (right):

https://codereview.chromium.org/816353010/diff/1130001/media/formats/mp4/hevc...
media/formats/mp4/hevc.cc:53: : first_byte(0) {}
nit: indent 4, not 2 (see l.21, above for different indent.)

[ddorwin, 2015-09-08 19:54:36]

LGTM overall with one question. I didn't review the code in detail (except
earlier reviews of the tests).

https://codereview.chromium.org/816353010/diff/1110001/media/formats/mp4/box_...
File media/formats/mp4/box_definitions.h (right):

https://codereview.chromium.org/816353010/diff/1110001/media/formats/mp4/box_...
media/formats/mp4/box_definitions.h:15: #include
"media/base/video_decoder_config.h"
On 2015/09/08 19:22:02, servolk wrote:
> On 2015/09/08 18:55:44, ddorwin wrote:
> > Is this necessary?
> 
> Yes, this is necessary, since media::VideoCodec and media::VideoCodecProfile
> enums are defined in that header and we now have those in VideoSampleEntry
box.

Hmm. We should consider extracting those into a video_codecs.h file, especially
if they are similarly used where we don't need the actual config. (The first
step would be to start a thread.)

https://codereview.chromium.org/816353010/diff/1130001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/816353010/diff/1130001/media/BUILD.gn#newcode707
media/BUILD.gn:707: if (enable_hevc_demuxing) {
Add "proprietary_codecs && " for consistency? I wonder whether we can just
enforce that enable_hevc_demuxing is never set without proprietary_codecs in
some higher level file.

[servolk, 2015-09-08 20:27:25]

https://codereview.chromium.org/816353010/diff/1110001/media/formats/mp4/box_...
File media/formats/mp4/box_definitions.h (right):

https://codereview.chromium.org/816353010/diff/1110001/media/formats/mp4/box_...
media/formats/mp4/box_definitions.h:15: #include
"media/base/video_decoder_config.h"
On 2015/09/08 19:54:36, ddorwin wrote:
> On 2015/09/08 19:22:02, servolk wrote:
> > On 2015/09/08 18:55:44, ddorwin wrote:
> > > Is this necessary?
> > 
> > Yes, this is necessary, since media::VideoCodec and media::VideoCodecProfile
> > enums are defined in that header and we now have those in VideoSampleEntry
> box.
> 
> Hmm. We should consider extracting those into a video_codecs.h file,
especially
> if they are similarly used where we don't need the actual config. (The first
> step would be to start a thread.)

Ok, I'll do that in a follow up CL.

https://codereview.chromium.org/816353010/diff/1130001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/816353010/diff/1130001/media/BUILD.gn#newcode707
media/BUILD.gn:707: if (enable_hevc_demuxing) {
On 2015/09/08 19:54:36, ddorwin wrote:
> Add "proprietary_codecs && " for consistency? I wonder whether we can just
> enforce that enable_hevc_demuxing is never set without proprietary_codecs in
> some higher level file.

This code is inside 'if (proprietary_codecs)' body. Would you prefer to keep it
here or move it out of the outer if body and make the condition more explicit
('proprietary_codecs && enable_hevc_demuxing')?

[servolk, 2015-09-08 20:48:44]

https://codereview.chromium.org/816353010/diff/1130001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/816353010/diff/1130001/media/BUILD.gn#newcode707
media/BUILD.gn:707: if (enable_hevc_demuxing) {
On 2015/09/08 20:27:25, servolk wrote:
> On 2015/09/08 19:54:36, ddorwin wrote:
> > Add "proprietary_codecs && " for consistency? I wonder whether we can just
> > enforce that enable_hevc_demuxing is never set without proprietary_codecs in
> > some higher level file.
> 
> This code is inside 'if (proprietary_codecs)' body. Would you prefer to keep
it
> here or move it out of the outer if body and make the condition more explicit
> ('proprietary_codecs && enable_hevc_demuxing')?

FYI: I've moved this code out of the outer 'if' and updated the condition to
explicitly mention enable_hevc_demuxing

https://codereview.chromium.org/816353010/diff/1130001/media/formats/mp4/hevc.cc
File media/formats/mp4/hevc.cc (right):

https://codereview.chromium.org/816353010/diff/1130001/media/formats/mp4/hevc...
media/formats/mp4/hevc.cc:53: : first_byte(0) {}
On 2015/09/08 19:53:10, wolenetz wrote:
> nit: indent 4, not 2 (see l.21, above for different indent.)

Done.

[servolk, 2015-09-08 20:49:04]

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

[commit-bot: I haz the power, 2015-09-08 20:49:29]

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

[ddorwin, 2015-09-08 20:53:28]

https://codereview.chromium.org/816353010/diff/1130001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/816353010/diff/1130001/media/BUILD.gn#newcode707
media/BUILD.gn:707: if (enable_hevc_demuxing) {
On 2015/09/08 20:27:25, servolk wrote:
> On 2015/09/08 19:54:36, ddorwin wrote:
> > Add "proprietary_codecs && " for consistency? I wonder whether we can just
> > enforce that enable_hevc_demuxing is never set without proprietary_codecs in
> > some higher level file.
> 
> This code is inside 'if (proprietary_codecs)' body. Would you prefer to keep
it
> here or move it out of the outer if body and make the condition more explicit
> ('proprietary_codecs && enable_hevc_demuxing')?

Oh, I didn't see the nesting correctly. That was fine.

https://codereview.chromium.org/816353010/diff/1130001/media/BUILD.gn#newcode707
media/BUILD.gn:707: if (enable_hevc_demuxing) {
On 2015/09/08 20:48:44, servolk wrote:
> On 2015/09/08 20:27:25, servolk wrote:
> > On 2015/09/08 19:54:36, ddorwin wrote:
> > > Add "proprietary_codecs && " for consistency? I wonder whether we can just
> > > enforce that enable_hevc_demuxing is never set without proprietary_codecs
in
> > > some higher level file.
> > 
> > This code is inside 'if (proprietary_codecs)' body. Would you prefer to keep
> it
> > here or move it out of the outer if body and make the condition more
explicit
> > ('proprietary_codecs && enable_hevc_demuxing')?
> 
> FYI: I've moved this code out of the outer 'if' and updated the condition to
> explicitly mention enable_hevc_demuxing

It was probably fine and perhaps better.

[servolk, 2015-09-08 21:17:56]

https://codereview.chromium.org/816353010/diff/1130001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/816353010/diff/1130001/media/BUILD.gn#newcode707
media/BUILD.gn:707: if (enable_hevc_demuxing) {
On 2015/09/08 20:53:28, ddorwin wrote:
> On 2015/09/08 20:48:44, servolk wrote:
> > On 2015/09/08 20:27:25, servolk wrote:
> > > On 2015/09/08 19:54:36, ddorwin wrote:
> > > > Add "proprietary_codecs && " for consistency? I wonder whether we can
just
> > > > enforce that enable_hevc_demuxing is never set without
proprietary_codecs
> in
> > > > some higher level file.
> > > 
> > > This code is inside 'if (proprietary_codecs)' body. Would you prefer to
keep
> > it
> > > here or move it out of the outer if body and make the condition more
> explicit
> > > ('proprietary_codecs && enable_hevc_demuxing')?
> > 
> > FYI: I've moved this code out of the outer 'if' and updated the condition to
> > explicitly mention enable_hevc_demuxing
> 
> It was probably fine and perhaps better.

Ok, moved it back in PS #60

[servolk, 2015-09-08 21:18:55]

The CQ bit was checked by servolk@chromium.org

[servolk, 2015-09-08 21:18:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org, wolenetz@chromium.org, ddorwin@chromium.org
Link to the patchset: https://codereview.chromium.org/816353010/#ps1170001
(title: "Moved back h265_parser_unittest.cc in media/BUILD.gn")

[commit-bot: I haz the power, 2015-09-08 21:19:19]

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

[commit-bot: I haz the power, 2015-09-08 21:30:59]

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

[commit-bot: I haz the power, 2015-09-08 21:31:00]

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

[servolk, 2015-09-08 21:37:40]

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

[servolk, 2015-09-08 21:38:24]

On 2015/09/08 21:31:00, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Need approval from content/test/BUILD.gn owner.
+sky@ could you please take a look?

[sky, 2015-09-08 22:24:00]

LGTM

[servolk, 2015-09-08 22:25:35]

The CQ bit was checked by servolk@chromium.org

[commit-bot: I haz the power, 2015-09-08 22:25:57]

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

[commit-bot: I haz the power, 2015-09-08 22:33:56]

Committed patchset #60 (id:1170001)

[commit-bot: I haz the power, 2015-09-08 22:34:41]

Patchset 60 (id:??) landed as
https://crrev.com/4585056bcf1491d19262aaef0577542840f54f19
Cr-Commit-Position: refs/heads/master@{#347814}