Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
763883006:
Add Mojo CDM Service. (Closed)
|
Created:
6 years ago by xhwang Modified:
6 years ago CC:
chromium-reviews, feature-media-reviews_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org, viettrungluu Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd Mojo CDM Service.
New files added:
- mojo CDM interface
- MojoCdmService: the mojo CDM implementation.
- MojoCdm: a proxy for media code to talk to the mojo CDM service.
BUG=432998
Committed: https://crrev.com/c4721f1b9dce7700de2f9810bd56b106f1940e18
Cr-Commit-Position: refs/heads/master@{#309106}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Add more comments. #
Total comments: 60
Patch Set 3 : comments addressed #Patch Set 4 : More updates. #
Total comments: 10
Patch Set 5 : comments addressed #
Total comments: 21
Patch Set 6 : comments addressed #Patch Set 7 : comments addressed #
Total comments: 2
Messages
Total messages: 44 (5 generated)
xhwang@chromium.org changed reviewers: + ddorwin@chromium.org, jamesr@chromium.org
This CL is not complete yet; many glue code is missing, which I'll added later. Since the CL is getting larger and larger, I'd like to get some early feedback before I go too far. Please do now review all the details in this CL, but ddorwin: please review the general API/structure/naming etc. jamresr: please review the mojom interface and all the type conversions involved back and forth. Thanks! https://codereview.chromium.org/763883006/diff/1/media/mojo/services/mojo_cdm.cc File media/mojo/services/mojo_cdm.cc (right): https://codereview.chromium.org/763883006/diff/1/media/mojo/services/mojo_cdm... media/mojo/services/mojo_cdm.cc:36: MojoCdm::MojoCdm(mojo::ServiceProvider* media_renderer_provider, This needs to be updated. https://codereview.chromium.org/763883006/diff/1/media/mojo/services/mojo_cdm... media/mojo/services/mojo_cdm.cc:58: // Connection to |remote_media_renderer_| will error-out here. ditto
https://codereview.chromium.org/763883006/diff/1/media/mojo/interfaces/decryp... File media/mojo/interfaces/decryptor.mojom (right): https://codereview.chromium.org/763883006/diff/1/media/mojo/interfaces/decryp... media/mojo/interfaces/decryptor.mojom:15: interface Decryptor { This is not implemented yet. I'll do that in later CLs. https://codereview.chromium.org/763883006/diff/1/media/mojo/services/mojo_cdm... File media/mojo/services/mojo_cdm_service.cc (right): https://codereview.chromium.org/763883006/diff/1/media/mojo/services/mojo_cdm... media/mojo/services/mojo_cdm_service.cc:22: if (CanUseAesDecryptor(key_system)) { This is just an example for now; we'll need a new test key system for this, e.g. org.chromium.mojoclearkey
could you tell me which bit of code you want me to look at? I'm not sure what all this stuff does, so it's hard to figure out where to begin. https://codereview.chromium.org/763883006/diff/1/media/mojo/interfaces/conten... File media/mojo/interfaces/content_decryption_module.mojom (right): https://codereview.chromium.org/763883006/diff/1/media/mojo/interfaces/conten... media/mojo/interfaces/content_decryption_module.mojom:5: module mojo; shouldn't this be in a media module? or perhaps something more focused? https://codereview.chromium.org/763883006/diff/1/media/mojo/interfaces/conten... media/mojo/interfaces/content_decryption_module.mojom:20: Decryptor? decryptor; can you add some comments to these guys? what does it mean for the decryptor to be null? https://codereview.chromium.org/763883006/diff/1/media/mojo/interfaces/conten... media/mojo/interfaces/content_decryption_module.mojom:52: => (CdmPromiseResult result, string? session_id); what does it mean for session_id to be null?
> could you tell me which bit of code you want me to look at? I'm not sure what all this stuff does, so it's hard to figure out where to begin. jamesr: I added more comments in the mojom files. I want you to help check whether the mojom files are in good shape from mojo's perspective (grammar, style, etc). Your comments are a good start! Also please help check whether the conversion in from C++/mojo to mojo/C++ in MojoCdm and MojoCdmService makes sense. https://codereview.chromium.org/763883006/diff/1/media/mojo/interfaces/conten... File media/mojo/interfaces/content_decryption_module.mojom (right): https://codereview.chromium.org/763883006/diff/1/media/mojo/interfaces/conten... media/mojo/interfaces/content_decryption_module.mojom:5: module mojo; On 2014/12/02 22:25:59, jamesr wrote: > shouldn't this be in a media module? or perhaps something more focused? The MediaRenderer interface is in mojo module and I was following that. "media" module makes sense. But that'll put a lot of stuff in media namespace and we'll have a lot of naming conflicts. Or I'll need to find a new name for each struct/class, which is painful. Another option is to have a media.mojo module, which will be translated to media::mojo namespace. WDYT? https://codereview.chromium.org/763883006/diff/1/media/mojo/interfaces/conten... media/mojo/interfaces/content_decryption_module.mojom:20: Decryptor? decryptor; On 2014/12/02 22:25:59, jamesr wrote: > can you add some comments to these guys? what does it mean for the decryptor to > be null? Done. https://codereview.chromium.org/763883006/diff/1/media/mojo/interfaces/conten... media/mojo/interfaces/content_decryption_module.mojom:52: => (CdmPromiseResult result, string? session_id); On 2014/12/02 22:25:59, jamesr wrote: > what does it mean for session_id to be null? Done.
On 2014/12/03 01:23:42, xhwang wrote: > https://codereview.chromium.org/763883006/diff/1/media/mojo/interfaces/conten... > media/mojo/interfaces/content_decryption_module.mojom:5: module mojo; > On 2014/12/02 22:25:59, jamesr wrote: > > shouldn't this be in a media module? or perhaps something more focused? > > The MediaRenderer interface is in mojo module and I was following that. > > "media" module makes sense. But that'll put a lot of stuff in media namespace > and we'll have a lot of naming conflicts. Or I'll need to find a new name for > each struct/class, which is painful. > > Another option is to have a media.mojo module, which will be translated to > media::mojo namespace. > > WDYT? It's up to the media owners whether they want this in the 'media' namespace or not, but these things *definitely* do not belong in the 'mojo' namespace. media.mojo seems ok
https://codereview.chromium.org/763883006/diff/20001/media/mojo/services/mojo... File media/mojo/services/mojo_cdm.h (right): https://codereview.chromium.org/763883006/diff/20001/media/mojo/services/mojo... media/mojo/services/mojo_cdm.h:32: const SessionExpirationUpdateCB& session_expiration_update_cb); ddorwin: This makes me think whether we should also have a CdmClient interface in media C++ code. Currently we are passing these 6 callbacks in many layers, which is a pain to type/read. Having a client interface would save us a lot of boilerplate code.
Reviewed the interfaces. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/co... File media/mojo/interfaces/content_decryption_module.mojom (right): https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/co... media/mojo/interfaces/content_decryption_module.mojom:10: // This is used for ContentDecryptionModule (CDM) promise rejections or session note: session errors only exist in prefixed. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/co... media/mojo/interfaces/content_decryption_module.mojom:25: // A CDM implementation must choose to support either an explicit or an implicit nit: drop second "an" https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/co... media/mojo/interfaces/content_decryption_module.mojom:27: // - Explicit decryptor: non-null |decryptor| and invalid |cdm_id|. The client I wonder if this interface should have getters that fail when the wrong one is called rather than just being null/invalid. Maybe too much for too little, though. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/co... media/mojo/interfaces/content_decryption_module.mojom:35: Decryptor? decryptor; How do these things get initialized? https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/co... media/mojo/interfaces/content_decryption_module.mojom:40: // - When |success| is true, the promise is resolved and all other fields should This is weird. And what about the values we need for success? Can we have a different callback/client function for failure? Oh, I guess not since the function is 1:1. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/co... media/mojo/interfaces/content_decryption_module.mojom:46: CdmException exception_code; _code is unnecessary? https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/co... media/mojo/interfaces/content_decryption_module.mojom:58: PERSISTENT_SESSION See CDM_7 comments. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/co... media/mojo/interfaces/content_decryption_module.mojom:66: // Creates a session with the |init_data_type|, |init_data| and |session_type| nit: s/a/the/ here and below? https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/co... media/mojo/interfaces/content_decryption_module.mojom:75: // false, the output |session_id| will be null. There's also the "not found" case, so I think it can be null in either case. This text could imply it won't be. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/co... media/mojo/interfaces/content_decryption_module.mojom:86: // Removes stored session data associated with the session specified by ...the _active_... session... ? It can't just be used like load(). https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/co... media/mojo/interfaces/content_decryption_module.mojom:109: OnSessionError(string session_id, CdmException exception_code, ditto on _code https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/de... File media/mojo/interfaces/decryptor.mojom (right): https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/de... media/mojo/interfaces/decryptor.mojom:17: // make this interface as close as media::Decryptor. nit: missing words https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/de... media/mojo/interfaces/decryptor.mojom:20: // Status of a decrypt or a decrypt-and-decode operation. The returned nit: drop second "a" https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/de... media/mojo/interfaces/decryptor.mojom:26: ERROR // Key is available but an error occurred during decryption. OOC, this always results in DECODE_ERROR, right? https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/de... media/mojo/interfaces/decryptor.mojom:31: // At most one decrypt call is allowed at any time for a |stream_type|. We should replace stream_type with IDs before this interface becomes "stable." https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/de... media/mojo/interfaces/decryptor.mojom:49: // At most one decrypt-and-decode call is allowed at any time for a Are D and D&D calls also mutually-exclusive? https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/de... media/mojo/interfaces/decryptor.mojom:56: // Resets the decoder for |stream_type| to a clean initialized state, cancels nit: s/,/and/ https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/de... media/mojo/interfaces/decryptor.mojom:69: DeinitializeDecoder(DemuxerStream.Type stream_type); Do we want this to be sync? Same for ResetDecoder. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/de... media/mojo/interfaces/decryptor.mojom:73: // Indicates that a new usable key is available in the CDM associated with the I don't think we need this (and it seems like the wrong direction). https://codereview.chromium.org/763883006/diff/20001/media/mojo/services/mojo... File media/mojo/services/mojo_cdm.h (right): https://codereview.chromium.org/763883006/diff/20001/media/mojo/services/mojo... media/mojo/services/mojo_cdm.h:5: #ifndef MEDIA_MOJO_SERVICES_MOJO_CDM_H_ It seems like this should be in media/ or media/mojo/ rather than in the same directory as the service. https://codereview.chromium.org/763883006/diff/20001/media/mojo/services/mojo... media/mojo/services/mojo_cdm.h:32: const SessionExpirationUpdateCB& session_expiration_update_cb); On 2014/12/04 02:11:22, xhwang wrote: > ddorwin: This makes me think whether we should also have a CdmClient interface > in media C++ code. Currently we are passing these 6 callbacks in many layers, > which is a pain to type/read. Having a client interface would save us a lot of > boilerplate code. SGTM. When we originally implemented this, we were told there is a preference (in media?) for CBs vs. clients. Maybe things have changed. We also have to be careful not to violate layers. https://codereview.chromium.org/763883006/diff/20001/media/mojo/services/mojo... media/mojo/services/mojo_cdm.h:39: void CreateSession(const std::string& init_data_type, See the comments about creating sessions in the CDM_7 CL. This case would involve an extra IPC, but it would be rare. https://codereview.chromium.org/763883006/diff/20001/media/mojo/services/mojo... media/mojo/services/mojo_cdm.h:46: void UpdateSession(const std::string& session_id, We _could_ reconsider whether to have session objects. Mojo makes this easier than it would have been with Pepper or normal IPCs. It's possible that we should wait until we replace all Decryptor interfaces with the Mojo one (to avoid the need for more conversions). https://codereview.chromium.org/763883006/diff/20001/media/mojo/services/mojo... media/mojo/services/mojo_cdm.h:62: const mojo::String& destination_url) final; This will need CDM_7 updates. https://codereview.chromium.org/763883006/diff/20001/media/mojo/services/mojo... File media/mojo/services/mojo_cdm_promise.h (right): https://codereview.chromium.org/763883006/diff/20001/media/mojo/services/mojo... media/mojo/services/mojo_cdm_promise.h:20: // Note: Variadic template was considered, but two parameter packs are not As discussed, see if you can use traits. https://codereview.chromium.org/763883006/diff/20001/media/mojo/services/mojo... File media/mojo/services/mojo_cdm_service.h (right): https://codereview.chromium.org/763883006/diff/20001/media/mojo/services/mojo... media/mojo/services/mojo_cdm_service.h:22: MojoCdmService(const mojo::String& key_system); FYI, We may need more information eventually. For examples, see https://w3c.github.io/encrypted-media/#mediakeysystemconfiguration-dictionary. https://codereview.chromium.org/763883006/diff/20001/media/mojo/services/mojo... media/mojo/services/mojo_cdm_service.h:31: ContentDecryptionModule::SessionType session_type, Lots of ditto from the previous file. :)
comments addressed
Updated. For this CL, I'd like the mojo interfaces to be consistent with media::MediaKeys and media::Decryptor interfaces. I'll update them when we update these two interfaces. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/co... File media/mojo/interfaces/content_decryption_module.mojom (right): https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/co... media/mojo/interfaces/content_decryption_module.mojom:10: // This is used for ContentDecryptionModule (CDM) promise rejections or session On 2014/12/08 23:46:48, ddorwin wrote: > note: session errors only exist in prefixed. Done. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/co... media/mojo/interfaces/content_decryption_module.mojom:25: // A CDM implementation must choose to support either an explicit or an implicit On 2014/12/08 23:46:48, ddorwin wrote: > nit: drop second "an" Done. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/co... media/mojo/interfaces/content_decryption_module.mojom:27: // - Explicit decryptor: non-null |decryptor| and invalid |cdm_id|. The client On 2014/12/08 23:46:48, ddorwin wrote: > I wonder if this interface should have getters that fail when the wrong one is > called rather than just being null/invalid. Maybe too much for too little, > though. The current approach seems clean to me. I'll see how this evolves. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/co... media/mojo/interfaces/content_decryption_module.mojom:35: Decryptor? decryptor; On 2014/12/08 23:46:48, ddorwin wrote: > How do these things get initialized? I think they will be initialized to null by default. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/co... media/mojo/interfaces/content_decryption_module.mojom:40: // - When |success| is true, the promise is resolved and all other fields should On 2014/12/08 23:46:48, ddorwin wrote: > This is weird. And what about the values we need for success? Can we have a > different callback/client function for failure? > > Oh, I guess not since the function is 1:1. Acknowledged. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/co... media/mojo/interfaces/content_decryption_module.mojom:46: CdmException exception_code; On 2014/12/08 23:46:48, ddorwin wrote: > _code is unnecessary? Done. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/co... media/mojo/interfaces/content_decryption_module.mojom:58: PERSISTENT_SESSION On 2014/12/08 23:46:48, ddorwin wrote: > See CDM_7 comments. I 'll update this after MediaKeys interface is updated. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/co... media/mojo/interfaces/content_decryption_module.mojom:66: // Creates a session with the |init_data_type|, |init_data| and |session_type| On 2014/12/08 23:46:48, ddorwin wrote: > nit: s/a/the/ here and below? For LoadSession, "the" makes sense. For CreateSession, since we are creating "a" new session, shouldn't "a" makes more sense? https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/co... media/mojo/interfaces/content_decryption_module.mojom:75: // false, the output |session_id| will be null. On 2014/12/08 23:46:48, ddorwin wrote: > There's also the "not found" case, so I think it can be null in either case. > This text could imply it won't be. Done. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/co... media/mojo/interfaces/content_decryption_module.mojom:86: // Removes stored session data associated with the session specified by On 2014/12/08 23:46:48, ddorwin wrote: > ...the _active_... session... ? > > It can't just be used like load(). Done. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/co... media/mojo/interfaces/content_decryption_module.mojom:109: OnSessionError(string session_id, CdmException exception_code, On 2014/12/08 23:46:48, ddorwin wrote: > ditto on _code Done. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/de... File media/mojo/interfaces/decryptor.mojom (right): https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/de... media/mojo/interfaces/decryptor.mojom:17: // make this interface as close as media::Decryptor. On 2014/12/08 23:46:48, ddorwin wrote: > nit: missing words Dropped. We may want to clean up decryptor.h at some point. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/de... media/mojo/interfaces/decryptor.mojom:20: // Status of a decrypt or a decrypt-and-decode operation. The returned On 2014/12/08 23:46:48, ddorwin wrote: > nit: drop second "a" Done. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/de... media/mojo/interfaces/decryptor.mojom:26: ERROR // Key is available but an error occurred during decryption. On 2014/12/08 23:46:49, ddorwin wrote: > OOC, this always results in DECODE_ERROR, right? Yes. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/de... media/mojo/interfaces/decryptor.mojom:31: // At most one decrypt call is allowed at any time for a |stream_type|. On 2014/12/08 23:46:48, ddorwin wrote: > We should replace stream_type with IDs before this interface becomes "stable." Hmm, but the calls are different on different stream types. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/de... media/mojo/interfaces/decryptor.mojom:49: // At most one decrypt-and-decode call is allowed at any time for a On 2014/12/08 23:46:49, ddorwin wrote: > Are D and D&D calls also mutually-exclusive? That's a grey area. We never do it in our code. But in theory, I don't see why a CDM can't support D and D&D calls at the same time. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/de... media/mojo/interfaces/decryptor.mojom:56: // Resets the decoder for |stream_type| to a clean initialized state, cancels On 2014/12/08 23:46:48, ddorwin wrote: > nit: s/,/and/ Done. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/de... media/mojo/interfaces/decryptor.mojom:69: DeinitializeDecoder(DemuxerStream.Type stream_type); On 2014/12/08 23:46:48, ddorwin wrote: > Do we want this to be sync? Same for ResetDecoder. This is how things work now. Non of our CDM/decoders need an async Reset/Deinitialize. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/de... media/mojo/interfaces/decryptor.mojom:73: // Indicates that a new usable key is available in the CDM associated with the On 2014/12/08 23:46:48, ddorwin wrote: > I don't think we need this (and it seems like the wrong direction). The client needs a way to retry. This is the replacement of the Decryptor::NewKeyCB. https://codereview.chromium.org/763883006/diff/20001/media/mojo/services/mojo... File media/mojo/services/mojo_cdm.h (right): https://codereview.chromium.org/763883006/diff/20001/media/mojo/services/mojo... media/mojo/services/mojo_cdm.h:5: #ifndef MEDIA_MOJO_SERVICES_MOJO_CDM_H_ On 2014/12/08 23:46:49, ddorwin wrote: > It seems like this should be in media/ or media/mojo/ rather than in the same > directory as the service. Acked. I am still in the process of discussing namespace etc with mojo people. Will probably refactor the folder structure as well in the future. https://codereview.chromium.org/763883006/diff/20001/media/mojo/services/mojo... media/mojo/services/mojo_cdm.h:32: const SessionExpirationUpdateCB& session_expiration_update_cb); On 2014/12/08 23:46:49, ddorwin wrote: > On 2014/12/04 02:11:22, xhwang wrote: > > ddorwin: This makes me think whether we should also have a CdmClient interface > > in media C++ code. Currently we are passing these 6 callbacks in many layers, > > which is a pain to type/read. Having a client interface would save us a lot of > > boilerplate code. > > SGTM. When we originally implemented this, we were told there is a preference > (in media?) for CBs vs. clients. Maybe things have changed. > > We also have to be careful not to violate layers. Acknowledged. https://codereview.chromium.org/763883006/diff/20001/media/mojo/services/mojo... media/mojo/services/mojo_cdm.h:39: void CreateSession(const std::string& init_data_type, On 2014/12/08 23:46:49, ddorwin wrote: > See the comments about creating sessions in the CDM_7 CL. This case would > involve an extra IPC, but it would be rare. Acknowledged. https://codereview.chromium.org/763883006/diff/20001/media/mojo/services/mojo... media/mojo/services/mojo_cdm.h:46: void UpdateSession(const std::string& session_id, On 2014/12/08 23:46:49, ddorwin wrote: > We _could_ reconsider whether to have session objects. Mojo makes this easier > than it would have been with Pepper or normal IPCs. > > It's possible that we should wait until we replace all Decryptor interfaces with > the Mojo one (to avoid the need for more conversions). Agreed that it's worth considering after we drop the pepper layers. https://codereview.chromium.org/763883006/diff/20001/media/mojo/services/mojo... media/mojo/services/mojo_cdm.h:62: const mojo::String& destination_url) final; On 2014/12/08 23:46:49, ddorwin wrote: > This will need CDM_7 updates. I'll keep working on this until media::MeidaKeys is updated. https://codereview.chromium.org/763883006/diff/20001/media/mojo/services/mojo... File media/mojo/services/mojo_cdm_promise.h (right): https://codereview.chromium.org/763883006/diff/20001/media/mojo/services/mojo... media/mojo/services/mojo_cdm_promise.h:20: // Note: Variadic template was considered, but two parameter packs are not On 2014/12/08 23:46:49, ddorwin wrote: > As discussed, see if you can use traits. Done. https://codereview.chromium.org/763883006/diff/20001/media/mojo/services/mojo... File media/mojo/services/mojo_cdm_service.h (right): https://codereview.chromium.org/763883006/diff/20001/media/mojo/services/mojo... media/mojo/services/mojo_cdm_service.h:22: MojoCdmService(const mojo::String& key_system); On 2014/12/08 23:46:49, ddorwin wrote: > FYI, We may need more information eventually. For examples, see > https://w3c.github.io/encrypted-media/#mediakeysystemconfiguration-dictionary. Acknowledged. https://codereview.chromium.org/763883006/diff/20001/media/mojo/services/mojo... media/mojo/services/mojo_cdm_service.h:31: ContentDecryptionModule::SessionType session_type, On 2014/12/08 23:46:49, ddorwin wrote: > Lots of ditto from the previous file. :) Acknowledged.
qsr: Could you please see the compile failure on android_chromium_gn_compile_dbg bot? I am not familiar with mojo targets on Android build; how do I trigger that failure locally? (I tried my clank build and can't get that error.)
https://codereview.chromium.org/763883006/diff/60001/media/mojo/interfaces/co... File media/mojo/interfaces/content_decryption_module.mojom (right): https://codereview.chromium.org/763883006/diff/60001/media/mojo/interfaces/co... media/mojo/interfaces/content_decryption_module.mojom:101: GetCdmContext(CdmContext& cdm_context); hmm - the & syntax is for an InterfaceRequest but CdmContext is a struct, not an interface. Did you mean to use a return value here or refer to an interface type?
I think all the generators should reject this syntax:
struct Foo { }
interface Bar {
GetFoo(Foo& foo_request);
}
On 2014/12/11 17:40:54, xhwang wrote: > qsr: Could you please see the compile failure on android_chromium_gn_compile_dbg > bot? I am not familiar with mojo targets on Android build; how do I trigger that > failure locally? (I tried my clank build and can't get that error.) As James said, the issue is with the mojom. I'm pretty sure that writing a test using this method would also not compile in C++, and probably cause runtime failure in js and python.
On 2014/12/12 09:47:28, qsr wrote: > On 2014/12/11 17:40:54, xhwang wrote: > > qsr: Could you please see the compile failure on > android_chromium_gn_compile_dbg > > bot? I am not familiar with mojo targets on Android build; how do I trigger > that > > failure locally? (I tried my clank build and can't get that error.) > > As James said, the issue is with the mojom. I'm pretty sure that writing a test > using this method would also not compile in C++, and probably cause runtime > failure in js and python. Thank you both for the help, I think I can generate the cdm_id from the client side, and then pass it into the service. That'll solve this issue.
LG. A few comments in both PS2 and PS4. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/co... File media/mojo/interfaces/content_decryption_module.mojom (right): https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/co... media/mojo/interfaces/content_decryption_module.mojom:66: // Creates a session with the |init_data_type|, |init_data| and |session_type| On 2014/12/10 04:59:15, xhwang wrote: > On 2014/12/08 23:46:48, ddorwin wrote: > > nit: s/a/the/ here and below? > > For LoadSession, "the" makes sense. For CreateSession, since we are creating "a" > new session, shouldn't "a" makes more sense? Acknowledged. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/de... File media/mojo/interfaces/decryptor.mojom (right): https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/de... media/mojo/interfaces/decryptor.mojom:31: // At most one decrypt call is allowed at any time for a |stream_type|. On 2014/12/10 04:59:16, xhwang wrote: > On 2014/12/08 23:46:48, ddorwin wrote: > > We should replace stream_type with IDs before this interface becomes "stable." > > Hmm, but the calls are different on different stream types. Decrypt callbacks aren't, right? What I'm really saying is that we should not assume there is only a single stream of each type before we expose this to apps other than Chromium. That's one thing we know _could_ change in the future. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/de... media/mojo/interfaces/decryptor.mojom:49: // At most one decrypt-and-decode call is allowed at any time for a On 2014/12/10 04:59:16, xhwang wrote: > On 2014/12/08 23:46:49, ddorwin wrote: > > Are D and D&D calls also mutually-exclusive? > > That's a grey area. We never do it in our code. But in theory, I don't see why a > CDM can't support D and D&D calls at the same time. Perhaps per stream ID (if we supported multiple). In that case, I think we'd also need to pass an ID here. https://codereview.chromium.org/763883006/diff/60001/media/mojo/services/mojo... File media/mojo/services/mojo_cdm.h (right): https://codereview.chromium.org/763883006/diff/60001/media/mojo/services/mojo... media/mojo/services/mojo_cdm.h:77: void OnResponse(scoped_ptr<CdmPromiseTemplate<T...>> promise, "Response" could be misleading since this handles other calls. Maybe OnPromiseResolved or OnOperationComplete/Success. https://codereview.chromium.org/763883006/diff/60001/media/mojo/services/mojo... File media/mojo/services/mojo_cdm_promise.cc (right): https://codereview.chromium.org/763883006/diff/60001/media/mojo/services/mojo... media/mojo/services/mojo_cdm_promise.cc:56: template class MojoCdmPromise<std::string>; Need std includes: string and vector. https://codereview.chromium.org/763883006/diff/60001/media/mojo/services/mojo... File media/mojo/services/mojo_cdm_promise.h (right): https://codereview.chromium.org/763883006/diff/60001/media/mojo/services/mojo... media/mojo/services/mojo_cdm_promise.h:14: #include "mojo/public/cpp/bindings/array.h" Not used? https://codereview.chromium.org/763883006/diff/60001/media/mojo/services/mojo... media/mojo/services/mojo_cdm_promise.h:17: #include "mojo/public/cpp/bindings/string.h" Is it correct to use this for std::string?
LG. A few comments in both PS2 and PS4.
comments addressed
Thanks for the comments. PTAL again. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/de... File media/mojo/interfaces/decryptor.mojom (right): https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/de... media/mojo/interfaces/decryptor.mojom:31: // At most one decrypt call is allowed at any time for a |stream_type|. On 2014/12/12 19:25:13, ddorwin wrote: > On 2014/12/10 04:59:16, xhwang wrote: > > On 2014/12/08 23:46:48, ddorwin wrote: > > > We should replace stream_type with IDs before this interface becomes > "stable." > > > > Hmm, but the calls are different on different stream types. > > Decrypt callbacks aren't, right? > > What I'm really saying is that we should not assume there is only a single > stream of each type before we expose this to apps other than Chromium. That's > one thing we know _could_ change in the future. I see. Let's iterate on this later. We can update the C++ and mojo interfaces together. https://codereview.chromium.org/763883006/diff/20001/media/mojo/interfaces/de... media/mojo/interfaces/decryptor.mojom:49: // At most one decrypt-and-decode call is allowed at any time for a On 2014/12/12 19:25:12, ddorwin wrote: > On 2014/12/10 04:59:16, xhwang wrote: > > On 2014/12/08 23:46:49, ddorwin wrote: > > > Are D and D&D calls also mutually-exclusive? > > > > That's a grey area. We never do it in our code. But in theory, I don't see why > a > > CDM can't support D and D&D calls at the same time. > > Perhaps per stream ID (if we supported multiple). In that case, I think we'd > also need to pass an ID here. Acknowledged. https://codereview.chromium.org/763883006/diff/60001/media/mojo/interfaces/co... File media/mojo/interfaces/content_decryption_module.mojom (right): https://codereview.chromium.org/763883006/diff/60001/media/mojo/interfaces/co... media/mojo/interfaces/content_decryption_module.mojom:101: GetCdmContext(CdmContext& cdm_context); On 2014/12/11 18:32:37, jamesr wrote: > hmm - the & syntax is for an InterfaceRequest but CdmContext is a struct, not an > interface. Did you mean to use a return value here or refer to an interface > type? Changed to pass a client side ID in. It's a bit hacky, but should work well for now. https://codereview.chromium.org/763883006/diff/60001/media/mojo/services/mojo... File media/mojo/services/mojo_cdm.h (right): https://codereview.chromium.org/763883006/diff/60001/media/mojo/services/mojo... media/mojo/services/mojo_cdm.h:77: void OnResponse(scoped_ptr<CdmPromiseTemplate<T...>> promise, On 2014/12/12 19:25:13, ddorwin wrote: > "Response" could be misleading since this handles other calls. Maybe > OnPromiseResolved or OnOperationComplete/Success. Done. https://codereview.chromium.org/763883006/diff/60001/media/mojo/services/mojo... File media/mojo/services/mojo_cdm_promise.cc (right): https://codereview.chromium.org/763883006/diff/60001/media/mojo/services/mojo... media/mojo/services/mojo_cdm_promise.cc:56: template class MojoCdmPromise<std::string>; On 2014/12/12 19:25:13, ddorwin wrote: > Need std includes: string and vector. Done. https://codereview.chromium.org/763883006/diff/60001/media/mojo/services/mojo... File media/mojo/services/mojo_cdm_promise.h (right): https://codereview.chromium.org/763883006/diff/60001/media/mojo/services/mojo... media/mojo/services/mojo_cdm_promise.h:14: #include "mojo/public/cpp/bindings/array.h" On 2014/12/12 19:25:13, ddorwin wrote: > Not used? Done. https://codereview.chromium.org/763883006/diff/60001/media/mojo/services/mojo... media/mojo/services/mojo_cdm_promise.h:17: #include "mojo/public/cpp/bindings/string.h" On 2014/12/12 19:25:13, ddorwin wrote: > Is it correct to use this for std::string? Done.
https://codereview.chromium.org/763883006/diff/80001/media/mojo/interfaces/co... File media/mojo/interfaces/content_decryption_module.mojom (right): https://codereview.chromium.org/763883006/diff/80001/media/mojo/interfaces/co... media/mojo/interfaces/content_decryption_module.mojom:94: GetCdmContext(int32 cdm_id, Decryptor&? decryptor); Are these parameters mutually exclusive? Does the client know which to use or does it always pass and ID and find out later if it gets a decryptor back? (Is |decryptor| an (asynchronous?) output parameter?) "Get" seems odd here. Also, there is no "Context" involved. https://codereview.chromium.org/763883006/diff/80001/media/mojo/services/mojo... File media/mojo/services/mojo_cdm.h (right): https://codereview.chromium.org/763883006/diff/80001/media/mojo/services/mojo... media/mojo/services/mojo_cdm.h:77: void OnPromiseResult(scoped_ptr<CdmPromiseTemplate<T...>> promise, nit: Failure/rejection could be a "Result". If you think this is clear in context, it's fine.
https://codereview.chromium.org/763883006/diff/80001/media/mojo/interfaces/co... File media/mojo/interfaces/content_decryption_module.mojom (right): https://codereview.chromium.org/763883006/diff/80001/media/mojo/interfaces/co... media/mojo/interfaces/content_decryption_module.mojom:94: GetCdmContext(int32 cdm_id, Decryptor&? decryptor); On 2014/12/13 02:34:02, ddorwin wrote: > Are these parameters mutually exclusive? > Does the client know which to use or does it always pass and ID and find out > later if it gets a decryptor back? (Is |decryptor| an (asynchronous?) output > parameter?) > "Get" seems odd here. Also, there is no "Context" involved. I agree this interface is not ideal, but so far I don't have a better solution. Maybe we can iterate on this later. In media C++ code, the cdm_id and decryptor are mutually exclusive: We either use a decryptor OR a cdm_id, but not both. Here, we always assign a |cdm_id|, so we could end up with both a |decryptor| and a |cdm_id|. But since each media::Renderer will only "USE" either a |decryptor| or a |cdm_id|, this should still work. For example, on desktop we use media::RendererImpl, which will always look for a |decryptor|. If there's no decryptor available, an error will be thrown. In this case, the |cdm_id| will be ignored altogether. In HTMLViewer, if we use MojoRendererImpl, we will always look for the |cdm_id|, and will ignore the |decryptor|. Another way to do this is to have a GetDecryptor() , and a SetCdmId(int32). The reason I choose GetCdmContext() is because this is really a workaround of MediaKeys::GetCdmContext(). They serve the same purpose, but are doing it in different ways given the limitation of a mojo call. That being said, I have no strong opinions on this. Let me know which you'd prefer. https://codereview.chromium.org/763883006/diff/80001/media/mojo/services/mojo... File media/mojo/services/mojo_cdm.h (right): https://codereview.chromium.org/763883006/diff/80001/media/mojo/services/mojo... media/mojo/services/mojo_cdm.h:77: void OnPromiseResult(scoped_ptr<CdmPromiseTemplate<T...>> promise, On 2014/12/13 02:34:02, ddorwin wrote: > nit: Failure/rejection could be a "Result". If you think this is clear in > context, it's fine. This function handles both success and failure, and "Result" is already used this way (for both success and failure) in many places, e.g. WebContentDecryptionModuleResult. So I feel this is clear.
ddorwin: Could you please OWNERS approve? jamesr: I updated the content_decryption_module mojom interface to provide a client side ID. It doesn't look great but should work for our purpose. We may iterate on this later. If you are interested please take a look.
xhwang@chromium.org changed reviewers: + jrummell@chromium.org
jrummell: Could you please do a detailed review of this CL? ddorwin reviewed high level stuff already. Thanks!
On 2014/12/17 19:15:18, xhwang wrote: > jrummell: Could you please do a detailed review of this CL? ddorwin reviewed > high level stuff already. Thanks! jrummell: This can also help you get familiar with mojo CDM code. Next time when we update media::MediaKeys, we'll also need to update mojo CDM files :)
https://codereview.chromium.org/763883006/diff/80001/media/mojo/interfaces/co... File media/mojo/interfaces/content_decryption_module.mojom (right): https://codereview.chromium.org/763883006/diff/80001/media/mojo/interfaces/co... media/mojo/interfaces/content_decryption_module.mojom:13: NOT_SUPPORTED_ERROR, Do all the enums need to be fixed values since this is intended to be a stable interface. I'm fine with having a TODO until we're really ready.
LG. https://codereview.chromium.org/763883006/diff/80001/media/mojo/services/mojo... File media/mojo/services/mojo_cdm.cc (right): https://codereview.chromium.org/763883006/diff/80001/media/mojo/services/mojo... media/mojo/services/mojo_cdm.cc:21: mojo::Array<uint8_t> array; Why the different types in these two definitions? Shouldn't we standardize of one way? https://codereview.chromium.org/763883006/diff/80001/media/mojo/services/mojo... media/mojo/services/mojo_cdm.cc:29: promise->reject(static_cast<MediaKeys::Exception>(result->exception), Is there a need for compile-time checks that the 2 sets of values don't get out of sync? https://codereview.chromium.org/763883006/diff/80001/media/mojo/services/mojo... media/mojo/services/mojo_cdm.cc:41: session_message_cb_(session_message_cb), Should these cb's be checked for !null (since they are called without a check below)? https://codereview.chromium.org/763883006/diff/80001/media/mojo/services/mojo... media/mojo/services/mojo_cdm.cc:49: DCHECK(remote_cdm_); Is this necessary given the following line will crash if it's null? I don't mind it as it makes it clear that you want to make sure something valid is passed in, but sometimes reviewers on other CLs don't think it is necessary. https://codereview.chromium.org/763883006/diff/80001/media/mojo/services/mojo... File media/mojo/services/mojo_cdm_service.cc (right): https://codereview.chromium.org/763883006/diff/80001/media/mojo/services/mojo... media/mojo/services/mojo_cdm_service.cc:15: typedef MojoCdmPromise<> SimpleMojoCdmPromise; Any reason not to put these in mojo_cdm_promise.h? https://codereview.chromium.org/763883006/diff/80001/media/mojo/services/mojo... media/mojo/services/mojo_cdm_service.cc:43: certificate_data_vector.empty() ? nullptr : &certificate_data_vector[0], Now that we support C++11, can you just use .data()?
https://codereview.chromium.org/763883006/diff/80001/media/mojo/services/mojo... File media/mojo/services/mojo_cdm_service.cc (right): https://codereview.chromium.org/763883006/diff/80001/media/mojo/services/mojo... media/mojo/services/mojo_cdm_service.cc:43: certificate_data_vector.empty() ? nullptr : &certificate_data_vector[0], On 2014/12/18 00:02:12, jrummell wrote: > Now that we support C++11, can you just use .data()? No. std::vector::data() is a C++11 standard library feature and we don't have C++11 standard libraries on all platforms (specifically we don't on android). We have C++11 *language* support everywhere, but that doesn't help here.
comments addressed
Thanks for the comments. Please take a look again. https://codereview.chromium.org/763883006/diff/80001/media/mojo/interfaces/co... File media/mojo/interfaces/content_decryption_module.mojom (right): https://codereview.chromium.org/763883006/diff/80001/media/mojo/interfaces/co... media/mojo/interfaces/content_decryption_module.mojom:13: NOT_SUPPORTED_ERROR, On 2014/12/17 19:17:51, ddorwin wrote: > Do all the enums need to be fixed values since this is intended to be a stable > interface. > > I'm fine with having a TODO until we're really ready. Added static assert for this so that these values are consistent with media::MediaKeys. https://codereview.chromium.org/763883006/diff/80001/media/mojo/services/mojo... File media/mojo/services/mojo_cdm.cc (right): https://codereview.chromium.org/763883006/diff/80001/media/mojo/services/mojo... media/mojo/services/mojo_cdm.cc:21: mojo::Array<uint8_t> array; On 2014/12/18 00:02:12, jrummell wrote: > Why the different types in these two definitions? Shouldn't we standardize of > one way? This is how we construct an mojo::Array now: https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi... https://codereview.chromium.org/763883006/diff/80001/media/mojo/services/mojo... media/mojo/services/mojo_cdm.cc:29: promise->reject(static_cast<MediaKeys::Exception>(result->exception), On 2014/12/18 00:02:12, jrummell wrote: > Is there a need for compile-time checks that the 2 sets of values don't get out > of sync? Done. https://codereview.chromium.org/763883006/diff/80001/media/mojo/services/mojo... media/mojo/services/mojo_cdm.cc:41: session_message_cb_(session_message_cb), On 2014/12/18 00:02:12, jrummell wrote: > Should these cb's be checked for !null (since they are called without a check > below)? Done. https://codereview.chromium.org/763883006/diff/80001/media/mojo/services/mojo... media/mojo/services/mojo_cdm.cc:49: DCHECK(remote_cdm_); On 2014/12/18 00:02:12, jrummell wrote: > Is this necessary given the following line will crash if it's null? I don't mind > it as it makes it clear that you want to make sure something valid is passed in, > but sometimes reviewers on other CLs don't think it is necessary. Done. https://codereview.chromium.org/763883006/diff/80001/media/mojo/services/mojo... File media/mojo/services/mojo_cdm_service.cc (right): https://codereview.chromium.org/763883006/diff/80001/media/mojo/services/mojo... media/mojo/services/mojo_cdm_service.cc:15: typedef MojoCdmPromise<> SimpleMojoCdmPromise; On 2014/12/18 00:02:12, jrummell wrote: > Any reason not to put these in mojo_cdm_promise.h? I want to keep mojo_cdm_promise.h generic/clean. Imagine if another file wants to use MojoCdmPromise with another type, it should do it's own typedef instead of doing this in mojo_cdm_promise.h... https://codereview.chromium.org/763883006/diff/80001/media/mojo/services/mojo... media/mojo/services/mojo_cdm_service.cc:43: certificate_data_vector.empty() ? nullptr : &certificate_data_vector[0], On 2014/12/18 00:39:22, jamesr wrote: > On 2014/12/18 00:02:12, jrummell wrote: > > Now that we support C++11, can you just use .data()? > > No. std::vector::data() is a C++11 standard library feature and we don't have > C++11 standard libraries on all platforms (specifically we don't on android). > We have C++11 *language* support everywhere, but that doesn't help here. Acknowledged.
lgtm. https://codereview.chromium.org/763883006/diff/80001/media/mojo/services/mojo... File media/mojo/services/mojo_cdm.cc (right): https://codereview.chromium.org/763883006/diff/80001/media/mojo/services/mojo... media/mojo/services/mojo_cdm.cc:21: mojo::Array<uint8_t> array; On 2014/12/18 17:04:59, xhwang wrote: > On 2014/12/18 00:02:12, jrummell wrote: > > Why the different types in these two definitions? Shouldn't we standardize of > > one way? > > This is how we construct an mojo::Array now: > https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi... As discussed offline, my question was around using both uint8 and uint8_t as types in this method.
https://codereview.chromium.org/763883006/diff/80001/media/mojo/services/mojo... File media/mojo/services/mojo_cdm.cc (right): https://codereview.chromium.org/763883006/diff/80001/media/mojo/services/mojo... media/mojo/services/mojo_cdm.cc:21: mojo::Array<uint8_t> array; On 2014/12/18 20:01:14, jrummell wrote: > On 2014/12/18 17:04:59, xhwang wrote: > > On 2014/12/18 00:02:12, jrummell wrote: > > > Why the different types in these two definitions? Shouldn't we standardize > of > > > one way? > > > > This is how we construct an mojo::Array now: > > > https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi... > > As discussed offline, my question was around using both uint8 and uint8_t as > types in this method. In general in chromium you should always use the _t family and not the deprecated uint8/uint16/etc types: https://code.google.com/p/chromium/codesearch#chromium/src/base/basictypes.h&...
comments addressed
On 2014/12/18 20:01:15, jrummell wrote: > lgtm. > > https://codereview.chromium.org/763883006/diff/80001/media/mojo/services/mojo... > File media/mojo/services/mojo_cdm.cc (right): > > https://codereview.chromium.org/763883006/diff/80001/media/mojo/services/mojo... > media/mojo/services/mojo_cdm.cc:21: mojo::Array<uint8_t> array; > On 2014/12/18 17:04:59, xhwang wrote: > > On 2014/12/18 00:02:12, jrummell wrote: > > > Why the different types in these two definitions? Shouldn't we standardize > of > > > one way? > > > > This is how we construct an mojo::Array now: > > > https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi... > > As discussed offline, my question was around using both uint8 and uint8_t as > types in this method. Done.
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/763883006/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/763883006/diff/120001/media/mojo/DEPS File media/mojo/DEPS (right): https://codereview.chromium.org/763883006/diff/120001/media/mojo/DEPS#newcode3 media/mojo/DEPS:3: "+mojo/common", jamesr: Please OWNERS approve this DEPS change. Added it because I need "mojo/common/common_type_converters.h".
https://codereview.chromium.org/763883006/diff/120001/media/mojo/DEPS File media/mojo/DEPS (right): https://codereview.chromium.org/763883006/diff/120001/media/mojo/DEPS#newcode3 media/mojo/DEPS:3: "+mojo/common", On 2014/12/18 23:05:08, xhwang wrote: > jamesr: Please OWNERS approve this DEPS change. Added it because I need > "mojo/common/common_type_converters.h". lgtm
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/763883006/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/c4721f1b9dce7700de2f9810bd56b106f1940e18 Cr-Commit-Position: refs/heads/master@{#309106} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
