Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
193523002:
Encrypted Media: Implement IPC based SetCdm(). (Closed)
|
Created:
6 years, 9 months ago by xhwang Modified:
6 years, 8 months ago CC:
chromium-reviews, fischman+watch_chromium.org, jam, mcasas+watch_chromium.org, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionEncrypted Media: Implement IPC based SetCdm().
BUG=338910
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260592
Patch Set 1 #
Total comments: 54
Patch Set 2 : comments addressed #
Total comments: 7
Patch Set 3 : comments addressed #
Total comments: 30
Patch Set 4 : Manage cdm_id_ in CdmSessionAdapter #
Total comments: 6
Patch Set 5 : comments addressed #
Total comments: 12
Patch Set 6 : comments addressed #Patch Set 7 : rebase only #
Total comments: 7
Patch Set 8 : comments addressed #
Total comments: 2
Patch Set 9 : comments addressed #Patch Set 10 : rebase only #
Total comments: 1
Messages
Total messages: 51 (0 generated)
PTAL https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:1350: manager_->SetMediaKeys(player_id_, cdm_id); This is kind of ugly; we'll need some cleanup. But this is the easiest we can have now. Let me know what you think about it :)
https://codereview.chromium.org/193523002/diff/1/content/browser/media/androi... File content/browser/media/android/browser_media_player_manager.h (right): https://codereview.chromium.org/193523002/diff/1/content/browser/media/androi... content/browser/media/android/browser_media_player_manager.h:232: std::map<int, int> pending_set_media_keys_map_; This is used when we call SetMediaKeys() before we call video.load(). The Player is created only during video.load().
Let's discuss. Maybe I'm missing something. https://codereview.chromium.org/193523002/diff/1/content/browser/media/androi... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/193523002/diff/1/content/browser/media/androi... content/browser/media/android/browser_media_player_manager.cc:538: int cdm_id = pending_set_media_keys_map_[player_id]; Better to save a temp iterator above? https://codereview.chromium.org/193523002/diff/1/content/browser/media/androi... content/browser/media/android/browser_media_player_manager.cc:539: MediaDrmBridge* drm_bridge = GetDrmBridge(cdm_id); Should we erase it from the map? Otherwise, we'll grow indefinitely, right? https://codereview.chromium.org/193523002/diff/1/content/browser/media/androi... content/browser/media/android/browser_media_player_manager.cc:540: if (!drm_bridge) Should this ever fail? https://codereview.chromium.org/193523002/diff/1/content/browser/media/androi... content/browser/media/android/browser_media_player_manager.cc:817: // SetMediaKeys() may be called before the player is loaded. How do we know the player_id in that case? https://codereview.chromium.org/193523002/diff/1/content/browser/media/androi... File content/browser/media/android/browser_media_player_manager.h (right): https://codereview.chromium.org/193523002/diff/1/content/browser/media/androi... content/browser/media/android/browser_media_player_manager.h:232: std::map<int, int> pending_set_media_keys_map_; On 2014/03/10 23:36:32, xhwang wrote: > This is used when we call SetMediaKeys() before we call video.load(). The Player > is created only during video.load(). How do we have a player_id before video.load()? Isn't Blink set up to call setCDM() when the load occurs? https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:1227: if (!decryptor_ready_cb_.is_null()) { This is only for CK now? https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:1229: .Run(proxy_decryptor_->GetDecryptor()); proxy_decryptor_ will never be NULL in this case? (The check was removed.) https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:1232: int cdm_id = proxy_decryptor_->GetCdmId(); Do you still want to run this for CK? https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:1333: void WebMediaPlayerAndroid::setContentDecryptionModule( PSA: When adding APIs on the Blink side, let's try to remember to update all the WMP children with at least NOTREACHED or TODOs and bugs. WDYT? https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:1337: // TODO(xhwang): Support setMediaKeys(0) if necessary: http://crbug.com/330324 Just a thought: Maybe we should ask the CDM if it will allow it. https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:1348: int cdm_id = web_cdm_->GetCdmId(); ditto about CK https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:1350: manager_->SetMediaKeys(player_id_, cdm_id); On 2014/03/10 23:35:40, xhwang wrote: > This is kind of ugly; we'll need some cleanup. But this is the easiest we can > have now. Let me know what you think about it :) What in particular (that I haven't mentioned)? I think this function should be called on load, which means we shouldn't need to store the pending map. https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:1442: DCHECK(!(proxy_decryptor_ && web_cdm_)); nit: simplify to ! || ! https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:1450: decryptor_ready_cb.Run(web_cdm_->GetDecryptor()); not really just used by CK? https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... File content/renderer/media/android/webmediaplayer_android.h (right): https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.h:452: // Used by clear key implementation. Can we get rid of this somehow? https://codereview.chromium.org/193523002/diff/1/content/renderer/media/cdm_s... File content/renderer/media/cdm_session_adapter.h (right): https://codereview.chromium.org/193523002/diff/1/content/renderer/media/cdm_s... content/renderer/media/cdm_session_adapter.h:68: // Returns the CDM ID associated with this object. May be kInvalidCdmId if no Why does any of the Blink code (here and WCDM_impl) need this? https://codereview.chromium.org/193523002/diff/1/content/renderer/media/crypt... File content/renderer/media/crypto/proxy_decryptor.h (right): https://codereview.chromium.org/193523002/diff/1/content/renderer/media/crypt... content/renderer/media/crypto/proxy_decryptor.h:73: // Returns the CDM ID associated with this object. May be kInvalidCdmId if no Why does WMPI need this? https://codereview.chromium.org/193523002/diff/1/media/base/media_keys.h File media/base/media_keys.h (right): https://codereview.chromium.org/193523002/diff/1/media/base/media_keys.h#newc... media/base/media_keys.h:78: virtual int GetCdmId(); Too bad this has to be on the main interface. :(
Updated comments after discussion. https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... File content/renderer/media/android/proxy_media_keys.cc (right): https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/proxy_media_keys.cc:25: cdm_id_(cdm_id), Let's make this class responsible for initializing the CDM ID to a unique value instead of putting that on the caller, which shouldn't care. https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... File content/renderer/media/android/proxy_media_keys.h (right): https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/proxy_media_keys.h:45: virtual int GetCdmId() OVERRIDE; This will not be in the base class, but the factory will use it when it knows it has created this object. We could use a Create method that returns the ID. I'm not sure whether that will be better or worse at the factory call site. https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:20: #include "content/renderer/media/android/proxy_media_keys.h" Remove - ProxyMediaKeys is not used. We will temporarily need media_keys.h, but we should have a TODO to remove it with the prefixed APIs. (Probably ditto in WMPI.) https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:1350: manager_->SetMediaKeys(player_id_, cdm_id); I'm not sure that SetMediaKeys is the right term. We're setting the CDM or decryptor to be used by the media player. Note that MediaKeys is not actually exposed to WMPA/WMPI prior to this CL, except for the KeyError enum in the legacy OnKeyError. SetCdm is probably the better name, especially since it's the CDM ID we're passing. This also means we should not be using MediaKeys::kInvalidCdmId. https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... File content/renderer/media/android/webmediaplayer_android.h (right): https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.h:452: // Used by clear key implementation. On 2014/03/11 04:06:33, ddorwin wrote: > Can we get rid of this somehow? It sounds like no. It would be nice to have consistent calls or an explicit way to pick the right method of setting the CDM. https://codereview.chromium.org/193523002/diff/1/content/renderer/media/cdm_s... File content/renderer/media/cdm_session_adapter.h (right): https://codereview.chromium.org/193523002/diff/1/content/renderer/media/cdm_s... content/renderer/media/cdm_session_adapter.h:68: // Returns the CDM ID associated with this object. May be kInvalidCdmId if no On 2014/03/11 04:06:33, ddorwin wrote: > Why does any of the Blink code (here and WCDM_impl) need this? As discussed, this is only for being able to tell the IPC player what IPC CDM to use. Maybe we can pass this back up from Initialize() and make WCDMI store the value, avoiding passing it https://codereview.chromium.org/193523002/diff/1/content/renderer/media/crypt... File content/renderer/media/crypto/proxy_decryptor.h (right): https://codereview.chromium.org/193523002/diff/1/content/renderer/media/crypt... content/renderer/media/crypto/proxy_decryptor.h:73: // Returns the CDM ID associated with this object. May be kInvalidCdmId if no On 2014/03/11 04:06:33, ddorwin wrote: > Why does WMPI need this? We can delete this in the new design. https://codereview.chromium.org/193523002/diff/1/content/renderer/media/webco... File content/renderer/media/webcontentdecryptionmodule_impl.cc (right): https://codereview.chromium.org/193523002/diff/1/content/renderer/media/webco... content/renderer/media/webcontentdecryptionmodule_impl.cc:42: UTF16ToASCII(key_system))) { Let's have an output parameter for the CDM ID and return that at line 69. https://codereview.chromium.org/193523002/diff/1/media/base/media_keys.h File media/base/media_keys.h (right): https://codereview.chromium.org/193523002/diff/1/media/base/media_keys.h#newc... media/base/media_keys.h:78: virtual int GetCdmId(); On 2014/03/11 04:06:33, ddorwin wrote: > Too bad this has to be on the main interface. :( As discussed, I think we can avoid this.
comments addressed
https://codereview.chromium.org/193523002/diff/1/content/browser/media/androi... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/193523002/diff/1/content/browser/media/androi... content/browser/media/android/browser_media_player_manager.cc:538: int cdm_id = pending_set_media_keys_map_[player_id]; On 2014/03/11 04:06:33, ddorwin wrote: > Better to save a temp iterator above? Obsolete. https://codereview.chromium.org/193523002/diff/1/content/browser/media/androi... content/browser/media/android/browser_media_player_manager.cc:539: MediaDrmBridge* drm_bridge = GetDrmBridge(cdm_id); On 2014/03/11 04:06:33, ddorwin wrote: > Should we erase it from the map? Otherwise, we'll grow indefinitely, right? You are right. But this code is obsolete now. https://codereview.chromium.org/193523002/diff/1/content/browser/media/androi... content/browser/media/android/browser_media_player_manager.cc:540: if (!drm_bridge) On 2014/03/11 04:06:33, ddorwin wrote: > Should this ever fail? This code is obsolete. But this is possible. OnInitializeCdm() or MediaDrmBridge::Create() could fail, in which cases we do nothing. Then when update() is called, we send a session error back. https://codereview.chromium.org/193523002/diff/1/content/browser/media/androi... content/browser/media/android/browser_media_player_manager.cc:817: // SetMediaKeys() may be called before the player is loaded. On 2014/03/11 04:06:33, ddorwin wrote: > How do we know the player_id in that case? Removed. https://codereview.chromium.org/193523002/diff/1/content/browser/media/androi... File content/browser/media/android/browser_media_player_manager.h (right): https://codereview.chromium.org/193523002/diff/1/content/browser/media/androi... content/browser/media/android/browser_media_player_manager.h:232: std::map<int, int> pending_set_media_keys_map_; On 2014/03/11 04:06:33, ddorwin wrote: > On 2014/03/10 23:36:32, xhwang wrote: > > This is used when we call SetMediaKeys() before we call video.load(). The > Player > > is created only during video.load(). > > How do we have a player_id before video.load()? Isn't Blink set up to call > setCDM() when the load occurs? Removed. You are right: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... File content/renderer/media/android/proxy_media_keys.cc (right): https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/proxy_media_keys.cc:25: cdm_id_(cdm_id), On 2014/03/11 18:05:14, ddorwin wrote: > Let's make this class responsible for initializing the CDM ID to a unique value > instead of putting that on the caller, which shouldn't care. Done. https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... File content/renderer/media/android/proxy_media_keys.h (right): https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/proxy_media_keys.h:45: virtual int GetCdmId() OVERRIDE; On 2014/03/11 18:05:14, ddorwin wrote: > This will not be in the base class, but the factory will use it when it knows it > has created this object. > > We could use a Create method that returns the ID. I'm not sure whether that will > be better or worse at the factory call site. Done. I am keeping a GetCdmId() function in ProxyMediaKeys because the class needs to store the cdm_id_ anyways (for sending IPCs). https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:20: #include "content/renderer/media/android/proxy_media_keys.h" On 2014/03/11 18:05:14, ddorwin wrote: > Remove - ProxyMediaKeys is not used. > We will temporarily need media_keys.h, but we should have a TODO to remove it > with the prefixed APIs. > (Probably ditto in WMPI.) Done. https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:1227: if (!decryptor_ready_cb_.is_null()) { On 2014/03/11 04:06:33, ddorwin wrote: > This is only for CK now? Yes. https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:1229: .Run(proxy_decryptor_->GetDecryptor()); On 2014/03/11 04:06:33, ddorwin wrote: > proxy_decryptor_ will never be NULL in this case? (The check was removed.) Yes, see line 1205 and 1227. https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:1232: int cdm_id = proxy_decryptor_->GetCdmId(); On 2014/03/11 04:06:33, ddorwin wrote: > Do you still want to run this for CK? Here we don't have key system specific info. We can use |decryptor_ready_cb_.is_null()| but that's not reliable: when decryptor_ready_cb_.is_null() we may still be using clearkey; the pipeline takes some time to initialize and request a decryptor. So here I am trying to keep these two paths orthogonal. https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:1333: void WebMediaPlayerAndroid::setContentDecryptionModule( On 2014/03/11 04:06:33, ddorwin wrote: > PSA: When adding APIs on the Blink side, let's try to remember to update all the > WMP children with at least NOTREACHED or TODOs and bugs. WDYT? Agreed. My bad. https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:1337: // TODO(xhwang): Support setMediaKeys(0) if necessary: http://crbug.com/330324 On 2014/03/11 04:06:33, ddorwin wrote: > Just a thought: Maybe we should ask the CDM if it will allow it. sgtm. So setContentDecryptionModule() should return an exception? https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:1348: int cdm_id = web_cdm_->GetCdmId(); On 2014/03/11 04:06:33, ddorwin wrote: > ditto about CK ditto. https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:1350: manager_->SetMediaKeys(player_id_, cdm_id); On 2014/03/11 18:05:14, ddorwin wrote: > I'm not sure that SetMediaKeys is the right term. We're setting the CDM or > decryptor to be used by the media player. Note that MediaKeys is not actually > exposed to WMPA/WMPI prior to this CL, except for the KeyError enum in the > legacy OnKeyError. > > SetCdm is probably the better name, especially since it's the CDM ID we're > passing. This also means we should not be using MediaKeys::kInvalidCdmId. Renamed. https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:1350: manager_->SetMediaKeys(player_id_, cdm_id); On 2014/03/11 04:06:33, ddorwin wrote: > On 2014/03/10 23:35:40, xhwang wrote: > > This is kind of ugly; we'll need some cleanup. But this is the easiest we can > > have now. Let me know what you think about it :) > > What in particular (that I haven't mentioned)? > I think this function should be called on load, which means we shouldn't need to > store the pending map. I was talking about having two ways to setMediaKeys: SetCdm() and decryptor_ready_cb_.Run(). That's requires some larger scale refactoring which I'd like to think more about it and do it later. You are right about no need to have the pending map. https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:1442: DCHECK(!(proxy_decryptor_ && web_cdm_)); On 2014/03/11 04:06:33, ddorwin wrote: > nit: simplify to ! || ! Done. https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:1450: decryptor_ready_cb.Run(web_cdm_->GetDecryptor()); On 2014/03/11 04:06:33, ddorwin wrote: > not really just used by CK? ditto https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... File content/renderer/media/android/webmediaplayer_android.h (right): https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.h:452: // Used by clear key implementation. On 2014/03/11 18:05:14, ddorwin wrote: > On 2014/03/11 04:06:33, ddorwin wrote: > > Can we get rid of this somehow? > > It sounds like no. It would be nice to have consistent calls or an explicit way > to pick the right method of setting the CDM. Agreed. Sounds like a good future cleanup task :\ https://codereview.chromium.org/193523002/diff/1/content/renderer/media/cdm_s... File content/renderer/media/cdm_session_adapter.h (right): https://codereview.chromium.org/193523002/diff/1/content/renderer/media/cdm_s... content/renderer/media/cdm_session_adapter.h:68: // Returns the CDM ID associated with this object. May be kInvalidCdmId if no On 2014/03/11 18:05:14, ddorwin wrote: > On 2014/03/11 04:06:33, ddorwin wrote: > > Why does any of the Blink code (here and WCDM_impl) need this? > > As discussed, this is only for being able to tell the IPC player what IPC CDM to > use. > Maybe we can pass this back up from Initialize() and make WCDMI store the value, > avoiding passing it Done. https://codereview.chromium.org/193523002/diff/1/content/renderer/media/crypt... File content/renderer/media/crypto/proxy_decryptor.h (right): https://codereview.chromium.org/193523002/diff/1/content/renderer/media/crypt... content/renderer/media/crypto/proxy_decryptor.h:73: // Returns the CDM ID associated with this object. May be kInvalidCdmId if no On 2014/03/11 18:05:14, ddorwin wrote: > On 2014/03/11 04:06:33, ddorwin wrote: > > Why does WMPI need this? > > We can delete this in the new design. I am still storing |cdm_id_| in ProxyDecryptor. For unprefixed EME, we are storing it in WebCDMImpl. https://codereview.chromium.org/193523002/diff/1/content/renderer/media/webco... File content/renderer/media/webcontentdecryptionmodule_impl.cc (right): https://codereview.chromium.org/193523002/diff/1/content/renderer/media/webco... content/renderer/media/webcontentdecryptionmodule_impl.cc:42: UTF16ToASCII(key_system))) { On 2014/03/11 18:05:14, ddorwin wrote: > Let's have an output parameter for the CDM ID and return that at line 69. Done. https://codereview.chromium.org/193523002/diff/1/media/base/media_keys.h File media/base/media_keys.h (right): https://codereview.chromium.org/193523002/diff/1/media/base/media_keys.h#newc... media/base/media_keys.h:78: virtual int GetCdmId(); On 2014/03/11 18:05:14, ddorwin wrote: > On 2014/03/11 04:06:33, ddorwin wrote: > > Too bad this has to be on the main interface. :( > > As discussed, I think we can avoid this. Removed.
Please do not review the whole CL yet. See my question below. Thanks. https://codereview.chromium.org/193523002/diff/20001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodule_impl.cc (right): https://codereview.chromium.org/193523002/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:65: #endif This code (l.37-65) is really messy with #ifdefs. And I don't find a cleaner way to do this. Note that: - Only for Android we need line 38. - WebCDMImpl::Ctor takes one or two parameters, so that: * The "return" code on l.40 and l.51 are different. * The ctor definition (l.57-65) is hard to write cleanly with inline #ifdefs. That's why I have them separate (58-60 and 62-64, well I may reuse line 58 and 62, but still...) I feel having the adapter to store the cdm_id_ and return it via a CdmSessionAdapter::GetCdmId() call makes the code a bit cleaner. WDYT?
I replied to your question and added a few comments, but I didn't review most of the changes. https://codereview.chromium.org/193523002/diff/1/content/browser/media/androi... File content/browser/media/android/browser_media_player_manager.h (right): https://codereview.chromium.org/193523002/diff/1/content/browser/media/androi... content/browser/media/android/browser_media_player_manager.h:232: std::map<int, int> pending_set_media_keys_map_; On 2014/03/12 01:07:52, xhwang wrote: > On 2014/03/11 04:06:33, ddorwin wrote: > > On 2014/03/10 23:36:32, xhwang wrote: > > > This is used when we call SetMediaKeys() before we call video.load(). The > > Player > > > is created only during video.load(). > > > > How do we have a player_id before video.load()? Isn't Blink set up to call > > setCDM() when the load occurs? > > Removed. You are right: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Is there a place that we can DCHECK this? https://codereview.chromium.org/193523002/diff/20001/content/browser/media/an... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/193523002/diff/20001/content/browser/media/an... content/browser/media/android/browser_media_player_manager.cc:801: MediaPlayerAndroid* player = GetPlayer(player_id); nit: The old order of player then bridge seems better. https://codereview.chromium.org/193523002/diff/20001/content/browser/media/an... content/browser/media/android/browser_media_player_manager.cc:802: if (!drm_bridge || !player) { Should we be DCHECKing that player was found per the ordering discussion? https://codereview.chromium.org/193523002/diff/20001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodule_impl.cc (right): https://codereview.chromium.org/193523002/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:65: #endif On 2014/03/12 01:20:11, xhwang wrote: > This code (l.37-65) is really messy with #ifdefs. And I don't find a cleaner way > to do this. Note that: > - Only for Android we need line 38. > - WebCDMImpl::Ctor takes one or two parameters, so that: > * The "return" code on l.40 and l.51 are different. > * The ctor definition (l.57-65) is hard to write cleanly with inline #ifdefs. > That's why I have them separate (58-60 and 62-64, well I may reuse line 58 and > 62, but still...) > > I feel having the adapter to store the cdm_id_ and return it via a > CdmSessionAdapter::GetCdmId() call makes the code a bit cleaner. WDYT? > How much smaller is the diff if we track this in the adapter? We still need line 80-85 for WMPA to call, so adding that to the adapter would be worse. I would try to avoid duplicating calls and just ifdef individual parameters where possible.
PS3 addresses your comments. PS4 manages the cdm_id_ in CdmSessionAdapter to avoid too many ifdefs (as I mentioned in my last comment). I like PS4 better but am okay with PS3. Let me know your preference :) PTAL again! https://codereview.chromium.org/193523002/diff/20001/content/browser/media/an... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/193523002/diff/20001/content/browser/media/an... content/browser/media/android/browser_media_player_manager.cc:801: MediaPlayerAndroid* player = GetPlayer(player_id); On 2014/03/18 22:51:08, ddorwin wrote: > nit: The old order of player then bridge seems better. Done. https://codereview.chromium.org/193523002/diff/20001/content/browser/media/an... content/browser/media/android/browser_media_player_manager.cc:802: if (!drm_bridge || !player) { On 2014/03/18 22:51:08, ddorwin wrote: > Should we be DCHECKing that player was found per the ordering discussion? hmm, I don't remember the ordering discussion now :( It seems to me OnInitialize() could fail to create and add the player, in which case |player| could be NULL here. https://codereview.chromium.org/193523002/diff/20001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodule_impl.cc (right): https://codereview.chromium.org/193523002/diff/20001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.cc:65: #endif On 2014/03/18 22:51:08, ddorwin wrote: > On 2014/03/12 01:20:11, xhwang wrote: > > This code (l.37-65) is really messy with #ifdefs. And I don't find a cleaner > way > > to do this. Note that: > > - Only for Android we need line 38. > > - WebCDMImpl::Ctor takes one or two parameters, so that: > > * The "return" code on l.40 and l.51 are different. > > * The ctor definition (l.57-65) is hard to write cleanly with inline > #ifdefs. > > That's why I have them separate (58-60 and 62-64, well I may reuse line 58 and > > 62, but still...) > > > > I feel having the adapter to store the cdm_id_ and return it via a > > CdmSessionAdapter::GetCdmId() call makes the code a bit cleaner. WDYT? > > > > How much smaller is the diff if we track this in the adapter? We still need line > 80-85 for WMPA to call, so adding that to the adapter would be worse. > > I would try to avoid duplicating calls and just ifdef individual parameters > where possible. ifdef'ed individual parameters where possible.
PS4 is fine. Most of my comments are in PS3. https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/193523002/diff/1/content/renderer/media/andro... content/renderer/media/android/webmediaplayer_android.cc:1337: // TODO(xhwang): Support setMediaKeys(0) if necessary: http://crbug.com/330324 On 2014/03/12 01:07:52, xhwang wrote: > On 2014/03/11 04:06:33, ddorwin wrote: > > Just a thought: Maybe we should ask the CDM if it will allow it. > > sgtm. So setContentDecryptionModule() should return an exception? Yes, but I think we might need an explicit call since setMediaKeys() will likely start a task (https://www.w3.org/Bugs/Public/show_bug.cgi?id=24216). https://codereview.chromium.org/193523002/diff/40001/content/browser/media/an... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/193523002/diff/40001/content/browser/media/an... content/browser/media/android/browser_media_player_manager.cc:788: DVLOG(1) << "MediaDrmBridge and MediaPlayerAndroid must be present!"; nit: "present" doesn't seem like the right word. https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/a... File content/renderer/media/android/renderer_media_player_manager.cc (right): https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/a... content/renderer/media/android/renderer_media_player_manager.cc:244: if (cdm_id == kInvalidCdmId) Ditto from .h file. https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/a... File content/renderer/media/android/renderer_media_player_manager.h (right): https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/a... content/renderer/media/android/renderer_media_player_manager.h:86: // Does nothing if |cdm_id| is kInvalidCdmId. Why? Shouldn't it detach the two? (Equivalent to setMediaKeys(0)) Do we have appropriate code for setMediaKeys(other_cdm_id) or should we assert that this is not the case (valid ID => valid ID transition)? Oh... because we might send this for Clear Key? https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/a... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:1272: manager_->SetCdm(player_id_, proxy_decryptor_->GetCdmId()); As mentioned before, It seems odd that we make this call even in the Clear Key case. Maybe ProxyDecryptor should somehow abstract this. (Probably not worth doing since this will all be deleted.) https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:1373: blink::WebContentDecryptionModule* cdm) { Why was DCHECK(main_loop_->BelongsToCurrentThread()); from PS1 removed? https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:1385: manager_->SetCdm(player_id_, web_cdm_->GetCdmId()); Ditto from l1272, though we don't have an abstraction here. Hmm. maybe we need a way for CK to say this CDM ID is handled in the renderer. https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/a... File content/renderer/media/android/webmediaplayer_android.h (right): https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.h:441: // Used by clear key implementation. Can we improve this comment? It's not clear why (and is rather unexpected). nit: Capitalize Clear Key. Should there be a TODO to clean up as we discussed in PS1? https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/c... File content/renderer/media/cdm_session_adapter.cc (right): https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/c... content/renderer/media/cdm_session_adapter.cc:37: #if defined(ENABLE_PEPPER_CDMS) It would be nice if we could combine these into some CreatePlatformSpecificCdmCB and eliminate these CBs. We're slowly whittling away the differences. https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/c... File content/renderer/media/cdm_session_adapter.h (right): https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/c... content/renderer/media/cdm_session_adapter.h:72: int GetCdmId() const; Remove. https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/c... content/renderer/media/cdm_session_adapter.h:101: base::WeakPtrFactory<CdmSessionAdapter> weak_ptr_factory_; This comment was lost: // NOTE: Weak pointers must be invalidated before all other member variables. https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/c... File content/renderer/media/crypto/content_decryption_module_factory.cc (right): https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/c... content/renderer/media/crypto/content_decryption_module_factory.cc:40: DCHECK_EQ(*cdm_id, RendererMediaPlayerManager::kInvalidCdmId); You could move this to 68 and avoid this extra ifdef. https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/c... File content/renderer/media/crypto/proxy_decryptor.h (right): https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/c... content/renderer/media/crypto/proxy_decryptor.h:73: // CDM ID is associated. Comment why that might happen? Can we just disallow calling it before Initialize()? https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodule_impl.h (right): https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.h:52: // CDM ID is associated. Ditto about why this might happen. https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.h:65: int cdm_id); nit: if you flip the order, you only need to ifdef this line. https://codereview.chromium.org/193523002/diff/70001/content/renderer/media/c... File content/renderer/media/cdm_session_adapter.cc (right): https://codereview.chromium.org/193523002/diff/70001/content/renderer/media/c... content/renderer/media/cdm_session_adapter.cc:16: #include "content/renderer/media/android/renderer_media_player_manager.h" This is a very odd include in this file. Would it be bad to just initialize to 0? https://codereview.chromium.org/193523002/diff/70001/content/renderer/media/c... content/renderer/media/cdm_session_adapter.cc:39: remove https://codereview.chromium.org/193523002/diff/70001/content/renderer/media/c... File content/renderer/media/cdm_session_adapter.h (right): https://codereview.chromium.org/193523002/diff/70001/content/renderer/media/c... content/renderer/media/cdm_session_adapter.h:36: #endif // defined(ENABLE_PEPPER_CDMS) nit: Comment not necessary for a simple ifdef.
comments addressed
https://codereview.chromium.org/193523002/diff/40001/content/browser/media/an... File content/browser/media/android/browser_media_player_manager.cc (right): https://codereview.chromium.org/193523002/diff/40001/content/browser/media/an... content/browser/media/android/browser_media_player_manager.cc:788: DVLOG(1) << "MediaDrmBridge and MediaPlayerAndroid must be present!"; On 2014/03/25 23:03:59, ddorwin wrote: > nit: "present" doesn't seem like the right word. Done. https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/a... File content/renderer/media/android/renderer_media_player_manager.cc (right): https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/a... content/renderer/media/android/renderer_media_player_manager.cc:244: if (cdm_id == kInvalidCdmId) On 2014/03/25 23:03:59, ddorwin wrote: > Ditto from .h file. Ditto. https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/a... File content/renderer/media/android/renderer_media_player_manager.h (right): https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/a... content/renderer/media/android/renderer_media_player_manager.h:86: // Does nothing if |cdm_id| is kInvalidCdmId. On 2014/03/25 23:03:59, ddorwin wrote: > Why? Shouldn't it detach the two? (Equivalent to setMediaKeys(0)) > > Do we have appropriate code for setMediaKeys(other_cdm_id) or should we assert > that this is not the case (valid ID => valid ID transition)? > > Oh... because we might send this for Clear Key? No, we don't support setMediaKeys(0). Currently we do nothing when setMediaKeys(0) is called (see WMPA). With l.86, WMPA can always call manager_->SetCdm(GetCdmId()) without checking GetCdmId(). This is consistent with decryptor_ready_cb_.Run(web_cdm_->GetDecryptor()), where we don't check GetDecryptor(). https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/a... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:1272: manager_->SetCdm(player_id_, proxy_decryptor_->GetCdmId()); On 2014/03/25 23:03:59, ddorwin wrote: > As mentioned before, It seems odd that we make this call even in the Clear Key > case. Maybe ProxyDecryptor should somehow abstract this. (Probably not worth > doing since this will all be deleted.) As mentioned earlier, if we check GetCdmId() before calling SetCdm(), we can avoid supporting kIncalidCdmId in RenderMediaPlayerManager, that would prevent us from calling SetCdm() for Clear Key (because AesDecryptor doesn't have a valid cdm ID). WDYT of that? For abstraction/clean-up, I'd like to do that later because: 1, A lot of these code will go away. 2, We may want to implement Clear Key in the browser side. 3, We may want to merge WMPI and WMPA. All of these make me feel we need some larger scale refactoring than small clean-up in WMPA. https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:1373: blink::WebContentDecryptionModule* cdm) { On 2014/03/25 23:03:59, ddorwin wrote: > Why was DCHECK(main_loop_->BelongsToCurrentThread()); from PS1 removed? Because main_loop_ doesn't exist any more after rebase. Fixed now. https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:1385: manager_->SetCdm(player_id_, web_cdm_->GetCdmId()); On 2014/03/25 23:03:59, ddorwin wrote: > Ditto from l1272, though we don't have an abstraction here. Hmm. maybe we need a > way for CK to say this CDM ID is handled in the renderer. For Clear Key, GetCdmId is kInvalidCdmId and hence this call does nothing. https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/a... File content/renderer/media/android/webmediaplayer_android.h (right): https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.h:441: // Used by clear key implementation. On 2014/03/25 23:03:59, ddorwin wrote: > Can we improve this comment? It's not clear why (and is rather unexpected). > nit: Capitalize Clear Key. Done > Should there be a TODO to clean up as we discussed in PS1? Hmm, I can't remember which clean up we discussed in PS1... Any hint? https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/c... File content/renderer/media/cdm_session_adapter.cc (right): https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/c... content/renderer/media/cdm_session_adapter.cc:37: #if defined(ENABLE_PEPPER_CDMS) On 2014/03/25 23:03:59, ddorwin wrote: > It would be nice if we could combine these into some CreatePlatformSpecificCdmCB > and eliminate these CBs. We're slowly whittling away the differences. Agreed that we should hide all these differences somewhere. Need to think more on this. https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/c... File content/renderer/media/cdm_session_adapter.h (right): https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/c... content/renderer/media/cdm_session_adapter.h:72: int GetCdmId() const; On 2014/03/25 23:03:59, ddorwin wrote: > Remove. This is used in PS4. https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/c... content/renderer/media/cdm_session_adapter.h:101: base::WeakPtrFactory<CdmSessionAdapter> weak_ptr_factory_; On 2014/03/25 23:03:59, ddorwin wrote: > This comment was lost: > // NOTE: Weak pointers must be invalidated before all other member variables. Done. https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/c... File content/renderer/media/crypto/content_decryption_module_factory.cc (right): https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/c... content/renderer/media/crypto/content_decryption_module_factory.cc:40: DCHECK_EQ(*cdm_id, RendererMediaPlayerManager::kInvalidCdmId); On 2014/03/25 23:03:59, ddorwin wrote: > You could move this to 68 and avoid this extra ifdef. Done. https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/c... File content/renderer/media/crypto/proxy_decryptor.h (right): https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/c... content/renderer/media/crypto/proxy_decryptor.h:73: // CDM ID is associated. On 2014/03/25 23:03:59, ddorwin wrote: > Comment why that might happen? > Can we just disallow calling it before Initialize()? Even after Initialize() this could still happen, e.g. when AesDecryptor was chosen. https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/w... File content/renderer/media/webcontentdecryptionmodule_impl.h (right): https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.h:52: // CDM ID is associated. On 2014/03/25 23:03:59, ddorwin wrote: > Ditto about why this might happen. ditto https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/w... content/renderer/media/webcontentdecryptionmodule_impl.h:65: int cdm_id); On 2014/03/25 23:03:59, ddorwin wrote: > nit: if you flip the order, you only need to ifdef this line. Obsolete in PS4. https://codereview.chromium.org/193523002/diff/70001/content/renderer/media/c... File content/renderer/media/cdm_session_adapter.cc (right): https://codereview.chromium.org/193523002/diff/70001/content/renderer/media/c... content/renderer/media/cdm_session_adapter.cc:16: #include "content/renderer/media/android/renderer_media_player_manager.h" On 2014/03/25 23:04:00, ddorwin wrote: > This is a very odd include in this file. Would it be bad to just initialize to > 0? GetCdmId() should only be called after Initialize() so initializing to 0 sgtm. But since we are doing so, I deleted the DCHECK in ContentDecryptionModuleFactory because we can't just assume kInvalidCmdId is zero. ContentDecryptionModuleFactory::Create() will always set |cdm_id| so we are safe. https://codereview.chromium.org/193523002/diff/70001/content/renderer/media/c... content/renderer/media/cdm_session_adapter.cc:39: On 2014/03/25 23:04:00, ddorwin wrote: > remove Done. https://codereview.chromium.org/193523002/diff/70001/content/renderer/media/c... File content/renderer/media/cdm_session_adapter.h (right): https://codereview.chromium.org/193523002/diff/70001/content/renderer/media/c... content/renderer/media/cdm_session_adapter.h:36: #endif // defined(ENABLE_PEPPER_CDMS) On 2014/03/25 23:04:00, ddorwin wrote: > nit: Comment not necessary for a simple ifdef. Done.
LG. There is one comment/question in PS3. https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/a... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:1272: manager_->SetCdm(player_id_, proxy_decryptor_->GetCdmId()); On 2014/03/26 06:02:04, xhwang wrote: > On 2014/03/25 23:03:59, ddorwin wrote: > > As mentioned before, It seems odd that we make this call even in the Clear Key > > case. Maybe ProxyDecryptor should somehow abstract this. (Probably not worth > > doing since this will all be deleted.) > > As mentioned earlier, if we check GetCdmId() before calling SetCdm(), we can > avoid supporting kIncalidCdmId in RenderMediaPlayerManager, that would prevent > us from calling SetCdm() for Clear Key (because AesDecryptor doesn't have a > valid cdm ID). WDYT of that? Does this mean we'd move kInvalidCdmId up the stack? Another option would be to provide an ID for all CDMs. That would probably need to be Android-specific code in CDMFactory. While it might add more ifdefs in its files, at least it might contain it. It seems that either of these would allow us to DCHECK on 0 until we implement support for setMediaKeys(0). > > For abstraction/clean-up, I'd like to do that later because: > 1, A lot of these code will go away. > 2, We may want to implement Clear Key in the browser side. > 3, We may want to merge WMPI and WMPA. > All of these make me feel we need some larger scale refactoring than small > clean-up in WMPA. Are you saying we shouldn't do what you said in the first paragraph because we want to clean these up? https://codereview.chromium.org/193523002/diff/350001/content/renderer/media/... File content/renderer/media/android/renderer_media_player_manager.h (right): https://codereview.chromium.org/193523002/diff/350001/content/renderer/media/... content/renderer/media/android/renderer_media_player_manager.h:86: // Does nothing if |cdm_id| is kInvalidCdmId. We should have some type of DCHECK or comment somewhere that notes this needs to be updated if we want to support setMediaKeys(0). Or maybe just update that bug with a note about this. https://codereview.chromium.org/193523002/diff/350001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.h (right): https://codereview.chromium.org/193523002/diff/350001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.h:441: // This is only Used by Clear Key key system implementation, where a render nit: renderer-side? ^^^ https://codereview.chromium.org/193523002/diff/350001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.h:443: // systems, a browser side CDM will be used and we set CDM my calling s/my/by/ https://codereview.chromium.org/193523002/diff/350001/content/renderer/media/... File content/renderer/media/crypto/content_decryption_module_factory.cc (right): https://codereview.chromium.org/193523002/diff/350001/content/renderer/media/... content/renderer/media/crypto/content_decryption_module_factory.cc:20: #include "content/renderer/media/android/renderer_media_player_manager.h" we don't need this file. already fwd decl'd in .h. https://codereview.chromium.org/193523002/diff/350001/content/renderer/media/... File content/renderer/media/crypto/proxy_decryptor.h (right): https://codereview.chromium.org/193523002/diff/350001/content/renderer/media/... content/renderer/media/crypto/proxy_decryptor.h:72: // Returns the CDM ID associated with this object. May be kInvalidCdmId if no Should this be ifdef'd? https://codereview.chromium.org/193523002/diff/350001/content/renderer/media/... content/renderer/media/crypto/proxy_decryptor.h:73: // CDM ID is associated. I meant we might add something like: ", such as when Clear Key is used." Same in webcontentdecryptionmodule_impl.h. WDYT?
comments addressed
https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/a... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/193523002/diff/40001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:1272: manager_->SetCdm(player_id_, proxy_decryptor_->GetCdmId()); On 2014/03/26 19:25:59, ddorwin wrote: > On 2014/03/26 06:02:04, xhwang wrote: > > On 2014/03/25 23:03:59, ddorwin wrote: > > > As mentioned before, It seems odd that we make this call even in the Clear > Key > > > case. Maybe ProxyDecryptor should somehow abstract this. (Probably not worth > > > doing since this will all be deleted.) > > > > As mentioned earlier, if we check GetCdmId() before calling SetCdm(), we can > > avoid supporting kIncalidCdmId in RenderMediaPlayerManager, that would prevent > > us from calling SetCdm() for Clear Key (because AesDecryptor doesn't have a > > valid cdm ID). WDYT of that? > > Does this mean we'd move kInvalidCdmId up the stack? > Another option would be to provide an ID for all CDMs. That would probably need > to be Android-specific code in CDMFactory. While it might add more ifdefs in its > files, at least it might contain it. > > It seems that either of these would allow us to DCHECK on 0 until we implement > support for setMediaKeys(0). > > > > For abstraction/clean-up, I'd like to do that later because: > > 1, A lot of these code will go away. > > 2, We may want to implement Clear Key in the browser side. > > 3, We may want to merge WMPI and WMPA. > > All of these make me feel we need some larger scale refactoring than small > > clean-up in WMPA. > > Are you saying we shouldn't do what you said in the first paragraph because we > want to clean these up? Added TODO in RMPM. https://codereview.chromium.org/193523002/diff/350001/content/renderer/media/... File content/renderer/media/android/renderer_media_player_manager.h (right): https://codereview.chromium.org/193523002/diff/350001/content/renderer/media/... content/renderer/media/android/renderer_media_player_manager.h:86: // Does nothing if |cdm_id| is kInvalidCdmId. On 2014/03/26 19:25:59, ddorwin wrote: > We should have some type of DCHECK or comment somewhere that notes this needs to > be updated if we want to support setMediaKeys(0). Or maybe just update that bug > with a note about this. Done. https://codereview.chromium.org/193523002/diff/350001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.h (right): https://codereview.chromium.org/193523002/diff/350001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.h:441: // This is only Used by Clear Key key system implementation, where a render On 2014/03/26 19:25:59, ddorwin wrote: > nit: renderer-side? > ^^^ Done. https://codereview.chromium.org/193523002/diff/350001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.h:443: // systems, a browser side CDM will be used and we set CDM my calling On 2014/03/26 19:25:59, ddorwin wrote: > s/my/by/ Done. https://codereview.chromium.org/193523002/diff/350001/content/renderer/media/... File content/renderer/media/crypto/content_decryption_module_factory.cc (right): https://codereview.chromium.org/193523002/diff/350001/content/renderer/media/... content/renderer/media/crypto/content_decryption_module_factory.cc:20: #include "content/renderer/media/android/renderer_media_player_manager.h" On 2014/03/26 19:25:59, ddorwin wrote: > we don't need this file. already fwd decl'd in .h. Done. https://codereview.chromium.org/193523002/diff/350001/content/renderer/media/... File content/renderer/media/crypto/proxy_decryptor.h (right): https://codereview.chromium.org/193523002/diff/350001/content/renderer/media/... content/renderer/media/crypto/proxy_decryptor.h:72: // Returns the CDM ID associated with this object. May be kInvalidCdmId if no On 2014/03/26 19:25:59, ddorwin wrote: > Should this be ifdef'd? Done. https://codereview.chromium.org/193523002/diff/350001/content/renderer/media/... content/renderer/media/crypto/proxy_decryptor.h:73: // CDM ID is associated. On 2014/03/26 19:25:59, ddorwin wrote: > I meant we might add something like: > ", such as when Clear Key is used." > Same in webcontentdecryptionmodule_impl.h. > WDYT? Done.
lgtm
dcheng@chromium.org: Please OWNERS review changes in content/common/media/media_player_messages_android.h, where we add a new message to support unprefixed EME API. Thanks!
rebase only
The CQ bit was checked by xhwang@chromium.org
The CQ bit was unchecked by xhwang@chromium.org
TBRing dcheng...
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/193523002/410001
The CQ bit was unchecked by dcheng@chromium.org
On 2014/03/28 16:37:54, xhwang wrote: > TBRing dcheng... If it's an urgent review, please ping me on chat. I can understand TBRing it if you're removing messages, but that's not what this patch does.
On 2014/03/28 17:17:16, dcheng wrote: > On 2014/03/28 16:37:54, xhwang wrote: > > TBRing dcheng... > > If it's an urgent review, please ping me on chat. I can understand TBRing it if > you're removing messages, but that's not what this patch does. The CL only adds a message that takes two integer parameters so I thought it's trivial to review. Sorry about that. Removed TBR and please review it.
LGTM. Some optional random thoughts. https://codereview.chromium.org/193523002/diff/410001/content/renderer/media/... File content/renderer/media/cdm_session_adapter.cc (right): https://codereview.chromium.org/193523002/diff/410001/content/renderer/media/... content/renderer/media/cdm_session_adapter.cc:24: cdm_id_(0), Maybe use kInvalidCdmId here too? https://codereview.chromium.org/193523002/diff/410001/content/renderer/media/... File content/renderer/media/crypto/content_decryption_module_factory.cc (right): https://codereview.chromium.org/193523002/diff/410001/content/renderer/media/... content/renderer/media/crypto/content_decryption_module_factory.cc:39: return scoped_ptr<media::MediaKeys>( It might be nice to explicitly set cdm_id = kInvalidCdmId here.
See comments. https://codereview.chromium.org/193523002/diff/410001/content/renderer/media/... File content/renderer/media/cdm_session_adapter.cc (right): https://codereview.chromium.org/193523002/diff/410001/content/renderer/media/... content/renderer/media/cdm_session_adapter.cc:24: cdm_id_(0), On 2014/03/28 20:59:06, dcheng wrote: > Maybe use kInvalidCdmId here too? ddorwin@ suggested this and we were trying avoid the include of RMPM.h only for this. This value will always be set in Initialize() on l.46 so I thought it's fine. WDYT? https://codereview.chromium.org/193523002/diff/410001/content/renderer/media/... File content/renderer/media/crypto/content_decryption_module_factory.cc (right): https://codereview.chromium.org/193523002/diff/410001/content/renderer/media/... content/renderer/media/crypto/content_decryption_module_factory.cc:39: return scoped_ptr<media::MediaKeys>( On 2014/03/28 20:59:06, dcheng wrote: > It might be nice to explicitly set cdm_id = kInvalidCdmId here. cdm_id is only used in android code, where we always set it on l.64.
https://codereview.chromium.org/193523002/diff/410001/content/renderer/media/... File content/renderer/media/cdm_session_adapter.cc (right): https://codereview.chromium.org/193523002/diff/410001/content/renderer/media/... content/renderer/media/cdm_session_adapter.cc:24: cdm_id_(0), On 2014/03/28 21:38:16, xhwang wrote: > On 2014/03/28 20:59:06, dcheng wrote: > > Maybe use kInvalidCdmId here too? > > ddorwin@ suggested this and we were trying avoid the include of RMPM.h only for > this. This value will always be set in Initialize() on l.46 so I thought it's > fine. WDYT? I don't feel strongly about this. Maybe it's a sign the constant should live elsewhere. https://codereview.chromium.org/193523002/diff/410001/content/renderer/media/... File content/renderer/media/crypto/content_decryption_module_factory.cc (right): https://codereview.chromium.org/193523002/diff/410001/content/renderer/media/... content/renderer/media/crypto/content_decryption_module_factory.cc:39: return scoped_ptr<media::MediaKeys>( On 2014/03/28 21:38:16, xhwang wrote: > On 2014/03/28 20:59:06, dcheng wrote: > > It might be nice to explicitly set cdm_id = kInvalidCdmId here. > > cdm_id is only used in android code, where we always set it on l.64. Are you saying clear key (I'm assuming this is what the aes decryptor is?) isn't ever used on Android?
comments addressed
https://codereview.chromium.org/193523002/diff/410001/content/renderer/media/... File content/renderer/media/crypto/content_decryption_module_factory.cc (right): https://codereview.chromium.org/193523002/diff/410001/content/renderer/media/... content/renderer/media/crypto/content_decryption_module_factory.cc:39: return scoped_ptr<media::MediaKeys>( On 2014/03/28 21:42:50, dcheng wrote: > On 2014/03/28 21:38:16, xhwang wrote: > > On 2014/03/28 20:59:06, dcheng wrote: > > > It might be nice to explicitly set cdm_id = kInvalidCdmId here. > > > > cdm_id is only used in android code, where we always set it on l.64. > > Are you saying clear key (I'm assuming this is what the aes decryptor is?) isn't > ever used on Android? Ah, you are right. I had this in earlier PS then got confused myself and removed it. Done.
LGTM with nit fixed. https://codereview.chromium.org/193523002/diff/430001/content/renderer/media/... File content/renderer/media/crypto/content_decryption_module_factory.cc (right): https://codereview.chromium.org/193523002/diff/430001/content/renderer/media/... content/renderer/media/crypto/content_decryption_module_factory.cc:40: *cdm_id = RendererMediaPlayerManager::kInvalidCdmId; Nit: indent =)
comments addressed
https://codereview.chromium.org/193523002/diff/430001/content/renderer/media/... File content/renderer/media/crypto/content_decryption_module_factory.cc (right): https://codereview.chromium.org/193523002/diff/430001/content/renderer/media/... content/renderer/media/crypto/content_decryption_module_factory.cc:40: *cdm_id = RendererMediaPlayerManager::kInvalidCdmId; On 2014/03/28 22:29:28, dcheng wrote: > Nit: indent =) oops, fixed
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/193523002/450001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/193523002/450001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/193523002/450001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
rebase only
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/193523002/470001
Message was sent while issue was closed.
Change committed as 260592
Message was sent while issue was closed.
Does WebContentDecryptionModule really need to be aware of the CDM id (which is to me a detail of the IPC mechanism). https://codereview.chromium.org/193523002/diff/470001/content/renderer/media/... File content/renderer/media/webcontentdecryptionmodule_impl.h (right): https://codereview.chromium.org/193523002/diff/470001/content/renderer/media/... content/renderer/media/webcontentdecryptionmodule_impl.h:46: #if defined(OS_ANDROID) This should be guarded by a feature flag like ENABLE_IPC_CDM. |
