|
|
Created:
6 years, 7 months ago by ycheo (away) Modified:
6 years, 7 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, avayvod+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd MediaDrmBridge::AddKeySystemUuidMapping().
This method is needed to inject the other DRMs' keysystem and uuid mapping
in MediaDrmBridge.
BUG=322395
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272498
Patch Set 1 : #
Total comments: 26
Patch Set 2 : addressed reviewers' comments. #
Total comments: 10
Patch Set 3 : addressed xhwang's comments. #
Messages
Total messages: 10 (0 generated)
PTAL.
https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... File media/base/android/media_drm_bridge.cc (right): https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... media/base/android/media_drm_bridge.cc:30: namespace { In media code, we use "static" a lot. Some people prefer this to anonymous namespaces. https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... media/base/android/media_drm_bridge.cc:73: typedef std::vector<KeySystemUuidPair> KeySystemUuidTable; 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? https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... media/base/android/media_drm_bridge.cc:74: KeySystemUuidTable* key_system_uuid_table = NULL; global variable needs to be named as g_foo https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... media/base/android/media_drm_bridge.cc:95: } If you use a hashmap, you don't need this function. https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... media/base/android/media_drm_bridge.cc:104: return std::vector<uint8>(); nit: we like to handle abnormal cases first, i.e. if (not found) return UUID(); return found_uuid; https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... media/base/android/media_drm_bridge.cc:268: void MediaDrmBridge::AddKeySystem(const std::string& key_system, nit: This name is ambiguous. People may think that this is adding a key system support. Actually it's just adding a UUID mapping. https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... media/base/android/media_drm_bridge.cc:274: // Shouldn't overwrite the existing keysystem. DCHECK this condition as well? https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... File media/base/android/media_drm_bridge_unittest.cc (right): https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... media/base/android/media_drm_bridge_unittest.cc:29: const char kAcmeKeySystem[] = "com.acme.keysystem"; s/acme/foo or example to make it more obvious? https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... media/base/android/media_drm_bridge_unittest.cc:104: // Use WV uuid, because it is the only MediaDrm we can guarentee s/uuid/UUID s/MediaDrm/key system https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... media/base/android/media_drm_bridge_unittest.cc:105: // that it is installed in the target device. nit: target/test
https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... File media/base/android/media_drm_bridge.cc (right): https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... media/base/android/media_drm_bridge.cc:73: typedef std::vector<KeySystemUuidPair> KeySystemUuidTable; On 2014/05/15 15:10:29, xhwang wrote: > 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? This would also allow s/Table/Map/. "Map" is used a lot in the media code. I don't think "Table" is. https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... media/base/android/media_drm_bridge.cc:85: KeySystemUuidTable::iterator FindKeySystem(const std::string& key_system) { 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. https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... media/base/android/media_drm_bridge.cc:98: if (key_system_uuid_table == NULL) This code appears twice. Why not initialize the table in the constructor?
PTAL. I changed vector<> to hash_map<>. And also I wrapped the related functions with a new class KeySystemUuidManager. Actually, I wonder if I need to extract this to a separate file. https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... File media/base/android/media_drm_bridge.cc (right): https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... media/base/android/media_drm_bridge.cc:30: namespace { On 2014/05/15 15:10:29, xhwang wrote: > In media code, we use "static" a lot. Some people prefer this to anonymous > namespaces. Got it. I'll revert it to static. https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... media/base/android/media_drm_bridge.cc:73: typedef std::vector<KeySystemUuidPair> KeySystemUuidTable; On 2014/05/15 15:10:29, xhwang wrote: > 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? Done. https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... media/base/android/media_drm_bridge.cc:73: typedef std::vector<KeySystemUuidPair> KeySystemUuidTable; On 2014/05/15 19:00:45, ddorwin wrote: > On 2014/05/15 15:10:29, xhwang wrote: > > 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? > > This would also allow s/Table/Map/. "Map" is used a lot in the media code. I > don't think "Table" is. Done. https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... media/base/android/media_drm_bridge.cc:74: KeySystemUuidTable* key_system_uuid_table = NULL; On 2014/05/15 15:10:29, xhwang wrote: > global variable needs to be named as g_foo Done. https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... media/base/android/media_drm_bridge.cc:85: KeySystemUuidTable::iterator FindKeySystem(const std::string& key_system) { On 2014/05/15 19:00:45, ddorwin wrote: > 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. The method is removed by using hash_map. https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... media/base/android/media_drm_bridge.cc:95: } On 2014/05/15 15:10:29, xhwang wrote: > If you use a hashmap, you don't need this function. Done. https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... media/base/android/media_drm_bridge.cc:98: if (key_system_uuid_table == NULL) On 2014/05/15 19:00:45, ddorwin wrote: > This code appears twice. Why not initialize the table in the constructor? Removed by using the lazy initializer. https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... media/base/android/media_drm_bridge.cc:104: return std::vector<uint8>(); On 2014/05/15 15:10:29, xhwang wrote: > nit: we like to handle abnormal cases first, i.e. > > if (not found) > return UUID(); > > return found_uuid; Done. https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... media/base/android/media_drm_bridge.cc:268: void MediaDrmBridge::AddKeySystem(const std::string& key_system, On 2014/05/15 15:10:29, xhwang wrote: > nit: This name is ambiguous. People may think that this is adding a key system > support. Actually it's just adding a UUID mapping. Renamed AddKeySystemUuidMapping(). https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... media/base/android/media_drm_bridge.cc:274: // Shouldn't overwrite the existing keysystem. On 2014/05/15 15:10:29, xhwang wrote: > DCHECK this condition as well? Done. https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... File media/base/android/media_drm_bridge_unittest.cc (right): https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... media/base/android/media_drm_bridge_unittest.cc:29: const char kAcmeKeySystem[] = "com.acme.keysystem"; On 2014/05/15 15:10:29, xhwang wrote: > s/acme/foo or example to make it more obvious? Done. https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... media/base/android/media_drm_bridge_unittest.cc:104: // Use WV uuid, because it is the only MediaDrm we can guarentee On 2014/05/15 15:10:29, xhwang wrote: > s/uuid/UUID > > s/MediaDrm/key system Done. https://codereview.chromium.org/284183003/diff/20001/media/base/android/media... media/base/android/media_drm_bridge_unittest.cc:105: // that it is installed in the target device. On 2014/05/15 15:10:29, xhwang wrote: > nit: target/test Done.
PTAL.
lgtm % nits https://codereview.chromium.org/284183003/diff/90001/media/base/android/media... File media/base/android/media_drm_bridge.cc (right): https://codereview.chromium.org/284183003/diff/90001/media/base/android/media... media/base/android/media_drm_bridge.cc:69: typedef std::vector<uint8> UUID; nit: Add an empty line after this. https://codereview.chromium.org/284183003/diff/90001/media/base/android/media... media/base/android/media_drm_bridge.cc:77: typedef base::hash_map<std::string, UUID> KeySystemUuidMap; add include for hash_map https://codereview.chromium.org/284183003/diff/90001/media/base/android/media... media/base/android/media_drm_bridge.cc:85: key_system_uuid_map_[kWidevineKeySystem] = Add a comment that Widevine is always supported. https://codereview.chromium.org/284183003/diff/90001/media/base/android/media... media/base/android/media_drm_bridge.cc:100: << "Shouldn't overwrite the existing keysystem"; nit: s/the/an/ nit: s/keysystem/key system/ nit: add period at the end https://codereview.chromium.org/284183003/diff/90001/media/base/android/media... media/base/android/media_drm_bridge.cc:268: void MediaDrmBridge::AddKeySystemUuidMapping(const std::string& key_system, Add a comment that this gives other platforms a chance to register their own UUID mappings. Otherwise people may wonder how/when this is used.
Thanks for the review. https://codereview.chromium.org/284183003/diff/90001/media/base/android/media... File media/base/android/media_drm_bridge.cc (right): https://codereview.chromium.org/284183003/diff/90001/media/base/android/media... media/base/android/media_drm_bridge.cc:69: typedef std::vector<uint8> UUID; On 2014/05/21 17:11:08, xhwang wrote: > nit: Add an empty line after this. Done. https://codereview.chromium.org/284183003/diff/90001/media/base/android/media... media/base/android/media_drm_bridge.cc:77: typedef base::hash_map<std::string, UUID> KeySystemUuidMap; On 2014/05/21 17:11:08, xhwang wrote: > add include for hash_map Done. https://codereview.chromium.org/284183003/diff/90001/media/base/android/media... media/base/android/media_drm_bridge.cc:85: key_system_uuid_map_[kWidevineKeySystem] = On 2014/05/21 17:11:08, xhwang wrote: > Add a comment that Widevine is always supported. Done. https://codereview.chromium.org/284183003/diff/90001/media/base/android/media... media/base/android/media_drm_bridge.cc:100: << "Shouldn't overwrite the existing keysystem"; On 2014/05/21 17:11:08, xhwang wrote: > nit: s/the/an/ > nit: s/keysystem/key system/ > nit: add period at the end Done. https://codereview.chromium.org/284183003/diff/90001/media/base/android/media... media/base/android/media_drm_bridge.cc:268: void MediaDrmBridge::AddKeySystemUuidMapping(const std::string& key_system, On 2014/05/21 17:11:08, xhwang wrote: > Add a comment that this gives other platforms a chance to register their own > UUID mappings. Otherwise people may wonder how/when this is used. Applied this on the header.
The CQ bit was checked by ycheo@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ycheo@chromium.org/284183003/110001
Message was sent while issue was closed.
Change committed as 272498 |