Use 'pssh' data to determine key_id properly

Use 'pssh' data to determine key_id properly

Required regenerating the test files to use Common System format
for the relevant 'pssh' box.

BUG=460308
TEST=modified tests pass

Committed: https://crrev.com/3b08f4973d951753a94c51ec934e01ff3a214a04
Cr-Commit-Position: refs/heads/master@{#335405}

[jrummell, 2015-06-04 02:46:57]

jrummell@chromium.org changed reviewers:
+ ddorwin@chromium.org, sandersd@chromium.org

[jrummell, 2015-06-04 02:46:57]

PTAL.

[ddorwin, 2015-06-04 19:57:18]

Thanks.

I think "Common Encryption" in the description might be "Common System."

You may need to update the README for the binary files.

https://codereview.chromium.org/1163713007/diff/1/media/cdm/cenc_utils.h
File media/cdm/cenc_utils.h (right):

https://codereview.chromium.org/1163713007/diff/1/media/cdm/cenc_utils.h#newc...
media/cdm/cenc_utils.h:24: MEDIA_EXPORT bool GetKeyIdsForCommonSystemId(const
std::vector<uint8_t>& input,
Are there any call sites that were checking for the bool and the size that we
can now clean up?

https://codereview.chromium.org/1163713007/diff/1/media/cdm/cenc_utils_unitte...
File media/cdm/cenc_utils_unittest.cc (right):

https://codereview.chromium.org/1163713007/diff/1/media/cdm/cenc_utils_unitte...
media/cdm/cenc_utils_unittest.cc:61: // Add Clear Key SystemID.
s/CK/Common/

https://codereview.chromium.org/1163713007/diff/1/media/cdm/cenc_utils_unitte...
media/cdm/cenc_utils_unittest.cc:225: TEST_F(CencUtilsTest, PSSHVersion1Plus0) {
There should be a test for Version2 then Version1 - the keys from v1 should be
used.

https://codereview.chromium.org/1163713007/diff/1/media/cdm/cenc_utils_unitte...
media/cdm/cenc_utils_unittest.cc:258: EXPECT_EQ(key_ids[2], Key3());
Does the code pull key IDs out of all boxes with the Common SystemID? It should
only use the first matching box. If there is a bug, just add a TODO and fix it
in a separate CL.

https://codereview.chromium.org/1163713007/diff/1/media/cdm/cenc_utils_unitte...
media/cdm/cenc_utils_unittest.cc:262: TEST_F(CencUtilsTest, InvalidPSSH) {
The test name should describe *how* it is invalid.

https://codereview.chromium.org/1163713007/diff/1/media/cdm/cenc_utils_unitte...
media/cdm/cenc_utils_unittest.cc:277: TEST_F(CencUtilsTest, InvalidSystemID) {
s/Invalid/Unrecognized/

https://codereview.chromium.org/1163713007/diff/1/media/cdm/cenc_utils_unitte...
media/cdm/cenc_utils_unittest.cc:322: TEST_F(CencUtilsTest, NoSize) {
SizeIsZero (there is a size)

https://codereview.chromium.org/1163713007/diff/1/media/cdm/cenc_utils_unitte...
media/cdm/cenc_utils_unittest.cc:340:
ValidatePsshInput(std::vector<uint8_t>(data, data + arraysize(data))));
Is size 0 valid?

https://codereview.chromium.org/1163713007/diff/1/media/cdm/cenc_utils_unitte...
media/cdm/cenc_utils_unittest.cc:364: EXPECT_FALSE(
Explain why it fails.

https://codereview.chromium.org/1163713007/diff/1/media/test/pipeline_integra...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1163713007/diff/1/media/test/pipeline_integra...
media/test/pipeline_integration_test.cc:88: const uint8 kKeyId[] = {
This is now only used for the rotation test (unless we start checking the
results of the initdata parsing). I wonder if we should move it to that test
class.

https://codereview.chromium.org/1163713007/diff/1/media/test/pipeline_integra...
media/test/pipeline_integration_test.cc:325: // Clear Key needs the key ID from
|init_data|.
We should really wait for the message and extract the ID from there. As long as
this works, this is simpler. Perhaps note that we're taking a shortcut.

The above is the only thing special here. Getting the key ID from the init_data
would work for any system if a valid request is not required, as is the case for
Clear Key. I suggest something like "Get the key ID for the license from
|init_Data|."

https://codereview.chromium.org/1163713007/diff/1/media/test/pipeline_integra...
media/test/pipeline_integration_test.cc:337: bool GetKeyId(EmeInitDataType
init_data_type,
This can be a static function at the top.

https://codereview.chromium.org/1163713007/diff/1/media/test/pipeline_integra...
media/test/pipeline_integration_test.cc:340: // For WebM, init_data is key_id;
for ISO CENC, init_data should
Function level comment.

nit: use pipes ('|')

https://codereview.chromium.org/1163713007/diff/1/media/test/pipeline_integra...
media/test/pipeline_integration_test.cc:342: // will contain the key_id.
You might note that keyids does not need to be supported because it's not
contained in files. Perhaps include the enum with NOTREACHED() below.

https://codereview.chromium.org/1163713007/diff/1/media/test/pipeline_integra...
media/test/pipeline_integration_test.cc:343: DCHECK_GE(init_data.size(),
arraysize(kKeyId));
arraysize(kKeyId) looks weird. Can we define a separate const below kKeyId that
is kKeyIdBytes?

https://codereview.chromium.org/1163713007/diff/1/media/test/pipeline_integra...
media/test/pipeline_integration_test.cc:363: return false;
Should we add this?
NOTREACHED() << "Attempted to use MP4 file without USE_PROPRIETARY_CODECS"

https://codereview.chromium.org/1163713007/diff/1/media/test/pipeline_integra...
media/test/pipeline_integration_test.cc:367: // Nothing else is supported.
NOTREACHED?

https://codereview.chromium.org/1163713007/diff/1/media/test/pipeline_integra...
media/test/pipeline_integration_test.cc:371: return false;
You can move this up to 368. Or remove default so compilers can catch missing
values.

https://codereview.chromium.org/1163713007/diff/1/media/test/pipeline_integra...
media/test/pipeline_integration_test.cc:426: // The Key and KeyId for this
testing key provider are created by left
Do we still need to do this for the key ID since we can extract it successfully
now?

[jrummell, 2015-06-15 22:22:11]

Updated. PS#2 is a rebase to get the recent cenc_util changes, PS#3 is the rest.
Biggest change in PS#3 is that UpdateSession() is now called in response to a
message event.

https://codereview.chromium.org/1163713007/diff/1/media/cdm/cenc_utils.h
File media/cdm/cenc_utils.h (right):

https://codereview.chromium.org/1163713007/diff/1/media/cdm/cenc_utils.h#newc...
media/cdm/cenc_utils.h:24: MEDIA_EXPORT bool GetKeyIdsForCommonSystemId(const
std::vector<uint8_t>& input,
On 2015/06/04 19:57:17, ddorwin wrote:
> Are there any call sites that were checking for the bool and the size that we
> can now clean up?

Nope. The only caller (other than tests) is aes_decryptor.cc, and it will simply
generate a license request with whatever keys it gets back (which could have
been none).

https://codereview.chromium.org/1163713007/diff/1/media/cdm/cenc_utils_unitte...
File media/cdm/cenc_utils_unittest.cc (right):

https://codereview.chromium.org/1163713007/diff/1/media/cdm/cenc_utils_unitte...
media/cdm/cenc_utils_unittest.cc:61: // Add Clear Key SystemID.
On 2015/06/04 19:57:17, ddorwin wrote:
> s/CK/Common/

Done in rebase (https://codereview.chromium.org/1149023002/).

https://codereview.chromium.org/1163713007/diff/1/media/cdm/cenc_utils_unitte...
media/cdm/cenc_utils_unittest.cc:225: TEST_F(CencUtilsTest, PSSHVersion1Plus0) {
On 2015/06/04 19:57:17, ddorwin wrote:
> There should be a test for Version2 then Version1 - the keys from v1 should be
> used.

Done in rebase (https://codereview.chromium.org/1149023002/).

https://codereview.chromium.org/1163713007/diff/1/media/cdm/cenc_utils_unitte...
media/cdm/cenc_utils_unittest.cc:258: EXPECT_EQ(key_ids[2], Key3());
On 2015/06/04 19:57:17, ddorwin wrote:
> Does the code pull key IDs out of all boxes with the Common SystemID? It
should
> only use the first matching box. If there is a bug, just add a TODO and fix it
> in a separate CL.

Done.

https://codereview.chromium.org/1163713007/diff/1/media/cdm/cenc_utils_unitte...
media/cdm/cenc_utils_unittest.cc:262: TEST_F(CencUtilsTest, InvalidPSSH) {
On 2015/06/04 19:57:17, ddorwin wrote:
> The test name should describe *how* it is invalid.

Done.

https://codereview.chromium.org/1163713007/diff/1/media/cdm/cenc_utils_unitte...
media/cdm/cenc_utils_unittest.cc:277: TEST_F(CencUtilsTest, InvalidSystemID) {
On 2015/06/04 19:57:17, ddorwin wrote:
> s/Invalid/Unrecognized/

Done.

https://codereview.chromium.org/1163713007/diff/1/media/cdm/cenc_utils_unitte...
media/cdm/cenc_utils_unittest.cc:322: TEST_F(CencUtilsTest, NoSize) {
On 2015/06/04 19:57:17, ddorwin wrote:
> SizeIsZero (there is a size)

Done.

https://codereview.chromium.org/1163713007/diff/1/media/cdm/cenc_utils_unitte...
media/cdm/cenc_utils_unittest.cc:340:
ValidatePsshInput(std::vector<uint8_t>(data, data + arraysize(data))));
On 2015/06/04 19:57:17, ddorwin wrote:
> Is size 0 valid?

Yes. It means the box extends to the end of the data. Could be used by somebody
that creates a 'pssh' box to pass as data (like reading from a manifest) to
simplify creation (no need to count bytes).

https://codereview.chromium.org/1163713007/diff/1/media/cdm/cenc_utils_unitte...
media/cdm/cenc_utils_unittest.cc:364: EXPECT_FALSE(
On 2015/06/04 19:57:17, ddorwin wrote:
> Explain why it fails.

Done.

https://codereview.chromium.org/1163713007/diff/1/media/test/pipeline_integra...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1163713007/diff/1/media/test/pipeline_integra...
media/test/pipeline_integration_test.cc:88: const uint8 kKeyId[] = {
On 2015/06/04 19:57:17, ddorwin wrote:
> This is now only used for the rotation test (unless we start checking the
> results of the initdata parsing). I wonder if we should move it to that test
> class.

Added check that key ID matches in non-rotating test.

https://codereview.chromium.org/1163713007/diff/1/media/test/pipeline_integra...
media/test/pipeline_integration_test.cc:325: // Clear Key needs the key ID from
|init_data|.
On 2015/06/04 19:57:17, ddorwin wrote:
> We should really wait for the message and extract the ID from there. As long
as
> this works, this is simpler. Perhaps note that we're taking a shortcut.
> 
> The above is the only thing special here. Getting the key ID from the
init_data
> would work for any system if a valid request is not required, as is the case
for
> Clear Key. I suggest something like "Get the key ID for the license from
> |init_Data|."

Waiting for message means we don't need to care about the format of |init_data|.
Done.

https://codereview.chromium.org/1163713007/diff/1/media/test/pipeline_integra...
media/test/pipeline_integration_test.cc:337: bool GetKeyId(EmeInitDataType
init_data_type,
On 2015/06/04 19:57:17, ddorwin wrote:
> This can be a static function at the top.

Removed.

https://codereview.chromium.org/1163713007/diff/1/media/test/pipeline_integra...
media/test/pipeline_integration_test.cc:340: // For WebM, init_data is key_id;
for ISO CENC, init_data should
On 2015/06/04 19:57:17, ddorwin wrote:
> Function level comment.
> 
> nit: use pipes ('|')

Removed.

https://codereview.chromium.org/1163713007/diff/1/media/test/pipeline_integra...
media/test/pipeline_integration_test.cc:342: // will contain the key_id.
On 2015/06/04 19:57:18, ddorwin wrote:
> You might note that keyids does not need to be supported because it's not
> contained in files. Perhaps include the enum with NOTREACHED() below.

Removed.

https://codereview.chromium.org/1163713007/diff/1/media/test/pipeline_integra...
media/test/pipeline_integration_test.cc:343: DCHECK_GE(init_data.size(),
arraysize(kKeyId));
On 2015/06/04 19:57:17, ddorwin wrote:
> arraysize(kKeyId) looks weird. Can we define a separate const below kKeyId
that
> is kKeyIdBytes?

Removed.

https://codereview.chromium.org/1163713007/diff/1/media/test/pipeline_integra...
media/test/pipeline_integration_test.cc:363: return false;
On 2015/06/04 19:57:17, ddorwin wrote:
> Should we add this?
> NOTREACHED() << "Attempted to use MP4 file without USE_PROPRIETARY_CODECS"

Removed.

https://codereview.chromium.org/1163713007/diff/1/media/test/pipeline_integra...
media/test/pipeline_integration_test.cc:367: // Nothing else is supported.
On 2015/06/04 19:57:17, ddorwin wrote:
> NOTREACHED?

Removed.

https://codereview.chromium.org/1163713007/diff/1/media/test/pipeline_integra...
media/test/pipeline_integration_test.cc:371: return false;
On 2015/06/04 19:57:18, ddorwin wrote:
> You can move this up to 368. Or remove default so compilers can catch missing
> values.

Removed.

https://codereview.chromium.org/1163713007/diff/1/media/test/pipeline_integra...
media/test/pipeline_integration_test.cc:426: // The Key and KeyId for this
testing key provider are created by left
On 2015/06/04 19:57:17, ddorwin wrote:
> Do we still need to do this for the key ID since we can extract it
successfully
> now?

Yes. The key for the rotated key ID is |kSecretKey| rotated by the same amount.
We could prebuild a table with all rotations, but it doesn't seem necessary.

[sandersd (OOO until July 31), 2015-06-16 22:16:41]

lgtm

[ddorwin, 2015-06-17 22:42:06]

https://codereview.chromium.org/1163713007/diff/40001/media/cdm/cenc_utils_un...
File media/cdm/cenc_utils_unittest.cc (right):

https://codereview.chromium.org/1163713007/diff/40001/media/cdm/cenc_utils_un...
media/cdm/cenc_utils_unittest.cc:269: TEST_F(CencUtilsTest, TruncatedAtEndPSSH)
{
// Tries every size less than the actual size.

I'm not sure that's necessary, but okay.

https://codereview.chromium.org/1163713007/diff/40001/media/cdm/cenc_utils_un...
media/cdm/cenc_utils_unittest.cc:269: TEST_F(CencUtilsTest, TruncatedAtEndPSSH)
{
PsshBoxLessThanSize might be better. Oh, I misunderstood the test. It's actually
a Size that is less than the box! Even more important to accurately describe it.

https://codereview.chromium.org/1163713007/diff/40001/media/cdm/cenc_utils_un...
media/cdm/cenc_utils_unittest.cc:269: TEST_F(CencUtilsTest, TruncatedAtEndPSSH)
{
is there a test where size is greater than the box?

https://codereview.chromium.org/1163713007/diff/40001/media/cdm/cenc_utils_un...
media/cdm/cenc_utils_unittest.cc:280: TEST_F(CencUtilsTest,
TruncatedAtStartPSSH) {
ditto

https://codereview.chromium.org/1163713007/diff/40001/media/cdm/cenc_utils_un...
media/cdm/cenc_utils_unittest.cc:379: // These tests fail as the box size is
huge (0xffffffffffffffff) and there
nit: s/tests/calls/
"tests" could be ambiguous.

[jrummell, 2015-06-17 23:54:10]

Updated.

https://codereview.chromium.org/1163713007/diff/40001/media/cdm/cenc_utils_un...
File media/cdm/cenc_utils_unittest.cc (right):

https://codereview.chromium.org/1163713007/diff/40001/media/cdm/cenc_utils_un...
media/cdm/cenc_utils_unittest.cc:269: TEST_F(CencUtilsTest, TruncatedAtEndPSSH)
{
On 2015/06/17 22:42:05, ddorwin wrote:
> PsshBoxLessThanSize might be better. Oh, I misunderstood the test. It's
actually
> a Size that is less than the box! Even more important to accurately describe
it.

Done.

https://codereview.chromium.org/1163713007/diff/40001/media/cdm/cenc_utils_un...
media/cdm/cenc_utils_unittest.cc:269: TEST_F(CencUtilsTest, TruncatedAtEndPSSH)
{
On 2015/06/17 22:42:06, ddorwin wrote:
> // Tries every size less than the actual size.
> 
> I'm not sure that's necessary, but okay.

Done.

https://codereview.chromium.org/1163713007/diff/40001/media/cdm/cenc_utils_un...
media/cdm/cenc_utils_unittest.cc:269: TEST_F(CencUtilsTest, TruncatedAtEndPSSH)
{
On 2015/06/17 22:42:05, ddorwin wrote:
> is there a test where size is greater than the box?

Added.

https://codereview.chromium.org/1163713007/diff/40001/media/cdm/cenc_utils_un...
media/cdm/cenc_utils_unittest.cc:280: TEST_F(CencUtilsTest,
TruncatedAtStartPSSH) {
On 2015/06/17 22:42:05, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/1163713007/diff/40001/media/cdm/cenc_utils_un...
media/cdm/cenc_utils_unittest.cc:379: // These tests fail as the box size is
huge (0xffffffffffffffff) and there
On 2015/06/17 22:42:06, ddorwin wrote:
> nit: s/tests/calls/
> "tests" could be ambiguous.

Done.

[ddorwin, 2015-06-18 01:02:15]

https://codereview.chromium.org/1163713007/diff/60001/media/cdm/cenc_utils_un...
File media/cdm/cenc_utils_unittest.cc (right):

https://codereview.chromium.org/1163713007/diff/60001/media/cdm/cenc_utils_un...
media/cdm/cenc_utils_unittest.cc:273: // Tries every size less than the actual
size.
This was my previous interpretation (perhaps based on the comment below), but I
think it is the opposite that you are testing.

https://codereview.chromium.org/1163713007/diff/60001/media/cdm/cenc_utils_un...
media/cdm/cenc_utils_unittest.cc:275: // Modify size of box passed to be less
than real size.
"Modify size of box" confused me as modifying the size value. Perhaps "Truncate
the box to be..."

https://codereview.chromium.org/1163713007/diff/60001/media/cdm/cenc_utils_un...
media/cdm/cenc_utils_unittest.cc:300: TEST_F(CencUtilsTest,
PsshBoxOffsetFromStart) {
You could probably just test that anything that doesn't start with 'pssh' fails.

[jrummell, 2015-06-18 02:04:12]

Updated.

https://codereview.chromium.org/1163713007/diff/60001/media/cdm/cenc_utils_un...
File media/cdm/cenc_utils_unittest.cc (right):

https://codereview.chromium.org/1163713007/diff/60001/media/cdm/cenc_utils_un...
media/cdm/cenc_utils_unittest.cc:273: // Tries every size less than the actual
size.
On 2015/06/18 01:02:15, ddorwin wrote:
> This was my previous interpretation (perhaps based on the comment below), but
I
> think it is the opposite that you are testing.

Modified.

https://codereview.chromium.org/1163713007/diff/60001/media/cdm/cenc_utils_un...
media/cdm/cenc_utils_unittest.cc:275: // Modify size of box passed to be less
than real size.
On 2015/06/18 01:02:15, ddorwin wrote:
> "Modify size of box" confused me as modifying the size value. Perhaps
"Truncate
> the box to be..."

Done.

https://codereview.chromium.org/1163713007/diff/60001/media/cdm/cenc_utils_un...
media/cdm/cenc_utils_unittest.cc:300: TEST_F(CencUtilsTest,
PsshBoxOffsetFromStart) {
On 2015/06/18 01:02:14, ddorwin wrote:
> You could probably just test that anything that doesn't start with 'pssh'
fails.

Removed as testing non-pssh boxes is done in
https://codereview.chromium.org/1189073004/.

[ddorwin, 2015-06-19 20:38:52]

LGTM with comment. Thanks.

https://codereview.chromium.org/1163713007/diff/80001/media/cdm/cenc_utils_un...
File media/cdm/cenc_utils_unittest.cc (right):

https://codereview.chromium.org/1163713007/diff/80001/media/cdm/cenc_utils_un...
media/cdm/cenc_utils_unittest.cc:273: // Tries every buffer size less than the
actual 'pssh' box size.
s/actual/indicated/?

[jrummell, 2015-06-19 23:03:42]

Thanks for the reviews.

https://codereview.chromium.org/1163713007/diff/80001/media/cdm/cenc_utils_un...
File media/cdm/cenc_utils_unittest.cc (right):

https://codereview.chromium.org/1163713007/diff/80001/media/cdm/cenc_utils_un...
media/cdm/cenc_utils_unittest.cc:273: // Tries every buffer size less than the
actual 'pssh' box size.
On 2015/06/19 20:38:52, ddorwin wrote:
> s/actual/indicated/?

Done.

[jrummell, 2015-06-19 23:03:48]

The CQ bit was checked by jrummell@chromium.org

[jrummell, 2015-06-19 23:03:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
sandersd@chromium.org, ddorwin@chromium.org
Link to the patchset: https://codereview.chromium.org/1163713007/#ps100001
(title: "change comment")

[commit-bot: I haz the power, 2015-06-19 23:06:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1163713007/100001

[commit-bot: I haz the power, 2015-06-20 00:47:43]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-06-20 00:48:51]

Patchset 6 (id:??) landed as
https://crrev.com/3b08f4973d951753a94c51ec934e01ff3a214a04
Cr-Commit-Position: refs/heads/master@{#335405}