Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23513055:
Populate canPlayType to renderer (Closed)
|
Created:
7 years, 3 months ago by qinmin Modified:
7 years, 3 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, scherkus (not reviewing) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPopulate canPlayType to renderer
This CL implements the AddKeySystem() call, and using sync IPC to send the results back to the render process.
TBR=jschuh
BUG=224793
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=224064
Patch Set 1 #
Total comments: 1
Patch Set 2 : moving ipc to chrome/common/ #
Total comments: 55
Patch Set 3 : addressing comments #
Total comments: 30
Patch Set 4 : addressing ddorwin's comments #
Total comments: 14
Patch Set 5 : rebase #
Total comments: 14
Patch Set 6 : nits #
Total comments: 2
Patch Set 7 : add dcheck #
Total comments: 4
Patch Set 8 : add dcheck logs #Patch Set 9 : adding hrsurface enum #
Total comments: 4
Patch Set 10 : fixing commnets #
Total comments: 8
Messages
Total messages: 40 (0 generated)
PTAL
Since you have multiple reviewers, please specify which files each reviewer should take a look at. don't add encrypted_media_messages_android.h to chrome/common/media. just keep it in chrome/common. the only benefit of putting it in a media subdir is that to have separate owners files for it, but ipc files are all owned by the same set of people that are in chrome/common/OWNERS. i see that there's webrtc_logging_messages.h in there, it would be good if that's moved out as well but in a separate cl. Regarding the sync ipc: in general we avoid these like the plague. We have to use them when we have existing web platform features that were standardized long ago and we have no choice but to do things synchronously. what does the callstack look like here? can we change the callers to handle things asynchronously?
@jam, Done moving the encrypted_media_message_android.h file to chrome/common. @ddorwin, correct me if I am wrong. I think the sync ipc is needed because this is to answer the canplaytype, which is synchronous. ＠xhwang， ddorwin for all reviewing all the code @jam for OWNERS /ipc and /chrome/ On 2013/09/13 22:06:43, jam wrote: > Since you have multiple reviewers, please specify which files each reviewer > should take a look at. > > don't add encrypted_media_messages_android.h to chrome/common/media. just keep > it in chrome/common. the only benefit of putting it in a media subdir is that to > have separate owners files for it, but ipc files are all owned by the same set > of people that are in chrome/common/OWNERS. i see that there's > webrtc_logging_messages.h in there, it would be good if that's moved out as well > but in a separate cl. > > Regarding the sync ipc: in general we avoid these like the plague. We have to > use them when we have existing web platform features that were standardized long > ago and we have no choice but to do things synchronously. what does the > callstack look like here? can we change the callers to handle things > asynchronously?
On 2013/09/13 23:01:58, qinmin wrote: > @jam, Done moving the encrypted_media_message_android.h file to chrome/common. > @ddorwin, correct me if I am wrong. I think the sync ipc is needed because this > is to answer the canplaytype, which is synchronous. Why exactly does the answer to canplaytype need this information synchronously?
On 2013/09/16 16:39:23, jam wrote: > On 2013/09/13 23:01:58, qinmin wrote: > > @jam, Done moving the encrypted_media_message_android.h file to chrome/common. > > @ddorwin, correct me if I am wrong. I think the sync ipc is needed because > this > > is to answer the canplaytype, which is synchronous. > > Why exactly does the answer to canplaytype need this information synchronously? In the draft of EME that Chrome supports, key system support for container/codecs can be checked via canPlayType(). In later versions of EME, this is moved to isTypeSupported(). In both cases, this is a synchronous method that JS can use to quickly select the media (and key system) to use. We _could_ pre-populate this data in the browser and/or renderer, but we chose to implement a simple query and evaluate the impact. Also, we didn't want to burden all browser/renderer startups with the IPC when most will not need this data. The IPC is used to populate a lazily-initialized singleton, so we should only call it once per renderer process. jam, if we were just checking an in-memory bool, would the IPC be a problem? qinmin, how long do we expect the browser side of the IPC to take to run? https://codereview.chromium.org/23513055/diff/2001/chrome/browser/media/encry... File chrome/browser/media/encrypted_media_message_filter_android.cc (right): https://codereview.chromium.org/23513055/diff/2001/chrome/browser/media/encry... chrome/browser/media/encrypted_media_message_filter_android.cc:22: static const uint8 kWidevineUuid[16] = Should this be in widevine_cdm_constants.h? It appears in other key system code. However, see below. https://codereview.chromium.org/23513055/diff/2001/chrome/browser/media/encry... chrome/browser/media/encrypted_media_message_filter_android.cc:43: void EncryptedMediaMessageFilterAndroid::OnGetSupportedKeySystems( We should think about the best way to poll for the information the renderer wants. For example, list of UUIDs and bitmask of codecs. These might also appear in the output. https://codereview.chromium.org/23513055/diff/2001/chrome/browser/media/encry... chrome/browser/media/encrypted_media_message_filter_android.cc:48: std::vector<uint8> widevine_uuid(kWidevineUuid, kWidevineUuid + 16); Why does this code need to know about Widevine? Shouldn't the key systems code ask about specific key systems? https://codereview.chromium.org/23513055/diff/2001/chrome/browser/media/encry... chrome/browser/media/encrypted_media_message_filter_android.cc:58: video_composition_enabled_system.codecs.push_back(codecs_info[i].codecs); We can't just all all codecs because the platform currently only supports ISO CENC. Passing VP8, etc. up would lead to unexpected results. https://codereview.chromium.org/23513055/diff/2001/chrome/browser/media/encry... chrome/browser/media/encrypted_media_message_filter_android.cc:60: video_composition_disabled_system.codecs.push_back(codecs_info[i].codecs); TODO/bug to check whether composition is actually supported. https://codereview.chromium.org/23513055/diff/2001/chrome/browser/media/encry... chrome/browser/media/encrypted_media_message_filter_android.cc:65: video_composition_enabled_system.key_system = "com.widevine.alpha"; These two strings should not be known at this level IMO. See comment in struct definition. https://codereview.chromium.org/23513055/diff/2001/chrome/browser/media/encry... File chrome/browser/media/encrypted_media_message_filter_android.h (right): https://codereview.chromium.org/23513055/diff/2001/chrome/browser/media/encry... chrome/browser/media/encrypted_media_message_filter_android.h:18: // canPlayType information and pass it back to renderer. Try to avoid canPlayType references, since that is the old name. Also, the rest of the file uses "SupportedKeySystems". https://codereview.chromium.org/23513055/diff/2001/chrome/browser/media/encry... chrome/browser/media/encrypted_media_message_filter_android.h:31: // Retrieve the supported key system. system_s_ https://codereview.chromium.org/23513055/diff/2001/chrome/common/encrypted_me... File chrome/common/encrypted_media_messages_android.h (right): https://codereview.chromium.org/23513055/diff/2001/chrome/common/encrypted_me... chrome/common/encrypted_media_messages_android.h:19: // Name of the key system. I don't think the Android IPC layer needs to know about key system strings or parents. We probably do want to have some way to pass attribute(s) of the uuid so we can add subdomains. https://codereview.chromium.org/23513055/diff/2001/chrome/common/render_messa... File chrome/common/render_messages.h (right): https://codereview.chromium.org/23513055/diff/2001/chrome/common/render_messa... chrome/common/render_messages.h:651: Remove file from CL? https://codereview.chromium.org/23513055/diff/2001/chrome/renderer/chrome_con... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/23513055/diff/2001/chrome/renderer/chrome_con... chrome/renderer/chrome_content_renderer_client.cc:1317: RenderThread::Get()->GetChannel()->Send( Note: I used content::RenderThread::Get()->Send(), which did not requires MSG_ROUTING_NONE. Not sure which is better. https://codereview.chromium.org/23513055/diff/2001/chrome/renderer/chrome_con... chrome/renderer/chrome_content_renderer_client.cc:1320: for (unsigned i = 0; i < supported_systems.size(); ++i) { size_t https://codereview.chromium.org/23513055/diff/2001/chrome/renderer/chrome_con... chrome/renderer/chrome_content_renderer_client.cc:1322: SupportedKeySystemToKeySystemInfo(supported_systems[i])); See ToT and https://codereview.chromium.org/23828007/. My plan was to conditionally call Add<KeySystem> functions based on platform support. https://codereview.chromium.org/23513055/diff/2001/chrome/renderer/media/encr... File chrome/renderer/media/encrypted_media_utils_android.cc (right): https://codereview.chromium.org/23513055/diff/2001/chrome/renderer/media/encr... chrome/renderer/media/encrypted_media_utils_android.cc:15: const std::string CodecTypeToContainerType( This is already handled in AddWidevineWithCodecs(). https://codereview.chromium.org/23513055/diff/2001/chrome/renderer/media/encr... chrome/renderer/media/encrypted_media_utils_android.cc:17: if (codec_type == "mp4v") Most of Chrome uses AVC1. We should try to use the same values as the rest of Chrome as much as possible, meaning we should convert at the lower levels. https://codereview.chromium.org/23513055/diff/2001/chrome/renderer/media/encr... chrome/renderer/media/encrypted_media_utils_android.cc:34: content::KeySystemInfo SupportedKeySystemToKeySystemInfo( As mentioned earlier, I don't think we need to convert entire structs. https://codereview.chromium.org/23513055/diff/2001/content/public/renderer/ke... File content/public/renderer/key_system_info.h (right): https://codereview.chromium.org/23513055/diff/2001/content/public/renderer/ke... content/public/renderer/key_system_info.h:60: // TODO(qinmin): this maybe replaced by a new key_system string. Yes. We would create a different key system string before calling AddWidevineWithCodecs(). https://codereview.chromium.org/23513055/diff/2001/media/base/android/media_s... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/23513055/diff/2001/media/base/android/media_s... media/base/android/media_source_player.cc:31: bool MediaSourcePlayer::IsTypeSupported( What is this function used for? https://codereview.chromium.org/23513055/diff/2001/media/base/android/media_s... media/base/android/media_source_player.cc:34: const std::string& container, What is the plan for the container parameter?
https://codereview.chromium.org/23513055/diff/2001/chrome/common/encrypted_me... File chrome/common/encrypted_media_messages_android.h (right): https://codereview.chromium.org/23513055/diff/2001/chrome/common/encrypted_me... chrome/common/encrypted_media_messages_android.h:19: // Name of the key system. On 2013/09/16 17:58:10, ddorwin wrote: > I don't think the Android IPC layer needs to know about key system strings or > parents. We probably do want to have some way to pass attribute(s) of the uuid > so we can add subdomains. Yeah, we probably only care about UUID, container type and codecs here. KeySystemInfo et al will do the UUID -> key_system translation. https://codereview.chromium.org/23513055/diff/2001/chrome/common/encrypted_me... chrome/common/encrypted_media_messages_android.h:30: IPC_STRUCT_MEMBER(bool, video_composition_enabled) Also add container type here? For now, we only support MP4. https://codereview.chromium.org/23513055/diff/2001/chrome/common/encrypted_me... chrome/common/encrypted_media_messages_android.h:30: IPC_STRUCT_MEMBER(bool, video_composition_enabled) Can we have: IPC_STRUCT_MEMBER(std::vector<std::string>, compositing_codecs) and IPC_STRUCT_MEMBER(std::vector<std::string>, non_compositing_codecs) Then we can probably drop |video_composition_enabled|. https://codereview.chromium.org/23513055/diff/2001/chrome/common/encrypted_me... chrome/common/encrypted_media_messages_android.h:37: std::vector<SupportedKeySystem>) It seems the UUID, container types and codecs are orthogonal to each other. If we have two lists of codecs as suggested above (compositing_codecs and non_compositing_codecs). Then we don't need the std::vector here. Only one SupportedKeySystem will have all information we need. https://codereview.chromium.org/23513055/diff/2001/media/base/android/java/sr... File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (left): https://codereview.chromium.org/23513055/diff/2001/media/base/android/java/sr... media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:131: private static boolean isCryptoSchemeSupported(byte[] schemeUUID, String containerMimeType) { We'll need this in the future... https://codereview.chromium.org/23513055/diff/2001/media/base/android/media_d... File media/base/android/media_drm_bridge.cc (left): https://codereview.chromium.org/23513055/diff/2001/media/base/android/media_d... media/base/android/media_drm_bridge.cc:195: const std::string& container_mime_type) { ditto https://codereview.chromium.org/23513055/diff/2001/media/base/android/media_s... File media/base/android/media_source_player.cc (left): https://codereview.chromium.org/23513055/diff/2001/media/base/android/media_s... media/base/android/media_source_player.cc:37: DVLOG(1) << "UUID and container '" << container << "' not supported."; ditto, probably leave that as is?
I notice I'm cc'd on this CL -- anything I should be looking at? otherwise I'll go back to ignoring it
https://codereview.chromium.org/23513055/diff/2001/chrome/browser/media/encry... File chrome/browser/media/encrypted_media_message_filter_android.cc (right): https://codereview.chromium.org/23513055/diff/2001/chrome/browser/media/encry... chrome/browser/media/encrypted_media_message_filter_android.cc:22: static const uint8 kWidevineUuid[16] = passing UUID from IPC instead On 2013/09/16 17:58:10, ddorwin wrote: > Should this be in widevine_cdm_constants.h? It appears in other key system code. > However, see below. https://codereview.chromium.org/23513055/diff/2001/chrome/browser/media/encry... chrome/browser/media/encrypted_media_message_filter_android.cc:43: void EncryptedMediaMessageFilterAndroid::OnGetSupportedKeySystems( Ok. passing the UUID and SupportedCodecs in the IPC. and moving SupportedCodecs to widevine_cdm_constants.h On 2013/09/16 17:58:10, ddorwin wrote: > We should think about the best way to poll for the information the renderer > wants. For example, list of UUIDs and bitmask of codecs. These might also appear > in the output. https://codereview.chromium.org/23513055/diff/2001/chrome/browser/media/encry... chrome/browser/media/encrypted_media_message_filter_android.cc:65: video_composition_enabled_system.key_system = "com.widevine.alpha"; On 2013/09/16 17:58:10, ddorwin wrote: > These two strings should not be known at this level IMO. See comment in struct > definition. Done. https://codereview.chromium.org/23513055/diff/2001/chrome/browser/media/encry... File chrome/browser/media/encrypted_media_message_filter_android.h (right): https://codereview.chromium.org/23513055/diff/2001/chrome/browser/media/encry... chrome/browser/media/encrypted_media_message_filter_android.h:18: // canPlayType information and pass it back to renderer. On 2013/09/16 17:58:10, ddorwin wrote: > Try to avoid canPlayType references, since that is the old name. Also, the rest > of the file uses "SupportedKeySystems". Done. https://codereview.chromium.org/23513055/diff/2001/chrome/browser/media/encry... chrome/browser/media/encrypted_media_message_filter_android.h:31: // Retrieve the supported key system. On 2013/09/16 17:58:10, ddorwin wrote: > system_s_ Done. https://codereview.chromium.org/23513055/diff/2001/chrome/common/encrypted_me... File chrome/common/encrypted_media_messages_android.h (right): https://codereview.chromium.org/23513055/diff/2001/chrome/common/encrypted_me... chrome/common/encrypted_media_messages_android.h:19: // Name of the key system. On 2013/09/16 17:58:10, ddorwin wrote: > I don't think the Android IPC layer needs to know about key system strings or > parents. We probably do want to have some way to pass attribute(s) of the uuid > so we can add subdomains. Done. https://codereview.chromium.org/23513055/diff/2001/chrome/common/render_messa... File chrome/common/render_messages.h (right): https://codereview.chromium.org/23513055/diff/2001/chrome/common/render_messa... chrome/common/render_messages.h:651: On 2013/09/16 17:58:10, ddorwin wrote: > Remove file from CL? Done. https://codereview.chromium.org/23513055/diff/2001/chrome/renderer/chrome_con... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/23513055/diff/2001/chrome/renderer/chrome_con... chrome/renderer/chrome_content_renderer_client.cc:1317: RenderThread::Get()->GetChannel()->Send( ok, make it consistent with ur CL. On 2013/09/16 17:58:10, ddorwin wrote: > Note: I used content::RenderThread::Get()->Send(), which did not requires > MSG_ROUTING_NONE. Not sure which is better. https://codereview.chromium.org/23513055/diff/2001/chrome/renderer/chrome_con... chrome/renderer/chrome_content_renderer_client.cc:1320: for (unsigned i = 0; i < supported_systems.size(); ++i) { On 2013/09/16 17:58:10, ddorwin wrote: > size_t Done. https://codereview.chromium.org/23513055/diff/2001/media/base/android/media_s... File media/base/android/media_source_player.cc (right): https://codereview.chromium.org/23513055/diff/2001/media/base/android/media_s... media/base/android/media_source_player.cc:31: bool MediaSourcePlayer::IsTypeSupported( This function is not used anywhere, I suppose we can remove it. On 2013/09/16 17:58:10, ddorwin wrote: > What is this function used for? https://codereview.chromium.org/23513055/diff/2001/media/base/android/media_s... media/base/android/media_source_player.cc:34: const std::string& container, see above, the function is removed On 2013/09/16 17:58:10, ddorwin wrote: > What is the plan for the container parameter?
ok, since the sync IPC is just to the IO thread, lgtm https://codereview.chromium.org/23513055/diff/1/chrome/common/render_messages.h File chrome/common/render_messages.h (right): https://codereview.chromium.org/23513055/diff/1/chrome/common/render_messages... chrome/common/render_messages.h:651: ? https://codereview.chromium.org/23513055/diff/2001/chrome/browser/media/encry... File chrome/browser/media/encrypted_media_message_filter_android.cc (right): https://codereview.chromium.org/23513055/diff/2001/chrome/browser/media/encry... chrome/browser/media/encrypted_media_message_filter_android.cc:32: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); nit: this check isn't necessary, you're checking here that BrowserMessageFilter is doing its job.. we shouldn't need to test that in every user of it https://codereview.chromium.org/23513055/diff/2001/chrome/renderer/chrome_con... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/23513055/diff/2001/chrome/renderer/chrome_con... chrome/renderer/chrome_content_renderer_client.cc:1317: RenderThread::Get()->GetChannel()->Send( On 2013/09/16 17:58:10, ddorwin wrote: > Note: I used content::RenderThread::Get()->Send(), which did not requires > MSG_ROUTING_NONE. Not sure which is better. RenderThread::Get()->Send is enough regarding routing id, since it's a message not sent from a routed object (i.e. RenderView) and received in a message filter (i.e. not a routed class like RenderViewHost), this is right
On 2013/09/17 20:26:02, jam wrote: > ok, since the sync IPC is just to the IO thread, lgtm to be clear: the work on the IO thread to service this is cheap right?
PTAL https://codereview.chromium.org/23513055/diff/2001/chrome/browser/media/encry... File chrome/browser/media/encrypted_media_message_filter_android.cc (right): https://codereview.chromium.org/23513055/diff/2001/chrome/browser/media/encry... chrome/browser/media/encrypted_media_message_filter_android.cc:32: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); On 2013/09/17 20:26:02, jam wrote: > nit: this check isn't necessary, you're checking here that BrowserMessageFilter > is doing its job.. we shouldn't need to test that in every user of it Done. https://codereview.chromium.org/23513055/diff/2001/chrome/browser/media/encry... chrome/browser/media/encrypted_media_message_filter_android.cc:48: std::vector<uint8> widevine_uuid(kWidevineUuid, kWidevineUuid + 16); Passing UUIDs in IPC instead, so no more widevine keywords here. On 2013/09/16 17:58:10, ddorwin wrote: > Why does this code need to know about Widevine? Shouldn't the key systems code > ask about specific key systems? https://codereview.chromium.org/23513055/diff/2001/chrome/browser/media/encry... chrome/browser/media/encrypted_media_message_filter_android.cc:58: video_composition_enabled_system.codecs.push_back(codecs_info[i].codecs); The renderer will send a list of available codecs here, so if vp8 is not listed, then we don't need to check it. On 2013/09/16 17:58:10, ddorwin wrote: > We can't just all all codecs because the platform currently only supports ISO > CENC. Passing VP8, etc. up would lead to unexpected results. https://codereview.chromium.org/23513055/diff/2001/chrome/browser/media/encry... chrome/browser/media/encrypted_media_message_filter_android.cc:60: video_composition_disabled_system.codecs.push_back(codecs_info[i].codecs); On 2013/09/16 17:58:10, ddorwin wrote: > TODO/bug to check whether composition is actually supported. Done. https://codereview.chromium.org/23513055/diff/2001/chrome/common/encrypted_me... File chrome/common/encrypted_media_messages_android.h (right): https://codereview.chromium.org/23513055/diff/2001/chrome/common/encrypted_me... chrome/common/encrypted_media_messages_android.h:30: IPC_STRUCT_MEMBER(bool, video_composition_enabled) passing the codecs bitmask inside the IPC On 2013/09/17 18:56:46, xhwang wrote: > Also add container type here? For now, we only support MP4. https://codereview.chromium.org/23513055/diff/2001/chrome/common/encrypted_me... chrome/common/encrypted_media_messages_android.h:30: IPC_STRUCT_MEMBER(bool, video_composition_enabled) On 2013/09/17 18:56:46, xhwang wrote: > Can we have: > > IPC_STRUCT_MEMBER(std::vector<std::string>, compositing_codecs) > > and > > IPC_STRUCT_MEMBER(std::vector<std::string>, non_compositing_codecs) > > Then we can probably drop |video_composition_enabled|. Done. https://codereview.chromium.org/23513055/diff/2001/chrome/common/encrypted_me... chrome/common/encrypted_media_messages_android.h:37: std::vector<SupportedKeySystem>) On 2013/09/17 18:56:46, xhwang wrote: > It seems the UUID, container types and codecs are orthogonal to each other. If > we have two lists of codecs as suggested above (compositing_codecs and > non_compositing_codecs). Then we don't need the std::vector here. Only one > SupportedKeySystem will have all information we need. Done. https://codereview.chromium.org/23513055/diff/2001/chrome/renderer/chrome_con... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/23513055/diff/2001/chrome/renderer/chrome_con... chrome/renderer/chrome_content_renderer_client.cc:1317: RenderThread::Get()->GetChannel()->Send( On 2013/09/17 19:21:06, qinmin wrote: > ok, make it consistent with ur CL. > On 2013/09/16 17:58:10, ddorwin wrote: > > Note: I used content::RenderThread::Get()->Send(), which did not requires > > MSG_ROUTING_NONE. Not sure which is better. > This is moved to chrome_key_systems.cc, and using content::RenderThread::Get()->Send() https://codereview.chromium.org/23513055/diff/2001/chrome/renderer/chrome_con... chrome/renderer/chrome_content_renderer_client.cc:1322: SupportedKeySystemToKeySystemInfo(supported_systems[i])); On 2013/09/16 17:58:10, ddorwin wrote: > See ToT and https://codereview.chromium.org/23828007/. My plan was to > conditionally call Add<KeySystem> functions based on platform support. Done. https://codereview.chromium.org/23513055/diff/2001/chrome/renderer/media/encr... File chrome/renderer/media/encrypted_media_utils_android.cc (right): https://codereview.chromium.org/23513055/diff/2001/chrome/renderer/media/encr... chrome/renderer/media/encrypted_media_utils_android.cc:15: const std::string CodecTypeToContainerType( On 2013/09/16 17:58:10, ddorwin wrote: > This is already handled in AddWidevineWithCodecs(). Done. https://codereview.chromium.org/23513055/diff/2001/chrome/renderer/media/encr... chrome/renderer/media/encrypted_media_utils_android.cc:17: if (codec_type == "mp4v") On 2013/09/16 17:58:10, ddorwin wrote: > Most of Chrome uses AVC1. We should try to use the same values as the rest of > Chrome as much as possible, meaning we should convert at the lower levels. Done. https://codereview.chromium.org/23513055/diff/2001/chrome/renderer/media/encr... chrome/renderer/media/encrypted_media_utils_android.cc:34: content::KeySystemInfo SupportedKeySystemToKeySystemInfo( removed On 2013/09/16 17:58:10, ddorwin wrote: > As mentioned earlier, I don't think we need to convert entire structs. https://codereview.chromium.org/23513055/diff/2001/content/public/renderer/ke... File content/public/renderer/key_system_info.h (right): https://codereview.chromium.org/23513055/diff/2001/content/public/renderer/ke... content/public/renderer/key_system_info.h:60: // TODO(qinmin): this maybe replaced by a new key_system string. will wait for that CL to land On 2013/09/16 17:58:10, ddorwin wrote: > Yes. We would create a different key system string before calling > AddWidevineWithCodecs(). https://codereview.chromium.org/23513055/diff/2001/media/base/android/java/sr... File media/base/android/java/src/org/chromium/media/MediaDrmBridge.java (left): https://codereview.chromium.org/23513055/diff/2001/media/base/android/java/sr... media/base/android/java/src/org/chromium/media/MediaDrmBridge.java:131: private static boolean isCryptoSchemeSupported(byte[] schemeUUID, String containerMimeType) { hmm... ok. reverted On 2013/09/17 18:56:46, xhwang wrote: > We'll need this in the future... https://codereview.chromium.org/23513055/diff/2001/media/base/android/media_d... File media/base/android/media_drm_bridge.cc (left): https://codereview.chromium.org/23513055/diff/2001/media/base/android/media_d... media/base/android/media_drm_bridge.cc:195: const std::string& container_mime_type) { On 2013/09/17 18:56:46, xhwang wrote: > ditto Done. https://codereview.chromium.org/23513055/diff/2001/media/base/android/media_s... File media/base/android/media_source_player.cc (left): https://codereview.chromium.org/23513055/diff/2001/media/base/android/media_s... media/base/android/media_source_player.cc:37: DVLOG(1) << "UUID and container '" << container << "' not supported."; On 2013/09/17 18:56:46, xhwang wrote: > ditto, probably leave that as is? Done.
@jam, just checked the call, it takes about 38 ms, should I use IPC_MESSAGE_HANDLER_DELAY_REPLY instead?
https://codereview.chromium.org/23513055/diff/24001/chrome/browser/media/encr... File chrome/browser/media/encrypted_media_message_filter_android.cc (right): https://codereview.chromium.org/23513055/diff/24001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_message_filter_android.cc:10: #include "chrome/common/widevine_cdm_constants.h" We shouldn't include a widevine file here. This CL takes constants used only for Widevine, moves them to a Widevine file, then uses them generically (for any uuid). To avoid this and layering violations, we may want to duplicate the enum (with an appropriate compile assert where they intersect) in an Android-specific file. Maybe encrypted_media_messages_android.h. https://codereview.chromium.org/23513055/diff/24001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_message_filter_android.cc:23: SupportedCodecs supported_codecs, bool video_composition_allowed) { I think the naming is backwards. The parameter should be requested_codecs and the local should be supported_codecs. https://codereview.chromium.org/23513055/diff/24001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_message_filter_android.cc:23: SupportedCodecs supported_codecs, bool video_composition_allowed) { is_video_composition_allowed ^^^ This doesn't seem to indicate what we're asking. is_video_compositable? But we really want to get orthogonal sets. For true, we want as many as possible. For false, we want only those not in the first set. Is that what this does or do we get overlapping sets? For audio, I guess we would have overlapping. Hmm. I guess we can handle this at a higher level. Back to the naming, it may really be video_must_be_compositable or something like that. https://codereview.chromium.org/23513055/diff/24001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_message_filter_android.cc:25: if ((supported_codecs & WEBM_VP8_AND_VORBIS) && Until Android supports encrypted WebM and we can check this, you should always return false. https://codereview.chromium.org/23513055/diff/24001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_message_filter_android.cc:26: MediaCodecBridge::CanDecode("vorbis", !video_composition_allowed) && video_composition_allowed doesn't really apply to audio. Note that this logic is temporary anyway until secure composition is allowed. You probably need a TODO (maybe pointing to a bug) here. https://codereview.chromium.org/23513055/diff/24001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_message_filter_android.cc:33: MediaCodecBridge::CanDecode("mp4a", !video_composition_allowed)) { Same. https://codereview.chromium.org/23513055/diff/24001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_message_filter_android.cc:68: if (!MediaDrmBridge::IsCryptoSchemeSupported(request.uuid, "")) TODO: Check container types based on codecs in request? https://codereview.chromium.org/23513055/diff/24001/chrome/browser/media/encr... File chrome/browser/media/encrypted_media_message_filter_android.h (right): https://codereview.chromium.org/23513055/diff/24001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_message_filter_android.h:19: // SupportedKeySystems information and pass it back to renderer. nit: passing https://codereview.chromium.org/23513055/diff/24001/chrome/common/encrypted_m... File chrome/common/encrypted_media_messages_android.h (right): https://codereview.chromium.org/23513055/diff/24001/chrome/common/encrypted_m... chrome/common/encrypted_media_messages_android.h:11: #include "chrome/common/widevine_cdm_constants.h" As I mentioned earlier, we should avoid having specific key system information in any API files. https://codereview.chromium.org/23513055/diff/24001/chrome/common/encrypted_m... chrome/common/encrypted_media_messages_android.h:26: IPC_STRUCT_BEGIN(SupportedKeySystemResponse) Do the members of this struct get initialized anywhere? https://codereview.chromium.org/23513055/diff/24001/chrome/common/encrypted_m... chrome/common/encrypted_media_messages_android.h:42: SupportedKeySystemRequest /* key system information request*/, extra space before and missing after https://codereview.chromium.org/23513055/diff/24001/chrome/common/widevine_cd... File chrome/common/widevine_cdm_constants.h (right): https://codereview.chromium.org/23513055/diff/24001/chrome/common/widevine_cd... chrome/common/widevine_cdm_constants.h:11: // Defines bitmask values used to specify supported codecs. As mentioned before, since this could be used for non-Widevine uuids, it should not be in this file. https://codereview.chromium.org/23513055/diff/24001/chrome/common/widevine_cd... chrome/common/widevine_cdm_constants.h:32: extern const uint8 kWidevineUuid[16]; We only seem to use this in one location. Do we really need to move it here? (This file is currently only for Pepper CDMs. (I wonder if it's missing some ifdefs.) https://codereview.chromium.org/23513055/diff/24001/chrome/renderer/media/chr... File chrome/renderer/media/chrome_key_systems.cc (right): https://codereview.chromium.org/23513055/diff/24001/chrome/renderer/media/chr... chrome/renderer/media/chrome_key_systems.cc:105: request.uuid.begin(), kWidevineUuid, kWidevineUuid + 16); s/16/arraysize/ https://codereview.chromium.org/23513055/diff/24001/chrome/renderer/media/chr... chrome/renderer/media/chrome_key_systems.cc:112: // TODO(qinmin): using different key system types for compositing and s/using/Use/
https://codereview.chromium.org/23513055/diff/24001/chrome/browser/media/encr... File chrome/browser/media/encrypted_media_message_filter_android.cc (right): https://codereview.chromium.org/23513055/diff/24001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_message_filter_android.cc:10: #include "chrome/common/widevine_cdm_constants.h" On 2013/09/17 22:29:08, ddorwin wrote: > We shouldn't include a widevine file here. > > This CL takes constants used only for Widevine, moves them to a Widevine file, > then uses them generically (for any uuid). To avoid this and layering > violations, we may want to duplicate the enum (with an appropriate compile > assert where they intersect) in an Android-specific file. Maybe > encrypted_media_messages_android.h. ok, removed the include. Duplicated the enum and added the compile asserts https://codereview.chromium.org/23513055/diff/24001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_message_filter_android.cc:23: SupportedCodecs supported_codecs, bool video_composition_allowed) { On 2013/09/17 22:29:08, ddorwin wrote: > I think the naming is backwards. The parameter should be requested_codecs and > the local should be supported_codecs. Done. https://codereview.chromium.org/23513055/diff/24001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_message_filter_android.cc:23: SupportedCodecs supported_codecs, bool video_composition_allowed) { On 2013/09/17 22:29:08, ddorwin wrote: > is_video_composition_allowed > ^^^ > > This doesn't seem to indicate what we're asking. is_video_compositable? > > But we really want to get orthogonal sets. For true, we want as many as > possible. For false, we want only those not in the first set. Is that what this > does or do we get overlapping sets? For audio, I guess we would have > overlapping. Hmm. I guess we can handle this at a higher level. > > Back to the naming, it may really be video_must_be_compositable or something > like that. Done. https://codereview.chromium.org/23513055/diff/24001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_message_filter_android.cc:25: if ((supported_codecs & WEBM_VP8_AND_VORBIS) && On 2013/09/17 22:29:08, ddorwin wrote: > Until Android supports encrypted WebM and we can check this, you should always > return false. Done. https://codereview.chromium.org/23513055/diff/24001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_message_filter_android.cc:26: MediaCodecBridge::CanDecode("vorbis", !video_composition_allowed) && On 2013/09/17 22:29:08, ddorwin wrote: > video_composition_allowed doesn't really apply to audio. > > Note that this logic is temporary anyway until secure composition is allowed. > You probably need a TODO (maybe pointing to a bug) here. the composition flag don't affect audio. Anyway, changed to false. https://codereview.chromium.org/23513055/diff/24001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_message_filter_android.cc:33: MediaCodecBridge::CanDecode("mp4a", !video_composition_allowed)) { On 2013/09/17 22:29:08, ddorwin wrote: > Same. Done. https://codereview.chromium.org/23513055/diff/24001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_message_filter_android.cc:68: if (!MediaDrmBridge::IsCryptoSchemeSupported(request.uuid, "")) On 2013/09/17 22:29:08, ddorwin wrote: > TODO: Check container types based on codecs in request? Done. https://codereview.chromium.org/23513055/diff/24001/chrome/browser/media/encr... File chrome/browser/media/encrypted_media_message_filter_android.h (right): https://codereview.chromium.org/23513055/diff/24001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_message_filter_android.h:19: // SupportedKeySystems information and pass it back to renderer. On 2013/09/17 22:29:08, ddorwin wrote: > nit: passing Done. https://codereview.chromium.org/23513055/diff/24001/chrome/common/encrypted_m... File chrome/common/encrypted_media_messages_android.h (right): https://codereview.chromium.org/23513055/diff/24001/chrome/common/encrypted_m... chrome/common/encrypted_media_messages_android.h:11: #include "chrome/common/widevine_cdm_constants.h" On 2013/09/17 22:29:08, ddorwin wrote: > As I mentioned earlier, we should avoid having specific key system information > in any API files. Done. https://codereview.chromium.org/23513055/diff/24001/chrome/common/encrypted_m... chrome/common/encrypted_media_messages_android.h:26: IPC_STRUCT_BEGIN(SupportedKeySystemResponse) separated the struct definition from IPC On 2013/09/17 22:29:08, ddorwin wrote: > Do the members of this struct get initialized anywhere? https://codereview.chromium.org/23513055/diff/24001/chrome/common/encrypted_m... chrome/common/encrypted_media_messages_android.h:42: SupportedKeySystemRequest /* key system information request*/, On 2013/09/17 22:29:08, ddorwin wrote: > extra space before and missing after Done. https://codereview.chromium.org/23513055/diff/24001/chrome/common/widevine_cd... File chrome/common/widevine_cdm_constants.h (right): https://codereview.chromium.org/23513055/diff/24001/chrome/common/widevine_cd... chrome/common/widevine_cdm_constants.h:11: // Defines bitmask values used to specify supported codecs. On 2013/09/17 22:29:08, ddorwin wrote: > As mentioned before, since this could be used for non-Widevine uuids, it should > not be in this file. Done. https://codereview.chromium.org/23513055/diff/24001/chrome/common/widevine_cd... chrome/common/widevine_cdm_constants.h:32: extern const uint8 kWidevineUuid[16]; ok, moved it back On 2013/09/17 22:29:08, ddorwin wrote: > We only seem to use this in one location. Do we really need to move it here? > > (This file is currently only for Pepper CDMs. (I wonder if it's missing some > ifdefs.) https://codereview.chromium.org/23513055/diff/24001/chrome/renderer/media/chr... File chrome/renderer/media/chrome_key_systems.cc (right): https://codereview.chromium.org/23513055/diff/24001/chrome/renderer/media/chr... chrome/renderer/media/chrome_key_systems.cc:105: request.uuid.begin(), kWidevineUuid, kWidevineUuid + 16); On 2013/09/17 22:29:08, ddorwin wrote: > s/16/arraysize/ Done. https://codereview.chromium.org/23513055/diff/24001/chrome/renderer/media/chr... chrome/renderer/media/chrome_key_systems.cc:112: // TODO(qinmin): using different key system types for compositing and On 2013/09/17 22:29:08, ddorwin wrote: > s/using/Use/ Done.
LGTM % comments. Thanks! https://codereview.chromium.org/23513055/diff/39001/chrome/browser/media/encr... File chrome/browser/media/encrypted_media_message_filter_android.cc (right): https://codereview.chromium.org/23513055/diff/39001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_message_filter_android.cc:25: DCHECK(!(requested_codecs & android::WEBM_VP8_AND_VORBIS)); This is probably worth commenting/reference other TODO. https://codereview.chromium.org/23513055/diff/39001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_message_filter_android.cc:32: supported_codecs | android::MP4_AAC); This doesn't fit on 2 lines? https://codereview.chromium.org/23513055/diff/39001/chrome/common/encrypted_m... File chrome/common/encrypted_media_messages_android.h (right): https://codereview.chromium.org/23513055/diff/39001/chrome/common/encrypted_m... chrome/common/encrypted_media_messages_android.h:35: SupportedCodecs requested_codecs; nit: requested_ is redundant in the type (and hopefully names of instances of this struct). https://codereview.chromium.org/23513055/diff/39001/chrome/common/widevine_cd... File chrome/common/widevine_cdm_constants.cc (right): https://codereview.chromium.org/23513055/diff/39001/chrome/common/widevine_cd... chrome/common/widevine_cdm_constants.cc:18: #if defined(OS_ANDROID) You can remove this now. It's not exposed anymore. https://codereview.chromium.org/23513055/diff/39001/chrome/renderer/media/chr... File chrome/renderer/media/chrome_key_systems.cc (right): https://codereview.chromium.org/23513055/diff/39001/chrome/renderer/media/chr... chrome/renderer/media/chrome_key_systems.cc:115: #define COMPILE_ASSERT_MATCHING_ENUM(name) \ Maybe put this up at line 51 so it's near the enum definition but still in the ANDROID section https://codereview.chromium.org/23513055/diff/39001/chrome/renderer/media/chr... chrome/renderer/media/chrome_key_systems.cc:140: // non-compositing codecs. Need to ignore the extra bit masks defined in What extra bit masks?
https://codereview.chromium.org/23513055/diff/39001/chrome/browser/media/encr... File chrome/browser/media/encrypted_media_message_filter_android.cc (right): https://codereview.chromium.org/23513055/diff/39001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_message_filter_android.cc:25: DCHECK(!(requested_codecs & android::WEBM_VP8_AND_VORBIS)); On 2013/09/18 03:58:31, ddorwin wrote: > This is probably worth commenting/reference other TODO. Done. https://codereview.chromium.org/23513055/diff/39001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_message_filter_android.cc:32: supported_codecs | android::MP4_AAC); On 2013/09/18 03:58:31, ddorwin wrote: > This doesn't fit on 2 lines? Done. https://codereview.chromium.org/23513055/diff/39001/chrome/common/encrypted_m... File chrome/common/encrypted_media_messages_android.h (right): https://codereview.chromium.org/23513055/diff/39001/chrome/common/encrypted_m... chrome/common/encrypted_media_messages_android.h:35: SupportedCodecs requested_codecs; On 2013/09/18 03:58:31, ddorwin wrote: > nit: requested_ is redundant in the type (and hopefully names of instances of > this struct). Done. https://codereview.chromium.org/23513055/diff/39001/chrome/common/widevine_cd... File chrome/common/widevine_cdm_constants.cc (right): https://codereview.chromium.org/23513055/diff/39001/chrome/common/widevine_cd... chrome/common/widevine_cdm_constants.cc:18: #if defined(OS_ANDROID) On 2013/09/18 03:58:31, ddorwin wrote: > You can remove this now. It's not exposed anymore. oops, missed it. https://codereview.chromium.org/23513055/diff/39001/chrome/renderer/media/chr... File chrome/renderer/media/chrome_key_systems.cc (right): https://codereview.chromium.org/23513055/diff/39001/chrome/renderer/media/chr... chrome/renderer/media/chrome_key_systems.cc:115: #define COMPILE_ASSERT_MATCHING_ENUM(name) \ On 2013/09/18 03:58:31, ddorwin wrote: > Maybe put this up at line 51 so it's near the enum definition but still in the > ANDROID section Done. https://codereview.chromium.org/23513055/diff/39001/chrome/renderer/media/chr... chrome/renderer/media/chrome_key_systems.cc:140: // non-compositing codecs. Need to ignore the extra bit masks defined in The COMPILER_ASSERT only catches the 3 known enums to this class. However, it does not guarantee the size of both enums are the same. So if android::SupportedCodecs defines more bits, then this class will have no idea what those bits are used for. Not sure whether that case will happen in the future if we add other key systems. On 2013/09/18 03:58:31, ddorwin wrote: > What extra bit masks?
+jschuh for ipc_message_start.h OWNER
It doesn't look like your upload worked correctly. If re-uploading doesn't work, try removing the ps # and hash from src/.git/config. Two comments in two patch sets below. https://codereview.chromium.org/23513055/diff/39001/chrome/renderer/media/chr... File chrome/renderer/media/chrome_key_systems.cc (right): https://codereview.chromium.org/23513055/diff/39001/chrome/renderer/media/chr... chrome/renderer/media/chrome_key_systems.cc:140: // non-compositing codecs. Need to ignore the extra bit masks defined in You could do a runtime check on the results: DCHECK_EQ(mask >> 3, 0); That's probably a good idea to do on the other end too. However, I don't think we should get responses here that we didn't ask for, right? You could also: DCHECK_EQ(~result & request, 0) On 2013/09/18 05:08:21, qinmin wrote: > The COMPILER_ASSERT only catches the 3 known enums to this class. > However, it does not guarantee the size of both enums are the same. So if > android::SupportedCodecs defines more bits, then this class will have no idea > what those bits are used for. Not sure whether that case will happen in the > future if we add other key systems. > > > On 2013/09/18 03:58:31, ddorwin wrote: > > What extra bit masks? > https://codereview.chromium.org/23513055/diff/60001/chrome/browser/media/encr... File chrome/browser/media/encrypted_media_message_filter_android.cc (right): https://codereview.chromium.org/23513055/diff/60001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_message_filter_android.cc:26: // once webm support is added for EME. s/for EME/to MediaDrm/ (or to Android). EME supports it. ;)
lgtm % nits https://codereview.chromium.org/23513055/diff/47001/chrome/browser/media/encr... File chrome/browser/media/encrypted_media_message_filter_android.cc (right): https://codereview.chromium.org/23513055/diff/47001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_message_filter_android.cc:25: DCHECK(!(requested_codecs & android::WEBM_VP8_AND_VORBIS)); why? Add a comment or todo? https://codereview.chromium.org/23513055/diff/47001/chrome/browser/media/encr... File chrome/browser/media/encrypted_media_message_filter_android.h (right): https://codereview.chromium.org/23513055/diff/47001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_message_filter_android.h:8: #include <vector> drop? https://codereview.chromium.org/23513055/diff/47001/chrome/common/encrypted_m... File chrome/common/encrypted_media_messages_android.h (right): https://codereview.chromium.org/23513055/diff/47001/chrome/common/encrypted_m... chrome/common/encrypted_media_messages_android.h:8: #include <string> drop? https://codereview.chromium.org/23513055/diff/47001/chrome/common/encrypted_m... chrome/common/encrypted_media_messages_android.h:52: #endif // CHROME_COMMON_RENDER_MESSAGES_H_ CHROME_COMMON_ENCRYPTED_MEDIA_MESSAGES_ANDROID_H ? https://codereview.chromium.org/23513055/diff/47001/chrome/common/widevine_cd... File chrome/common/widevine_cdm_constants.cc (right): https://codereview.chromium.org/23513055/diff/47001/chrome/common/widevine_cd... chrome/common/widevine_cdm_constants.cc:20: { 0xED, 0xEF, 0x8B, 0xA9, 0x79, 0xD6, 0x4A, 0xCE, move { to the end of previous line. actually drop this as it's not used in this CL? https://codereview.chromium.org/23513055/diff/47001/chrome/renderer/media/chr... File chrome/renderer/media/chrome_key_systems.cc (right): https://codereview.chromium.org/23513055/diff/47001/chrome/renderer/media/chr... chrome/renderer/media/chrome_key_systems.cc:179: #undef COMPILE_ASSERT_MATCHING_ENUM So there's no way to only define those enum in one place? https://codereview.chromium.org/23513055/diff/47001/chrome/renderer/media/chr... chrome/renderer/media/chrome_key_systems.cc:195: request, &response)); fit in one line?
On 2013/09/17 21:52:53, qinmin wrote: > @jam, just checked the call, it takes about 38 ms, should I use > IPC_MESSAGE_HANDLER_DELAY_REPLY instead? hmm, 38ms is pretty expensive, equivalent to a few disk lookups. Generally we don't want to block the IO thread for that long, since it stops all other IPCs from being dispatched even on the UI thread. I'm assuming you mean use IPC_MESSAGE_HANDLER_DELAY_REPLY to send the request to another thread right? BrowserMessageFilter already handles this, just override OverrideThreadForMessage and set the dispatching thread to be BrowserThread::FILE
On 2013/09/18 15:57:32, jam wrote: > On 2013/09/17 21:52:53, qinmin wrote: > > @jam, just checked the call, it takes about 38 ms, should I use > > IPC_MESSAGE_HANDLER_DELAY_REPLY instead? > > hmm, 38ms is pretty expensive, equivalent to a few disk lookups. Generally we > don't want to block the IO thread for that long, since it stops all other IPCs > from being dispatched even on the UI thread. > > I'm assuming you mean use IPC_MESSAGE_HANDLER_DELAY_REPLY to send the request to > another thread right? BrowserMessageFilter already handles this, just override > OverrideThreadForMessage and set the dispatching thread to be > BrowserThread::FILE Done, moved the IPC handling to FILE thread
https://codereview.chromium.org/23513055/diff/47001/chrome/browser/media/encr... File chrome/browser/media/encrypted_media_message_filter_android.cc (right): https://codereview.chromium.org/23513055/diff/47001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_message_filter_android.cc:25: DCHECK(!(requested_codecs & android::WEBM_VP8_AND_VORBIS)); already added in PS6 On 2013/09/18 07:10:57, xhwang wrote: > why? Add a comment or todo? https://codereview.chromium.org/23513055/diff/47001/chrome/browser/media/encr... File chrome/browser/media/encrypted_media_message_filter_android.h (right): https://codereview.chromium.org/23513055/diff/47001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_message_filter_android.h:8: #include <vector> On 2013/09/18 07:10:57, xhwang wrote: > drop? Done. https://codereview.chromium.org/23513055/diff/47001/chrome/common/encrypted_m... File chrome/common/encrypted_media_messages_android.h (right): https://codereview.chromium.org/23513055/diff/47001/chrome/common/encrypted_m... chrome/common/encrypted_media_messages_android.h:8: #include <string> On 2013/09/18 07:10:57, xhwang wrote: > drop? Done. https://codereview.chromium.org/23513055/diff/47001/chrome/common/encrypted_m... chrome/common/encrypted_media_messages_android.h:52: #endif // CHROME_COMMON_RENDER_MESSAGES_H_ On 2013/09/18 07:10:57, xhwang wrote: > CHROME_COMMON_ENCRYPTED_MEDIA_MESSAGES_ANDROID_H ? Done. https://codereview.chromium.org/23513055/diff/47001/chrome/common/widevine_cd... File chrome/common/widevine_cdm_constants.cc (right): https://codereview.chromium.org/23513055/diff/47001/chrome/common/widevine_cd... chrome/common/widevine_cdm_constants.cc:20: { 0xED, 0xEF, 0x8B, 0xA9, 0x79, 0xD6, 0x4A, 0xCE, dropped in PS6 On 2013/09/18 07:10:57, xhwang wrote: > move { to the end of previous line. actually drop this as it's not used in this > CL? https://codereview.chromium.org/23513055/diff/47001/chrome/renderer/media/chr... File chrome/renderer/media/chrome_key_systems.cc (right): https://codereview.chromium.org/23513055/diff/47001/chrome/renderer/media/chr... chrome/renderer/media/chrome_key_systems.cc:179: #undef COMPILE_ASSERT_MATCHING_ENUM Ya, i agree this is not satisfactory. We have these enums also in GTV code. May need to reconsider how to move these enums, but that may come with a later patch though On 2013/09/18 07:10:57, xhwang wrote: > So there's no way to only define those enum in one place? https://codereview.chromium.org/23513055/diff/47001/chrome/renderer/media/chr... chrome/renderer/media/chrome_key_systems.cc:195: request, &response)); On 2013/09/18 07:10:57, xhwang wrote: > fit in one line? Done. https://codereview.chromium.org/23513055/diff/60001/chrome/browser/media/encr... File chrome/browser/media/encrypted_media_message_filter_android.cc (right): https://codereview.chromium.org/23513055/diff/60001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_message_filter_android.cc:26: // once webm support is added for EME. On 2013/09/18 05:19:54, ddorwin wrote: > s/for EME/to MediaDrm/ (or to Android). EME supports it. ;) Done.
lgtm++
Diffs are still broken. chrome_key_systems.cc for example. Not sure if you saw this comment in an older PS: https://codereview.chromium.org/23513055/diff/39001/chrome/renderer/media/chr...
https://codereview.chromium.org/23513055/diff/39001/chrome/renderer/media/chr... File chrome/renderer/media/chrome_key_systems.cc (right): https://codereview.chromium.org/23513055/diff/39001/chrome/renderer/media/chr... chrome/renderer/media/chrome_key_systems.cc:140: // non-compositing codecs. Need to ignore the extra bit masks defined in seems I missed it, a DCHECK is added here and in the message filter, though the DCHECK here should imply the ones in the message filter. On 2013/09/18 05:19:54, ddorwin wrote: > You could do a runtime check on the results: > DCHECK_EQ(mask >> 3, 0); > > That's probably a good idea to do on the other end too. > > However, I don't think we should get responses here that we didn't ask for, > right? You could also: > DCHECK_EQ(~result & request, 0) > > On 2013/09/18 05:08:21, qinmin wrote: > > The COMPILER_ASSERT only catches the 3 known enums to this class. > > However, it does not guarantee the size of both enums are the same. So if > > android::SupportedCodecs defines more bits, then this class will have no idea > > what those bits are used for. Not sure whether that case will happen in the > > future if we add other key systems. > > > > > > On 2013/09/18 03:58:31, ddorwin wrote: > > > What extra bit masks? > > >
Thanks. lgtm with the right DCHECK. https://codereview.chromium.org/23513055/diff/27001/chrome/browser/media/encr... File chrome/browser/media/encrypted_media_message_filter_android.cc (right): https://codereview.chromium.org/23513055/diff/27001/chrome/browser/media/encr... chrome/browser/media/encrypted_media_message_filter_android.cc:88: DCHECK_EQ(response.non_compositing_codecs >> 3, 0); This only checks that this file is consistent. I think you want to do this to the value of request.codecs, which comes from the renderer. Also, add something like: << "unrecognized codec"; https://codereview.chromium.org/23513055/diff/27001/chrome/renderer/media/chr... File chrome/renderer/media/chrome_key_systems.cc (right): https://codereview.chromium.org/23513055/diff/27001/chrome/renderer/media/chr... chrome/renderer/media/chrome_key_systems.cc:197: DCHECK_EQ(supported_codecs >> 3, 0); Add something like: << "unrecognized codec";
https://chromiumcodereview.appspot.com/23513055/diff/27001/chrome/browser/med... File chrome/browser/media/encrypted_media_message_filter_android.cc (right): https://chromiumcodereview.appspot.com/23513055/diff/27001/chrome/browser/med... chrome/browser/media/encrypted_media_message_filter_android.cc:88: DCHECK_EQ(response.non_compositing_codecs >> 3, 0); On 2013/09/18 18:13:02, ddorwin wrote: > This only checks that this file is consistent. I think you want to do this to > the value of request.codecs, which comes from the renderer. > > Also, add something like: > << "unrecognized codec"; Done. https://chromiumcodereview.appspot.com/23513055/diff/27001/chrome/renderer/me... File chrome/renderer/media/chrome_key_systems.cc (right): https://chromiumcodereview.appspot.com/23513055/diff/27001/chrome/renderer/me... chrome/renderer/media/chrome_key_systems.cc:197: DCHECK_EQ(supported_codecs >> 3, 0); On 2013/09/18 18:13:02, ddorwin wrote: > Add something like: > << "unrecognized codec"; Done.
rebased after jrummell's change, added WIDEVINE_HRSURFACE enum for android.
lgtm % comments. It's all coming together! https://chromiumcodereview.appspot.com/23513055/diff/100001/chrome/renderer/m... File chrome/renderer/media/chrome_key_systems.cc (right): https://chromiumcodereview.appspot.com/23513055/diff/100001/chrome/renderer/m... chrome/renderer/media/chrome_key_systems.cc:125: #if defined(OS_ANDROID) These should be up with the enum after the rebase. Maybe move the CdmType enum to be first then put these before 110. You can also make 110 #else. https://chromiumcodereview.appspot.com/23513055/diff/100001/chrome/renderer/m... chrome/renderer/media/chrome_key_systems.cc:234: AddWidevineWithCodecs( might not matter in practice, but we should only add if the codec lists are non-empty.
https://codereview.chromium.org/23513055/diff/100001/chrome/renderer/media/ch... File chrome/renderer/media/chrome_key_systems.cc (right): https://codereview.chromium.org/23513055/diff/100001/chrome/renderer/media/ch... chrome/renderer/media/chrome_key_systems.cc:125: #if defined(OS_ANDROID) On 2013/09/18 23:48:07, ddorwin wrote: > These should be up with the enum after the rebase. Maybe move the CdmType enum > to be first then put these before 110. You can also make 110 #else. Done. https://codereview.chromium.org/23513055/diff/100001/chrome/renderer/media/ch... chrome/renderer/media/chrome_key_systems.cc:234: AddWidevineWithCodecs( Added a if statement here to check if the returned list is empty. On 2013/09/18 23:48:07, ddorwin wrote: > might not matter in practice, but we should only add if the codec lists are > non-empty.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/23513055/105001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/23513055/105001
https://codereview.chromium.org/23513055/diff/105001/chrome/renderer/media/ch... File chrome/renderer/media/chrome_key_systems.cc (right): https://codereview.chromium.org/23513055/diff/105001/chrome/renderer/media/ch... chrome/renderer/media/chrome_key_systems.cc:232: if (response.compositing_codecs > 0) { This should be !=, but we can fix that later.
A couple more items for a follow-up CL. https://codereview.chromium.org/23513055/diff/105001/chrome/renderer/media/ch... File chrome/renderer/media/chrome_key_systems.cc (right): https://codereview.chromium.org/23513055/diff/105001/chrome/renderer/media/ch... chrome/renderer/media/chrome_key_systems.cc:153: case WIDEVINE_HRSURFACE: Should we ifdef ANDROID this since it only applies there? https://codereview.chromium.org/23513055/diff/105001/chrome/renderer/media/ch... chrome/renderer/media/chrome_key_systems.cc:154: info.key_system.append(".hrsurface"); We need to add an istypesupported test for this like the one we have for .hr. For now, I think it will always be FALSE.
Message was sent while issue was closed.
Change committed as 224064
Message was sent while issue was closed.
https://codereview.chromium.org/23513055/diff/105001/chrome/renderer/media/ch... File chrome/renderer/media/chrome_key_systems.cc (right): https://codereview.chromium.org/23513055/diff/105001/chrome/renderer/media/ch... chrome/renderer/media/chrome_key_systems.cc:153: case WIDEVINE_HRSURFACE: hmm... so then we should also ifdef on the enum definition, otherwise we are missing a switch case here. that makes the code look a little messy, i think those enums should be available to all platforms. On 2013/09/19 06:31:51, ddorwin wrote: > Should we ifdef ANDROID this since it only applies there? https://codereview.chromium.org/23513055/diff/105001/chrome/renderer/media/ch... chrome/renderer/media/chrome_key_systems.cc:154: info.key_system.append(".hrsurface"); ok, will add that later. On 2013/09/19 06:31:51, ddorwin wrote: > We need to add an istypesupported test for this like the one we have for .hr. > For now, I think it will always be FALSE. https://codereview.chromium.org/23513055/diff/105001/chrome/renderer/media/ch... chrome/renderer/media/chrome_key_systems.cc:232: if (response.compositing_codecs > 0) { makes sense, will change this On 2013/09/19 00:38:17, ddorwin wrote: > This should be !=, but we can fix that later.
Message was sent while issue was closed.
https://codereview.chromium.org/23513055/diff/105001/chrome/common/encrypted_... File chrome/common/encrypted_media_messages_android.h (right): https://codereview.chromium.org/23513055/diff/105001/chrome/common/encrypted_... chrome/common/encrypted_media_messages_android.h:27: struct SupportedKeySystemRequest { sorry I just happened to see this. there's no need to declare a struct here, you can combine line 27-47 with lines 58-67. instead of using the IPC_STRUCT_TRAITS_BEGIN, you can use IPC_STRUCT_BEGIN. The former set of macros are used to define the serialization code for structs that are in a layer that doesn't know about IPC, say wekbit or media. But if the struct is going to be defined in chrome, you might as well use the macros that declare the struct and also its serialization. so this would be: IPC_STRUCT_BEGIN(SupportedKeySystemsRequest) IPC_STRUCT_MEMBER(std::vector<uint8>, uuid) IPC_STRUCT_MEMBER(SupportedCodecs, codecs, NO_SUPPORTED_CODECS) IPC_STRUCT_END() IPC_STRUCT_BEGIN(SupportedKeySystemResponse) IPC_STRUCT_MEMBER(std::vector<uint8>, uuid) IPC_STRUCT_MEMBER(SupportedCodecs, compositing_codecs, NO_SUPPORTED_CODECS) IPC_STRUCT_MEMBER(SupportedCodecs, non_compositing_codecs, NO_SUPPORTED_CODECS) IPC_STRUCT_END() and get rid of the cc file
Message was sent while issue was closed.
https://codereview.chromium.org/23513055/diff/105001/chrome/common/encrypted_... File chrome/common/encrypted_media_messages_android.h (right): https://codereview.chromium.org/23513055/diff/105001/chrome/common/encrypted_... chrome/common/encrypted_media_messages_android.h:27: struct SupportedKeySystemRequest { Thanks, jam, will follow up on that. On 2013/09/19 15:32:14, jam wrote: > sorry I just happened to see this. there's no need to declare a struct here, you > can combine line 27-47 with lines 58-67. instead of using the > IPC_STRUCT_TRAITS_BEGIN, you can use IPC_STRUCT_BEGIN. The former set of macros > are used to define the serialization code for structs that are in a layer that > doesn't know about IPC, say wekbit or media. But if the struct is going to be > defined in chrome, you might as well use the macros that declare the struct and > also its serialization. so this would be: > > IPC_STRUCT_BEGIN(SupportedKeySystemsRequest) > IPC_STRUCT_MEMBER(std::vector<uint8>, uuid) > IPC_STRUCT_MEMBER(SupportedCodecs, codecs, NO_SUPPORTED_CODECS) > IPC_STRUCT_END() > > IPC_STRUCT_BEGIN(SupportedKeySystemResponse) > IPC_STRUCT_MEMBER(std::vector<uint8>, uuid) > IPC_STRUCT_MEMBER(SupportedCodecs, compositing_codecs, NO_SUPPORTED_CODECS) > IPC_STRUCT_MEMBER(SupportedCodecs, non_compositing_codecs, > NO_SUPPORTED_CODECS) > IPC_STRUCT_END() > > and get rid of the cc file |
