media: Allow config change between clear and encrypted streams

media: Allow config change between clear and encrypted streams

In the demuxer, we allow DecryptConfig change upon config change,
including but not limited to:
- switching between clear and encrypted
- encryption scheme change

Media Renderer implementation should support such changes. The detailed
requirement from the spec's perspective is tracked at:
https://github.com/w3c/encrypted-media/issues/251

Currently the default media Renderer (RendererImpl) supports switching
from encrypted to clear, because:
- Decrypt-and-decode mode: Decrypting{Audio|Video}Decoder supports clear
  buffer.
- Decrypt-only mode: DecryptingDemuxerStream supports clear buffer.

However, switching from clear to encrypted is not supported in
RendererImpl, because the clear decoder doesn't support decryption. This
will be fixed in a later CL.

BUG=597443
TEST=Updated pipeline_integration_tests.

Review-Url: https://codereview.chromium.org/2543623003
Cr-Commit-Position: refs/heads/master@{#451212}
Committed: https://chromium.googlesource.com/chromium/src/+/65c23034a1deaa9b221ef98928925bee36304fd6

[xhwang, 2016-12-01 08:28:32]

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

[commit-bot: I haz the power, 2016-12-01 08:28: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-12-01 10:16:56]

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

[commit-bot: I haz the power, 2016-12-01 10:16:56]

Dry run: This issue passed the CQ dry run.

[xhwang, 2016-12-01 20:30:32]

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

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

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

[xhwang, 2016-12-01 20:36:15]

xhwang@chromium.org changed reviewers:
+ ddorwin@chromium.org, dougsteed@chromium.org, joeyparrish@chromium.org

[xhwang, 2016-12-01 20:36:15]

xhwang@chromium.org changed reviewers:
+ ddorwin@chromium.org, dougsteed@chromium.org, joeyparrish@chromium.org

[xhwang, 2016-12-01 20:36:16]

This CL is working but I need to add more tests. Before that could you please
take a quick look at the high level changes.

The biggest change is in the demuxer where we allow all changes regarding the
DecryptConfig. In the bug, Joey proposed that we allow switching b/w encrypted
and clear, but disallow switching b/w encryption schemes, e.g. cbc to ctr. But I
feel it's cleaner to just let one entity (the media Renderer) to decide what is
supported or not.

[xhwang, 2016-12-01 20:36:16]

This CL is working but I need to add more tests. Before that could you please
take a quick look at the high level changes.

The biggest change is in the demuxer where we allow all changes regarding the
DecryptConfig. In the bug, Joey proposed that we allow switching b/w encrypted
and clear, but disallow switching b/w encryption schemes, e.g. cbc to ctr. But I
feel it's cleaner to just let one entity (the media Renderer) to decide what is
supported or not.

[xhwang, 2016-12-01 20:38:04]

In RendererImpl, long term, we should support switching decoders. I look at the
current code and supporting that shouldn't be hard, and will have other
benefits. I'll write a design doc sometime but I assume that is not urgent.

[commit-bot: I haz the power, 2016-12-01 20:56:50]

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

[commit-bot: I haz the power, 2016-12-01 20:56:51]

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

[xhwang, 2016-12-01 21:38:20]

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

[commit-bot: I haz the power, 2016-12-01 21:38:55]

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

[Joey Parrish, 2016-12-01 22:28:09]

joeyparrish@google.com changed reviewers:
+ joeyparrish@google.com

[Joey Parrish, 2016-12-01 22:28:10]

LGTM otherwise

https://codereview.chromium.org/2543623003/diff/40001/media/filters/decryptin...
File media/filters/decrypting_audio_decoder.cc (right):

https://codereview.chromium.org/2543623003/diff/40001/media/filters/decryptin...
media/filters/decrypting_audio_decoder.cc:76: DCHECK(config.is_encrypted());
This is protecting the renderer?  Might be good to have a comment to that
effect, since it appears to be protecting the decoder.

[commit-bot: I haz the power, 2016-12-01 23:09:47]

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

[commit-bot: I haz the power, 2016-12-01 23:09:48]

Dry run: This issue passed the CQ dry run.

[ddorwin, 2016-12-05 23:53:04]

I reviewed the code. I have more general concerns about only implementing one
direction and not requiring it for all implementations. We should also have
browser tests or similar, including for both orderings of setMediaKeys() and
setting the source.

https://codereview.chromium.org/2543623003/diff/40001/media/filters/decryptin...
File media/filters/decrypting_audio_decoder.cc (right):

https://codereview.chromium.org/2543623003/diff/40001/media/filters/decryptin...
media/filters/decrypting_audio_decoder.cc:76: DCHECK(config.is_encrypted());
There should probably be a comment/TODO with the crbug.

https://codereview.chromium.org/2543623003/diff/40001/media/filters/decryptin...
media/filters/decrypting_audio_decoder.cc:86: // Reinitialization (i.e. upon a
config change, the new config can be
The new text seems like a separate thought and maybe a new sentence outside the
().

https://codereview.chromium.org/2543623003/diff/40001/media/filters/decryptin...
File media/filters/decrypting_audio_decoder_unittest.cc (right):

https://codereview.chromium.org/2543623003/diff/40001/media/filters/decryptin...
media/filters/decrypting_audio_decoder_unittest.cc:362: // TODO(xhwang): Add a
test for switching from encrypted to clear.
Can we at least keep the previous test (but expect true) and have a TODO to
improve it?

https://codereview.chromium.org/2543623003/diff/40001/media/filters/decryptin...
File media/filters/decrypting_demuxer_stream.cc (right):

https://codereview.chromium.org/2543623003/diff/40001/media/filters/decryptin...
media/filters/decrypting_demuxer_stream.cc:235: if (!buffer->decrypt_config()) {
Why is this different from the block at 242? I would think we would prepend
"!buffer->decrypt_config() ||" to that condition.

It's good if we can avoid the copy, but it's not clear why. There hsould
probably be an explanation.

https://codereview.chromium.org/2543623003/diff/40001/media/filters/decryptin...
media/filters/decrypting_demuxer_stream.cc:238:
base::ResetAndReturn(&read_cb_).Run(kOk, buffer);
Should we report |status| instead like 231?

https://codereview.chromium.org/2543623003/diff/40001/media/filters/decryptin...
File media/filters/decrypting_video_decoder.cc (right):

https://codereview.chromium.org/2543623003/diff/40001/media/filters/decryptin...
media/filters/decrypting_video_decoder.cc:62: DCHECK(config.is_encrypted());
Comment/TODO

https://codereview.chromium.org/2543623003/diff/40001/media/filters/decryptin...
media/filters/decrypting_video_decoder.cc:72: // Reinitialization. |config| can
be encrypted or clear.
Note that this is a separate thought. (Context: Earlier comment.)

https://codereview.chromium.org/2543623003/diff/40001/media/test/pipeline_int...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/2543623003/diff/40001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1479: EXPECT_EQ(PIPELINE_ERROR_DECODE,
WaitUntilEndedOrError());
This seems bad. Authors do not like the decode error, and this just adds another
cause vs. failing on appending.

[xhwang, 2016-12-16 06:01:13]

rebase only

[xhwang, 2016-12-16 07:00:24]

Description was changed from

==========
media: Allow config change between clear and encrypted streams

In the demuxer, we allow DecryptConfig change upon config change,
including but not limited to:
- switching between clear and encrypted
- encryption scheme change

It'll be up to the media Renderer to decide whether such change is
supported. If not, it may trigger a decode error.

Currently the default media Renderer (RendererImpl) supports switching
from encrypted to clear, because:
- Decrypt-and-decode mode: Decrypting{Audio|Video}Decoder supports clear
  buffer.
- Decrypt-only mode: DecryptingDemuxerStream supports clear buffer.

However, switching from clear to encrypted is not supported in
RendererImpl, because the clear decoder doesn't support decryption. This
will be fixed in a later CL.

BUG=597443
TEST=Updated pipeline_integration_tests.
==========

to

==========
media: Allow config change between clear and encrypted streams

In the demuxer, we allow DecryptConfig change upon config change,
including but not limited to:
- switching between clear and encrypted
- encryption scheme change

Media Renderer implementation should support such changes. The detailed
requirement from the spec's perspective is tracked at:
https://github.com/w3c/encrypted-media/issues/251

Currently the default media Renderer (RendererImpl) supports switching
from encrypted to clear, because:
- Decrypt-and-decode mode: Decrypting{Audio|Video}Decoder supports clear
  buffer.
- Decrypt-only mode: DecryptingDemuxerStream supports clear buffer.

However, switching from clear to encrypted is not supported in
RendererImpl, because the clear decoder doesn't support decryption. This
will be fixed in a later CL.

BUG=597443
TEST=Updated pipeline_integration_tests.
==========

[xhwang, 2016-12-16 20:07:01]

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

[commit-bot: I haz the power, 2016-12-16 20:08:14]

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

[xhwang, 2016-12-16 20:10:50]

On 2016/12/05 23:53:04, ddorwin wrote:
> I reviewed the code. I have more general concerns about only implementing one
> direction and not requiring it for all implementations. 

Updated CL description and bug to make sure we have traction on the spec land
and all Renderer impls should follow what the spec says.

> We should also have browser tests or similar, 

Updated both browser_tests and content_browsertests

> including for both orderings of setMediaKeys() and setting the source.

setting MediaKeys before play() seems a non-trivial change. I filed a bug and
we'll do it in separately.

See https://bugs.chromium.org/p/chromium/issues/detail?id=675011

[xhwang, 2016-12-16 20:12:27]

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

[xhwang, 2016-12-16 20:12:29]

I think I addressed all comments and added more tests.

ddorwin: Please take another look! Let me know if you'd like to defer the
detailed review to others.

jrummell: FYI

https://codereview.chromium.org/2543623003/diff/40001/media/filters/decryptin...
File media/filters/decrypting_audio_decoder.cc (right):

https://codereview.chromium.org/2543623003/diff/40001/media/filters/decryptin...
media/filters/decrypting_audio_decoder.cc:76: DCHECK(config.is_encrypted());
On 2016/12/05 23:53:04, ddorwin wrote:
> There should probably be a comment/TODO with the crbug.

joeyparrish: This is an internal contract between DecoderSelector and
DecryptingAudioDecoder. Today we would only choose DAD when the stream is
encrypted.

ddorwin: comment added. This check will probably stay and I don't feel we need a
TODO here.

https://codereview.chromium.org/2543623003/diff/40001/media/filters/decryptin...
media/filters/decrypting_audio_decoder.cc:86: // Reinitialization (i.e. upon a
config change, the new config can be
On 2016/12/05 23:53:04, ddorwin wrote:
> The new text seems like a separate thought and maybe a new sentence outside
the
> ().

Done.

https://codereview.chromium.org/2543623003/diff/40001/media/filters/decryptin...
File media/filters/decrypting_audio_decoder_unittest.cc (right):

https://codereview.chromium.org/2543623003/diff/40001/media/filters/decryptin...
media/filters/decrypting_audio_decoder_unittest.cc:362: // TODO(xhwang): Add a
test for switching from encrypted to clear.
On 2016/12/05 23:53:04, ddorwin wrote:
> Can we at least keep the previous test (but expect true) and have a TODO to
> improve it?

Due to the new DCHECK the previous test would crash. There are some trivial
differences b/w DAD and DVD and this is one of them. I planned to fix it but
it's always low priority. See the original TODO here:

https://cs.chromium.org/chromium/src/media/filters/decrypting_audio_decoder.c...

https://codereview.chromium.org/2543623003/diff/40001/media/filters/decryptin...
File media/filters/decrypting_demuxer_stream.cc (right):

https://codereview.chromium.org/2543623003/diff/40001/media/filters/decryptin...
media/filters/decrypting_demuxer_stream.cc:235: if (!buffer->decrypt_config()) {
On 2016/12/05 23:53:04, ddorwin wrote:
> Why is this different from the block at 242? I would think we would prepend
> "!buffer->decrypt_config() ||" to that condition.
> 
> It's good if we can avoid the copy, but it's not clear why. There hsould
> probably be an explanation.

I agree this is a bit odd. In the block at 242, we do a copy because there's no
way to easily flip the "is_encrypted" flag. Now is_encrypted() becomes a
method... I am not sure whether we can simply pass through as we do here. I'll
add a TODO and figure this out in a separate CL.

See previous discussion here:

https://bugs.chromium.org/p/chromium/issues/detail?id=171785#c6

https://codereview.chromium.org/2543623003/diff/40001/media/filters/decryptin...
media/filters/decrypting_demuxer_stream.cc:238:
base::ResetAndReturn(&read_cb_).Run(kOk, buffer);
On 2016/12/05 23:53:04, ddorwin wrote:
> Should we report |status| instead like 231?

|status| must be kOk here. Added a DCHECK and fixed 231 to make it clear.

https://codereview.chromium.org/2543623003/diff/40001/media/filters/decryptin...
File media/filters/decrypting_video_decoder.cc (right):

https://codereview.chromium.org/2543623003/diff/40001/media/filters/decryptin...
media/filters/decrypting_video_decoder.cc:72: // Reinitialization. |config| can
be encrypted or clear.
On 2016/12/05 23:53:04, ddorwin wrote:
> Note that this is a separate thought. (Context: Earlier comment.)

Done.

https://codereview.chromium.org/2543623003/diff/40001/media/test/pipeline_int...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/2543623003/diff/40001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:1479: EXPECT_EQ(PIPELINE_ERROR_DECODE,
WaitUntilEndedOrError());
On 2016/12/05 23:53:04, ddorwin wrote:
> This seems bad. Authors do not like the decode error, and this just adds
another
> cause vs. failing on appending.

Agreed. But I feel this is the problem with decode error and we are working
separately to address that.

Also, we do plan to fix this. See the TODO above.

[commit-bot: I haz the power, 2016-12-16 21:17:19]

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

[commit-bot: I haz the power, 2016-12-16 21:17:20]

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

[xhwang, 2017-01-04 22:15:41]

kindly post-holiday ping!

[Joey Parrish, 2017-01-04 22:47:43]

lgtm

[xhwang, 2017-01-05 22:17:21]

rebase only

[xhwang, 2017-01-05 22:17:27]

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

[commit-bot: I haz the power, 2017-01-05 22:18:14]

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-01-05 22:55:13]

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

[commit-bot: I haz the power, 2017-01-05 22:55:14]

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

[xhwang, 2017-01-06 07:20:48]

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

[commit-bot: I haz the power, 2017-01-06 07:20:55]

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

[xhwang, 2017-01-06 07:21:30]

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

[commit-bot: I haz the power, 2017-01-06 07:21:36]

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-01-06 09:17:53]

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

[commit-bot: I haz the power, 2017-01-06 09:17:54]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[xhwang, 2017-01-07 07:59:59]

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

[commit-bot: I haz the power, 2017-01-07 08:00:09]

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-01-07 09:19:00]

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

[commit-bot: I haz the power, 2017-01-07 09:19:01]

Dry run: This issue passed the CQ dry run.

[xhwang, 2017-01-09 17:54:26]

ddorwin: Would you like to finish the detailed review of this CL? Or shall I
find someone else to do it? Thanks!

[ddorwin, 2017-02-12 04:28:53]

LG overall. We need to address the spec/interop, though.

https://codereview.chromium.org/2543623003/diff/140001/chrome/browser/media/e...
File chrome/browser/media/encrypted_media_browsertest.cc (right):

https://codereview.chromium.org/2543623003/diff/140001/chrome/browser/media/e...
chrome/browser/media/encrypted_media_browsertest.cc:39: // Available key
systems.
It's interesting that none of this test is in a namespace compared to the next
file.

Also, these should technically be anonymous or static.

https://codereview.chromium.org/2543623003/diff/140001/chrome/browser/media/e...
chrome/browser/media/encrypted_media_browsertest.cc:380: const std::string&
expectation = kEnded) {
There are only 3 call sites where this is used. Maybe just remove default value?

https://codereview.chromium.org/2543623003/diff/140001/chrome/browser/media/e...
chrome/browser/media/encrypted_media_browsertest.cc:382: DVLOG(0) << "Config
change only happens when using MSE.";
Each one of these tests involves invoking an instance of Chrome. We do nothing
in many of them. It would be nice if there was a way to fail early in a clearly
associated way. Maybe we should file a test infra bug to consider this. WDYT?

https://codereview.chromium.org/2543623003/diff/140001/chrome/browser/media/e...
chrome/browser/media/encrypted_media_browsertest.cc:386: if
(!IsPlayBackPossible(CurrentKeySystem())) {
One solution to the above problem would be to have a separate list of
playback-capable key systems and only iterate over those.

https://codereview.chromium.org/2543623003/diff/140001/content/browser/media/...
File content/browser/media/encrypted_media_browsertest.cc (right):

https://codereview.chromium.org/2543623003/diff/140001/content/browser/media/...
content/browser/media/encrypted_media_browsertest.cc:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
Same comments as the previous file.

https://codereview.chromium.org/2543623003/diff/140001/content/browser/media/...
File content/browser/media/media_browsertest.cc (right):

https://codereview.chromium.org/2543623003/diff/140001/content/browser/media/...
content/browser/media/media_browsertest.cc:22: // TODO(sandersd): Change the
tests to use a more unique message.
How is this different from 32? Why doesn't 32 have the bug reference?

https://codereview.chromium.org/2543623003/diff/140001/content/browser/media/...
content/browser/media/media_browsertest.cc:28: // Uncapitalized event name as
set by Utils.failTest().
nit: "Lower case"?

https://codereview.chromium.org/2543623003/diff/140001/content/browser/media/...
content/browser/media/media_browsertest.cc:31: // Capitalized event name set by
Utils.installTitleEventHandler().
nit: Upper case

https://codereview.chromium.org/2543623003/diff/140001/media/cdm/ppapi/extern...
File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right):

https://codereview.chromium.org/2543623003/diff/140001/media/cdm/ppapi/extern...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:101: if
(input_buffer.iv_size != 0) {
Should we have a similar TODO as below here?

Or a TODO to add an is_encrypted() method, even if it just relies on this logic
(and maybe has some DCHECKs) internally.

https://codereview.chromium.org/2543623003/diff/140001/media/filters/decrypti...
File media/filters/decrypting_audio_decoder.cc (right):

https://codereview.chromium.org/2543623003/diff/140001/media/filters/decrypti...
media/filters/decrypting_audio_decoder.cc:76: // DecoderSelector only chooses
|this| when the stream is encrypted.
This will change in the next CL, right? Maybe add a TODO so that's clear for
anyone doing archaeology.

https://codereview.chromium.org/2543623003/diff/140001/media/filters/decrypti...
File media/filters/decrypting_audio_decoder_unittest.cc (right):

https://codereview.chromium.org/2543623003/diff/140001/media/filters/decrypti...
media/filters/decrypting_audio_decoder_unittest.cc:283: 
Isn't the removed test still true for this CL? IOW, restore it until the next
CL?

https://codereview.chromium.org/2543623003/diff/140001/media/filters/decrypti...
media/filters/decrypting_audio_decoder_unittest.cc:378: 
ASSERT_TRUE(new_config.is_encrypted());

https://codereview.chromium.org/2543623003/diff/140001/media/filters/decrypti...
media/filters/decrypting_audio_decoder_unittest.cc:399:
EXPECT_FALSE(new_config.is_encrypted());
ASSERT? This would be a code bug.

https://codereview.chromium.org/2543623003/diff/140001/media/filters/decrypti...
File media/filters/decrypting_demuxer_stream_unittest.cc (right):

https://codereview.chromium.org/2543623003/diff/140001/media/filters/decrypti...
media/filters/decrypting_demuxer_stream_unittest.cc:320: // TODO(xhwang): Unify
clear buffer handling in clear and encrypted stream.
How is this relevant here? Will that eliminate one of these?

https://codereview.chromium.org/2543623003/diff/140001/media/filters/decrypti...
File media/filters/decrypting_video_decoder.cc (right):

https://codereview.chromium.org/2543623003/diff/140001/media/filters/decrypti...
media/filters/decrypting_video_decoder.cc:62: // DecoderSelector only chooses
|this| when the stream is encrypted.
ditto

https://codereview.chromium.org/2543623003/diff/140001/media/filters/decrypti...
File media/filters/decrypting_video_decoder_unittest.cc (right):

https://codereview.chromium.org/2543623003/diff/140001/media/filters/decrypti...
media/filters/decrypting_video_decoder_unittest.cc:279:
Reinitialize(TestVideoConfig::LargeEncrypted());
OOC, why is this test different from the Audio version?

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

https://codereview.chromium.org/2543623003/diff/140001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:1546:
ASSERT_TRUE(source.AppendAtTime(base::TimeDelta::FromSeconds(kAppendTimeSec),
EXPECT_?

[xhwang, 2017-02-14 19:26:03]

rebase only

[xhwang, 2017-02-14 21:07:49]

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

[commit-bot: I haz the power, 2017-02-14 21:08:24]

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

[commit-bot: I haz the power, 2017-02-14 22:53:34]

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

[commit-bot: I haz the power, 2017-02-14 22:53:35]

Dry run: This issue passed the CQ dry run.

[xhwang, 2017-02-14 23:56:57]

comments addressed

[xhwang, 2017-02-14 23:59:07]

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

[xhwang, 2017-02-14 23:59:33]

ddorwin: PTAL again!

https://codereview.chromium.org/2543623003/diff/140001/chrome/browser/media/e...
File chrome/browser/media/encrypted_media_browsertest.cc (right):

https://codereview.chromium.org/2543623003/diff/140001/chrome/browser/media/e...
chrome/browser/media/encrypted_media_browsertest.cc:39: // Available key
systems.
On 2017/02/12 04:28:52, ddorwin wrote:
> It's interesting that none of this test is in a namespace compared to the next
> file.
> 
> Also, these should technically be anonymous or static.

Traditionally chrome/ files are not in "chrome" namespace. But now I see more
and more "chrome" namespace usage now.

Done.

https://codereview.chromium.org/2543623003/diff/140001/chrome/browser/media/e...
chrome/browser/media/encrypted_media_browsertest.cc:380: const std::string&
expectation = kEnded) {
On 2017/02/12 04:28:52, ddorwin wrote:
> There are only 3 call sites where this is used. Maybe just remove default
value?

Dropped now the failing test is disabled.

https://codereview.chromium.org/2543623003/diff/140001/chrome/browser/media/e...
chrome/browser/media/encrypted_media_browsertest.cc:382: DVLOG(0) << "Config
change only happens when using MSE.";
On 2017/02/12 04:28:52, ddorwin wrote:
> Each one of these tests involves invoking an instance of Chrome. We do nothing
> in many of them. It would be nice if there was a way to fail early in a
clearly
> associated way. Maybe we should file a test infra bug to consider this. WDYT?

Added TODO. We probably should have more details control over the test
parameters for each test.

https://codereview.chromium.org/2543623003/diff/140001/chrome/browser/media/e...
chrome/browser/media/encrypted_media_browsertest.cc:386: if
(!IsPlayBackPossible(CurrentKeySystem())) {
On 2017/02/12 04:28:52, ddorwin wrote:
> One solution to the above problem would be to have a separate list of
> playback-capable key systems and only iterate over those.

But this also depends on whether we have a license server..

https://codereview.chromium.org/2543623003/diff/140001/content/browser/media/...
File content/browser/media/encrypted_media_browsertest.cc (right):

https://codereview.chromium.org/2543623003/diff/140001/content/browser/media/...
content/browser/media/encrypted_media_browsertest.cc:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
On 2017/02/12 04:28:52, ddorwin wrote:
> Same comments as the previous file.

Done.

https://codereview.chromium.org/2543623003/diff/140001/content/browser/media/...
File content/browser/media/media_browsertest.cc (right):

https://codereview.chromium.org/2543623003/diff/140001/content/browser/media/...
content/browser/media/media_browsertest.cc:22: // TODO(sandersd): Change the
tests to use a more unique message.
On 2017/02/12 04:28:52, ddorwin wrote:
> How is this different from 32? Why doesn't 32 have the bug reference?

Removed TODO on l.32.

https://codereview.chromium.org/2543623003/diff/140001/content/browser/media/...
content/browser/media/media_browsertest.cc:28: // Uncapitalized event name as
set by Utils.failTest().
On 2017/02/12 04:28:52, ddorwin wrote:
> nit: "Lower case"?

Done.

https://codereview.chromium.org/2543623003/diff/140001/content/browser/media/...
content/browser/media/media_browsertest.cc:31: // Capitalized event name set by
Utils.installTitleEventHandler().
On 2017/02/12 04:28:52, ddorwin wrote:
> nit: Upper case

Done.

https://codereview.chromium.org/2543623003/diff/140001/media/cdm/ppapi/extern...
File media/cdm/ppapi/external_clear_key/clear_key_cdm.cc (right):

https://codereview.chromium.org/2543623003/diff/140001/media/cdm/ppapi/extern...
media/cdm/ppapi/external_clear_key/clear_key_cdm.cc:101: if
(input_buffer.iv_size != 0) {
On 2017/02/12 04:28:52, ddorwin wrote:
> Should we have a similar TODO as below here?
> 
> Or a TODO to add an is_encrypted() method, even if it just relies on this
logic
> (and maybe has some DCHECKs) internally.

Done.

https://codereview.chromium.org/2543623003/diff/140001/media/filters/decrypti...
File media/filters/decrypting_audio_decoder.cc (right):

https://codereview.chromium.org/2543623003/diff/140001/media/filters/decrypti...
media/filters/decrypting_audio_decoder.cc:76: // DecoderSelector only chooses
|this| when the stream is encrypted.
On 2017/02/12 04:28:52, ddorwin wrote:
> This will change in the next CL, right? Maybe add a TODO so that's clear for
> anyone doing archaeology.

Done.

https://codereview.chromium.org/2543623003/diff/140001/media/filters/decrypti...
File media/filters/decrypting_audio_decoder_unittest.cc (right):

https://codereview.chromium.org/2543623003/diff/140001/media/filters/decrypti...
media/filters/decrypting_audio_decoder_unittest.cc:283: 
On 2017/02/12 04:28:52, ddorwin wrote:
> Isn't the removed test still true for this CL? IOW, restore it until the next
> CL?

But the DCHECK will fire. We don't test cases not in the API contract.

https://codereview.chromium.org/2543623003/diff/140001/media/filters/decrypti...
media/filters/decrypting_audio_decoder_unittest.cc:378: 
On 2017/02/12 04:28:52, ddorwin wrote:
> ASSERT_TRUE(new_config.is_encrypted());

Done.

https://codereview.chromium.org/2543623003/diff/140001/media/filters/decrypti...
media/filters/decrypting_audio_decoder_unittest.cc:399:
EXPECT_FALSE(new_config.is_encrypted());
On 2017/02/12 04:28:52, ddorwin wrote:
> ASSERT? This would be a code bug.

Done.

https://codereview.chromium.org/2543623003/diff/140001/media/filters/decrypti...
File media/filters/decrypting_demuxer_stream_unittest.cc (right):

https://codereview.chromium.org/2543623003/diff/140001/media/filters/decrypti...
media/filters/decrypting_demuxer_stream_unittest.cc:320: // TODO(xhwang): Unify
clear buffer handling in clear and encrypted stream.
On 2017/02/12 04:28:52, ddorwin wrote:
> How is this relevant here? Will that eliminate one of these?

Yeah, the clear buffer from encrypted stream and clear stream should be the
same. Hence we only need one test here.

https://codereview.chromium.org/2543623003/diff/140001/media/filters/decrypti...
File media/filters/decrypting_video_decoder.cc (right):

https://codereview.chromium.org/2543623003/diff/140001/media/filters/decrypti...
media/filters/decrypting_video_decoder.cc:62: // DecoderSelector only chooses
|this| when the stream is encrypted.
On 2017/02/12 04:28:52, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/2543623003/diff/140001/media/filters/decrypti...
File media/filters/decrypting_video_decoder_unittest.cc (right):

https://codereview.chromium.org/2543623003/diff/140001/media/filters/decrypti...
media/filters/decrypting_video_decoder_unittest.cc:279:
Reinitialize(TestVideoConfig::LargeEncrypted());
On 2017/02/12 04:28:53, ddorwin wrote:
> OOC, why is this test different from the Audio version?

For historical reasons ;)

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

https://codereview.chromium.org/2543623003/diff/140001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:1546:
ASSERT_TRUE(source.AppendAtTime(base::TimeDelta::FromSeconds(kAppendTimeSec),
On 2017/02/12 04:28:53, ddorwin wrote:
> EXPECT_?

Done.

[commit-bot: I haz the power, 2017-02-14 23:59:46]

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

[commit-bot: I haz the power, 2017-02-15 02:43:37]

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

[commit-bot: I haz the power, 2017-02-15 02:43:38]

Dry run: This issue passed the CQ dry run.

[xhwang, 2017-02-15 04:14:53]

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

[xhwang, 2017-02-15 04:14:54]

bbudge: Please OWNERS review trivial changes in
content/renderer/pepper/content_decryptor_delegate.cc

[bbudge, 2017-02-15 15:46:13]

https://codereview.chromium.org/2543623003/diff/200001/content/renderer/peppe...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/2543623003/diff/200001/content/renderer/peppe...
content/renderer/pepper/content_decryptor_delegate.cc:121: return true;
It seems strange to return true here, if the block info hasn't been filled in.

[xhwang, 2017-02-15 17:29:15]

comments addressed

[xhwang, 2017-02-15 17:30:20]

https://codereview.chromium.org/2543623003/diff/200001/content/renderer/peppe...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/2543623003/diff/200001/content/renderer/peppe...
content/renderer/pepper/content_decryptor_delegate.cc:121: return true;
On 2017/02/15 15:46:13, bbudge wrote:
> It seems strange to return true here, if the block info hasn't been filled in.

The struct is initialized at line 107. Then when the buffer is not encrypted,
there's no need to fill any of the fields below as they are only used for
decryption.

I added a comment to make this more clear.

[xhwang, 2017-02-15 17:30:35]

bbudge: PTAL again!

[xhwang, 2017-02-15 17:30:38]

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

[commit-bot: I haz the power, 2017-02-15 17:31:48]

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

[bbudge, 2017-02-15 17:35:22]

Thanks. content/renderer/pepper lgtm

[commit-bot: I haz the power, 2017-02-15 19:37:30]

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

[commit-bot: I haz the power, 2017-02-15 19:37:31]

Dry run: This issue passed the CQ dry run.

[ddorwin, 2017-02-16 20:26:21]

LGTM with nits. One reply is in a previous PS.

https://codereview.chromium.org/2543623003/diff/140001/chrome/browser/media/e...
File chrome/browser/media/encrypted_media_browsertest.cc (right):

https://codereview.chromium.org/2543623003/diff/140001/chrome/browser/media/e...
chrome/browser/media/encrypted_media_browsertest.cc:386: if
(!IsPlayBackPossible(CurrentKeySystem())) {
On 2017/02/14 23:59:32, xhwang_slow wrote:
> On 2017/02/12 04:28:52, ddorwin wrote:
> > One solution to the above problem would be to have a separate list of
> > playback-capable key systems and only iterate over those.
> 
> But this also depends on whether we have a license server..

Right. We would #ifdef the list appropriately.

https://codereview.chromium.org/2543623003/diff/220001/chrome/browser/media/e...
File chrome/browser/media/encrypted_media_browsertest.cc (right):

https://codereview.chromium.org/2543623003/diff/220001/chrome/browser/media/e...
chrome/browser/media/encrypted_media_browsertest.cc:394: // TODO(xhwang): Even
when config change or playback is not supported we
bug #?

https://codereview.chromium.org/2543623003/diff/220001/chrome/browser/media/e...
chrome/browser/media/encrypted_media_browsertest.cc:395: // still start chrome
only to return directly here. We probably should not
nit: Chrome

[xhwang, 2017-02-17 00:45:02]

comments addressed

[xhwang, 2017-02-17 00:47:20]

https://codereview.chromium.org/2543623003/diff/140001/chrome/browser/media/e...
File chrome/browser/media/encrypted_media_browsertest.cc (right):

https://codereview.chromium.org/2543623003/diff/140001/chrome/browser/media/e...
chrome/browser/media/encrypted_media_browsertest.cc:386: if
(!IsPlayBackPossible(CurrentKeySystem())) {
On 2017/02/16 20:26:21, ddorwin wrote:
> On 2017/02/14 23:59:32, xhwang_slow wrote:
> > On 2017/02/12 04:28:52, ddorwin wrote:
> > > One solution to the above problem would be to have a separate list of
> > > playback-capable key systems and only iterate over those.
> > 
> > But this also depends on whether we have a license server..
> 
> Right. We would #ifdef the list appropriately.

Acknowledged.

https://codereview.chromium.org/2543623003/diff/220001/chrome/browser/media/e...
File chrome/browser/media/encrypted_media_browsertest.cc (right):

https://codereview.chromium.org/2543623003/diff/220001/chrome/browser/media/e...
chrome/browser/media/encrypted_media_browsertest.cc:394: // TODO(xhwang): Even
when config change or playback is not supported we
On 2017/02/16 20:26:21, ddorwin wrote:
> bug #?

Done.

https://codereview.chromium.org/2543623003/diff/220001/chrome/browser/media/e...
chrome/browser/media/encrypted_media_browsertest.cc:395: // still start chrome
only to return directly here. We probably should not
On 2017/02/16 20:26:21, ddorwin wrote:
> nit: Chrome

Done.

[xhwang, 2017-02-17 00:47:29]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2017-02-17 00:47:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
joeyparrish@google.com, bbudge@chromium.org, ddorwin@chromium.org
Link to the patchset: https://codereview.chromium.org/2543623003/#ps240001
(title: "comments addressed")

[commit-bot: I haz the power, 2017-02-17 00:49:04]

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

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

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

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

Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[xhwang, 2017-02-17 03:33:19]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2017-02-17 03:34:38]

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

[commit-bot: I haz the power, 2017-02-17 04:37:46]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1487302399652020,
"parent_rev": "063631d86e241cf9ddc12ece953e1f941175d46f", "commit_rev":
"65c23034a1deaa9b221ef98928925bee36304fd6"}

[commit-bot: I haz the power, 2017-02-17 04:38:45]

Description was changed from

==========
media: Allow config change between clear and encrypted streams

In the demuxer, we allow DecryptConfig change upon config change,
including but not limited to:
- switching between clear and encrypted
- encryption scheme change

Media Renderer implementation should support such changes. The detailed
requirement from the spec's perspective is tracked at:
https://github.com/w3c/encrypted-media/issues/251

Currently the default media Renderer (RendererImpl) supports switching
from encrypted to clear, because:
- Decrypt-and-decode mode: Decrypting{Audio|Video}Decoder supports clear
  buffer.
- Decrypt-only mode: DecryptingDemuxerStream supports clear buffer.

However, switching from clear to encrypted is not supported in
RendererImpl, because the clear decoder doesn't support decryption. This
will be fixed in a later CL.

BUG=597443
TEST=Updated pipeline_integration_tests.
==========

to

==========
media: Allow config change between clear and encrypted streams

In the demuxer, we allow DecryptConfig change upon config change,
including but not limited to:
- switching between clear and encrypted
- encryption scheme change

Media Renderer implementation should support such changes. The detailed
requirement from the spec's perspective is tracked at:
https://github.com/w3c/encrypted-media/issues/251

Currently the default media Renderer (RendererImpl) supports switching
from encrypted to clear, because:
- Decrypt-and-decode mode: Decrypting{Audio|Video}Decoder supports clear
  buffer.
- Decrypt-only mode: DecryptingDemuxerStream supports clear buffer.

However, switching from clear to encrypted is not supported in
RendererImpl, because the clear decoder doesn't support decryption. This
will be fixed in a later CL.

BUG=597443
TEST=Updated pipeline_integration_tests.

Review-Url: https://codereview.chromium.org/2543623003
Cr-Commit-Position: refs/heads/master@{#451212}
Committed:
https://chromium.googlesource.com/chromium/src/+/65c23034a1deaa9b221ef9892892...
==========

[commit-bot: I haz the power, 2017-02-17 04:38:48]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/65c23034a1deaa9b221ef9892892...