Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2446413003:
Add cdm::ContentDecryptionModule_9 (Closed)
|
Created:
4 years, 1 month ago by jrummell Modified:
3 years, 7 months ago CC:
cdm-api-reviews_chromium.org, feature-media-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium_deps Visibility:
Public. |
DescriptionAdd cdm::ContentDecryptionModule_9
Changes:
- Support host challenge
- Remove OnLegacySessionError()
- Remove |legacy_destination_url| from OnSessionMessage()
- Rename DecryptAndDecodeFrame() to DecryptAndDecodeVideo()
- Rename DecryptAndDecodeSamples() to DecryptAndDecodeAudio()
BUG=570216
Patch Set 1 #
Total comments: 23
Patch Set 2 : RejectionReason #
Total comments: 10
Patch Set 3 : export again #
Messages
Total messages: 18 (5 generated)
jrummell@chromium.org changed reviewers: + tinskip@chromium.org, xhwang@chromium.org
PTAL.
lgtm
ddorwin@chromium.org changed reviewers: + ddorwin@chromium.org
Not LGTM due to path issue. Also, it would be nice if PS1 was a copy of CDM8 so we can easily see the differences in CDM9. https://codereview.chromium.org/2446413003/diff/1/content_decryption_module.h File content_decryption_module.h (right): https://codereview.chromium.org/2446413003/diff/1/content_decryption_module.h... content_decryption_module.h:17: #include "media/cdm/api/content_decryption_module_export.h" This assumes the Chrome code structure, which may not apply for other browsers.
Comments on exceptions. See also https://bugs.chromium.org/p/chromium/issues/detail?id=570216#c11. https://codereview.chromium.org/2446413003/diff/1/content_decryption_module.h File content_decryption_module.h (right): https://codereview.chromium.org/2446413003/diff/1/content_decryption_module.h... content_decryption_module.h:98: // The following starts with the list of DOM4 exceptions from: Remove this sentence and the next one. Maybe just note that the values are the legacy code values from the URL below. https://codereview.chromium.org/2446413003/diff/1/content_decryption_module.h... content_decryption_module.h:99: // http://www.w3.org/TR/dom/#domexception These were moved to https://heycam.github.io/webidl/#idl-DOMException-error-names https://codereview.chromium.org/2446413003/diff/1/content_decryption_module.h... content_decryption_module.h:104: kInvalidAccessError = 15, Can we rename this to kTypeError? Rename may not make sense since we use the legacy code value from the other spec. Thus, we would need to add kTypeError (= 50?) and rename this to something like kDeprecatedInvalidAccessErrorWillBeTreatedAsTypeError. https://codereview.chromium.org/2446413003/diff/1/content_decryption_module.h... content_decryption_module.h:107: // Additional exceptions that do not have assigned codes. Are these three still used in CDM_8 implementations? If not, can we remove them? Even if so, can we remove them and have the host handle them as a default value? Or at least rename them Deprecated so that only hosts will use them and note that they will be removed when CDM_8 is removed.
tinskip@google.com changed reviewers: + tinskip@google.com
https://codereview.chromium.org/2446413003/diff/1/content_decryption_module.h File content_decryption_module.h (right): https://codereview.chromium.org/2446413003/diff/1/content_decryption_module.h... content_decryption_module.h:1089: virtual void SendPlatformChallenge(const char* service_id, I believe keeping the single PlatformChallenge API with a polymorphic message sent and received (RA vs host) was somewhat a bit cleaner. https://codereview.chromium.org/2446413003/diff/1/content_decryption_module_b... File content_decryption_module_broker.h (right): https://codereview.chromium.org/2446413003/diff/1/content_decryption_module_b... content_decryption_module_broker.h:23: void* user_data); s/user_data/context/ here and below? A bit clearer as "user_data" can be construed as, well, user data.
https://codereview.chromium.org/2446413003/diff/1/content_decryption_module.h File content_decryption_module.h (right): https://codereview.chromium.org/2446413003/diff/1/content_decryption_module.h... content_decryption_module.h:101: enum Error { These are exceptions intended to be used in OnRejectPromise(). We should rename it as such.
https://codereview.chromium.org/2446413003/diff/1/content_decryption_module_b... File content_decryption_module_broker.h (right): https://codereview.chromium.org/2446413003/diff/1/content_decryption_module_b... content_decryption_module_broker.h:31: const char** binary_file_paths, In chromium the chat type for base::FilePath is char on Posix and wchar_t on Windows: https://cs.chromium.org/chromium/src/base/files/file_path.h?rcl=0&l=12 So passing char** here seems problematic on Windows. https://codereview.chromium.org/2446413003/diff/1/content_decryption_module_b... content_decryption_module_broker.h:33: HostResponseCallback response_callback, Can we require this callback to be called synchronously? Otherwise, it's tricky to make sure |user_data|'s lifetime.
https://codereview.chromium.org/2446413003/diff/1/content_decryption_module_b... File content_decryption_module_broker.h (right): https://codereview.chromium.org/2446413003/diff/1/content_decryption_module_b... content_decryption_module_broker.h:33: HostResponseCallback response_callback, On 2016/10/28 06:34:25, xhwang wrote: > Can we require this callback to be called synchronously? Otherwise, it's tricky > to make sure |user_data|'s lifetime. Actually, if we require this callback to be called synchronously, does it make sense to just provide two output parameters? Is there a upper limit of the size of the response? If so, we can pre-allocate memory for the response and let the CDM broker to fill the response in.
https://codereview.chromium.org/2446413003/diff/1/content_decryption_module.h File content_decryption_module.h (right): https://codereview.chromium.org/2446413003/diff/1/content_decryption_module.h... content_decryption_module.h:17: #include "media/cdm/api/content_decryption_module_export.h" On 2016/10/26 03:54:45, ddorwin wrote: > This assumes the Chrome code structure, which may not apply for other browsers. Can we just have this here: #include "content_decryption_module_export.h" I *think* the current folder will always be searched (I could be wrong). The risk is that we may have another content_decryption_module_export.h somewhere in the include path, but I *think* the "current folder" is always searched first: https://gcc.gnu.org/onlinedocs/cpp/Search-Path.html https://msdn.microsoft.com/en-us/library/36k2cdd4.aspx
https://codereview.chromium.org/2446413003/diff/1/content_decryption_module_b... File content_decryption_module_broker.h (right): https://codereview.chromium.org/2446413003/diff/1/content_decryption_module_b... content_decryption_module_broker.h:21: typedef void (*HostResponseCallback)(const uint8_t* response, I am prototyping this and found "Callback" to be misleading, in the code it'll look like: response_callback(response, size, user_data); Maybe this should be "response_func"?
Patchset #2 (id:20001) has been deleted
Updated with most comments addressed. Not addressed: 1) Can ProcessHostChallenge() be done synchronously (i.e. no callback). 2) Merge SendPlatformChallenge() and SendHostChallenge(). For #2, I'd prefer to keep them separate. No confusion with a huge union type, and the responses are clear. Also avoiding protobufs since other users of the API may not want to try and decode it. https://codereview.chromium.org/2446413003/diff/1/content_decryption_module.h File content_decryption_module.h (right): https://codereview.chromium.org/2446413003/diff/1/content_decryption_module.h... content_decryption_module.h:17: #include "media/cdm/api/content_decryption_module_export.h" On 2016/10/26 03:54:45, ddorwin wrote: > This assumes the Chrome code structure, which may not apply for other browsers. Done. https://codereview.chromium.org/2446413003/diff/1/content_decryption_module.h... content_decryption_module.h:17: #include "media/cdm/api/content_decryption_module_export.h" On 2016/10/28 17:42:02, xhwang wrote: > On 2016/10/26 03:54:45, ddorwin wrote: > > This assumes the Chrome code structure, which may not apply for other > browsers. > > Can we just have this here: > > #include "content_decryption_module_export.h" > > I *think* the current folder will always be searched (I could be wrong). The > risk is that we may have another content_decryption_module_export.h somewhere in > the include path, but I *think* the "current folder" is always searched first: > > https://gcc.gnu.org/onlinedocs/cpp/Search-Path.html > https://msdn.microsoft.com/en-us/library/36k2cdd4.aspx I've merged the header into both files to avoid this issue. https://codereview.chromium.org/2446413003/diff/1/content_decryption_module.h... content_decryption_module.h:98: // The following starts with the list of DOM4 exceptions from: On 2016/10/26 23:58:49, ddorwin wrote: > Remove this sentence and the next one. Maybe just note that the values are the > legacy code values from the URL below. Done. https://codereview.chromium.org/2446413003/diff/1/content_decryption_module.h... content_decryption_module.h:99: // http://www.w3.org/TR/dom/#domexception On 2016/10/26 23:58:50, ddorwin wrote: > These were moved to > https://heycam.github.io/webidl/#idl-DOMException-error-names Done. https://codereview.chromium.org/2446413003/diff/1/content_decryption_module.h... content_decryption_module.h:101: enum Error { On 2016/10/27 00:40:21, ddorwin wrote: > These are exceptions intended to be used in OnRejectPromise(). We should rename > it as such. Changed to RejectionReason (since it's still used by OnLegacySessionError()). https://codereview.chromium.org/2446413003/diff/1/content_decryption_module.h... content_decryption_module.h:104: kInvalidAccessError = 15, On 2016/10/26 23:58:49, ddorwin wrote: > Can we rename this to kTypeError? > > Rename may not make sense since we use the legacy code value from the other > spec. > > Thus, we would need to add kTypeError (= 50?) and rename this to something like > kDeprecatedInvalidAccessErrorWillBeTreatedAsTypeError. Done (the long name). https://codereview.chromium.org/2446413003/diff/1/content_decryption_module.h... content_decryption_module.h:107: // Additional exceptions that do not have assigned codes. On 2016/10/26 23:58:50, ddorwin wrote: > Are these three still used in CDM_8 implementations? If not, can we remove them? > Even if so, can we remove them and have the host handle them as a default value? > Or at least rename them Deprecated so that only hosts will use them and note > that they will be removed when CDM_8 is removed. The only 1 not used appears to be kClientError. Rename them to indicate they shouldn't be used. https://codereview.chromium.org/2446413003/diff/1/content_decryption_module_b... File content_decryption_module_broker.h (right): https://codereview.chromium.org/2446413003/diff/1/content_decryption_module_b... content_decryption_module_broker.h:21: typedef void (*HostResponseCallback)(const uint8_t* response, On 2016/10/28 18:18:14, xhwang wrote: > I am prototyping this and found "Callback" to be misleading, in the code it'll > look like: > > response_callback(response, size, user_data); > > Maybe this should be "response_func"? Done. https://codereview.chromium.org/2446413003/diff/1/content_decryption_module_b... content_decryption_module_broker.h:23: void* user_data); On 2016/10/27 00:32:47, tinskip wrote: > s/user_data/context/ here and below? A bit clearer as "user_data" can be > construed as, well, user data. Done. https://codereview.chromium.org/2446413003/diff/1/content_decryption_module_b... content_decryption_module_broker.h:31: const char** binary_file_paths, On 2016/10/28 06:34:25, xhwang wrote: > In chromium the chat type for base::FilePath is char on Posix and wchar_t on > Windows: > > https://cs.chromium.org/chromium/src/base/files/file_path.h?rcl=0&l=12 > > So passing char** here seems problematic on Windows. Done.
https://codereview.chromium.org/2446413003/diff/40001/content_decryption_modu... File content_decryption_module.h (right): https://codereview.chromium.org/2446413003/diff/40001/content_decryption_modu... content_decryption_module.h:40: +ddorwin: Continuing on the comment on #include "content_decryption_module_export.h" This works, but if the CDM_API defined here and in the *broker.h file ever become different it would be confusing (even with the undef at the end). I still like the *_export.h file, but I don't have a strong opinion here. https://codereview.chromium.org/2446413003/diff/40001/content_decryption_modu... content_decryption_module.h:114: kTypeError = 50, OOC, where is 50 coming from? Is it just another arbitrary value? Should we stick with the existing 100 series? https://codereview.chromium.org/2446413003/diff/40001/content_decryption_modu... File content_decryption_module_broker.h (right): https://codereview.chromium.org/2446413003/diff/40001/content_decryption_modu... content_decryption_module_broker.h:37: typedef std::string::value_type CdmFilePathCharType; Can you use wchar_t and char directly? It's a bit odd to refer to wstring/string::value_type without using wstring/string directly. https://codereview.chromium.org/2446413003/diff/40001/content_decryption_modu... content_decryption_module_broker.h:37: typedef std::string::value_type CdmFilePathCharType; CdmFilePath is a bit misleading; some paths are not "cdm path". How about just FilePathCharType? https://codereview.chromium.org/2446413003/diff/40001/content_decryption_modu... content_decryption_module_broker.h:38: #endif // defined(OS_WIN) s/OS_WIN/WIN32?
Updated with cdm_export.h back in the PS. https://codereview.chromium.org/2446413003/diff/40001/content_decryption_modu... File content_decryption_module.h (right): https://codereview.chromium.org/2446413003/diff/40001/content_decryption_modu... content_decryption_module.h:40: On 2016/10/31 22:43:47, xhwang wrote: > +ddorwin: > > Continuing on the comment on > #include "content_decryption_module_export.h" > > This works, but if the CDM_API defined here and in the *broker.h file ever > become different it would be confusing (even with the undef at the end). I still > like the *_export.h file, but I don't have a strong opinion here. > export.h is back. https://codereview.chromium.org/2446413003/diff/40001/content_decryption_modu... content_decryption_module.h:114: kTypeError = 50, On 2016/10/31 22:43:47, xhwang wrote: > OOC, where is 50 coming from? Is it just another arbitrary value? Should we > stick with the existing 100 series? A long time ago new specs would simply add their own errors and specify the codes with them. The values here matched what DOMException used. However, several years ago they stopped assigning numbers, and the official legacy codes only go to 25 (see the link in the comments above this). blink now uses ExceptionCode which is unrelated to the "code" returned by DOMException (DOMException maps the code in the constructor). So these values can be completely independent, and we can use anything we want. For backwards compatibility I've preserved the previous values. Since we mention the legacy codes, as long as the value is >25 it is good enough. https://codereview.chromium.org/2446413003/diff/40001/content_decryption_modu... File content_decryption_module_broker.h (right): https://codereview.chromium.org/2446413003/diff/40001/content_decryption_modu... content_decryption_module_broker.h:37: typedef std::string::value_type CdmFilePathCharType; On 2016/10/31 22:43:47, xhwang wrote: > Can you use wchar_t and char directly? It's a bit odd to refer to > wstring/string::value_type without using wstring/string directly. Done. https://codereview.chromium.org/2446413003/diff/40001/content_decryption_modu... content_decryption_module_broker.h:37: typedef std::string::value_type CdmFilePathCharType; On 2016/10/31 22:43:47, xhwang wrote: > CdmFilePath is a bit misleading; some paths are not "cdm path". How about just > FilePathCharType? Done. https://codereview.chromium.org/2446413003/diff/40001/content_decryption_modu... content_decryption_module_broker.h:38: #endif // defined(OS_WIN) On 2016/10/31 22:43:47, xhwang wrote: > s/OS_WIN/WIN32? Done.
Description was changed from ========== Add cdm::ContentDecryptionModule_9 Changes: - Support host challenge - Remove OnLegacySessionError() - Remove |legacy_destination_url| from OnSessionMessage() - Rename DecryptAndDecodeFrame() to DecryptAndDecodeVideo() - Rename DecryptAndDecodeSamples() to DecryptAndDecodeAudio() BUG=570216 ========== to ========== Add cdm::ContentDecryptionModule_9 Changes: - Support host challenge - Remove OnLegacySessionError() - Remove |legacy_destination_url| from OnSessionMessage() - Rename DecryptAndDecodeFrame() to DecryptAndDecodeVideo() - Rename DecryptAndDecodeSamples() to DecryptAndDecodeAudio() BUG=570216 ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
