Declare intent to use sample-aes later in the stream.

Declare intent to use sample-aes later in the stream.

Allows the declaration in an unencrypted part of a stream of an intent
to use sample-aes encryption later. This allows the decoding backend to
be properly initialized with the appropriate encryption scheme.
This involves prefixing a subset of the encryption metadata that is used
to indicate encryption, but omits the key id and IV.

This will typically be used in a live stream, when tuning in during
an unencrypted part, e.g. a commercial.

BUG=568326
Review-Url: https://chromiumcodereview.appspot.com/2783703002
Cr-Commit-Position: refs/heads/master@{#505415}
Committed: https://chromium.googlesource.com/chromium/src/+/d99d354df109fa70d2cc15d2586d852d278320b2

[dougsteed, 2017-03-28 16:05:50]

Description was changed from

==========
Declare intent to use sample-aes later in the stream.

Allows the declaration in an unencrypted part of a stream of an intent
to use sample-aes encryption later. This allows the decoding backend to
be properly initialized with the appropriate encryption scheme.
This involves prefixing a subset of the encryption metadata that is used
to indicate encryption, but omits the key id and IV.

This will typically be used in a live stream, when tuning in during
an unencrypted part, e.g. a commercial.

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

to

==========
Declare intent to use sample-aes later in the stream.

Allows the declaration in an unencrypted part of a stream of an intent
to use sample-aes encryption later. This allows the decoding backend to
be properly initialized with the appropriate encryption scheme.
This involves prefixing a subset of the encryption metadata that is used
to indicate encryption, but omits the key id and IV.

This will typically be used in a live stream, when tuning in during
an unencrypted part, e.g. a commercial.

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

[dougsteed, 2017-03-28 16:05:50]

dougsteed@chromium.org changed reviewers:
+ servolk@chromium.org, xhwang@google.com, yucliu@google.com

[xhwang, 2017-04-04 19:13:21]

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

[xhwang, 2017-04-04 19:13:21]

I am in the middle of some other work so I'll probably take a high level look
later this week.

kqyang: Could you please take a look first since you are pretty familiar with
the code/context?

[servolk, 2017-04-04 20:08:18]

https://codereview.chromium.org/2783703002/diff/1/media/formats/mp2t/mp2t_str...
File media/formats/mp2t/mp2t_stream_parser.cc (right):

https://codereview.chromium.org/2783703002/diff/1/media/formats/mp2t/mp2t_str...
media/formats/mp2t/mp2t_stream_parser.cc:476: if (stream_type !=
kStreamTypeMpeg1Audio)
&& stream_type != kStreamTypeMpeg2Audio

https://codereview.chromium.org/2783703002/diff/1/media/formats/mp2t/mp2t_str...
File media/formats/mp2t/mp2t_stream_parser.h (right):

https://codereview.chromium.org/2783703002/diff/1/media/formats/mp2t/mp2t_str...
media/formats/mp2t/mp2t_stream_parser.h:76: // |pes_pid| is part of the Program
Map Table..
nit: extra dot at the end

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

https://codereview.chromium.org/2783703002/diff/1/media/test/data/README#newc...
media/test/data/README:104: to have an encryption_scheme (of AES-CBC). The
actual data is unencrypted, which is indicated by the lack of key ID and IV.
This ends upovery similar to how
nit, typo: 'upovery'

[kqyang, 2017-04-07 23:33:44]

https://codereview.chromium.org/2783703002/diff/1/media/formats/mp2t/descript...
File media/formats/mp2t/descriptors.cc (left):

https://codereview.chromium.org/2783703002/diff/1/media/formats/mp2t/descript...
media/formats/mp2t/descriptors.cc:139: RCHECK(reader.ReadBits(13, pssh_pid));
Are you expecting the inserted ca descriptor to contain a valid pssh pid? If
not, how are you going to deal with it? If yes, how do you ensure that?

https://codereview.chromium.org/2783703002/diff/1/media/formats/mp2t/mp2t_str...
File media/formats/mp2t/mp2t_stream_parser.cc (right):

https://codereview.chromium.org/2783703002/diff/1/media/formats/mp2t/mp2t_str...
media/formats/mp2t/mp2t_stream_parser.cc:440: if (is_AAC && !is_initialized_ &&
initial_scheme_.is_encrypted()) {
I understand that Marvel does not support switching encryption schemes, but I
don't understand why the encryption scheme needs to be signaled before starting
playing the clear "commercial". Why cannot it be signaled before playing the
actual encrypted stream? Can you explain?

https://codereview.chromium.org/2783703002/diff/1/media/formats/mp2t/mp2t_str...
media/formats/mp2t/mp2t_stream_parser.cc:511: if (!es_parser)
I think the code would be cleaner and easier to maintain with this:

switch (stream_type) {
  case kStreamTypeAVC:
#if BUILDFLAG(ENABLE_HLS_SAMPLE_AES)
    // Add some comments on when and why could this happen.
    if (!is_initialized_ && initial_scheme_.is_encrypted())) {
      CreateEncryptedH264Parser(pes_pid);
      break;
    }
#endif
    CreateH264Parser(pes_pid);
    break;
#if BUILDFLAG(ENABLE_HLS_SAMPLE_AES)
  case kStreamTypeAVCWithSampleAES:
    if (!descriptors.HasPrivateDataIndicator(
                 kSampleAESPrivateDataIndicatorAVC)) { FAIL ... }
    CreateEncryptedH264Parser(pes_pid);
    break;
#endif

  case kStreamTypeAAC:
#if BUILDFLAG(ENABLE_HLS_SAMPLE_AES)
    // Add some comments on when and why could this happen.
    if (!is_initialized_ && initial_scheme_.is_encrypted())) {
      CreateEncryptedAacParser(pes_pid);
      break;
    }
#endif
    CreateAacParser(pes_pid);
    break;
#if BUILDFLAG(ENABLE_HLS_SAMPLE_AES)
  case kStreamTypeAACWithSampleAES:
    if (!descriptors.HasPrivateDataIndicator(
                 kSampleAESPrivateDataIndicatorAAC)) { FAIL ... }
    CreateEncryptedAacParser(pes_pid);
    break;
#endif
  default:
    FAIL...
}

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

https://codereview.chromium.org/2783703002/diff/1/media/test/data/README#newc...
media/test/data/README:103: bear-1280x720-hls-sample-aes.ts. It's presence
indicates that SAMPLE_AES encryption may occur later in the stream, and causes
the initial audio and video configs
s/It's/Its/

[dougsteed, 2017-04-26 20:34:15]

https://codereview.chromium.org/2783703002/diff/1/media/formats/mp2t/descript...
File media/formats/mp2t/descriptors.cc (left):

https://codereview.chromium.org/2783703002/diff/1/media/formats/mp2t/descript...
media/formats/mp2t/descriptors.cc:139: RCHECK(reader.ReadBits(13, pssh_pid));
On 2017/04/07 23:33:43, kqyang wrote:
> Are you expecting the inserted ca descriptor to contain a valid pssh pid? If
> not, how are you going to deal with it? If yes, how do you ensure that?

The parser expects a 13-bit field which is interpreted as a PID, and will fail
if it's not present. In the scenario captured by this CL, the PSSH PID is not
actually used, i.e. no packet is actually received on that PID. But we still
require the CA descriptor because of other fields we do use.

https://codereview.chromium.org/2783703002/diff/1/media/formats/mp2t/mp2t_str...
File media/formats/mp2t/mp2t_stream_parser.cc (right):

https://codereview.chromium.org/2783703002/diff/1/media/formats/mp2t/mp2t_str...
media/formats/mp2t/mp2t_stream_parser.cc:440: if (is_AAC && !is_initialized_ &&
initial_scheme_.is_encrypted()) {
On 2017/04/07 23:33:43, kqyang wrote:
> I understand that Marvel does not support switching encryption schemes, but I
> don't understand why the encryption scheme needs to be signaled before
starting
> playing the clear "commercial". Why cannot it be signaled before playing the
> actual encrypted stream? Can you explain? 
> 

On this platform, we always need to initialize a decryptor (and get the scheme
correct) at the beginning of the stream, regardless of whether any part (or all)
of the stream is clear.

https://codereview.chromium.org/2783703002/diff/1/media/formats/mp2t/mp2t_str...
media/formats/mp2t/mp2t_stream_parser.cc:476: if (stream_type !=
kStreamTypeMpeg1Audio)
On 2017/04/04 20:08:18, servolk wrote:
> && stream_type != kStreamTypeMpeg2Audio

Done.

https://codereview.chromium.org/2783703002/diff/1/media/formats/mp2t/mp2t_str...
media/formats/mp2t/mp2t_stream_parser.cc:511: if (!es_parser)
On 2017/04/07 23:33:43, kqyang wrote:
> I think the code would be cleaner and easier to maintain with this:
> 
> switch (stream_type) {
>   case kStreamTypeAVC:
> #if BUILDFLAG(ENABLE_HLS_SAMPLE_AES)
>     // Add some comments on when and why could this happen.
>     if (!is_initialized_ && initial_scheme_.is_encrypted())) {
>       CreateEncryptedH264Parser(pes_pid);
>       break;
>     }
> #endif
>     CreateH264Parser(pes_pid);
>     break;
> #if BUILDFLAG(ENABLE_HLS_SAMPLE_AES)
>   case kStreamTypeAVCWithSampleAES:
>     if (!descriptors.HasPrivateDataIndicator(
>                  kSampleAESPrivateDataIndicatorAVC)) { FAIL ... }
>     CreateEncryptedH264Parser(pes_pid);
>     break;
> #endif
> 
>   case kStreamTypeAAC:
> #if BUILDFLAG(ENABLE_HLS_SAMPLE_AES)
>     // Add some comments on when and why could this happen.
>     if (!is_initialized_ && initial_scheme_.is_encrypted())) {
>       CreateEncryptedAacParser(pes_pid);
>       break;
>     }
> #endif
>     CreateAacParser(pes_pid);
>     break;
> #if BUILDFLAG(ENABLE_HLS_SAMPLE_AES)
>   case kStreamTypeAACWithSampleAES:
>     if (!descriptors.HasPrivateDataIndicator(
>                  kSampleAESPrivateDataIndicatorAAC)) { FAIL ... }
>     CreateEncryptedAacParser(pes_pid);
>     break;
> #endif
>   default:
>     FAIL...
> }

Adopted this general approach, but with some variation.

https://codereview.chromium.org/2783703002/diff/1/media/formats/mp2t/mp2t_str...
File media/formats/mp2t/mp2t_stream_parser.h (right):

https://codereview.chromium.org/2783703002/diff/1/media/formats/mp2t/mp2t_str...
media/formats/mp2t/mp2t_stream_parser.h:76: // |pes_pid| is part of the Program
Map Table..
On 2017/04/04 20:08:18, servolk wrote:
> nit: extra dot at the end

Done.

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

https://codereview.chromium.org/2783703002/diff/1/media/test/data/README#newc...
media/test/data/README:103: bear-1280x720-hls-sample-aes.ts. It's presence
indicates that SAMPLE_AES encryption may occur later in the stream, and causes
the initial audio and video configs
On 2017/04/07 23:33:43, kqyang wrote:
> s/It's/Its/

Done.

https://codereview.chromium.org/2783703002/diff/1/media/test/data/README#newc...
media/test/data/README:104: to have an encryption_scheme (of AES-CBC). The
actual data is unencrypted, which is indicated by the lack of key ID and IV.
This ends upovery similar to how
On 2017/04/04 20:08:18, servolk wrote:
> nit, typo: 'upovery'

Done.

[kqyang, 2017-05-06 00:06:12]

Thanks. Code looks good, but still not clear on why the encryption scheme needs
to be signaled before "commercial".

https://codereview.chromium.org/2783703002/diff/1/media/formats/mp2t/mp2t_str...
File media/formats/mp2t/mp2t_stream_parser.cc (right):

https://codereview.chromium.org/2783703002/diff/1/media/formats/mp2t/mp2t_str...
media/formats/mp2t/mp2t_stream_parser.cc:440: if (is_AAC && !is_initialized_ &&
initial_scheme_.is_encrypted()) {
On 2017/04/26 20:34:14, dougsteed wrote:
> On 2017/04/07 23:33:43, kqyang wrote:
> > I understand that Marvel does not support switching encryption schemes, but
I
> > don't understand why the encryption scheme needs to be signaled before
> starting
> > playing the clear "commercial". Why cannot it be signaled before playing the
> > actual encrypted stream? Can you explain? 
> > 
> 
> On this platform, we always need to initialize a decryptor (and get the scheme
> correct) at the beginning of the stream, regardless of whether any part (or
all)
> of the stream is clear.

Is this a limitation of Chromium media pipeline or Chromecast media pipeline?
Also, aren't the commercial and real content two different steams?

[dougsteed, 2017-09-26 17:53:42]

On 2017/05/06 00:06:12, kqyang wrote:
> Thanks. Code looks good, but still not clear on why the encryption scheme
needs
> to be signaled before "commercial".
> 
>
https://codereview.chromium.org/2783703002/diff/1/media/formats/mp2t/mp2t_str...
> File media/formats/mp2t/mp2t_stream_parser.cc (right):
> 
>
https://codereview.chromium.org/2783703002/diff/1/media/formats/mp2t/mp2t_str...
> media/formats/mp2t/mp2t_stream_parser.cc:440: if (is_AAC && !is_initialized_
&&
> initial_scheme_.is_encrypted()) {
> On 2017/04/26 20:34:14, dougsteed wrote:
> > On 2017/04/07 23:33:43, kqyang wrote:
> > > I understand that Marvel does not support switching encryption schemes,
but
> I
> > > don't understand why the encryption scheme needs to be signaled before
> > starting
> > > playing the clear "commercial". Why cannot it be signaled before playing
the
> > > actual encrypted stream? Can you explain? 
> > > 
> > 
> > On this platform, we always need to initialize a decryptor (and get the
scheme
> > correct) at the beginning of the stream, regardless of whether any part (or
> all)
> > of the stream is clear.
> 
> Is this a limitation of Chromium media pipeline or Chromecast media pipeline?
> Also, aren't the commercial and real content two different steams?

Sorry for the long delay in replying. And rietveld about to close!

I think it's mainly a limitation of chromecast pipeline, but this scenario ends
up
being very like the way a clear leader of an otherwise encrypted stream can
occur in MP4,
which is why that has always worked on our pipeline.

No the ad and real content are not separate schemes. This is HLS, so the ads are
inserted as unencrypted segments in the same manifest.

[kqyang, 2017-09-26 20:31:51]

lgtm

[dougsteed, 2017-09-27 21:10:20]

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

[commit-bot: I haz the power, 2017-09-27 21:10:26]

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-09-27 22:24:39]

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

[commit-bot: I haz the power, 2017-09-27 22:24:40]

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

[dougsteed, 2017-09-28 03:56:09]

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

[commit-bot: I haz the power, 2017-09-28 03:56:13]

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-09-28 05:46:04]

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

[commit-bot: I haz the power, 2017-09-28 05:46:06]

Dry run: This issue passed the CQ dry run.

[dougsteed, 2017-09-28 14:56:55]

The CQ bit was checked by dougsteed@chromium.org

[dougsteed, 2017-09-28 14:56:55]

The patchset sent to the CQ was uploaded after l-g-t-m from kqyang@chromium.org
Link to the patchset: https://codereview.chromium.org/2783703002/#ps40001
(title: "tweak needed for flush")

[commit-bot: I haz the power, 2017-09-28 14:57:07]

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-09-28 15:07:52]

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

[commit-bot: I haz the power, 2017-09-28 15:07:53]

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...)

[xhwang, 2017-09-29 00:08:08]

rs lgtm

[dougsteed, 2017-09-29 15:28:39]

The CQ bit was checked by dougsteed@chromium.org

[commit-bot: I haz the power, 2017-09-29 15:28:45]

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-09-29 17:48:40]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1506698919770970,
"parent_rev": "bac09f352af1282d5cd4efc26d0b26a88f75442d", "commit_rev":
"d99d354df109fa70d2cc15d2586d852d278320b2"}

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

Description was changed from

==========
Declare intent to use sample-aes later in the stream.

Allows the declaration in an unencrypted part of a stream of an intent
to use sample-aes encryption later. This allows the decoding backend to
be properly initialized with the appropriate encryption scheme.
This involves prefixing a subset of the encryption metadata that is used
to indicate encryption, but omits the key id and IV.

This will typically be used in a live stream, when tuning in during
an unencrypted part, e.g. a commercial.

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

to

==========
Declare intent to use sample-aes later in the stream.

Allows the declaration in an unencrypted part of a stream of an intent
to use sample-aes encryption later. This allows the decoding backend to
be properly initialized with the appropriate encryption scheme.
This involves prefixing a subset of the encryption metadata that is used
to indicate encryption, but omits the key id and IV.

This will typically be used in a live stream, when tuning in during
an unencrypted part, e.g. a commercial.

BUG=568326

Review-Url: https://chromiumcodereview.appspot.com/2783703002
Cr-Commit-Position: refs/heads/master@{#505415}
Committed:
https://chromium.googlesource.com/chromium/src/+/d99d354df109fa70d2cc15d2586d...
==========

[commit-bot: I haz the power, 2017-09-29 17:49:03]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/d99d354df109fa70d2cc15d2586d...