Support HLS MPEG2 TS with SAMPLE-AES encryption.

The actual encryption is assumed to have been carried out
according to the SAMPLE-AES encryption method cited by the
HLS spec (informational RFC called draft-pantos-http-live-
streaming).

This encryption method is being incorporated in the Common
Encryption specification ISO/IEC 23001-7 3rd Edition as the
'cbcs' scheme.

Parsing of the stream requires it to be prefixed with extra
TS packets that carry encryption metadata. These packets
are formatted according to ISO/IEC 23001-9 "Common
encryption of MPEG-2 transport streams".

This CL is dependent on a prior CL (1490613005) that allows
conveyance of the extended encryption metadata through the
pipeline.

BUG=568326
Committed: https://crrev.com/a66a6e417397bde59355594554931b0784ec1598
Cr-Commit-Position: refs/heads/master@{#436058}

[dougsteed, 2015-12-09 23:36:11]

Description was changed from

==========
Support for CENCv3 and HLS with SampleAES.

BUG=
==========

to

==========
Support for CENCv3 and HLS with SampleAES.

BUG=568326
==========

[dougsteed, 2015-12-09 23:45:28]

dougsteed@chromium.org changed reviewers:
+ dalecurtis@chromium.org, lcwu@chromium.org, yucliu@chromium.org

[DaleCurtis, 2015-12-09 23:47:34]

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

[DaleCurtis, 2015-12-09 23:47:35]

=>ddorwin

[ddorwin, 2015-12-10 20:10:59]

I skipped most of media/formats/mp2t/.

Does the define cover all CENC in TS, v3, or just the Sample AES hacks?

Description:
* Is this adding CENC for TS for the first time (i.e. Part 9) or is this
specific to v3. If the former, please be clear about that.
* Are there spec numbers (i.e. Part 9) that would be helpful?
* "CENC v3"? Or is the concatenation common?
* Same for "Sample AES

Test files:
* Describe them in their names. What content (i.e. bear?), codecs, etc.?
* What is "main"?
* Is .ts the best extension? We already have bear.m2ts.
* Describe how they were created in media/test/data/README.

https://codereview.chromium.org/1517473002/diff/20001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/1517473002/diff/20001/media/BUILD.gn#newcode790
media/BUILD.gn:790: deps += [ "//third_party/boringssl" ]
Is there a reason to use boringssl directly instead of //crypto? The latter
helps ensure we don't use crypto incorrectly.

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

https://codereview.chromium.org/1517473002/diff/20001/media/base/bit_reader.c...
media/base/bit_reader.cc:34: bool BitReader::ReadString(int num_bits,
std::string* str) {
nit: move above previous function.

https://codereview.chromium.org/1517473002/diff/20001/media/base/bit_reader.h
File media/base/bit_reader.h (right):

https://codereview.chromium.org/1517473002/diff/20001/media/base/bit_reader.h...
media/base/bit_reader.h:38: // Read |num_bits| of binary data into |str|.
|num_bits| must be a multiple
nit: move up with the other Read*().

https://codereview.chromium.org/1517473002/diff/20001/media/filters/h264_pars...
File media/filters/h264_parser.h (right):

https://codereview.chromium.org/1517473002/diff/20001/media/filters/h264_pars...
media/filters/h264_parser.h:56: int64 offset;
Is this needed/used?

https://codereview.chromium.org/1517473002/diff/20001/media/formats/mp2t/desc...
File media/formats/mp2t/descriptors.h (right):

https://codereview.chromium.org/1517473002/diff/20001/media/formats/mp2t/desc...
media/formats/mp2t/descriptors.h:21: // Representation of a list of descriptors,
used in the MPEG-2 Systems standard
Is there a reason we didn't need this before? Is this part of a new standard
(i.e. Part 9)?

https://codereview.chromium.org/1517473002/diff/20001/media/formats/mp2t/mp2t...
File media/formats/mp2t/mp2t_stream_parser_unittest.cc (right):

https://codereview.chromium.org/1517473002/diff/20001/media/formats/mp2t/mp2t...
media/formats/mp2t/mp2t_stream_parser_unittest.cc:9: #include <openssl/aes.h>
The Clear Key implementation supports AES via crypto/ without going directly to
openssl. What is special here?

You'll probably need a crypto/ owner to review this (if you can't use crypto/
instead).

https://codereview.chromium.org/1517473002/diff/20001/media/formats/mpeg/adts...
File media/formats/mpeg/adts_constants.h (right):

https://codereview.chromium.org/1517473002/diff/20001/media/formats/mpeg/adts...
media/formats/mpeg/adts_constants.h:17: kADTSHeaderSizeNoCrc = 7,
Is this relevant to this CL?

https://codereview.chromium.org/1517473002/diff/20001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/1517473002/diff/20001/media/media.gyp#newcode...
media/media.gyp:1209: '../third_party/boringssl/boringssl.gyp:boringssl',
ditto

[yucliu1, 2015-12-11 19:55:31]

https://codereview.chromium.org/1517473002/diff/40001/media/formats/mp2t/es_p...
File media/formats/mp2t/es_parser_adts.cc (right):

https://codereview.chromium.org/1517473002/diff/40001/media/formats/mp2t/es_p...
media/formats/mp2t/es_parser_adts.cc:131:
get_decrypt_config_cb_(GetDecryptConfigCB()),
Maybe simply get_decrypt_config_cb_()?

https://codereview.chromium.org/1517473002/diff/40001/media/formats/mp2t/es_p...
media/formats/mp2t/es_parser_adts.cc:140: const GetDecryptConfigCB
get_decrypt_config_cb,
const GetDecryptConfigCB&

https://codereview.chromium.org/1517473002/diff/40001/media/formats/mp2t/es_p...
File media/formats/mp2t/es_parser_adts.h (right):

https://codereview.chromium.org/1517473002/diff/40001/media/formats/mp2t/es_p...
media/formats/mp2t/es_parser_adts.h:40: const GetDecryptConfigCB
get_decrypt_config_cb,
const GetDecryptConfigCB&

https://codereview.chromium.org/1517473002/diff/40001/media/formats/mp2t/es_p...
File media/formats/mp2t/es_parser_h264.cc (right):

https://codereview.chromium.org/1517473002/diff/40001/media/formats/mp2t/es_p...
media/formats/mp2t/es_parser_h264.cc:169:
get_decrypt_config_cb_(GetDecryptConfigCB()),
ditto.

https://codereview.chromium.org/1517473002/diff/40001/media/formats/mp2t/es_p...
media/formats/mp2t/es_parser_h264.cc:401: kSampleAESEncryptBlocks,
kSampleAESSkipBlocks));
Add a TODO to support arbitrary enrypt:skip pattern.

[yucliu1, 2015-12-11 21:34:55]

https://codereview.chromium.org/1517473002/diff/40001/media/formats/mp2t/desc...
File media/formats/mp2t/descriptors.cc (right):

https://codereview.chromium.org/1517473002/diff/40001/media/formats/mp2t/desc...
media/formats/mp2t/descriptors.cc:147: return private_data_indicator = value;
I think the "=" here should be "==".

[dougsteed, 2015-12-14 22:51:46]

PTAL

Note that I had to temporarily suppress the new unittest pending suitable test
files. Will not be landed until it is re-enabled.

https://codereview.chromium.org/1517473002/diff/20001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/1517473002/diff/20001/media/BUILD.gn#newcode790
media/BUILD.gn:790: deps += [ "//third_party/boringssl" ]
On 2015/12/10 20:10:58, ddorwin wrote:
> Is there a reason to use boringssl directly instead of //crypto? The latter
> helps ensure we don't use crypto incorrectly.

Yes. The code in //crypto doesn't support CBC mode without padding. Since this
is just for a unit test I didn't think it worth the effort to make a change in
//crypto.

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

https://codereview.chromium.org/1517473002/diff/20001/media/base/bit_reader.c...
media/base/bit_reader.cc:34: bool BitReader::ReadString(int num_bits,
std::string* str) {
On 2015/12/10 20:10:58, ddorwin wrote:
> nit: move above previous function.

Done.

https://codereview.chromium.org/1517473002/diff/20001/media/base/bit_reader.h
File media/base/bit_reader.h (right):

https://codereview.chromium.org/1517473002/diff/20001/media/base/bit_reader.h...
media/base/bit_reader.h:38: // Read |num_bits| of binary data into |str|.
|num_bits| must be a multiple
On 2015/12/10 20:10:58, ddorwin wrote:
> nit: move up with the other Read*().

Done.

https://codereview.chromium.org/1517473002/diff/20001/media/filters/h264_pars...
File media/filters/h264_parser.h (right):

https://codereview.chromium.org/1517473002/diff/20001/media/filters/h264_pars...
media/filters/h264_parser.h:56: int64 offset;
On 2015/12/10 20:10:58, ddorwin wrote:
> Is this needed/used?

Looks like not. Holdover.

https://codereview.chromium.org/1517473002/diff/20001/media/formats/mp2t/desc...
File media/formats/mp2t/descriptors.h (right):

https://codereview.chromium.org/1517473002/diff/20001/media/formats/mp2t/desc...
media/formats/mp2t/descriptors.h:21: // Representation of a list of descriptors,
used in the MPEG-2 Systems standard
On 2015/12/10 20:10:58, ddorwin wrote:
> Is there a reason we didn't need this before? Is this part of a new standard
> (i.e. Part 9)?

Part 9 does require a descriptor, and so also does HLS SampleAES. I guess none
of our previous applications of MPEG2 TS did.

https://codereview.chromium.org/1517473002/diff/20001/media/formats/mp2t/mp2t...
File media/formats/mp2t/mp2t_stream_parser_unittest.cc (right):

https://codereview.chromium.org/1517473002/diff/20001/media/formats/mp2t/mp2t...
media/formats/mp2t/mp2t_stream_parser_unittest.cc:9: #include <openssl/aes.h>
On 2015/12/10 20:10:59, ddorwin wrote:
> The Clear Key implementation supports AES via crypto/ without going directly
to
> openssl. What is special here?
> 
> You'll probably need a crypto/ owner to review this (if you can't use crypto/
> instead).

See comment on my change to BUILD.gn.

https://codereview.chromium.org/1517473002/diff/20001/media/formats/mpeg/adts...
File media/formats/mpeg/adts_constants.h (right):

https://codereview.chromium.org/1517473002/diff/20001/media/formats/mpeg/adts...
media/formats/mpeg/adts_constants.h:17: kADTSHeaderSizeNoCrc = 7,
On 2015/12/10 20:10:59, ddorwin wrote:
> Is this relevant to this CL?

Yes. See formats/mp2t/es_parser_adts.cc

https://codereview.chromium.org/1517473002/diff/20001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/1517473002/diff/20001/media/media.gyp#newcode...
media/media.gyp:1209: '../third_party/boringssl/boringssl.gyp:boringssl',
On 2015/12/10 20:10:59, ddorwin wrote:
> ditto

See my earlier comment.

https://codereview.chromium.org/1517473002/diff/40001/media/formats/mp2t/desc...
File media/formats/mp2t/descriptors.cc (right):

https://codereview.chromium.org/1517473002/diff/40001/media/formats/mp2t/desc...
media/formats/mp2t/descriptors.cc:147: return private_data_indicator = value;
On 2015/12/11 21:34:55, yucliu1 wrote:
> I think the "=" here should be "==".

Good catch.

https://codereview.chromium.org/1517473002/diff/40001/media/formats/mp2t/es_p...
File media/formats/mp2t/es_parser_adts.cc (right):

https://codereview.chromium.org/1517473002/diff/40001/media/formats/mp2t/es_p...
media/formats/mp2t/es_parser_adts.cc:131:
get_decrypt_config_cb_(GetDecryptConfigCB()),
On 2015/12/11 19:55:31, yucliu1 wrote:
> Maybe simply get_decrypt_config_cb_()?

Done.

https://codereview.chromium.org/1517473002/diff/40001/media/formats/mp2t/es_p...
media/formats/mp2t/es_parser_adts.cc:140: const GetDecryptConfigCB
get_decrypt_config_cb,
On 2015/12/11 19:55:31, yucliu1 wrote:
> const GetDecryptConfigCB&

Done.

https://codereview.chromium.org/1517473002/diff/40001/media/formats/mp2t/es_p...
File media/formats/mp2t/es_parser_adts.h (right):

https://codereview.chromium.org/1517473002/diff/40001/media/formats/mp2t/es_p...
media/formats/mp2t/es_parser_adts.h:40: const GetDecryptConfigCB
get_decrypt_config_cb,
On 2015/12/11 19:55:31, yucliu1 wrote:
> const GetDecryptConfigCB&

Done.

https://codereview.chromium.org/1517473002/diff/40001/media/formats/mp2t/es_p...
File media/formats/mp2t/es_parser_h264.cc (right):

https://codereview.chromium.org/1517473002/diff/40001/media/formats/mp2t/es_p...
media/formats/mp2t/es_parser_h264.cc:169:
get_decrypt_config_cb_(GetDecryptConfigCB()),
On 2015/12/11 19:55:31, yucliu1 wrote:
> ditto.

Done.

https://codereview.chromium.org/1517473002/diff/40001/media/formats/mp2t/es_p...
media/formats/mp2t/es_parser_h264.cc:401: kSampleAESEncryptBlocks,
kSampleAESSkipBlocks));
On 2015/12/11 19:55:31, yucliu1 wrote:
> Add a TODO to support arbitrary enrypt:skip pattern.

Not needed for HLS SampleAES. But may be needed when/if we generalize this to
cover other uses of CENCv3. At that time there will presumably be extended
signalling to convey the (encrypt,skip) values.

[dougsteed, 2015-12-14 23:09:17]

Answers to ddorwin overall comment:

{Does the define cover all CENC in TS, v3, or just the Sample AES hacks?}

Only the parts that are required for SampleAES.

{Description:
* Is this adding CENC for TS for the first time (i.e. Part 9) or is this
specific to v3. If the former, please be clear about that.}

> The former.

{* Are there spec numbers (i.e. Part 9) that would be helpful?
* "CENC v3"? Or is the concatenation common?
* Same for "Sample AES"}

Will do.

{Test files:}

The test files I had originally included were not suitable and have been removed
from the CL pending getting appropriate ones.

[dougsteed, 2016-02-25 17:11:27]

Description was changed from

==========
Support for CENCv3 and HLS with SampleAES.

BUG=568326
==========

to

==========
The actual encryption is assumed to have been carried out
according to the SAMPLE-AES encryption method cited by the HLS spec
(informational RFC called draft-pantos-http-live-streaming).

This encryption method is being incorporated in the Common
Encryption specification ISO/IEC 23001-7 3rd Edition as the 'cbcs' scheme.

Parsing of the stream requires it to be prefixed with extra TS packets that
carry encryption metadata. These packets are formatted according to ISO/IEC
23001-9 "Common encryption of MPEG-2 transport streams".

This CL is dependent on a prior CL (1517473002) that allows conveyance of the
extended encryption metadata through the pipeline.

BUG=568326
==========

[dougsteed, 2016-02-25 17:12:00]

Description was changed from

==========
The actual encryption is assumed to have been carried out
according to the SAMPLE-AES encryption method cited by the HLS spec
(informational RFC called draft-pantos-http-live-streaming).

This encryption method is being incorporated in the Common
Encryption specification ISO/IEC 23001-7 3rd Edition as the 'cbcs' scheme.

Parsing of the stream requires it to be prefixed with extra TS packets that
carry encryption metadata. These packets are formatted according to ISO/IEC
23001-9 "Common encryption of MPEG-2 transport streams".

This CL is dependent on a prior CL (1517473002) that allows conveyance of the
extended encryption metadata through the pipeline.

BUG=568326
==========

to

==========
The actual encryption is assumed to have been carried out according to the
SAMPLE-AES encryption method cited by the HLS spec (informational RFC called
draft-pantos-http-live-streaming).

This encryption method is being incorporated in the Common Encryption
specification ISO/IEC 23001-7 3rd Edition as the 'cbcs' scheme.

Parsing of the stream requires it to be prefixed with extra TS packets that
carry encryption metadata. These packets are formatted according to ISO/IEC
23001-9 "Common encryption of MPEG-2 transport streams".

This CL is dependent on a prior CL (1517473002) that allows conveyance of the
extended encryption metadata through the pipeline.

BUG=568326
==========

[dougsteed, 2016-02-25 17:13:20]

Description was changed from

==========
The actual encryption is assumed to have been carried out according to the
SAMPLE-AES encryption method cited by the HLS spec (informational RFC called
draft-pantos-http-live-streaming).

This encryption method is being incorporated in the Common Encryption
specification ISO/IEC 23001-7 3rd Edition as the 'cbcs' scheme.

Parsing of the stream requires it to be prefixed with extra TS packets that
carry encryption metadata. These packets are formatted according to ISO/IEC
23001-9 "Common encryption of MPEG-2 transport streams".

This CL is dependent on a prior CL (1517473002) that allows conveyance of the
extended encryption metadata through the pipeline.

BUG=568326
==========

to

==========
The actual encryption is assumed to have been carried out
according to the SAMPLE-AES encryption method cited by the
HLS spec (informational RFC called draft-pantos-http-live-
streaming).

This encryption method is being incorporated in the Common
Encryption specification ISO/IEC 23001-7 3rd Edition as the
'cbcs' scheme.

Parsing of the stream requires it to be prefixed with extra
TS packets that carry encryption metadata. These packets
are formatted according to ISO/IEC 23001-9 "Common
encryption of MPEG-2 transport streams".

This CL is dependent on a prior CL (1517473002) that allows
conveyance of the extended encryption metadata through the
pipeline.

BUG=568326
==========

[dougsteed, 2016-03-10 18:37:17]

Description was changed from

==========
The actual encryption is assumed to have been carried out
according to the SAMPLE-AES encryption method cited by the
HLS spec (informational RFC called draft-pantos-http-live-
streaming).

This encryption method is being incorporated in the Common
Encryption specification ISO/IEC 23001-7 3rd Edition as the
'cbcs' scheme.

Parsing of the stream requires it to be prefixed with extra
TS packets that carry encryption metadata. These packets
are formatted according to ISO/IEC 23001-9 "Common
encryption of MPEG-2 transport streams".

This CL is dependent on a prior CL (1517473002) that allows
conveyance of the extended encryption metadata through the
pipeline.

BUG=568326
==========

to

==========
The actual encryption is assumed to have been carried out
according to the SAMPLE-AES encryption method cited by the
HLS spec (informational RFC called draft-pantos-http-live-
streaming).

This encryption method is being incorporated in the Common
Encryption specification ISO/IEC 23001-7 3rd Edition as the
'cbcs' scheme.

Parsing of the stream requires it to be prefixed with extra
TS packets that carry encryption metadata. These packets
are formatted according to ISO/IEC 23001-9 "Common
encryption of MPEG-2 transport streams".

This CL is dependent on a prior CL (1490613005) that allows
conveyance of the extended encryption metadata through the
pipeline.

BUG=568326
==========

[dougsteed, 2016-03-14 21:14:13]

dougsteed@chromium.org changed reviewers:
+ timav@chromium.org, xhwang@chromium.org

[slan, 2016-03-18 22:31:22]

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

[slan, 2016-03-18 22:31:23]

https://codereview.chromium.org/1517473002/diff/320001/media/media_options.gni
File media/media_options.gni (right):

https://codereview.chromium.org/1517473002/diff/320001/media/media_options.gn...
media/media_options.gni:50: enable_hls_sample_aes = proprietary_codecs &&
is_cast_desktop_build
Hey Doug, I think we want to enable this by default on all Chromecast Linux A/V
devices (when using proprietary codecs). So perhaps we should make this:

enable_hls_sample_aes = is_chromecast && is_linux && 
                        proprietary_codecs && !disable_display

[dougsteed, 2016-03-21 18:11:06]

xhwang@ now that the CL on which this depends has landed, would like to focus on
getting this one done. Can you take a look.

slan@ response to your comment.

https://codereview.chromium.org/1517473002/diff/320001/media/media_options.gni
File media/media_options.gni (right):

https://codereview.chromium.org/1517473002/diff/320001/media/media_options.gn...
media/media_options.gni:50: enable_hls_sample_aes = proprietary_codecs &&
is_cast_desktop_build
On 2016/03/18 22:31:23, slan wrote:
> Hey Doug, I think we want to enable this by default on all Chromecast Linux
A/V
> devices (when using proprietary codecs). So perhaps we should make this:
> 
> enable_hls_sample_aes = is_chromecast && is_linux && 
>                         proprietary_codecs && !disable_display

The intent was that the various OEMs would enable this explicitly when they
upgrade their code to support AES-CBC.

[slan, 2016-03-23 21:05:57]

On 2016/03/21 18:11:06, dougsteed wrote:
> xhwang@ now that the CL on which this depends has landed, would like to focus
on
> getting this one done. Can you take a look.
> 
> slan@ response to your comment.
> 
> https://codereview.chromium.org/1517473002/diff/320001/media/media_options.gni
> File media/media_options.gni (right):
> 
>
https://codereview.chromium.org/1517473002/diff/320001/media/media_options.gn...
> media/media_options.gni:50: enable_hls_sample_aes = proprietary_codecs &&
> is_cast_desktop_build
> On 2016/03/18 22:31:23, slan wrote:
> > Hey Doug, I think we want to enable this by default on all Chromecast Linux
> A/V
> > devices (when using proprietary codecs). So perhaps we should make this:
> > 
> > enable_hls_sample_aes = is_chromecast && is_linux && 
> >                         proprietary_codecs && !disable_display
> 
> The intent was that the various OEMs would enable this explicitly when they
> upgrade their code to support AES-CBC.

Chatted offline, good with the default value as-is for now.

[ddorwin, 2016-03-23 22:29:27]

https://codereview.chromium.org/1517473002/diff/320001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/1517473002/diff/320001/media/BUILD.gn#newcode22
media/BUILD.gn:22: "ENABLE_HLS_SAMPLE_AES=$enable_hls_sample_aes",
Is this a subset/dependent on enable_mse_mpeg2ts_stream_parser?

If so, this should be enforced somewhere. I think GN can throw errors.

Do we need a separate flag for the Part 9-compliant code? Is there any reason
not to build Part 9 if MPEG-2 TS MSE support is built? The non Part 9-complaint
code is another story.

https://codereview.chromium.org/1517473002/diff/320001/media/BUILD.gn#newcode309
media/BUILD.gn:309: if (proprietary_codecs && enable_hls_sample_aes) {
Should this be within in the enable_mse_mpeg2ts_stream_parser condition below?

https://codereview.chromium.org/1517473002/diff/320001/media/test/data/README
File media/test/data/README (right):

https://codereview.chromium.org/1517473002/diff/320001/media/test/data/README...
media/test/data/README:77: bear-1280x720-hls-sample-aes.ts - This was produced
in two stages. bear-1280x720.ts was run through an HLS segmenter tool, resulting
in bear-1280x720-hls.ts. This remultiplexed
Do you have the actual command lines? We try to include these, though that is
unfortunately not the case above.

https://codereview.chromium.org/1517473002/diff/320001/media/test/data/README...
media/test/data/README:80: need to mock key delivery). keyid=
We mock key delivery in existing EME tests. Is it that big of a burden? Using
the same key would be ideal.

[dougsteed, 2016-03-24 22:13:36]

https://codereview.chromium.org/1517473002/diff/320001/media/test/data/README
File media/test/data/README (right):

https://codereview.chromium.org/1517473002/diff/320001/media/test/data/README...
media/test/data/README:80: need to mock key delivery). keyid=
On 2016/03/23 22:29:26, ddorwin wrote:
> We mock key delivery in existing EME tests. Is it that big of a burden? Using
> the same key would be ideal.

Not sure what you are referring to there. Can you clarify? Note that I am
actually decrypting within the test, so as to verify that the subsample and
pattern parsing was done correctly. (That's why I have the clear and encrypted
versions of the same content). Are there existing tests that actually do
decryption to compare against clear text?

[ddorwin, 2016-03-24 22:44:52]

https://codereview.chromium.org/1517473002/diff/320001/media/test/data/README
File media/test/data/README (right):

https://codereview.chromium.org/1517473002/diff/320001/media/test/data/README...
media/test/data/README:80: need to mock key delivery). keyid=
On 2016/03/24 22:13:36, dougsteed wrote:
> On 2016/03/23 22:29:26, ddorwin wrote:
> > We mock key delivery in existing EME tests. Is it that big of a burden?
Using
> > the same key would be ideal.
> 
> Not sure what you are referring to there. Can you clarify? Note that I am
> actually decrypting within the test, so as to verify that the subsample and
> pattern parsing was done correctly. (That's why I have the clear and encrypted
> versions of the same content). Are there existing tests that actually do
> decryption to compare against clear text?

I commented in the test. I don't know that we test decryption directly (we
might), but we do have tests that convert a key ID to a key to provide to the
CDM. For example,
https://code.google.com/p/chromium/codesearch#chromium/src/media/test/pipelin...
emulates an EME app and provides the requested key.

If you use the same key ID and key, as we do for all other files, you'll have
the option to test these files there too.

https://codereview.chromium.org/1517473002/diff/340001/media/formats/mp2t/mp2...
File media/formats/mp2t/mp2t_stream_parser_unittest.cc (right):

https://codereview.chromium.org/1517473002/diff/340001/media/formats/mp2t/mp2...
media/formats/mp2t/mp2t_stream_parser_unittest.cc:128: std::string key =
buffer.decrypt_config()->key_id();
Related to my other comment, I suggest actually doing a lookup. All of our other
files are encrypted with the same key. See
https://code.google.com/p/chromium/codesearch#chromium/src/media/test/pipelin....
That would also allow these files to be run through pipeline and browser tests
if you wished.

[dougsteed, 2016-03-31 19:09:50]

ddorwin@ I think I took into account all your comments
xhwang@ can you take a look? We would love to get this into M51.

https://codereview.chromium.org/1517473002/diff/320001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/1517473002/diff/320001/media/BUILD.gn#newcode22
media/BUILD.gn:22: "ENABLE_HLS_SAMPLE_AES=$enable_hls_sample_aes",
On 2016/03/23 22:29:26, ddorwin wrote:
> Is this a subset/dependent on enable_mse_mpeg2ts_stream_parser?
> 
> If so, this should be enforced somewhere. I think GN can throw errors.
> 
> Do we need a separate flag for the Part 9-compliant code? Is there any reason
> not to build Part 9 if MPEG-2 TS MSE support is built? The non Part
9-complaint
> code is another story.

Done the gni variable check.

I have not include the Part 9 parsing functions except when
enable_hls_sample_aes is set, as discussed below.

https://codereview.chromium.org/1517473002/diff/320001/media/BUILD.gn#newcode309
media/BUILD.gn:309: if (proprietary_codecs && enable_hls_sample_aes) {
On 2016/03/23 22:29:26, ddorwin wrote:
> Should this be within in the enable_mse_mpeg2ts_stream_parser condition below?

I don't strongly object to that, but didn't do it yet because I didn't want to
open up the Part 9 stuff to more general use, instead keeping it as only for use
with HLS SAMPLE_AES. I figure that when if it gets used more widely we could
revisit this.

https://codereview.chromium.org/1517473002/diff/320001/media/test/data/README
File media/test/data/README (right):

https://codereview.chromium.org/1517473002/diff/320001/media/test/data/README...
media/test/data/README:77: bear-1280x720-hls-sample-aes.ts - This was produced
in two stages. bear-1280x720.ts was run through an HLS segmenter tool, resulting
in bear-1280x720-hls.ts. This remultiplexed
On 2016/03/23 22:29:27, ddorwin wrote:
> Do you have the actual command lines? We try to include these, though that is
> unfortunately not the case above.

Done.

https://codereview.chromium.org/1517473002/diff/320001/media/test/data/README...
media/test/data/README:80: need to mock key delivery). keyid=
On 2016/03/23 22:29:26, ddorwin wrote:
> We mock key delivery in existing EME tests. Is it that big of a burden? Using
> the same key would be ideal.

Done.

https://codereview.chromium.org/1517473002/diff/320001/media/test/data/README...
media/test/data/README:80: need to mock key delivery). keyid=
On 2016/03/24 22:44:52, ddorwin wrote:
> On 2016/03/24 22:13:36, dougsteed wrote:
> > On 2016/03/23 22:29:26, ddorwin wrote:
> > > We mock key delivery in existing EME tests. Is it that big of a burden?
> Using
> > > the same key would be ideal.
> > 
> > Not sure what you are referring to there. Can you clarify? Note that I am
> > actually decrypting within the test, so as to verify that the subsample and
> > pattern parsing was done correctly. (That's why I have the clear and
encrypted
> > versions of the same content). Are there existing tests that actually do
> > decryption to compare against clear text?
> 
> I commented in the test. I don't know that we test decryption directly (we
> might), but we do have tests that convert a key ID to a key to provide to the
> CDM. For example,
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/test/pipelin...
> emulates an EME app and provides the requested key.
> 
> If you use the same key ID and key, as we do for all other files, you'll have
> the option to test these files there too.

Done.

https://codereview.chromium.org/1517473002/diff/340001/media/formats/mp2t/mp2...
File media/formats/mp2t/mp2t_stream_parser_unittest.cc (right):

https://codereview.chromium.org/1517473002/diff/340001/media/formats/mp2t/mp2...
media/formats/mp2t/mp2t_stream_parser_unittest.cc:128: std::string key =
buffer.decrypt_config()->key_id();
On 2016/03/24 22:44:52, ddorwin wrote:
> Related to my other comment, I suggest actually doing a lookup. All of our
other
> files are encrypted with the same key. See
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/test/pipelin....
> That would also allow these files to be run through pipeline and browser tests
> if you wished.

Done.

[ddorwin, 2016-03-31 20:59:22]

Thanks.

https://codereview.chromium.org/1517473002/diff/320001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/1517473002/diff/320001/media/BUILD.gn#newcode309
media/BUILD.gn:309: if (proprietary_codecs && enable_hls_sample_aes) {
On 2016/03/31 19:09:50, dougsteed wrote:
> On 2016/03/23 22:29:26, ddorwin wrote:
> > Should this be within in the enable_mse_mpeg2ts_stream_parser condition
below?
> 
> I don't strongly object to that, but didn't do it yet because I didn't want to
> open up the Part 9 stuff to more general use, instead keeping it as only for
use
> with HLS SAMPLE_AES. I figure that when if it gets used more widely we could
> revisit this.

I meant should we move this condition and its contents down there. It doesn't
make sense to include these files without MSE MP2TS support. (The flags are
checked, but it seems nice to enforce that here as well.)

proprietary_codecs would be duplicated there, so it would just be if
(enable_hls_sample_aes).

https://codereview.chromium.org/1517473002/diff/400001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/1517473002/diff/400001/media/media.gyp#newcode43
media/media.gyp:43: 'enable_mse_mpeg2ts_stream_parser%': 1,
enable_hls_sample_aes was removed. Is that because this only needs to be
supported on GN?

[dougsteed, 2016-04-01 01:19:28]

https://codereview.chromium.org/1517473002/diff/320001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/1517473002/diff/320001/media/BUILD.gn#newcode309
media/BUILD.gn:309: if (proprietary_codecs && enable_hls_sample_aes) {
On 2016/03/31 20:59:21, ddorwin wrote:
> On 2016/03/31 19:09:50, dougsteed wrote:
> > On 2016/03/23 22:29:26, ddorwin wrote:
> > > Should this be within in the enable_mse_mpeg2ts_stream_parser condition
> below?
> > 
> > I don't strongly object to that, but didn't do it yet because I didn't want
to
> > open up the Part 9 stuff to more general use, instead keeping it as only for
> use
> > with HLS SAMPLE_AES. I figure that when if it gets used more widely we could
> > revisit this.
> 
> I meant should we move this condition and its contents down there. It doesn't
> make sense to include these files without MSE MP2TS support. (The flags are
> checked, but it seems nice to enforce that here as well.)
> 
> proprietary_codecs would be duplicated there, so it would just be if
> (enable_hls_sample_aes).

Done.

https://codereview.chromium.org/1517473002/diff/400001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/1517473002/diff/400001/media/media.gyp#newcode43
media/media.gyp:43: 'enable_mse_mpeg2ts_stream_parser%': 1,
On 2016/03/31 20:59:21, ddorwin wrote:
> enable_hls_sample_aes was removed. Is that because this only needs to be
> supported on GN?

No it was actually a rebasing error , it ended up being declared twice (see just
below). Although we have now moved off gyp and onto gn in cast builds, so I
don't anticipate maintaining the gyp file any longer.

[xhwang, 2016-04-11 23:13:14]

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

[xhwang, 2016-04-11 23:13:15]

I'll defer to ddorwin@ to finish the review of this CL as media/ OWNER. He has
way more context on this CL than me. Thanks!

[ddorwin, 2016-04-12 00:40:48]

I only skimmed media/formats/mp2t/. You'll need someone to review that code.

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/base/bit_...
File media/base/bit_reader.cc (right):

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/base/bit_...
media/base/bit_reader.cc:20: bool BitReader::ReadString(int num_bits,
std::string* str) {
Why not just pass in num_bytes?

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/base/bit_...
media/base/bit_reader.cc:22: DCHECK_EQ(num_bits % 8, 0);
Move the DCHECKs up to first line - they are checking an input assumption. Then
separate with an empty line.

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/base/bit_...
media/base/bit_reader.cc:25: char* ptr = &str->front();
Calling front() on an empty string has undefined behavior. We should at least
DCHECK_GT(num_bytes, 0);

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/base/bit_...
media/base/bit_reader.cc:25: char* ptr = &str->front();
Then add a note to .h that the num_ value must be at least one byte.

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/base/bit_...
media/base/bit_reader.cc:26: while (num_bytes--)
With the nesting, it's probably best to add braces for this one.

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/base/bit_...
media/base/bit_reader.cc:27: if (!ReadBits(8, ptr++))
Should we note in the .h that this does not guarantee the string is valid?

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/base/test...
File media/base/test_data_util.h (right):

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/base/test...
media/base/test_data_util.h:39: // If the provided |key_id| is that of the test
key, returns true and fills the
s/the/a/ - we could support multiple

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/base/test...
media/base/test_data_util.h:43: bool LookupTestKey(const std::vector<uint8_t>&
key_id,
Consider ...Vector (and ...String below)
https://google.github.io/styleguide/cppguide.html#Function_Overloading

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/base/test...
media/base/test_data_util.h:45: bool allowRotation);
This should be before the output parameter.

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
File media/formats/mp2t/descriptors.cc (right):

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
media/formats/mp2t/descriptors.cc:85: RCHECK(extra_bits % 8 == 0);
You might also need to check that it's > 0 if you assume that in ReadString()

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
File media/formats/mp2t/es_parser_adts.cc (right):

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
media/formats/mp2t/es_parser_adts.cc:138:
use_hls_sample_aes_(use_hls_sample_aes),
DCHECK_EQ(get_decrypt_config_cb_, use_hls_sample_aes_) ?

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
File media/formats/mp2t/es_parser_adts.h (right):

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
media/formats/mp2t/es_parser_adts.h:45: bool use_hls_sample_aes,
nit: suggest putting this first since it controls the use of the cb_, right?

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
media/formats/mp2t/es_parser_adts.h:46: bool sbr_in_mimetype);
I think the order should be consistent between the overloads. Thus, this should
come before the two new parameters.

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
media/formats/mp2t/es_parser_adts.h:89: bool use_hls_sample_aes_;
ditto on ordering

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
File media/formats/mp2t/es_parser_h264.cc (right):

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
media/formats/mp2t/es_parser_h264.cc:57: const int kSampleAESEncryptBlocks = 1;
Do these only apply to H.264 or should they be somewhere more general?

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
media/formats/mp2t/es_parser_h264.cc:189: #endif
  // BUILDFLAG(ENABLE_HLS_SAMPLE_AES)

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
media/formats/mp2t/es_parser_h264.cc:437:
EncryptionScheme(EncryptionScheme::CIPHER_MODE_AES_CBC,
Since this is fixed (and these constants may be needed elsewhere, I wonder
whether there should be a CreateSampleAESEncryptionScheme().

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
File media/formats/mp2t/es_parser_h264.h (right):

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
media/formats/mp2t/es_parser_h264.h:53: bool use_hls_sample_aes);
ditto on ordering

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
File media/formats/mp2t/mp2t_stream_parser.h (right):

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
media/formats/mp2t/mp2t_stream_parser.h:108: #if
BUILDFLAG(ENABLE_HLS_SAMPLE_AES)
Is this the best object for storing some of these things, especially the
DecryptConfig? Would it make any sense to put them in a separate object to
minimize the number of #ifdefs and intermingling/interdependence with the common
code?

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
media/formats/mp2t/mp2t_stream_parser.h:109: // Register the parser for CA
Table.
s/Register/Set/?

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
media/formats/mp2t/mp2t_stream_parser.h:123: const DecryptConfig*
GetDecryptConfig() { return decrypt_config_.get(); }
See below regarding "external" or other naming. This might apply to multiple of
these.

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
media/formats/mp2t/mp2t_stream_parser.h:164: scoped_ptr<DecryptConfig>
decrypt_config_;
Why does only Sample AES need a config? Perhaps this is an "external" config or
something like that. If so, it might be helpful to specify this in the name.

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
File media/formats/mp2t/mp2t_stream_parser_unittest.cc (right):

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
media/formats/mp2t/mp2t_stream_parser_unittest.cc:308: const
std::vector<uint8_t>& init_data) {
Should the NOTREACHED be #if !BUILDFLAG() instead of removed?

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
File media/formats/mp2t/ts_section_cets_ecm.cc (right):

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
media/formats/mp2t/ts_section_cets_ecm.cc:75: void TsSectionCetsEcm::Flush() {}
Explain why these two are nop?

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
File media/formats/mp2t/ts_section_cets_pssh.cc (right):

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
media/formats/mp2t/ts_section_cets_pssh.cc:44: void TsSectionCetsPssh::Flush()
{}
ditto

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
File media/formats/mp2t/ts_section_pmt.cc (right):

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
media/formats/mp2t/ts_section_pmt.cc:8: #include <string>
unused?

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
File media/formats/mp2t/ts_section_pmt.h (right):

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
media/formats/mp2t/ts_section_pmt.h:8: #include <string>
Not used?

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
media/formats/mp2t/ts_section_pmt.h:21: // RegisterPesCb::Run(int pes_pid, int
stream_type, Descriptors descriptors);
This line is unclear and uses the wrong type for |descriptors| Is this supposed
to be an example of calling?

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
File media/formats/mp2t/ts_section_psi.cc (right):

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
media/formats/mp2t/ts_section_psi.cc:8: #include "base/logging.h"
The empty line seems consistent with other files.

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
File media/formats/mpeg/adts_header_parser.cc (right):

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/formats/m...
media/formats/mpeg/adts_header_parser.cc:12: #include
"media/base/encryption_scheme.h"
Do you need this?

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/media_opt...
File media/media_options.gni (right):

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/media_opt...
media/media_options.gni:47: # Enable HLS with SAMPLE-AES decryption. Actual
decryption must be provided by
Is the second sentence relevant? It's always provided by some CDM
implementation, right?

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/media_opt...
media/media_options.gni:48: # the platform. Enabled by default on the cast
desktop implementation to allow
nit: I suggest separating the explanation of the logic on a new line.

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/test/pipe...
File media/test/pipeline_integration_test.cc (right):

https://chromiumcodereview.appspot.com/1517473002/diff/420001/media/test/pipe...
media/test/pipeline_integration_test.cc:279: // Provides test key in response to
the encrypted event.
nit: insert "the"

[dougsteed, 2016-05-08 23:18:45]

ddorwin@ PTAL. 
"I only skimmed media/formats/mp2t/. You'll need someone to review that code".
You have the background (e.g Part 9) of this work, as do Cast team members (but
they are not OWNERS). Who would you recommend ?

https://codereview.chromium.org/1517473002/diff/420001/media/base/bit_reader.cc
File media/base/bit_reader.cc (right):

https://codereview.chromium.org/1517473002/diff/420001/media/base/bit_reader....
media/base/bit_reader.cc:20: bool BitReader::ReadString(int num_bits,
std::string* str) {
On 2016/04/12 00:40:47, ddorwin wrote:
> Why not just pass in num_bytes?

All the other public functions are in terms of bits, and the class itself is
called "BitReader" so I thought that would be more consistent.

https://codereview.chromium.org/1517473002/diff/420001/media/base/bit_reader....
media/base/bit_reader.cc:22: DCHECK_EQ(num_bits % 8, 0);
On 2016/04/12 00:40:47, ddorwin wrote:
> Move the DCHECKs up to first line - they are checking an input assumption.
Then
> separate with an empty line.

Done.

https://codereview.chromium.org/1517473002/diff/420001/media/base/bit_reader....
media/base/bit_reader.cc:25: char* ptr = &str->front();
On 2016/04/12 00:40:47, ddorwin wrote:
> Then add a note to .h that the num_ value must be at least one byte.

Done.

https://codereview.chromium.org/1517473002/diff/420001/media/base/bit_reader....
media/base/bit_reader.cc:26: while (num_bytes--)
On 2016/04/12 00:40:47, ddorwin wrote:
> With the nesting, it's probably best to add braces for this one.

Done.

https://codereview.chromium.org/1517473002/diff/420001/media/base/bit_reader....
media/base/bit_reader.cc:27: if (!ReadBits(8, ptr++))
On 2016/04/12 00:40:47, ddorwin wrote:
> Should we note in the .h that this does not guarantee the string is valid?

Done.

https://codereview.chromium.org/1517473002/diff/420001/media/base/test_data_u...
File media/base/test_data_util.h (right):

https://codereview.chromium.org/1517473002/diff/420001/media/base/test_data_u...
media/base/test_data_util.h:39: // If the provided |key_id| is that of the test
key, returns true and fills the
On 2016/04/12 00:40:47, ddorwin wrote:
> s/the/a/ - we could support multiple

Done.

https://codereview.chromium.org/1517473002/diff/420001/media/base/test_data_u...
media/base/test_data_util.h:43: bool LookupTestKey(const std::vector<uint8_t>&
key_id,
On 2016/04/12 00:40:47, ddorwin wrote:
> Consider ...Vector (and ...String below)
> https://google.github.io/styleguide/cppguide.html#Function_Overloading

Done.

https://codereview.chromium.org/1517473002/diff/420001/media/base/test_data_u...
media/base/test_data_util.h:45: bool allowRotation);
On 2016/04/12 00:40:47, ddorwin wrote:
> This should be before the output parameter.

Done.

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/des...
File media/formats/mp2t/descriptors.cc (right):

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/des...
media/formats/mp2t/descriptors.cc:85: RCHECK(extra_bits % 8 == 0);
On 2016/04/12 00:40:47, ddorwin wrote:
> You might also need to check that it's > 0 if you assume that in ReadString()

Done.

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/es_...
File media/formats/mp2t/es_parser_adts.cc (right):

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/es_...
media/formats/mp2t/es_parser_adts.cc:138:
use_hls_sample_aes_(use_hls_sample_aes),
On 2016/04/12 00:40:47, ddorwin wrote:
> DCHECK_EQ(get_decrypt_config_cb_, use_hls_sample_aes_) ?

Done.

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/es_...
File media/formats/mp2t/es_parser_adts.h (right):

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/es_...
media/formats/mp2t/es_parser_adts.h:45: bool use_hls_sample_aes,
On 2016/04/12 00:40:47, ddorwin wrote:
> nit: suggest putting this first since it controls the use of the cb_, right?

Done.

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/es_...
media/formats/mp2t/es_parser_adts.h:46: bool sbr_in_mimetype);
On 2016/04/12 00:40:47, ddorwin wrote:
> I think the order should be consistent between the overloads. Thus, this
should
> come before the two new parameters.

Done.

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/es_...
media/formats/mp2t/es_parser_adts.h:89: bool use_hls_sample_aes_;
On 2016/04/12 00:40:47, ddorwin wrote:
> ditto on ordering

Done.

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/es_...
File media/formats/mp2t/es_parser_h264.cc (right):

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/es_...
media/formats/mp2t/es_parser_h264.cc:57: const int kSampleAESEncryptBlocks = 1;
On 2016/04/12 00:40:47, ddorwin wrote:
> Do these only apply to H.264 or should they be somewhere more general?

Only H.264

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/es_...
media/formats/mp2t/es_parser_h264.cc:189: #endif
On 2016/04/12 00:40:47, ddorwin wrote:
>   // BUILDFLAG(ENABLE_HLS_SAMPLE_AES)

Done.

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/es_...
media/formats/mp2t/es_parser_h264.cc:437:
EncryptionScheme(EncryptionScheme::CIPHER_MODE_AES_CBC,
On 2016/04/12 00:40:47, ddorwin wrote:
> Since this is fixed (and these constants may be needed elsewhere, I wonder
> whether there should be a CreateSampleAESEncryptionScheme().

This is currently the only place it's needed. Maybe wait until we see if it is
needed elsewhere before we factor it out?

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/es_...
File media/formats/mp2t/es_parser_h264.h (right):

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/es_...
media/formats/mp2t/es_parser_h264.h:53: bool use_hls_sample_aes);
On 2016/04/12 00:40:47, ddorwin wrote:
> ditto on ordering

Done.

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/mp2...
File media/formats/mp2t/mp2t_stream_parser.h (right):

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/mp2...
media/formats/mp2t/mp2t_stream_parser.h:108: #if
BUILDFLAG(ENABLE_HLS_SAMPLE_AES)
On 2016/04/12 00:40:48, ddorwin wrote:
> Is this the best object for storing some of these things, especially the
> DecryptConfig? Would it make any sense to put them in a separate object to
> minimize the number of #ifdefs and intermingling/interdependence with the
common
> code?
I've explored this a little, and it could be done. My approach would be to
define a Mpt2StreamParserCenc class that has the definitions and state
associated with Part 9. An Mp2tStreamParser instance would contain a
Mps2tStreamParserCenc instance if necessary. However, because of the way that
the CENC PIDs and TsSections need to be defined, this approach would require
some of the implementation details of Mp2tStreamParser (including StreamType and
PidState) to be factored out so they are available to both classes. Not sure if
that is a net gain worthwhile.

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/mp2...
media/formats/mp2t/mp2t_stream_parser.h:109: // Register the parser for CA
Table.
On 2016/04/12 00:40:47, ddorwin wrote:
> s/Register/Set/?

Was using the existing terminology q.v. RegisterPmt() and RegisterPes()

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/mp2...
media/formats/mp2t/mp2t_stream_parser.h:164: scoped_ptr<DecryptConfig>
decrypt_config_;
On 2016/04/12 00:40:48, ddorwin wrote:
> Why does only Sample AES need a config? Perhaps this is an "external" config
or
> something like that. If so, it might be helpful to specify this in the name.

It's not really "external", it is derived from information conveyed in an
CENC-ECM packet. Eventually this will be needed when Part 9 is fully
implemented. For now it is only needed for Sample AES. I thought about changing
the name to GetCurrentDecryptoConfig or GetCencDecryptConfig, but concluded that
I like the current name better.

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/mp2...
File media/formats/mp2t/mp2t_stream_parser_unittest.cc (right):

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/mp2...
media/formats/mp2t/mp2t_stream_parser_unittest.cc:308: const
std::vector<uint8_t>& init_data) {
On 2016/04/12 00:40:48, ddorwin wrote:
> Should the NOTREACHED be #if !BUILDFLAG() instead of removed?

Done.

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/ts_...
File media/formats/mp2t/ts_section_cets_ecm.cc (right):

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/ts_...
media/formats/mp2t/ts_section_cets_ecm.cc:75: void TsSectionCetsEcm::Flush() {}
On 2016/04/12 00:40:48, ddorwin wrote:
> Explain why these two are nop?

Done.

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/ts_...
File media/formats/mp2t/ts_section_cets_pssh.cc (right):

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/ts_...
media/formats/mp2t/ts_section_cets_pssh.cc:44: void TsSectionCetsPssh::Flush()
{}
On 2016/04/12 00:40:48, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/ts_...
File media/formats/mp2t/ts_section_pmt.cc (right):

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/ts_...
media/formats/mp2t/ts_section_pmt.cc:8: #include <string>
On 2016/04/12 00:40:48, ddorwin wrote:
> unused?

Done.

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/ts_...
File media/formats/mp2t/ts_section_pmt.h (right):

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/ts_...
media/formats/mp2t/ts_section_pmt.h:8: #include <string>
On 2016/04/12 00:40:48, ddorwin wrote:
> Not used?

Done.

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/ts_...
media/formats/mp2t/ts_section_pmt.h:21: // RegisterPesCb::Run(int pes_pid, int
stream_type, Descriptors descriptors);
On 2016/04/12 00:40:48, ddorwin wrote:
> This line is unclear and uses the wrong type for |descriptors| Is this
supposed
> to be an example of calling?

Done.

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/ts_...
File media/formats/mp2t/ts_section_psi.cc (right):

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mp2t/ts_...
media/formats/mp2t/ts_section_psi.cc:8: #include "base/logging.h"
On 2016/04/12 00:40:48, ddorwin wrote:
> The empty line seems consistent with other files.

Done.

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mpeg/adt...
File media/formats/mpeg/adts_header_parser.cc (right):

https://codereview.chromium.org/1517473002/diff/420001/media/formats/mpeg/adt...
media/formats/mpeg/adts_header_parser.cc:12: #include
"media/base/encryption_scheme.h"
On 2016/04/12 00:40:48, ddorwin wrote:
> Do you need this?

Since I use "EncryptionScheme" explicitly in this file, I think it is better to
include it.

https://codereview.chromium.org/1517473002/diff/420001/media/media_options.gni
File media/media_options.gni (right):

https://codereview.chromium.org/1517473002/diff/420001/media/media_options.gn...
media/media_options.gni:47: # Enable HLS with SAMPLE-AES decryption. Actual
decryption must be provided by
On 2016/04/12 00:40:48, ddorwin wrote:
> Is the second sentence relevant? It's always provided by some CDM
> implementation, right?

Done.

https://codereview.chromium.org/1517473002/diff/420001/media/media_options.gn...
media/media_options.gni:48: # the platform. Enabled by default on the cast
desktop implementation to allow
On 2016/04/12 00:40:48, ddorwin wrote:
> nit: I suggest separating the explanation of the logic on a new line.

Done.

https://codereview.chromium.org/1517473002/diff/420001/media/test/pipeline_in...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1517473002/diff/420001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:279: // Provides test key in response to
the encrypted event.
On 2016/04/12 00:40:48, ddorwin wrote:
> nit: insert "the"

Done.

[ddorwin, 2016-05-18 20:06:51]

LG.

On 2016/05/08 23:18:45, dougsteed wrote:
> ddorwin@ PTAL. 
> "I only skimmed media/formats/mp2t/. You'll need someone to review that code".
> You have the background (e.g Part 9) of this work, as do Cast team members
(but
> they are not OWNERS). Who would you recommend ?

I can approve it, but I didn't review all the logic. You should have someone on
the Cast team do a detailed review.

https://codereview.chromium.org/1517473002/diff/440001/media/formats/mp2t/des...
File media/formats/mp2t/descriptors.h (right):

https://codereview.chromium.org/1517473002/diff/440001/media/formats/mp2t/des...
media/formats/mp2t/descriptors.h:26: Descriptors(const Descriptors& other);
OOC, why did this become necessary?

https://codereview.chromium.org/1517473002/diff/440001/media/formats/mp2t/es_...
File media/formats/mp2t/es_parser_adts.cc (right):

https://codereview.chromium.org/1517473002/diff/440001/media/formats/mp2t/es_...
media/formats/mp2t/es_parser_adts.cc:139: sbr_in_mimetype_(sbr_in_mimetype) {
nit: order

https://codereview.chromium.org/1517473002/diff/440001/media/formats/mp2t/ts_...
File media/formats/mp2t/ts_section_cets_ecm.cc (right):

https://codereview.chromium.org/1517473002/diff/440001/media/formats/mp2t/ts_...
media/formats/mp2t/ts_section_cets_ecm.cc:76: // no pending state.
nits: Capital N's

https://codereview.chromium.org/1517473002/diff/440001/media/formats/mp2t/ts_...
File media/formats/mp2t/ts_section_cets_pssh.cc (right):

https://codereview.chromium.org/1517473002/diff/440001/media/formats/mp2t/ts_...
media/formats/mp2t/ts_section_cets_pssh.cc:24: // ignore if doesn't contain
PUSI.
nit: Capital 'I'.

https://codereview.chromium.org/1517473002/diff/440001/media/formats/mp2t/ts_...
media/formats/mp2t/ts_section_cets_pssh.cc:48: // no pending state.
ditto

[dougsteed, 2016-05-26 02:33:15]

https://chromiumcodereview.appspot.com/1517473002/diff/440001/media/formats/m...
File media/formats/mp2t/descriptors.h (right):

https://chromiumcodereview.appspot.com/1517473002/diff/440001/media/formats/m...
media/formats/mp2t/descriptors.h:26: Descriptors(const Descriptors& other);
On 2016/05/18 20:06:51, ddorwin wrote:
> OOC, why did this become necessary?

I think this was a style guide thing. Descriptors is a non-POD class that needs
a copy constructor. It will be provided one automatically, but I think the
automatic one will be inlined. By making it explicit, and defining it using "=
default" in the .cc file, it is not inlined.

https://chromiumcodereview.appspot.com/1517473002/diff/440001/media/formats/m...
File media/formats/mp2t/es_parser_adts.cc (right):

https://chromiumcodereview.appspot.com/1517473002/diff/440001/media/formats/m...
media/formats/mp2t/es_parser_adts.cc:139: sbr_in_mimetype_(sbr_in_mimetype) {
On 2016/05/18 20:06:51, ddorwin wrote:
> nit: order

This reflects the declaration order in the .h file. That in turn arose from my
goal to keep the callbacks together.

https://chromiumcodereview.appspot.com/1517473002/diff/440001/media/formats/m...
File media/formats/mp2t/ts_section_cets_ecm.cc (right):

https://chromiumcodereview.appspot.com/1517473002/diff/440001/media/formats/m...
media/formats/mp2t/ts_section_cets_ecm.cc:76: // no pending state.
On 2016/05/18 20:06:51, ddorwin wrote:
> nits: Capital N's

Done.

https://chromiumcodereview.appspot.com/1517473002/diff/440001/media/formats/m...
File media/formats/mp2t/ts_section_cets_pssh.cc (right):

https://chromiumcodereview.appspot.com/1517473002/diff/440001/media/formats/m...
media/formats/mp2t/ts_section_cets_pssh.cc:24: // ignore if doesn't contain
PUSI.
On 2016/05/18 20:06:51, ddorwin wrote:
> nit: Capital 'I'.

Done.

https://chromiumcodereview.appspot.com/1517473002/diff/440001/media/formats/m...
media/formats/mp2t/ts_section_cets_pssh.cc:48: // no pending state.
On 2016/05/18 20:06:51, ddorwin wrote:
> ditto

Done.

[yucliu1, 2016-05-26 23:28:30]

On 2016/05/26 02:33:15, dougsteed wrote:
>
https://chromiumcodereview.appspot.com/1517473002/diff/440001/media/formats/m...
> File media/formats/mp2t/descriptors.h (right):
> 
>
https://chromiumcodereview.appspot.com/1517473002/diff/440001/media/formats/m...
> media/formats/mp2t/descriptors.h:26: Descriptors(const Descriptors& other);
> On 2016/05/18 20:06:51, ddorwin wrote:
> > OOC, why did this become necessary?
> 
> I think this was a style guide thing. Descriptors is a non-POD class that
needs
> a copy constructor. It will be provided one automatically, but I think the
> automatic one will be inlined. By making it explicit, and defining it using "=
> default" in the .cc file, it is not inlined.
> 
>
https://chromiumcodereview.appspot.com/1517473002/diff/440001/media/formats/m...
> File media/formats/mp2t/es_parser_adts.cc (right):
> 
>
https://chromiumcodereview.appspot.com/1517473002/diff/440001/media/formats/m...
> media/formats/mp2t/es_parser_adts.cc:139: sbr_in_mimetype_(sbr_in_mimetype) {
> On 2016/05/18 20:06:51, ddorwin wrote:
> > nit: order
> 
> This reflects the declaration order in the .h file. That in turn arose from my
> goal to keep the callbacks together.
> 
>
https://chromiumcodereview.appspot.com/1517473002/diff/440001/media/formats/m...
> File media/formats/mp2t/ts_section_cets_ecm.cc (right):
> 
>
https://chromiumcodereview.appspot.com/1517473002/diff/440001/media/formats/m...
> media/formats/mp2t/ts_section_cets_ecm.cc:76: // no pending state.
> On 2016/05/18 20:06:51, ddorwin wrote:
> > nits: Capital N's
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/1517473002/diff/440001/media/formats/m...
> File media/formats/mp2t/ts_section_cets_pssh.cc (right):
> 
>
https://chromiumcodereview.appspot.com/1517473002/diff/440001/media/formats/m...
> media/formats/mp2t/ts_section_cets_pssh.cc:24: // ignore if doesn't contain
> PUSI.
> On 2016/05/18 20:06:51, ddorwin wrote:
> > nit: Capital 'I'.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/1517473002/diff/440001/media/formats/m...
> media/formats/mp2t/ts_section_cets_pssh.cc:48: // no pending state.
> On 2016/05/18 20:06:51, ddorwin wrote:
> > ditto
> 
> Done.

lgtm % nit

[yucliu1, 2016-05-26 23:28:47]

https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/des...
File media/formats/mp2t/descriptors.cc (right):

https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/des...
media/formats/mp2t/descriptors.cc:69: descriptors_.insert(Descriptor(tag,
std::string(data, length)));
nit: Use 'emplace' instead?

https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/mp2...
File media/formats/mp2t/mp2t_stream_parser.cc (right):

https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/mp2...
media/formats/mp2t/mp2t_stream_parser.cc:758:
.insert(PidMapElement(TsSection::kPidCat, cat_pid_state.release()))
nit: Replace insert with emplace?

And I think pids_ should be the owner of cat_pid_state, right? If so, PidMap can
be declared as std::map<int, std::unique_ptr<PidState>>. But given PidMap isn't
defined in this patch, it's better to have a separate CL.

https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/mp2...
media/formats/mp2t/mp2t_stream_parser.cc:778: pids_.insert(PidMapElement(ca_pid,
ecm_pid_state.release()));
ditto.

https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/mp2...
media/formats/mp2t/mp2t_stream_parser.cc:786:
pids_.insert(PidMapElement(pssh_pid, pssh_pid_state.release()));
ditto.

https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/mp2...
File media/formats/mp2t/mp2t_stream_parser.h (right):

https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/mp2...
media/formats/mp2t/mp2t_stream_parser.h:54: using PidMapElement = std::pair<int,
PidState*>;
Probably we can remove the define here if 'insert' is replaced with 'emplace'.

[ddorwin, 2016-06-23 19:42:01]

Minor stuff, plus please address yucliu1's comments. Other than that, L G T M.

https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/es_...
File media/formats/mp2t/es_parser_adts.cc (right):

https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/es_...
media/formats/mp2t/es_parser_adts.cc:132: bool sbr_in_mimetype,
On 2016/05/26 02:33:15, dougsteed wrote:
> On 2016/05/18 20:06:51, ddorwin wrote:
> > nit: order
> 
> This reflects the declaration order in the .h file. That in turn arose from my
> goal to keep the callbacks together.

Then maybe this order should be changed since you have this in the middle of
your callbacks.

https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/es_...
media/formats/mp2t/es_parser_adts.cc:269: ?
audio_decoder_config.samples_per_second() / 2
Can this be odd? Do you care?

https://codereview.chromium.org/1517473002/diff/480001/media/test/pipeline_in...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1517473002/diff/480001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:1770: #if
BUILDFLAG(ENABLE_MSE_MPEG2TS_STREAM_PARSER)
It would be better if we could always run this test but expect a failure. See,
for example, the HEVC tests in ToT.

https://codereview.chromium.org/1517473002/diff/480001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:1781: // When SBR is not taken into
account correctly by mpeg2ts parser, it will
[I guess this was added in another CL, but I'm only seeing it now (via the
rebase).]

Does/can this happen or does this describe why you are checking this. If the
latter, please reword.

https://codereview.chromium.org/1517473002/diff/480001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:1783: // buffered ranges (so this check
will fail) and stalled playback.
"stall"

[yucliu1, 2016-06-23 20:30:55]

https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/es_...
File media/formats/mp2t/es_parser_adts.cc (right):

https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/es_...
media/formats/mp2t/es_parser_adts.cc:269: ?
audio_decoder_config.samples_per_second() / 2
On 2016/06/23 19:42:00, ddorwin wrote:
> Can this be odd? Do you care?

This is changed in recent CL: https://codereview.chromium.org/2063443002/.
ParseAdtsHeader will return the original sample rate.

[dougsteed, 2016-09-25 21:52:30]

yucliu1@: sorry for the long delay. Can you check one more time, and if it is OK
with you I will merge. ddorwin@ already LGTM'd and left you with the last word.

https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/des...
File media/formats/mp2t/descriptors.cc (right):

https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/des...
media/formats/mp2t/descriptors.cc:69: descriptors_.insert(Descriptor(tag,
std::string(data, length)));
On 2016/05/26 23:28:47, yucliu1 wrote:
> nit: Use 'emplace' instead?

std::map::emplace is not yet supported.

https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/es_...
File media/formats/mp2t/es_parser_adts.cc (right):

https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/es_...
media/formats/mp2t/es_parser_adts.cc:132: bool sbr_in_mimetype,
On 2016/06/23 19:42:01, ddorwin wrote:
> On 2016/05/26 02:33:15, dougsteed wrote:
> > On 2016/05/18 20:06:51, ddorwin wrote:
> > > nit: order
> > 
> > This reflects the declaration order in the .h file. That in turn arose from
my
> > goal to keep the callbacks together.
> 
> Then maybe this order should be changed since you have this in the middle of
> your callbacks.

Yes my justification turned out to be a bit incoherent. I'm sure I meant
something at the time. Done.

https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/mp2...
File media/formats/mp2t/mp2t_stream_parser.cc (right):

https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/mp2...
media/formats/mp2t/mp2t_stream_parser.cc:758:
.insert(PidMapElement(TsSection::kPidCat, cat_pid_state.release()))
On 2016/05/26 23:28:47, yucliu1 wrote:
> nit: Replace insert with emplace?
> 
> And I think pids_ should be the owner of cat_pid_state, right? If so, PidMap
can
> be declared as std::map<int, std::unique_ptr<PidState>>. But given PidMap
isn't
> defined in this patch, it's better to have a separate CL. 

Can't replace insert with emplace. Agree with the second point though.

https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/mp2...
media/formats/mp2t/mp2t_stream_parser.cc:778: pids_.insert(PidMapElement(ca_pid,
ecm_pid_state.release()));
On 2016/05/26 23:28:47, yucliu1 wrote:
> ditto.

ditto

https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/mp2...
media/formats/mp2t/mp2t_stream_parser.cc:786:
pids_.insert(PidMapElement(pssh_pid, pssh_pid_state.release()));
On 2016/05/26 23:28:47, yucliu1 wrote:
> ditto.

ditto

https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/mp2...
File media/formats/mp2t/mp2t_stream_parser.h (right):

https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/mp2...
media/formats/mp2t/mp2t_stream_parser.h:54: using PidMapElement = std::pair<int,
PidState*>;
On 2016/05/26 23:28:47, yucliu1 wrote:
> Probably we can remove the define here if 'insert' is replaced with 'emplace'.

Sadly map::emplace is not yet supported.

https://codereview.chromium.org/1517473002/diff/480001/media/test/pipeline_in...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1517473002/diff/480001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:1770: #if
BUILDFLAG(ENABLE_MSE_MPEG2TS_STREAM_PARSER)
On 2016/06/23 19:42:01, ddorwin wrote:
> It would be better if we could always run this test but expect a failure. See,
> for example, the HEVC tests in ToT.

As you mention below, this was an unrelated change by servolk@. I will inform
him so he can consider these three comments.

https://codereview.chromium.org/1517473002/diff/480001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:1781: // When SBR is not taken into
account correctly by mpeg2ts parser, it will
On 2016/06/23 19:42:01, ddorwin wrote:
> [I guess this was added in another CL, but I'm only seeing it now (via the
> rebase).]
> 
> Does/can this happen or does this describe why you are checking this. If the
> latter, please reword.

will inform servolk@

https://codereview.chromium.org/1517473002/diff/480001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:1783: // buffered ranges (so this check
will fail) and stalled playback.
On 2016/06/23 19:42:01, ddorwin wrote:
> "stall"

ditto

[yucliu1, 2016-09-25 22:29:20]

On 2016/09/25 21:52:30, dougsteed wrote:
> yucliu1@: sorry for the long delay. Can you check one more time, and if it is
OK
> with you I will merge. ddorwin@ already LGTM'd and left you with the last
word.
> 
>
https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/des...
> File media/formats/mp2t/descriptors.cc (right):
> 
>
https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/des...
> media/formats/mp2t/descriptors.cc:69: descriptors_.insert(Descriptor(tag,
> std::string(data, length)));
> On 2016/05/26 23:28:47, yucliu1 wrote:
> > nit: Use 'emplace' instead?
> 
> std::map::emplace is not yet supported.
> 
>
https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/es_...
> File media/formats/mp2t/es_parser_adts.cc (right):
> 
>
https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/es_...
> media/formats/mp2t/es_parser_adts.cc:132: bool sbr_in_mimetype,
> On 2016/06/23 19:42:01, ddorwin wrote:
> > On 2016/05/26 02:33:15, dougsteed wrote:
> > > On 2016/05/18 20:06:51, ddorwin wrote:
> > > > nit: order
> > > 
> > > This reflects the declaration order in the .h file. That in turn arose
from
> my
> > > goal to keep the callbacks together.
> > 
> > Then maybe this order should be changed since you have this in the middle of
> > your callbacks.
> 
> Yes my justification turned out to be a bit incoherent. I'm sure I meant
> something at the time. Done.
> 
>
https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/mp2...
> File media/formats/mp2t/mp2t_stream_parser.cc (right):
> 
>
https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/mp2...
> media/formats/mp2t/mp2t_stream_parser.cc:758:
> .insert(PidMapElement(TsSection::kPidCat, cat_pid_state.release()))
> On 2016/05/26 23:28:47, yucliu1 wrote:
> > nit: Replace insert with emplace?
> > 
> > And I think pids_ should be the owner of cat_pid_state, right? If so, PidMap
> can
> > be declared as std::map<int, std::unique_ptr<PidState>>. But given PidMap
> isn't
> > defined in this patch, it's better to have a separate CL. 
> 
> Can't replace insert with emplace. Agree with the second point though.
> 
>
https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/mp2...
> media/formats/mp2t/mp2t_stream_parser.cc:778:
pids_.insert(PidMapElement(ca_pid,
> ecm_pid_state.release()));
> On 2016/05/26 23:28:47, yucliu1 wrote:
> > ditto.
> 
> ditto
> 
>
https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/mp2...
> media/formats/mp2t/mp2t_stream_parser.cc:786:
> pids_.insert(PidMapElement(pssh_pid, pssh_pid_state.release()));
> On 2016/05/26 23:28:47, yucliu1 wrote:
> > ditto.
> 
> ditto
> 
>
https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/mp2...
> File media/formats/mp2t/mp2t_stream_parser.h (right):
> 
>
https://codereview.chromium.org/1517473002/diff/480001/media/formats/mp2t/mp2...
> media/formats/mp2t/mp2t_stream_parser.h:54: using PidMapElement =
std::pair<int,
> PidState*>;
> On 2016/05/26 23:28:47, yucliu1 wrote:
> > Probably we can remove the define here if 'insert' is replaced with
'emplace'.
> 
> Sadly map::emplace is not yet supported.
> 
>
https://codereview.chromium.org/1517473002/diff/480001/media/test/pipeline_in...
> File media/test/pipeline_integration_test.cc (right):
> 
>
https://codereview.chromium.org/1517473002/diff/480001/media/test/pipeline_in...
> media/test/pipeline_integration_test.cc:1770: #if
> BUILDFLAG(ENABLE_MSE_MPEG2TS_STREAM_PARSER)
> On 2016/06/23 19:42:01, ddorwin wrote:
> > It would be better if we could always run this test but expect a failure.
See,
> > for example, the HEVC tests in ToT.
> 
> As you mention below, this was an unrelated change by servolk@. I will inform
> him so he can consider these three comments.
> 
>
https://codereview.chromium.org/1517473002/diff/480001/media/test/pipeline_in...
> media/test/pipeline_integration_test.cc:1781: // When SBR is not taken into
> account correctly by mpeg2ts parser, it will
> On 2016/06/23 19:42:01, ddorwin wrote:
> > [I guess this was added in another CL, but I'm only seeing it now (via the
> > rebase).]
> > 
> > Does/can this happen or does this describe why you are checking this. If the
> > latter, please reword.
> 
> will inform servolk@
> 
>
https://codereview.chromium.org/1517473002/diff/480001/media/test/pipeline_in...
> media/test/pipeline_integration_test.cc:1783: // buffered ranges (so this
check
> will fail) and stalled playback.
> On 2016/06/23 19:42:01, ddorwin wrote:
> > "stall"
> 
> ditto

lgtm

[servolk, 2016-09-26 23:15:19]

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

[servolk, 2016-09-26 23:15:21]

https://codereview.chromium.org/1517473002/diff/480001/media/test/pipeline_in...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1517473002/diff/480001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:1781: // When SBR is not taken into
account correctly by mpeg2ts parser, it will
On 2016/06/23 19:42:01, ddorwin wrote:
> [I guess this was added in another CL, but I'm only seeing it now (via the
> rebase).]
> 
> Does/can this happen or does this describe why you are checking this. If the
> latter, please reword.

It's both the former and the latter. This did happen in the past, but this test
will enforce that to not happen again.
This comment explains why we are checking GetBufferedTimeRanges() here. We've
seen that when SBR is not taken into account properly, then audio parser emits
buffers with incorrect durations that are too short and that leads to buffered
time ranges having gaps. Per MSE spec gaps in buffered ranges must stall
playback, so I've tried to explain here that incorrect treatment of SBR on
mpeg2ts parser level leads to gaps and that, in turn, leads to stalled playback.
I'll try to reword to make it clearer.

https://codereview.chromium.org/1517473002/diff/480001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:1783: // buffered ranges (so this check
will fail) and stalled playback.
On 2016/06/23 19:42:01, ddorwin wrote:
> "stall"

I actually meant to say here that "... incorrect durations lead to ... gaps and
[cause] stalled playback".

[dougsteed, 2016-10-07 17:35:51]

yucliu@. Once more, please. Merged in the fix for bug we discovered when testing
with another content provider.

[yucliu1, 2016-10-07 17:45:48]

On 2016/10/07 17:35:51, dougsteed wrote:
> yucliu@. Once more, please. Merged in the fix for bug we discovered when
testing
> with another content provider.

lgtm

[dougsteed, 2016-10-12 18:08:05]

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

[commit-bot: I haz the power, 2016-10-12 18:08:31]

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

[commit-bot: I haz the power, 2016-10-12 20:30:10]

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

[commit-bot: I haz the power, 2016-10-12 20:30:12]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dougsteed, 2016-10-13 14:40:44]

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

[commit-bot: I haz the power, 2016-10-13 14:40:54]

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

[commit-bot: I haz the power, 2016-10-13 14:43:13]

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

[commit-bot: I haz the power, 2016-10-13 14:43:14]

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

[dougsteed, 2016-10-13 15:01:10]

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

[commit-bot: I haz the power, 2016-10-13 15:01:44]

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

[commit-bot: I haz the power, 2016-10-13 16:17:03]

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

[commit-bot: I haz the power, 2016-10-13 16:17:06]

Dry run: This issue passed the CQ dry run.

[dougsteed, 2016-10-13 17:32:36]

ddorwin@: you seemed to approve this CL contingent on yucliu's endorsement, but
I still need owner approval. Can you convert your "L G T M" to "lgtm" ?

[ddorwin, 2016-11-07 18:20:43]

LGTM

Thank you servolk@ for addressing the discussion in
pipeline_integration_test.cc.

[dougsteed, 2016-11-29 23:17:40]

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

[commit-bot: I haz the power, 2016-11-29 23:18:50]

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

[commit-bot: I haz the power, 2016-11-30 01:22:14]

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

[commit-bot: I haz the power, 2016-11-30 01:22:15]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[dougsteed, 2016-12-02 17:36:59]

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

[commit-bot: I haz the power, 2016-12-02 17:37:20]

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

[commit-bot: I haz the power, 2016-12-02 21:52:15]

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

[commit-bot: I haz the power, 2016-12-02 21:52:17]

Dry run: This issue passed the CQ dry run.

[dougsteed, 2016-12-02 22:25:23]

The CQ bit was checked by dougsteed@chromium.org

[dougsteed, 2016-12-02 22:25:24]

The patchset sent to the CQ was uploaded after l-g-t-m from yucliu@chromium.org,
ddorwin@chromium.org
Link to the patchset: https://codereview.chromium.org/1517473002/#ps680001
(title: "rebase")

[commit-bot: I haz the power, 2016-12-02 22:26:03]

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

[commit-bot: I haz the power, 2016-12-02 22:39:10]

CQ is committing da patch.

Bot data: {"patchset_id": 680001, "attempt_start_ts": 1480717523461650,
"parent_rev": "32164f31d25e4454f90fca3b7f4980cdac192e46", "commit_rev":
"2ea5ad7a4a5140a00059cc38d4433f644993fe86"}

[commit-bot: I haz the power, 2016-12-02 22:39:44]

Description was changed from

==========
The actual encryption is assumed to have been carried out
according to the SAMPLE-AES encryption method cited by the
HLS spec (informational RFC called draft-pantos-http-live-
streaming).

This encryption method is being incorporated in the Common
Encryption specification ISO/IEC 23001-7 3rd Edition as the
'cbcs' scheme.

Parsing of the stream requires it to be prefixed with extra
TS packets that carry encryption metadata. These packets
are formatted according to ISO/IEC 23001-9 "Common
encryption of MPEG-2 transport streams".

This CL is dependent on a prior CL (1490613005) that allows
conveyance of the extended encryption metadata through the
pipeline.

BUG=568326
==========

to

==========
The actual encryption is assumed to have been carried out
according to the SAMPLE-AES encryption method cited by the
HLS spec (informational RFC called draft-pantos-http-live-
streaming).

This encryption method is being incorporated in the Common
Encryption specification ISO/IEC 23001-7 3rd Edition as the
'cbcs' scheme.

Parsing of the stream requires it to be prefixed with extra
TS packets that carry encryption metadata. These packets
are formatted according to ISO/IEC 23001-9 "Common
encryption of MPEG-2 transport streams".

This CL is dependent on a prior CL (1490613005) that allows
conveyance of the extended encryption metadata through the
pipeline.

BUG=568326
==========

[commit-bot: I haz the power, 2016-12-02 22:39:46]

Committed patchset #35 (id:680001)

[commit-bot: I haz the power, 2016-12-02 22:41:24]

Description was changed from

==========
The actual encryption is assumed to have been carried out
according to the SAMPLE-AES encryption method cited by the
HLS spec (informational RFC called draft-pantos-http-live-
streaming).

This encryption method is being incorporated in the Common
Encryption specification ISO/IEC 23001-7 3rd Edition as the
'cbcs' scheme.

Parsing of the stream requires it to be prefixed with extra
TS packets that carry encryption metadata. These packets
are formatted according to ISO/IEC 23001-9 "Common
encryption of MPEG-2 transport streams".

This CL is dependent on a prior CL (1490613005) that allows
conveyance of the extended encryption metadata through the
pipeline.

BUG=568326
==========

to

==========
The actual encryption is assumed to have been carried out
according to the SAMPLE-AES encryption method cited by the
HLS spec (informational RFC called draft-pantos-http-live-
streaming).

This encryption method is being incorporated in the Common
Encryption specification ISO/IEC 23001-7 3rd Edition as the
'cbcs' scheme.

Parsing of the stream requires it to be prefixed with extra
TS packets that carry encryption metadata. These packets
are formatted according to ISO/IEC 23001-9 "Common
encryption of MPEG-2 transport streams".

This CL is dependent on a prior CL (1490613005) that allows
conveyance of the extended encryption metadata through the
pipeline.

BUG=568326

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

[commit-bot: I haz the power, 2016-12-02 22:41:25]

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