Introduce Dolby Vision video codec and Demuxer support

Introduce Dolby Vision video codec and Demuxer support

- Introduce DolbyVision codec and active profile 0, 4, 5 and 7.
- new flag "enable_dolby_vision_demuxing" to enable Dolby Vision
  demuxing feature using MSE using mime type video/mp4.
  Currently the CL only supports profile 5 and is only enabled on
  Chromecast build.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

BUG=481726
Review-Url: https://codereview.chromium.org/2640113004
Cr-Commit-Position: refs/heads/master@{#453762}
Committed: https://chromium.googlesource.com/chromium/src/+/2cca0f26776baa99251145cc158b259c1c6fd4c7

[erickung1, 2017-01-19 22:20:39]

Description was changed from

==========
Enable Dolby Vision profile 5 Demuxer support

- Introduce DolbyVision video codec type
- Enable Dolby Vision profile 5 demuxing using MSE
==========

to

==========
Enable Dolby Vision profile 5 Demuxer support

- Introduce DolbyVision video codec type
- Enable Dolby Vision profile 5 demuxing using MSE
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[erickung1, 2017-01-19 22:21:31]

Description was changed from

==========
Enable Dolby Vision profile 5 Demuxer support

- Introduce DolbyVision video codec type
- Enable Dolby Vision profile 5 demuxing using MSE
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Enable Dolby Vision profile 5 Demuxer support

- Introduce DolbyVision video codec type
- Enable Dolby Vision profile 5 demuxing using MSE
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[erickung1, 2017-01-19 22:24:52]

erickung@chromium.org changed reviewers:
+ halliwell@chromium.org, wolenetz@chromium.org

[erickung1, 2017-01-19 22:25:31]

wolenetz@, please review the media change

[erickung1, 2017-01-19 22:26:08]

halliwell@ please review the chromecast change.

feedback from other people are welcome

[erickung1, 2017-01-19 23:30:59]

Description was changed from

==========
Enable Dolby Vision profile 5 Demuxer support

- Introduce DolbyVision video codec type
- Enable Dolby Vision profile 5 demuxing using MSE
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Enable Dolby Vision profile 5 Demuxer support

- Introduce DolbyVision video codec type
- Enable Dolby Vision profile 5 demuxing using MSE
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

BUG=481726
==========

[halliwell, 2017-01-20 02:05:04]

chromecast/ lgtm

https://codereview.chromium.org/2640113004/diff/1/chromecast/media/base/media...
File chromecast/media/base/media_codec_support.cc (right):

https://codereview.chromium.org/2640113004/diff/1/chromecast/media/base/media...
chromecast/media/base/media_codec_support.cc:77: codec_profile ==
::media::DOLBYVISION_PROFILE7)
nit, prefer braces { } for multi-line conditions

[wolenetz, 2017-01-24 21:52:24]

initial comments. //media not yet reviewed.

https://codereview.chromium.org/2640113004/diff/1/chromecast/media/base/media...
File chromecast/media/base/media_codec_support.cc (right):

https://codereview.chromium.org/2640113004/diff/1/chromecast/media/base/media...
chromecast/media/base/media_codec_support.cc:38: // Shared library should return
kDefault if the platform depends on sink
Is kDefault a bool? Or am I confused by this comment by assuming
MediaCapabilities::HdmiSinkSupportsDolbyVision() is the method in the shared lib
that the comment is referencing? Either way, fix or clarify better please.

https://codereview.chromium.org/2640113004/diff/1/chromecast/media/base/media...
chromecast/media/base/media_codec_support.cc:59: // Converts ::media::VideoCodec
to chromecast::media::VideoCodec. Any unknown or
comments like this belong in .h instead of .cc
Also, please commnet that |codec_profile| is ignored for codecs other than DV.
Or perform the appropriate checking of |codec_profile| for non-DV codecs.

https://codereview.chromium.org/2640113004/diff/1/chromecast/media/base/media...
chromecast/media/base/media_codec_support.cc:73: if (codec_profile ==
::media::DOLBYVISION_PROFILE0)
I'm unfamiliar with DV profiles, but compared to previous CL
(https://codereview.chromium.org/2541743002/diff/100001/chromecast/media/base/...),
this code doesn't recognize PROFILE1 or PROFILE2 as DVH264. Is that intentional?

https://codereview.chromium.org/2640113004/diff/1/chromecast/media/base/media...
chromecast/media/base/media_codec_support.cc:75: else if (codec_profile ==
::media::DOLBYVISION_PROFILE4 ||
Ditto: is omission of check for DV profiles 3 and 6 here intentional versus the
previous CL
(https://codereview.chromium.org/2541743002/diff/100001/chromecast/media/base/...)
?

https://codereview.chromium.org/2640113004/diff/1/chromecast/media/base/media...
chromecast/media/base/media_codec_support.cc:77: codec_profile ==
::media::DOLBYVISION_PROFILE7)
On 2017/01/20 02:05:04, halliwell wrote:
> nit, prefer braces { } for multi-line conditions

Chromium definitely needs { } here.

https://codereview.chromium.org/2640113004/diff/1/chromecast/media/base/media...
chromecast/media/base/media_codec_support.cc:87: // Converts
::media::VideoCodecProfile to chromecast::media::VideoProfile.
nit ditto: move comment to .h

https://codereview.chromium.org/2640113004/diff/1/chromecast/media/base/media...
chromecast/media/base/media_codec_support.cc:129: case
::media::DOLBYVISION_PROFILE0:
(Ditto of above query about intentionality of omission of other profiles that
were in the previous CL, here and elsewhere.)

https://codereview.chromium.org/2640113004/diff/1/gpu/ipc/common/gpu_info_str...
File gpu/ipc/common/gpu_info_struct_traits.cc (right):

https://codereview.chromium.org/2640113004/diff/1/gpu/ipc/common/gpu_info_str...
gpu/ipc/common/gpu_info_struct_traits.cc:112: case
gpu::VideoCodecProfile::DOLBYVISION_PROFILE7:
To be clear, why NOTREACHED() here versus HEVC? Is it because android MediaCodec
might have HEVC but not DV?

[wolenetz, 2017-01-25 23:42:42]

Looking pretty good overall. Lots of relatively small comments. Also, please add
tests to media_canplaytype_browsertest

https://codereview.chromium.org/2640113004/diff/1/content/renderer/media/rend...
File content/renderer/media/render_media_client.cc (right):

https://codereview.chromium.org/2640113004/diff/1/content/renderer/media/rend...
content/renderer/media/render_media_client.cc:111: case
media::kCodecDolbyVision:
To be clear, is there no MediaClient on chromecast (or is there some other
MediaClient on chromecast that's not this one?)

If this is the MediaClient used on chromecast, then I'm confused how
MimeUtil::AreSupportedCodecs ever indicates support for DV.

https://codereview.chromium.org/2640113004/diff/1/media/base/mime_util_intern...
File media/base/mime_util_internal.cc (right):

https://codereview.chromium.org/2640113004/diff/1/media/base/mime_util_intern...
media/base/mime_util_internal.cc:589: }
code formatting seems inconsistent here.
The VP9 case is using an outer { }, which none of the other cases do, and the
indentation is also messed up (see l.591 in this patch set for example of
strange resulting indentation.

https://codereview.chromium.org/2640113004/diff/1/media/base/mime_util_intern...
media/base/mime_util_internal.cc:591: return false;
l.588 returns already. this line should never be reached.

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

https://codereview.chromium.org/2640113004/diff/1/media/base/video_codecs.cc#...
media/base/video_codecs.cc:35: case kCodecDolbyVision:
nit: these cases are already not in sequence with the enum, rather seem to be
ordered by container. tl;dr: move DV case (not enum) to just after HEVC?

https://codereview.chromium.org/2640113004/diff/1/media/base/video_codecs.cc#...
media/base/video_codecs.cc:423: // Only profile 0, 4 ,5 and 7 are valid.
nit: s/profile/profiles/ and s/4 ,/4, /

https://codereview.chromium.org/2640113004/diff/1/media/base/video_codecs.cc#...
media/base/video_codecs.cc:438: DVLOG(4) << __func__ << ": depecrated
profile_id=" << profile_id;
nit: s/deprecated/deprecated and not supported/ ?

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

https://codereview.chromium.org/2640113004/diff/1/media/base/video_codecs.h#n...
media/base/video_codecs.h:78: DOLBYVISION_PROFILE5 = 21,
To be clear, this enumeration's lack of inclusion of DV profiles 1,2,3,6 and
usage of MIN/MAX to bound the range of the DV portion of the enum makes it
difficult to see how profiles 1,2,3,6 might ever be added. Is their omission now
(and in the foreseeable future) intentional?

https://codereview.chromium.org/2640113004/diff/1/media/base/video_codecs_uni...
File media/base/video_codecs_unittest.cc (right):

https://codereview.chromium.org/2640113004/diff/1/media/base/video_codecs_uni...
media/base/video_codecs_unittest.cc:165:
EXPECT_TRUE(ParseDolbyVisionCodecId("dvh1.05.07", &profile, &level_id));
nit: supported format includes those without a leading 0 for profile or
level_id. spot-test those?

https://codereview.chromium.org/2640113004/diff/1/media/filters/stream_parser...
File media/filters/stream_parser_factory.cc (right):

https://codereview.chromium.org/2640113004/diff/1/media/filters/stream_parser...
media/filters/stream_parser_factory.cc:175: "dvav.*", CodecInfo::VIDEO, NULL,
CodecInfo::HISTOGRAM_DOLBYVISION};
Hmm. Does Dolby Vision support require HEVC support? This doesn't appear to be
enforced, for instance, in BUILD.gn.

https://codereview.chromium.org/2640113004/diff/1/media/filters/stream_parser...
media/filters/stream_parser_factory.cc:177: "dvhe.*", CodecInfo::VIDEO, NULL,
CodecInfo::HISTOGRAM_DOLBYVISION};
Hmm. Does part of Dolby Vision support also require HEVC support? This doesn't
appear to be enforced, for instance, in BUILD.gn. Or can the two be varied
independently of each other (e.g. dvhe support, but no plain HEVC support)?

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/avc.cc
File media/formats/mp4/avc.cc (right):

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/avc.cc#ne...
media/formats/mp4/avc.cc:112: if (annexb_validation)
It looks like this production parameter is used only in debug builds. Smells
funny.

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/avc.cc#ne...
media/formats/mp4/avc.cc:148: if (annexb_validation)
ditto.

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/avc.cc#ne...
media/formats/mp4/avc.cc:324: post_annexb_validation_ = true;
generally, we set these in the initializers (or in fully new classes, in the
class declaration in the .h). tl;dr: move to initializer here?

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/avc.h
File media/formats/mp4/avc.h (right):

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/avc.h#new...
media/formats/mp4/avc.h:34: // encrypted data.
Please add comment for what |annexb_validation| means.

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/avc.h#new...
media/formats/mp4/avc.h:74: void DisablePostAnnexbValidation() {
post_annexb_validation_ = false; }
Please add comment describing the effect of this new method.

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/avc.h#new...
media/formats/mp4/avc.h:84: bool post_annexb_validation_;
Please add comment describing this new field.

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/box_defin...
File media/formats/mp4/box_definitions.cc (right):

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/box_defin...
media/formats/mp4/box_definitions.cc:28: #include
"media/formats/mp4/dolby_vision.h"
nit: ordering of include: dolby_vision < hevc alphabetically.

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/box_defin...
media/formats/mp4/box_definitions.cc:708: if (reader->HasChild(&pixel_aspect)) {
Why "maybe", if it has the child? Why didn't previous code work?

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/box_defin...
media/formats/mp4/box_definitions.cc:738: if (reader->HasChild(&dvccConfig) &&
reader->ReadChild(&dvccConfig)) {
Why not log reading dvcc here?

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/box_defin...
media/formats/mp4/box_definitions.cc:740: ->DisablePostAnnexbValidation();
Comment why, please, and note this is only changing debug-mode behavior (or
dcheck-always build's behavior).

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/box_defin...
media/formats/mp4/box_definitions.cc:761: if (reader->HasChild(&dvccConfig) &&
reader->ReadChild(&dvccConfig)) {
Also, why not log reading dvcc here?

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/box_defin...
media/formats/mp4/box_definitions.cc:761: if (reader->HasChild(&dvccConfig) &&
reader->ReadChild(&dvccConfig)) {
why not DisablePostAnnexbValidation on it here?

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/box_defin...
media/formats/mp4/box_definitions.cc:770: case FOURCC_DVAV: {
Is there no DVA1 or DVH1 needed?

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/box_defin...
media/formats/mp4/box_definitions.cc:775: frame_bitstream_converter =
Why not DisablePostAnnexbValidation on it here?

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/box_defin...
media/formats/mp4/box_definitions.cc:790: make_scoped_refptr(new
HEVCBitstreamConverter(std::move(hevcConfig)));
Why not DisablePostAnnexbValidation on it here?

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/box_defin...
media/formats/mp4/box_definitions.cc:836: case FOURCC_DVAV:
ditto: DVA1? DVH1?

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/dolby_vis...
File media/formats/mp4/dolby_vision.cc (right):

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/dolby_vis...
media/formats/mp4/dolby_vision.cc:59: << " profile type:" << codec_profile;
|codec_profile| isn't set yet.

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/dolby_vis...
File media/formats/mp4/dolby_vision_unittest.cc (right):

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/dolby_vis...
media/formats/mp4/dolby_vision_unittest.cc:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
s/2016/2017/ here and elsewhere in totally new source files.

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/dolby_vis...
media/formats/mp4/dolby_vision_unittest.cc:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
These test cases test for unsupported (and not even in the enumeration) profiles
only. How does this pass?? Also, no level_idc/etc tests seems strange here.

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/fourccs.h
File media/formats/mp4/fourccs.h (right):

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/fourccs.h...
media/formats/mp4/fourccs.h:46: #if BUILDFLAG(ENABLE_DOLBY_VISION_DEMUXING)
nit: these all begin with "D", so move them between CTTS and DINF, above?

[wolenetz, 2017-01-25 23:44:26]

You'll also need review from //tools and //gpu owners

[wolenetz, 2017-01-25 23:46:00]

Also, the CL description seems over-specific to profile 5, when there are other
profiles (0,1,4) also included in this CL.

An enumeration of the newly supported mime types (conditioned on the build flag)
in the CL description would help, too.

[erickung1, 2017-01-27 19:13:00]

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

[erickung1, 2017-02-03 18:18:32]

Please take a look Patch set 2.

This patch set addresses most comments except for following 2
- ignore annexB conversion check: the purpose is to ignore the check when it's
DV profile 5 or the video track contains only enhanced layer. We still need to
insert the parameter set to follow AnnexB spec, but some check inside avc.cc
actually checks if certain AU are expected and in order. hevc.cc doesn't do such
check. I may need to think about how to do it in a nicely way.

- modify MimeUtil::IsCodecSupportedOnPlatform() to always return false since
this is called only on Android platform, which doesn't really support it right
now(through chromium)

https://codereview.chromium.org/2640113004/diff/1/chromecast/media/base/media...
File chromecast/media/base/media_codec_support.cc (right):

https://codereview.chromium.org/2640113004/diff/1/chromecast/media/base/media...
chromecast/media/base/media_codec_support.cc:38: // Shared library should return
kDefault if the platform depends on sink
On 2017/01/24 21:52:23, wolenetz wrote:
> Is kDefault a bool? Or am I confused by this comment by assuming
> MediaCapabilities::HdmiSinkSupportsDolbyVision() is the method in the shared
lib
> that the comment is referencing? Either way, fix or clarify better please.

Done.

https://codereview.chromium.org/2640113004/diff/1/chromecast/media/base/media...
chromecast/media/base/media_codec_support.cc:59: // Converts ::media::VideoCodec
to chromecast::media::VideoCodec. Any unknown or
On 2017/01/24 21:52:24, wolenetz wrote:
> comments like this belong in .h instead of .cc
> Also, please commnet that |codec_profile| is ignored for codecs other than DV.
> Or perform the appropriate checking of |codec_profile| for non-DV codecs.

Done.

https://codereview.chromium.org/2640113004/diff/1/chromecast/media/base/media...
chromecast/media/base/media_codec_support.cc:73: if (codec_profile ==
::media::DOLBYVISION_PROFILE0)
On 2017/01/24 21:52:24, wolenetz wrote:
> I'm unfamiliar with DV profiles, but compared to previous CL
>
(https://codereview.chromium.org/2541743002/diff/100001/chromecast/media/base/...),
> this code doesn't recognize PROFILE1 or PROFILE2 as DVH264. Is that
intentional?

Yes, for the latest DV spec from Dolby, profile 1 2 3 and 6 will be deprecated.

profile 0 and 1 are encoded using H264

https://codereview.chromium.org/2640113004/diff/1/chromecast/media/base/media...
chromecast/media/base/media_codec_support.cc:75: else if (codec_profile ==
::media::DOLBYVISION_PROFILE4 ||
On 2017/01/24 21:52:24, wolenetz wrote:
> Ditto: is omission of check for DV profiles 3 and 6 here intentional versus
the
> previous CL
>
(https://codereview.chromium.org/2541743002/diff/100001/chromecast/media/base/...)
> ?

ditto, the profile 3 and 6 will deprecated.

https://codereview.chromium.org/2640113004/diff/1/chromecast/media/base/media...
chromecast/media/base/media_codec_support.cc:77: codec_profile ==
::media::DOLBYVISION_PROFILE7)
On 2017/01/20 02:05:04, halliwell wrote:
> nit, prefer braces { } for multi-line conditions

Done.

https://codereview.chromium.org/2640113004/diff/1/chromecast/media/base/media...
chromecast/media/base/media_codec_support.cc:87: // Converts
::media::VideoCodecProfile to chromecast::media::VideoProfile.
On 2017/01/24 21:52:24, wolenetz wrote:
> nit ditto: move comment to .h

Done.

https://codereview.chromium.org/2640113004/diff/1/chromecast/media/base/media...
chromecast/media/base/media_codec_support.cc:129: case
::media::DOLBYVISION_PROFILE0:
On 2017/01/24 21:52:24, wolenetz wrote:
> (Ditto of above query about intentionality of omission of other profiles that
> were in the previous CL, here and elsewhere.)

as mentioned above, profile 1 2 3 and 6 are deprecated by Dolby. we don't want
to define deprecated profile enum value in the chromium code

https://codereview.chromium.org/2640113004/diff/1/content/renderer/media/rend...
File content/renderer/media/render_media_client.cc (right):

https://codereview.chromium.org/2640113004/diff/1/content/renderer/media/rend...
content/renderer/media/render_media_client.cc:111: case
media::kCodecDolbyVision:
On 2017/01/25 23:42:40, wolenetz wrote:
> To be clear, is there no MediaClient on chromecast (or is there some other
> MediaClient on chromecast that's not this one?)
> 
> If this is the MediaClient used on chromecast, then I'm confused how
> MimeUtil::AreSupportedCodecs ever indicates support for DV.

cast has its own media::MediaClient implementation
https://cs.chromium.org/chromium/src/chromecast/common/media/cast_media_clien...

https://codereview.chromium.org/2640113004/diff/1/gpu/ipc/common/gpu_info_str...
File gpu/ipc/common/gpu_info_struct_traits.cc (right):

https://codereview.chromium.org/2640113004/diff/1/gpu/ipc/common/gpu_info_str...
gpu/ipc/common/gpu_info_struct_traits.cc:112: case
gpu::VideoCodecProfile::DOLBYVISION_PROFILE7:
On 2017/01/24 21:52:24, wolenetz wrote:
> To be clear, why NOTREACHED() here versus HEVC? Is it because android
MediaCodec
> might have HEVC but not DV?

Done.

https://codereview.chromium.org/2640113004/diff/1/media/base/mime_util_intern...
File media/base/mime_util_internal.cc (right):

https://codereview.chromium.org/2640113004/diff/1/media/base/mime_util_intern...
media/base/mime_util_internal.cc:589: }
On 2017/01/25 23:42:41, wolenetz wrote:
> code formatting seems inconsistent here.
> The VP9 case is using an outer { }, which none of the other cases do, and the
> indentation is also messed up (see l.591 in this patch set for example of
> strange resulting indentation.

Done.

https://codereview.chromium.org/2640113004/diff/1/media/base/mime_util_intern...
media/base/mime_util_internal.cc:591: return false;
On 2017/01/25 23:42:40, wolenetz wrote:
> l.588 returns already. this line should never be reached.

Done.

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

https://codereview.chromium.org/2640113004/diff/1/media/base/video_codecs.cc#...
media/base/video_codecs.cc:35: case kCodecDolbyVision:
On 2017/01/25 23:42:41, wolenetz wrote:
> nit: these cases are already not in sequence with the enum, rather seem to be
> ordered by container. tl;dr: move DV case (not enum) to just after HEVC?

Done.

https://codereview.chromium.org/2640113004/diff/1/media/base/video_codecs.cc#...
media/base/video_codecs.cc:423: // Only profile 0, 4 ,5 and 7 are valid.
On 2017/01/25 23:42:41, wolenetz wrote:
> nit: s/profile/profiles/ and s/4 ,/4, /

Done.

https://codereview.chromium.org/2640113004/diff/1/media/base/video_codecs.cc#...
media/base/video_codecs.cc:438: DVLOG(4) << __func__ << ": depecrated
profile_id=" << profile_id;
On 2017/01/25 23:42:41, wolenetz wrote:
> nit: s/deprecated/deprecated and not supported/ ?

Done.

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

https://codereview.chromium.org/2640113004/diff/1/media/base/video_codecs.h#n...
media/base/video_codecs.h:78: DOLBYVISION_PROFILE5 = 21,
On 2017/01/25 23:42:41, wolenetz wrote:
> To be clear, this enumeration's lack of inclusion of DV profiles 1,2,3,6 and
> usage of MIN/MAX to bound the range of the DV portion of the enum makes it
> difficult to see how profiles 1,2,3,6 might ever be added. Is their omission
now
> (and in the foreseeable future) intentional?
this is intentional since profile 1 2 3 and 6 are deprecated by Dolby

https://codereview.chromium.org/2640113004/diff/1/media/base/video_codecs_uni...
File media/base/video_codecs_unittest.cc (right):

https://codereview.chromium.org/2640113004/diff/1/media/base/video_codecs_uni...
media/base/video_codecs_unittest.cc:165:
EXPECT_TRUE(ParseDolbyVisionCodecId("dvh1.05.07", &profile, &level_id));
On 2017/01/25 23:42:41, wolenetz wrote:
> nit: supported format includes those without a leading 0 for profile or
> level_id. spot-test those?

according to Dolby Vision spec, the profile and level are reiquired and should
be two digits each. we have the negative test about such from #169~#182

Following is the part of the spec


--------------------------------------------------
[Dolby_Vision_fourCC].[Dovi_Profile_ID].[Dovi_Level_ID]

In this string:

• The [Dolby_Vision_fourCC] is a four-character value as listed in the Sample
description entry code table that indicates the codec type. This value can be
obtained from Dolby Vision sample entries, as described in the Dolby Vision
streams within the ISO basemedia file format specification.

• The [Dovi_Profile_ID] is a two-digit value representing the Dolby Vision
profile ID as listed in the Dolby Vision profiles section. This value can be
obtained from Dolby Vision configuration box, as described in the Dolby Vision
streams within the ISO basemedia file format specification.

• The [Dovi_Level_ID] is a two-digit value representing the Dolby Vision level
ID as listed in the Dolby Vision levels section. This value can be obtained from
Dolby Vision configuration box, as described in the Dolby Vision streams within
the ISO basemedia file format specification.
----------------------------------------------

https://codereview.chromium.org/2640113004/diff/1/media/filters/stream_parser...
File media/filters/stream_parser_factory.cc (right):

https://codereview.chromium.org/2640113004/diff/1/media/filters/stream_parser...
media/filters/stream_parser_factory.cc:175: "dvav.*", CodecInfo::VIDEO, NULL,
CodecInfo::HISTOGRAM_DOLBYVISION};
On 2017/01/25 23:42:41, wolenetz wrote:
> Hmm. Does Dolby Vision support require HEVC support? This doesn't appear to be
> enforced, for instance, in BUILD.gn.

Done.

https://codereview.chromium.org/2640113004/diff/1/media/filters/stream_parser...
media/filters/stream_parser_factory.cc:177: "dvhe.*", CodecInfo::VIDEO, NULL,
CodecInfo::HISTOGRAM_DOLBYVISION};
On 2017/01/25 23:42:41, wolenetz wrote:
> Hmm. Does part of Dolby Vision support also require HEVC support? This doesn't
> appear to be enforced, for instance, in BUILD.gn. Or can the two be varied
> independently of each other (e.g. dvhe support, but no plain HEVC support)?

Dolby Vision profile 0 requires AVC support
Dolby Vision profile 4 5 7 require HEVC support.

per discussion offline, I'll use #if BUILDFLAG(ENABLE_HEVC_DEMUXING) to protect
dolby vision profiles which are encoded using HEVC

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/avc.cc
File media/formats/mp4/avc.cc (right):

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/avc.cc#ne...
media/formats/mp4/avc.cc:324: post_annexb_validation_ = true;
On 2017/01/25 23:42:41, wolenetz wrote:
> generally, we set these in the initializers (or in fully new classes, in the
> class declaration in the .h). tl;dr: move to initializer here?

Done.

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/avc.h
File media/formats/mp4/avc.h (right):

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/avc.h#new...
media/formats/mp4/avc.h:34: // encrypted data.
On 2017/01/25 23:42:41, wolenetz wrote:
> Please add comment for what |annexb_validation| means.

Done.

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/avc.h#new...
media/formats/mp4/avc.h:74: void DisablePostAnnexbValidation() {
post_annexb_validation_ = false; }
On 2017/01/25 23:42:41, wolenetz wrote:
> Please add comment describing the effect of this new method.

Done.

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/avc.h#new...
media/formats/mp4/avc.h:84: bool post_annexb_validation_;
On 2017/01/25 23:42:41, wolenetz wrote:
> Please add comment describing this new field.

Done.

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/box_defin...
File media/formats/mp4/box_definitions.cc (right):

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/box_defin...
media/formats/mp4/box_definitions.cc:28: #include
"media/formats/mp4/dolby_vision.h"
On 2017/01/25 23:42:42, wolenetz wrote:
> nit: ordering of include: dolby_vision < hevc alphabetically.

Done.

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/box_defin...
media/formats/mp4/box_definitions.cc:708: if (reader->HasChild(&pixel_aspect)) {
On 2017/01/25 23:42:41, wolenetz wrote:
> Why "maybe", if it has the child? Why didn't previous code work?

Done. Remove the code since it was just experiment.

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/box_defin...
media/formats/mp4/box_definitions.cc:738: if (reader->HasChild(&dvccConfig) &&
reader->ReadChild(&dvccConfig)) {
On 2017/01/25 23:42:41, wolenetz wrote:
> Why not log reading dvcc here?

Done.

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/box_defin...
media/formats/mp4/box_definitions.cc:761: if (reader->HasChild(&dvccConfig) &&
reader->ReadChild(&dvccConfig)) {
On 2017/01/25 23:42:42, wolenetz wrote:
> Also, why not log reading dvcc here?

Done.

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/box_defin...
media/formats/mp4/box_definitions.cc:770: case FOURCC_DVAV: {
On 2017/01/25 23:42:42, wolenetz wrote:
> Is there no DVA1 or DVH1 needed?

Done.

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/box_defin...
media/formats/mp4/box_definitions.cc:836: case FOURCC_DVAV:
On 2017/01/25 23:42:41, wolenetz wrote:
> ditto: DVA1? DVH1?

Done.

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/dolby_vis...
File media/formats/mp4/dolby_vision.cc (right):

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/dolby_vis...
media/formats/mp4/dolby_vision.cc:59: << " profile type:" << codec_profile;
On 2017/01/25 23:42:42, wolenetz wrote:
> |codec_profile| isn't set yet.

Done.

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/dolby_vis...
File media/formats/mp4/dolby_vision_unittest.cc (right):

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/dolby_vis...
media/formats/mp4/dolby_vision_unittest.cc:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
On 2017/01/25 23:42:42, wolenetz wrote:
> s/2016/2017/ here and elsewhere in totally new source files.

Done.

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/fourccs.h
File media/formats/mp4/fourccs.h (right):

https://codereview.chromium.org/2640113004/diff/1/media/formats/mp4/fourccs.h...
media/formats/mp4/fourccs.h:46: #if BUILDFLAG(ENABLE_DOLBY_VISION_DEMUXING)
On 2017/01/25 23:42:42, wolenetz wrote:
> nit: these all begin with "D", so move them between CTTS and DINF, above?

Done.

[wolenetz, 2017-02-08 00:45:12]

Thanks for updating - I'll look deeply at this in the morning. Just a quick
thing I noticed is the CL description is not yet updated. My previous comment
about that:

"Also, the CL description seems over-specific to profile 5, when there are other
profiles (0,1,4) also included in this CL.

An enumeration of the newly supported mime types (conditioned on the build flag)
in the CL description would help, too."

[wolenetz, 2017-02-09 00:32:57]

On 2017/02/08 00:45:12, wolenetz wrote:
> Thanks for updating - I'll look deeply at this in the morning. Just a quick
> thing I noticed is the CL description is not yet updated. My previous comment
> about that:
> 
> "Also, the CL description seems over-specific to profile 5, when there are
other
> profiles (0,1,4) also included in this CL.
> 
> An enumeration of the newly supported mime types (conditioned on the build
flag)
> in the CL description would help, too."

Comments coming tomorrow.

[erickung1, 2017-02-10 19:41:44]

Description was changed from

==========
Enable Dolby Vision profile 5 Demuxer support

- Introduce DolbyVision video codec type
- Enable Dolby Vision profile 5 demuxing using MSE
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

BUG=481726
==========

to

==========
Introduce Dolby Vision video codec and Demuxer support

- Introduce DolbyVision codec and active profile 0, 4, 5 and 7.
- new flag "enable_dolby_vision_demuxing" to enable Dolby Vision
  demuxing feature using MSE. Currently the CL only supports
  profile 5 and is only enabled on Chromecast build.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

BUG=481726
==========

[erickung1, 2017-02-10 19:43:09]

Description was changed from

==========
Introduce Dolby Vision video codec and Demuxer support

- Introduce DolbyVision codec and active profile 0, 4, 5 and 7.
- new flag "enable_dolby_vision_demuxing" to enable Dolby Vision
  demuxing feature using MSE. Currently the CL only supports
  profile 5 and is only enabled on Chromecast build.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

BUG=481726
==========

to

==========
Introduce Dolby Vision video codec and Demuxer support

- Introduce DolbyVision codec and active profile 0, 4, 5 and 7.
- new flag "enable_dolby_vision_demuxing" to enable Dolby Vision
  demuxing feature using MSE using mime type video/mp4.
  Currently the CL only supports profile 5 and is only enabled on
  Chromecast build.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

BUG=481726
==========

[erickung1, 2017-02-10 19:43:24]

update CL description

[erickung1, 2017-02-13 18:26:07]

erickung@chromium.org changed reviewers:
+ jbauman@chromium.org

[erickung1, 2017-02-13 18:29:07]

erickung@chromium.org changed reviewers:
+ dpranke@chromium.org

[erickung1, 2017-02-13 18:30:00]

Add dpranke@ to review tools/
Add jbauman@ to review gpu/

Thanks.

[jbauman, 2017-02-13 23:49:44]

gpu/ lgtm assuming media people are happy with the VideoCodecProfile changes.

[Dirk Pranke, 2017-02-13 23:53:05]

dpranke@chromium.org changed reviewers:
+ asvitkine@chromium.org, isherman@chromium.org

[Dirk Pranke, 2017-02-13 23:53:06]

I'm not a histograms owner, you'll want asvitkine or isherman

[Ilya Sherman, 2017-02-14 00:20:20]

histograms.xml lgtm

[Alexei Svitkine (slow), 2017-02-14 15:37:29]

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

[erickung1, 2017-02-17 00:14:13]

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

[commit-bot: I haz the power, 2017-02-17 00:15:27]

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

[commit-bot: I haz the power, 2017-02-17 00:52:54]

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

[commit-bot: I haz the power, 2017-02-17 00:52:55]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[erickung1, 2017-02-17 09:19:00]

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

[commit-bot: I haz the power, 2017-02-17 09:19:17]

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

[commit-bot: I haz the power, 2017-02-17 09:56:18]

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

[commit-bot: I haz the power, 2017-02-17 09:56:19]

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

[wolenetz, 2017-02-18 00:01:03]

A couple other comments:
1) CL title mismatches the first line of description. Please make them
consistent.
2) bot is unhappy with current patch set 5:
https://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_lin...
:
[ RUN      ] ParseDolbyVisionCodecIdTest.InvalidDolbyVisionCodecIds
../../media/base/video_codecs_unittest.cc:185: Failure
Value of: ParseDolbyVisionCodecId("dvav.5.7", &profile, &level_id)
  Actual: true
Expected: false
[  FAILED  ] ParseDolbyVisionCodecIdTest.InvalidDolbyVisionCodecIds (0 ms)

All my comments are nits. LGTM % them.
I will be OoO Feb 18 - Mar 5. In my absence, please loop in dalecurtis@ for
finding a reviewer for subsequent changes. I'd be happy to review your changes
on my return, too.

https://codereview.chromium.org/2640113004/diff/80001/chromecast/media/base/m...
File chromecast/media/base/media_codec_support.h (right):

https://codereview.chromium.org/2640113004/diff/80001/chromecast/media/base/m...
chromecast/media/base/media_codec_support.h:30: // TODO(erickung): Removes
|codec_profile| parameter by changing VideoCodec enum
nit: s/Removes/Remove/

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

https://codereview.chromium.org/2640113004/diff/80001/media/base/mime_util_in...
media/base/mime_util_internal.h:44: DolbyVision,
nit: use MACRO_STYLE here for consistency (see
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md).
So, DOLBY_VISION (or DOLBYVISION)

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

https://codereview.chromium.org/2640113004/diff/80001/media/base/video_codecs...
media/base/video_codecs.h:76: HEVCPROFILE_MAIN_STILL_PICTURE = 18,
Was there unintentional deletion of HEVCPROFILE_MAX enum value here?

https://codereview.chromium.org/2640113004/diff/80001/media/formats/mp4/dolby...
File media/formats/mp4/dolby_vision.cc (right):

https://codereview.chromium.org/2640113004/diff/80001/media/formats/mp4/dolby...
media/formats/mp4/dolby_vision.cc:68: NOTREACHED();
Are you sure you want to DCHECK(false) here? There could be a buggy stream that
isn't supported -> we shouldn't crash here, we should just return false and emit
parse failure (even on a debug build), right?

https://codereview.chromium.org/2640113004/diff/80001/media/formats/mp4/dolby...
media/formats/mp4/dolby_vision.cc:72: if (media_log.get()) {
Why the conditioning of the DVLOG on media_log?

[erickung1, 2017-02-27 16:37:14]

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

[commit-bot: I haz the power, 2017-02-27 16:37:45]

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

[commit-bot: I haz the power, 2017-02-27 16:40:23]

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

[commit-bot: I haz the power, 2017-02-27 16:40:24]

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

[erickung1, 2017-02-27 17:25:37]

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

[commit-bot: I haz the power, 2017-02-27 17:26:04]

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

[erickung1, 2017-02-27 17:33:10]

https://codereview.chromium.org/2640113004/diff/80001/chromecast/media/base/m...
File chromecast/media/base/media_codec_support.h (right):

https://codereview.chromium.org/2640113004/diff/80001/chromecast/media/base/m...
chromecast/media/base/media_codec_support.h:30: // TODO(erickung): Removes
|codec_profile| parameter by changing VideoCodec enum
On 2017/02/18 00:01:02, wolenetz_OoO_Feb18-Mar5 wrote:
> nit: s/Removes/Remove/

Done.

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

https://codereview.chromium.org/2640113004/diff/80001/media/base/mime_util_in...
media/base/mime_util_internal.h:44: DolbyVision,
On 2017/02/18 00:01:02, wolenetz_OoO_Feb18-Mar5 wrote:
> nit: use MACRO_STYLE here for consistency (see
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md).
> So, DOLBY_VISION (or DOLBYVISION)

Done.

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

https://codereview.chromium.org/2640113004/diff/80001/media/base/video_codecs...
media/base/video_codecs.h:76: HEVCPROFILE_MAIN_STILL_PICTURE = 18,
On 2017/02/18 00:01:03, wolenetz_OoO_Feb18-Mar5 wrote:
> Was there unintentional deletion of HEVCPROFILE_MAX enum value here?

Done.

https://codereview.chromium.org/2640113004/diff/80001/media/formats/mp4/dolby...
File media/formats/mp4/dolby_vision.cc (right):

https://codereview.chromium.org/2640113004/diff/80001/media/formats/mp4/dolby...
media/formats/mp4/dolby_vision.cc:68: NOTREACHED();
On 2017/02/18 00:01:03, wolenetz_OoO_Feb18-Mar5 wrote:
> Are you sure you want to DCHECK(false) here? There could be a buggy stream
that
> isn't supported -> we shouldn't crash here, we should just return false and
emit
> parse failure (even on a debug build), right?

Done.

https://codereview.chromium.org/2640113004/diff/80001/media/formats/mp4/dolby...
media/formats/mp4/dolby_vision.cc:72: if (media_log.get()) {
On 2017/02/18 00:01:03, wolenetz_OoO_Feb18-Mar5 wrote:
> Why the conditioning of the DVLOG on media_log?

Done.

[commit-bot: I haz the power, 2017-02-27 17:48:51]

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

[commit-bot: I haz the power, 2017-02-27 17:48:53]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[erickung1, 2017-02-28 02:35:47]

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

[commit-bot: I haz the power, 2017-02-28 02:36:35]

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

[commit-bot: I haz the power, 2017-02-28 06:43:12]

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

[commit-bot: I haz the power, 2017-02-28 06:43:14]

Dry run: This issue passed the CQ dry run.

[erickung1, 2017-02-28 14:22:01]

The CQ bit was checked by erickung@chromium.org

[erickung1, 2017-02-28 14:22:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
halliwell@chromium.org, jbauman@chromium.org, isherman@chromium.org,
wolenetz@chromium.org
Link to the patchset: https://codereview.chromium.org/2640113004/#ps140001
(title: "fix build break on Android")

[commit-bot: I haz the power, 2017-02-28 14:22:21]

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

[commit-bot: I haz the power, 2017-02-28 14:31:26]

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

[commit-bot: I haz the power, 2017-02-28 14:31:27]

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

[erickung1, 2017-02-28 15:00:43]

erickung@chromium.org changed reviewers:
+ wfh@chromium.org

[erickung1, 2017-02-28 15:04:15]

wfh@ can you please help reviewing the following changes? Thanks.

Missing LGTM from an OWNER for these files:
    gpu/ipc/common/gpu_info.mojom
    gpu/ipc/common/gpu_info_struct_traits.cc

[erickung1, 2017-02-28 15:04:17]

wfh@ can you please help reviewing the following changes? Thanks.

Missing LGTM from an OWNER for these files:
    gpu/ipc/common/gpu_info.mojom
    gpu/ipc/common/gpu_info_struct_traits.cc

[Will Harris, 2017-02-28 17:56:53]

lgtm for mojo/ipc files.

[erickung1, 2017-03-01 00:06:06]

The CQ bit was checked by erickung@chromium.org

[commit-bot: I haz the power, 2017-03-01 00:06:59]

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

[commit-bot: I haz the power, 2017-03-01 00:13:25]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1488326766084430,
"parent_rev": "40fdca287f3492e18cd4e9ff513c3b846a1d8089", "commit_rev":
"2cca0f26776baa99251145cc158b259c1c6fd4c7"}

[commit-bot: I haz the power, 2017-03-01 00:14:51]

Description was changed from

==========
Introduce Dolby Vision video codec and Demuxer support

- Introduce DolbyVision codec and active profile 0, 4, 5 and 7.
- new flag "enable_dolby_vision_demuxing" to enable Dolby Vision
  demuxing feature using MSE using mime type video/mp4.
  Currently the CL only supports profile 5 and is only enabled on
  Chromecast build.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

BUG=481726
==========

to

==========
Introduce Dolby Vision video codec and Demuxer support

- Introduce DolbyVision codec and active profile 0, 4, 5 and 7.
- new flag "enable_dolby_vision_demuxing" to enable Dolby Vision
  demuxing feature using MSE using mime type video/mp4.
  Currently the CL only supports profile 5 and is only enabled on
  Chromecast build.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

BUG=481726

Review-Url: https://codereview.chromium.org/2640113004
Cr-Commit-Position: refs/heads/master@{#453762}
Committed:
https://chromium.googlesource.com/chromium/src/+/2cca0f26776baa99251145cc158b...
==========

[commit-bot: I haz the power, 2017-03-01 00:14:53]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/2cca0f26776baa99251145cc158b...

[wolenetz, 2017-03-08 20:08:12]

On 2017/02/27 17:33:10, erickung1 wrote:
>
https://codereview.chromium.org/2640113004/diff/80001/chromecast/media/base/m...
> File chromecast/media/base/media_codec_support.h (right):
> 
>
https://codereview.chromium.org/2640113004/diff/80001/chromecast/media/base/m...
> chromecast/media/base/media_codec_support.h:30: // TODO(erickung): Removes
> |codec_profile| parameter by changing VideoCodec enum
> On 2017/02/18 00:01:02, wolenetz_OoO_Feb18-Mar5 wrote:
> > nit: s/Removes/Remove/
> 
> Done.
> 
>
https://codereview.chromium.org/2640113004/diff/80001/media/base/mime_util_in...
> File media/base/mime_util_internal.h (right):
> 
>
https://codereview.chromium.org/2640113004/diff/80001/media/base/mime_util_in...
> media/base/mime_util_internal.h:44: DolbyVision,
> On 2017/02/18 00:01:02, wolenetz_OoO_Feb18-Mar5 wrote:
> > nit: use MACRO_STYLE here for consistency (see
> >
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md).
> > So, DOLBY_VISION (or DOLBYVISION)
> 
> Done.
> 
>
https://codereview.chromium.org/2640113004/diff/80001/media/base/video_codecs.h
> File media/base/video_codecs.h (right):
> 
>
https://codereview.chromium.org/2640113004/diff/80001/media/base/video_codecs...
> media/base/video_codecs.h:76: HEVCPROFILE_MAIN_STILL_PICTURE = 18,
> On 2017/02/18 00:01:03, wolenetz_OoO_Feb18-Mar5 wrote:
> > Was there unintentional deletion of HEVCPROFILE_MAX enum value here?
> 
> Done.
> 
>
https://codereview.chromium.org/2640113004/diff/80001/media/formats/mp4/dolby...
> File media/formats/mp4/dolby_vision.cc (right):
> 
>
https://codereview.chromium.org/2640113004/diff/80001/media/formats/mp4/dolby...
> media/formats/mp4/dolby_vision.cc:68: NOTREACHED();
> On 2017/02/18 00:01:03, wolenetz_OoO_Feb18-Mar5 wrote:
> > Are you sure you want to DCHECK(false) here? There could be a buggy stream
> that
> > isn't supported -> we shouldn't crash here, we should just return false and
> emit
> > parse failure (even on a debug build), right?
> 
> Done.
> 
>
https://codereview.chromium.org/2640113004/diff/80001/media/formats/mp4/dolby...
> media/formats/mp4/dolby_vision.cc:72: if (media_log.get()) {
> On 2017/02/18 00:01:03, wolenetz_OoO_Feb18-Mar5 wrote:
> > Why the conditioning of the DVLOG on media_log?
> 
> Done.

(post-vacation re-lgtm) :)