Add MediaDrmBridge::AddKeySystemUuidMapping().

media/base/android/media_drm_bridge.cc

Index: media/base/android/media_drm_bridge.cc
diff --git a/media/base/android/media_drm_bridge.cc b/media/base/android/media_drm_bridge.cc
index dbcd1871b81b542ade6750b9b5247366dba2758a..3bef42eb1066218fbbf29e537ff58414a7e9f6d6 100644
--- a/media/base/android/media_drm_bridge.cc
+++ b/media/base/android/media_drm_bridge.cc
@@ -27,14 +27,16 @@ using base::android::ScopedJavaLocalRef;
 
 namespace media {
 
-static uint32 ReadUint32(const uint8_t* data) {
+namespace {

[xhwang, 2014/05/15 15:10:29]

In media code, we use "static" a lot. Some people prefer this to anonymous
namespaces.

[ycheo (away), 2014/05/16 04:53:38]

Got it. I'll revert it to static.

+
+uint32 ReadUint32(const uint8_t* data) {
   uint32 value = 0;
   for (int i = 0; i < 4; ++i)
     value = (value << 8) | data[i];
   return value;
 }
 
-static uint64 ReadUint64(const uint8_t* data) {
+uint64 ReadUint64(const uint8_t* data) {
   uint64 value = 0;
   for (int i = 0; i < 8; ++i)
     value = (value << 8) | data[i];
@@ -65,13 +67,40 @@ const uint8 kWidevineUuid[16] = {
     0xED, 0xEF, 0x8B, 0xA9, 0x79, 0xD6, 0x4A, 0xCE,
     0xA3, 0xC8, 0x27, 0xDC, 0xD5, 0x1D, 0x21, 0xED };
 
-static std::vector<uint8> GetUUID(const std::string& key_system) {
-  // For security reasons, we only do exact string comparisons here - we don't
-  // try to parse the |key_system| in any way.
-  if (key_system == kWidevineKeySystem) {
-    return std::vector<uint8>(kWidevineUuid,
-                              kWidevineUuid + arraysize(kWidevineUuid));
+typedef std::pair<std::string, std::vector<uint8> > KeySystemUuidPair;
+// Use std::vector<>, not std::map<> since the size of the table will be
+// expected to be resonably small.
+typedef std::vector<KeySystemUuidPair> KeySystemUuidTable;

[xhwang, 2014/05/15 15:10:29]

Since we use UUID a lot now, how about

typedef std::vector<uint8> UUID?

Then can you use a base::hash_map<std::string, UUID> instead of vector of pairs?

[ddorwin, 2014/05/15 19:00:45]

This would also allow s/Table/Map/. "Map" is used a lot in the media code. I
don't think "Table" is.

[ycheo (away), 2014/05/16 04:53:38]

Done.

[ycheo (away), 2014/05/16 04:53:38]

Done.

+KeySystemUuidTable* key_system_uuid_table = NULL;

[xhwang, 2014/05/15 15:10:29]

global variable needs to be named as g_foo

[ycheo (away), 2014/05/16 04:53:38]

Done.

+
+void InitializeKeySystemUuidTable() {
+  key_system_uuid_table = new KeySystemUuidTable();
+
+  key_system_uuid_table->push_back(make_pair(
+      kWidevineKeySystem,
+      std::vector<uint8>(kWidevineUuid,
+                         kWidevineUuid + arraysize(kWidevineUuid))));
+}
+
+KeySystemUuidTable::iterator FindKeySystem(const std::string& key_system) {

[ddorwin, 2014/05/15 19:00:45]

This name does not accurately describe the behavior. It "gets an iterator to a
pair in a table". That's an odd thing to return, especially since the callers
don't want an iterator or even the pair.

I think it should be called GetUuid() and either return false or an empty UUID
when not found.

[ycheo (away), 2014/05/16 04:53:38]

The method is removed by using hash_map.

+  KeySystemUuidTable::iterator it;
+  for (it = key_system_uuid_table->begin();
+       it != key_system_uuid_table->end(); ++it) {
+    // For security reasons, we only do exact string comparisons here - we don't
+    // try to parse the |key_system| in any way.
+    if (key_system == it->first)
+      return it;
   }
+  return it;
+}

[xhwang, 2014/05/15 15:10:29]

If you use a hashmap, you don't need this function.

[ycheo (away), 2014/05/16 04:53:38]

Done.

+
+std::vector<uint8> GetUUID(const std::string& key_system) {
+  if (key_system_uuid_table == NULL)

[ddorwin, 2014/05/15 19:00:45]

This code appears twice. Why not initialize the table in the constructor?

[ycheo (away), 2014/05/16 04:53:38]

Removed by using the lazy initializer.

+    InitializeKeySystemUuidTable();
+
+  KeySystemUuidTable::iterator it = FindKeySystem(key_system);
+  if (it != key_system_uuid_table->end())
+    return it->second;
   return std::vector<uint8>();

[xhwang, 2014/05/15 15:10:29]

nit: we like to handle abnormal cases first, i.e.

if (not found)
  return UUID();

return found_uuid;

[ycheo (away), 2014/05/16 04:53:38]

Done.

 }
 
@@ -82,9 +111,9 @@ static std::vector<uint8> GetUUID(const std::string& key_system) {
 // 1, If multiple PSSH boxes are found,the "Data" of the first matching PSSH box
 // will be set in |pssh_data|.
 // 2, Only PSSH and TENC boxes are allowed in |data|. TENC boxes are skipped.
-static bool GetPsshData(const uint8* data, int data_size,
-                        const std::vector<uint8>& uuid,
-                        std::vector<uint8>* pssh_data) {
+bool GetPsshData(const uint8* data, int data_size,
+                 const std::vector<uint8>& uuid,
+                 std::vector<uint8>* pssh_data) {
   const uint8* cur = data;
   const uint8* data_end = data + data_size;
   int bytes_left = data_size;
@@ -159,7 +188,7 @@ static bool GetPsshData(const uint8* data, int data_size,
   return false;
 }
 
-static MediaDrmBridge::SecurityLevel GetSecurityLevelFromString(
+MediaDrmBridge::SecurityLevel GetSecurityLevelFromString(
     const std::string& security_level_str) {
   if (0 == security_level_str.compare("L1"))
     return MediaDrmBridge::SECURITY_LEVEL_1;
@@ -169,7 +198,7 @@ static MediaDrmBridge::SecurityLevel GetSecurityLevelFromString(
   return MediaDrmBridge::SECURITY_LEVEL_NONE;
 }
 
-static std::string GetSecurityLevelString(
+std::string GetSecurityLevelString(
     MediaDrmBridge::SecurityLevel security_level) {
   switch (security_level) {
     case MediaDrmBridge::SECURITY_LEVEL_NONE:
@@ -187,7 +216,7 @@ static std::string GetSecurityLevelString(
 // TODO(xhwang): The |container_mime_type| is not the same as contentType in
 // the EME spec. Revisit this once the spec issue with initData type is
 // resolved.
-static bool IsKeySystemSupportedWithTypeImpl(
+bool IsKeySystemSupportedWithTypeImpl(
     const std::string& key_system,
     const std::string& container_mime_type) {
   if (!MediaDrmBridge::IsAvailable())
@@ -206,6 +235,8 @@ static bool IsKeySystemSupportedWithTypeImpl(
       env, j_scheme_uuid.obj(), j_container_mime_type.obj());
 }
 
+}  // namespace
+
 // static
 bool MediaDrmBridge::IsAvailable() {
   return base::android::BuildInfo::GetInstance()->sdk_int() >= 19;
@@ -233,6 +264,18 @@ bool MediaDrmBridge::IsSecurityLevelSupported(const std::string& key_system,
   return media_drm_bridge->SetSecurityLevel(security_level);
 }
 
+//static
+void MediaDrmBridge::AddKeySystem(const std::string& key_system,

[xhwang, 2014/05/15 15:10:29]

nit: This name is ambiguous. People may think that this is adding a key system
support. Actually it's just adding a UUID mapping.

[ycheo (away), 2014/05/16 04:53:38]

Renamed AddKeySystemUuidMapping().

+                                  const std::vector<uint8>& uuid) {
+  if (key_system_uuid_table == NULL)
+    InitializeKeySystemUuidTable();
+
+  KeySystemUuidTable::iterator it = FindKeySystem(key_system);
+  // Shouldn't overwrite the existing keysystem.

[xhwang, 2014/05/15 15:10:29]

DCHECK this condition as well?

[ycheo (away), 2014/05/16 04:53:38]

Done.

+  if (it == key_system_uuid_table->end())
+    key_system_uuid_table->push_back(make_pair(key_system, uuid));
+}
+
 // static
 bool MediaDrmBridge::IsKeySystemSupported(const std::string& key_system) {
   DCHECK(!key_system.empty());