Use 'pssh' data to determine key_id properly

media/test/pipeline_integration_test.cc

Index: media/test/pipeline_integration_test.cc
diff --git a/media/test/pipeline_integration_test.cc b/media/test/pipeline_integration_test.cc
index 78ebfe053dde6cb089d68c5a68193a5562a6b024..054dcbebd7872ad31250c0e9fb5ca9461eea97c7 100644
--- a/media/test/pipeline_integration_test.cc
+++ b/media/test/pipeline_integration_test.cc
@@ -24,6 +24,10 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "url/gurl.h"
 
+#if defined(USE_PROPRIETARY_CODECS)
+#include "media/cdm/cenc_utils.h"
+#endif
+
 #if defined(MOJO_RENDERER)
 #include "media/mojo/services/mojo_renderer_impl.h"
 #include "mojo/application/public/cpp/application_impl.h"
@@ -312,41 +316,61 @@ class KeyProvidingApp : public FakeEncryptedMedia::AppBase {
     prev_init_data_ = init_data;
 
     if (current_session_id_.empty()) {
-      if (init_data_type == EmeInitDataType::CENC) {
-        // Since the 'cenc' files are not created with proper 'pssh' boxes,
-        // simply pretend that this is a webm file and pass the expected
-        // key ID as the init_data.
-        // http://crbug.com/460308
-        decryptor->CreateSessionAndGenerateRequest(
-            MediaKeys::TEMPORARY_SESSION, EmeInitDataType::WEBM,
-            std::vector<uint8>(kKeyId, kKeyId + arraysize(kKeyId)),
-            CreateSessionPromise(RESOLVED));
-      } else {
-        decryptor->CreateSessionAndGenerateRequest(
-            MediaKeys::TEMPORARY_SESSION, init_data_type, init_data,
-            CreateSessionPromise(RESOLVED));
-      }
+      decryptor->CreateSessionAndGenerateRequest(
+          MediaKeys::TEMPORARY_SESSION, init_data_type, init_data,
+          CreateSessionPromise(RESOLVED));
       EXPECT_FALSE(current_session_id_.empty());
     }
 
-    // Clear Key really needs the key ID from |init_data|. For WebM, they are
-    // the same, but this is not the case for ISO CENC (key ID embedded in a
-    // 'pssh' box). Therefore, provide the correct key ID.
-    const uint8* key_id = vector_as_array(&init_data);
-    size_t key_id_length = init_data.size();
-    if (init_data_type == EmeInitDataType::CENC) {
-      key_id = kKeyId;
-      key_id_length = arraysize(kKeyId);
-    }
+    // Clear Key needs the key ID from |init_data|.

[ddorwin, 2015/06/04 19:57:17]

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

[jrummell, 2015/06/15 22:22:10]

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

+    std::vector<uint8> key_id;
+    EXPECT_TRUE(GetKeyId(init_data_type, init_data, &key_id));
 
     // Convert key into a JSON structure and then add it.
-    std::string jwk = GenerateJWKSet(
-        kSecretKey, arraysize(kSecretKey), key_id, key_id_length);
+    std::string jwk = GenerateJWKSet(kSecretKey, arraysize(kSecretKey),
+                                     vector_as_array(&key_id), key_id.size());
     decryptor->UpdateSession(current_session_id_,
                              std::vector<uint8>(jwk.begin(), jwk.end()),
                              CreatePromise(RESOLVED));
   }
 
+  bool GetKeyId(EmeInitDataType init_data_type,

[ddorwin, 2015/06/04 19:57:17]

This can be a static function at the top.

[jrummell, 2015/06/15 22:22:10]

Removed.

+                const std::vector<uint8>& init_data,
+                std::vector<uint8>* key_id) {
+    // For WebM, init_data is key_id; for ISO CENC, init_data should

[ddorwin, 2015/06/04 19:57:17]

Function level comment.

nit: use pipes ('|')

[jrummell, 2015/06/15 22:22:10]

Removed.

+    // contain a 'pssh' box for the Common Encryption SystemID which
+    // will contain the key_id.

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

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.

[jrummell, 2015/06/15 22:22:10]

Removed.

+    DCHECK_GE(init_data.size(), arraysize(kKeyId));

[ddorwin, 2015/06/04 19:57:17]

arraysize(kKeyId) looks weird. Can we define a separate const below kKeyId that
is kKeyIdBytes?

[jrummell, 2015/06/15 22:22:10]

Removed.

+    DCHECK_EQ(key_id->size(), 0u);
+
+    switch (init_data_type) {
+      case EmeInitDataType::WEBM:
+        DCHECK_EQ(init_data.size(), arraysize(kKeyId));
+        key_id->assign(init_data.begin(), init_data.end());
+        return true;
+
+      case EmeInitDataType::CENC:
+#if defined(USE_PROPRIETARY_CODECS)
+        // |init_data| is a set of 0 or more concatenated 'pssh' boxes.
+        {
+          std::vector<std::vector<uint8_t>> keys;
+          EXPECT_TRUE(GetKeyIdsForCommonSystemId(init_data, &keys));
+          if (!keys.empty())
+            key_id->assign(keys[0].begin(), keys[0].end());
+          return !keys.empty();
+        }
+#else
+        return false;

[ddorwin, 2015/06/04 19:57:17]

Should we add this?
NOTREACHED() << "Attempted to use MP4 file without USE_PROPRIETARY_CODECS"

[jrummell, 2015/06/15 22:22:10]

Removed.

+#endif
+
+      default:
+        // Nothing else is supported.

[ddorwin, 2015/06/04 19:57:17]

NOTREACHED?

[jrummell, 2015/06/15 22:22:10]

Removed.

+        break;
+    }
+
+    return false;

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

You can move this up to 368. Or remove default so compilers can catch missing
values.

[jrummell, 2015/06/15 22:22:10]

Removed.

+  }
+
   std::string current_session_id_;
   std::vector<uint8> prev_init_data_;
 };
@@ -371,21 +395,11 @@ class RotatingKeyProvidingApp : public KeyProvidingApp {
 
     std::vector<uint8> key_id;
     std::vector<uint8> key;
-    EXPECT_TRUE(GetKeyAndKeyId(init_data, &key, &key_id));
+    EXPECT_TRUE(GetKeyAndKeyId(init_data_type, init_data, &key, &key_id));
 
-    if (init_data_type == EmeInitDataType::CENC) {
-      // Since the 'cenc' files are not created with proper 'pssh' boxes,
-      // simply pretend that this is a webm file and pass the expected
-      // key ID as the init_data.
-      // http://crbug.com/460308
-      decryptor->CreateSessionAndGenerateRequest(
-          MediaKeys::TEMPORARY_SESSION, EmeInitDataType::WEBM, key_id,
-          CreateSessionPromise(RESOLVED));
-    } else {
-      decryptor->CreateSessionAndGenerateRequest(
-          MediaKeys::TEMPORARY_SESSION, init_data_type, init_data,
-          CreateSessionPromise(RESOLVED));
-    }
+    decryptor->CreateSessionAndGenerateRequest(MediaKeys::TEMPORARY_SESSION,
+                                               init_data_type, init_data,
+                                               CreateSessionPromise(RESOLVED));
 
     // Convert key into a JSON structure and then add it.
     std::string jwk = GenerateJWKSet(vector_as_array(&key),
@@ -398,15 +412,13 @@ class RotatingKeyProvidingApp : public KeyProvidingApp {
   }
 
  private:
-  bool GetKeyAndKeyId(std::vector<uint8> init_data,
+  bool GetKeyAndKeyId(EmeInitDataType init_data_type,
+                      const std::vector<uint8>& init_data,
                       std::vector<uint8>* key,
                       std::vector<uint8>* key_id) {
-    // For WebM, init_data is key_id; for ISO CENC, init_data should contain
-    // the key_id. We assume key_id is in the end of init_data here (that is
-    // only a reasonable assumption for WebM and clear key ISO CENC).
     DCHECK_GE(init_data.size(), arraysize(kKeyId));
-    std::vector<uint8> key_id_from_init_data(
-        init_data.end() - arraysize(kKeyId), init_data.end());
+    std::vector<uint8> key_id_from_init_data;
+    EXPECT_TRUE(GetKeyId(init_data_type, init_data, &key_id_from_init_data));
 
     key->assign(kSecretKey, kSecretKey + arraysize(kSecretKey));
     key_id->assign(kKeyId, kKeyId + arraysize(kKeyId));