Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10857027:
Add content decryptor related structs and update PP{P|B}_ContentDecryptor_Private. (Closed)
|
Created:
8 years, 4 months ago by xhwang Modified:
8 years, 4 months ago CC:
chromium-reviews, piman+watch_chromium.org, darin-cc_chromium.org, yzshen+watch_chromium.org, ihf+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd content decryptor related structs and update PP{P|B}_ContentDecryptor_Private.
PP_EncryptedBlockInfo contains necessary information for decryption and decrypt request tracking. We pass PP_EncryptedBlockInfo in PPP_ContentDecryptor_Private::Decrypt() calls. Similarly we pass PP_DecryptedBlockInfo in PPB_ContentDecryptor_Private::Deliver*() calls.
BUG=none
TEST=none
Patch Set 1 #
Total comments: 30
Patch Set 2 : Resolve comments. #
Total comments: 6
Patch Set 3 : Add PP_DecryptResult and Rename. #
Total comments: 8
Patch Set 4 : Resolve comments. #Patch Set 5 : Rename to pp_content_decryptor.idl #
Messages
Total messages: 12 (0 generated)
Hello dmichael, ddorwin and tomfinegan, This is the CL that updates the new ContentDecryptor interfaces to so that we pass real info needed for decryption and tracking. This CL is interface only. I tried to update the implementation today based on Tom's implementation CL but it's not fun (especially the proxy stuff). I feel it makes sense to commit this interface first then Tom can hook everything up in one shot instead of either of us painfully rebase. PTAL! xhwang http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... File ppapi/api/private/pp_decrypt_config.idl (right): http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:87: uint8_t[68] key_id; As per dmichael@: 1, 8-byte data need to be on 8-byte boundary. 2, if a struct has 8byte data (e.g. timestamp in tracking_info), then the size of the struct needs to be a multiple of 8 (through padding if necessary). That's why I have "68" here just to avoid extra padding. Maybe I should avoid these magic numbers and use something like "kMaxKeyIdSize"? As per fgalligan@, the KeyId can be as large as 2048bytes in WebM. If we do need to use that many, maybe we should move this out as a separate PP_Var parameter.
Non-Pepper review. http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... File ppapi/api/private/pp_decrypt_config.idl (right): http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:21: * client can associate the decrypted (and decoded) data with an input buffer. Why isn't the above member enough for association? http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:28: int64_t duration; Pending scherkus's response in the other CL. http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:70: * The data structure that contains all information needed to decrypt a buffer. Is "The data structure that" wording common in PPAPI? Sounds funny. http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:77: PP_DecryptTrackingInfo tracking_info; Not really decryption configuration. Can we use a separate parameter or are we out of parameter space in the calls? http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:87: uint8_t[68] key_id; On 2012/08/16 03:14:41, xhwang wrote: > As per dmichael@: > 1, 8-byte data need to be on 8-byte boundary. > 2, if a struct has 8byte data (e.g. timestamp in tracking_info), then the size > of the struct needs to be a multiple of 8 (through padding if necessary). > > That's why I have "68" here just to avoid extra padding. > > Maybe I should avoid these magic numbers and use something like "kMaxKeyIdSize"? > > As per fgalligan@, the KeyId can be as large as 2048bytes in WebM. If we do need > to use that many, maybe we should move this out as a separate PP_Var parameter. I still don't understand the reason for 68 here. Maybe you can explain offline. http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:103: * Subsamples of the buffer to be decrypted. Subsample information/map of... http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:105: PP_DecryptSubsampleEntry[16] subsamples; Strange to always have 1 KB of data appended. May have similar problems if we have to support large Key IDs.
http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... File ppapi/api/private/pp_decrypt_config.idl (right): http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:7: * The data structure that can be used to associate the decrypted data with a This comment is worded strangely. We usually use complete sentences, and maybe start with the struct name... like: "PP_DecryptTrackingInfo is a struct that can be used..." http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:14: * this value client can associate the decrypted data with a decryption "value client" -> "value, the client" http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:23: int64_t timestamp; We usually use PP_Time (which really just typedefs a double) for time stamps. Would that work okay for you? It would make your time easier to associate with things like input events, file times, or wall clock times, if those ever matter for CDMs. http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:28: int64_t duration; On 2012/08/16 04:31:52, ddorwin wrote: > Pending scherkus's response in the other CL. 64 bits seems really huge for milliseconds... are you sure that's really the resolution you want? Did you mean microseconds, or int32_t? http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:42: * For decryption, all of the cipher bytes in a buffer should be concatenated Do you really have to concatenate the "cipher bytes"? I think you just mean that they should be considered as a contiguous sequence of bytes during decryption, ignoring the "clear" portion. The way you word it seems to dictate an implementation detail, and I think the implementation would actually just want to read the cipher text as-is from the subsamples without any intermediate step of concatenating the encrypted data. Make sense? I'm not sure how best to word that, though. http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:57: struct PP_DecryptSubsampleEntry { Maybe a different name, since this isn't really a Subsample Entry so much as a "Subsample Description"? http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:70: * The data structure that contains all information needed to decrypt a buffer. On 2012/08/16 04:31:52, ddorwin wrote: > Is "The data structure that" wording common in PPAPI? Sounds funny. Not common. We'd usually say "PP_DecryptConfig contains..." http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:87: uint8_t[68] key_id; On 2012/08/16 03:14:41, xhwang wrote: > As per dmichael@: > 1, 8-byte data need to be on 8-byte boundary. Well, it's not *our* rule :-). The compiler will align it on an 8-byte boundary for you on 64-bit platforms, and sometimes 4-byte boundaries on 32-bit (e.g., double aligns to 32-bit on NaCl and Windows). We try to keep consistent sizes to make the proxy easier. > 2, if a struct has 8byte data (e.g. timestamp in tracking_info), then the size > of the struct needs to be a multiple of 8 (through padding if necessary). That is, the compiler will enforce that (sometimes only on 64-bit, though), so that if you put those types in an array, all the structs and their 8-byte-aligned members remain 8-byte-aligned. To keep consistent size across platforms, we usually pad. (Admittedly, this only really matters when we talk to NaCl from 64-bit trusted code, and only when we do cheap memcpy serialization as we do here). > > That's why I have "68" here just to avoid extra padding. > > Maybe I should avoid these magic numbers and use something like "kMaxKeyIdSize"? A constant might be better, yeah. And I would just use the correct size for your data, whatever it is. Add padding as a separate field with a nice explanatory comment (see some of our other padding comments on structs for examples). > > As per fgalligan@, the KeyId can be as large as 2048bytes in WebM. If we do need > to use that many, maybe we should move this out as a separate PP_Var parameter. http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:107: }; This is pretty big... you *could* consider making it a PP_Resource. - Is it likely to change a lot (like, adding new fields, changing their size)? - Do you have to worry about backwards compatibility, like working with older CDM modules that were built for an older version of PPAPI? If yes on those questions, you should probably use PP_Resource. If not, then the way you have it is probably simpler (at least to start).
Comments mostly resolved. PTAL again! http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... File ppapi/api/private/pp_decrypt_config.idl (right): http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:7: * The data structure that can be used to associate the decrypted data with a On 2012/08/16 17:36:33, dmichael wrote: > This comment is worded strangely. We usually use complete sentences, and maybe > start with the struct name... like: > "PP_DecryptTrackingInfo is a struct that can be used..." Done. http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:14: * this value client can associate the decrypted data with a decryption On 2012/08/16 17:36:33, dmichael wrote: > "value client" -> "value, the client" Done. http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:21: * client can associate the decrypted (and decoded) data with an input buffer. On 2012/08/16 04:31:52, ddorwin wrote: > Why isn't the above member enough for association? Added more explanation: "This is needed as the decoded buffers can be out of order." http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:23: int64_t timestamp; On 2012/08/16 17:36:33, dmichael wrote: > We usually use PP_Time (which really just typedefs a double) for time stamps. > Would that work okay for you? It would make your time easier to associate with > things like input events, file times, or wall clock times, if those ever matter > for CDMs. This timestamp is really only for tracking and in theory it can be any unique identifier. We won't use it for any input events, file times etc. So I'll just keep it here as int64_t to avoid any unnecessary conversion. http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:28: int64_t duration; On 2012/08/16 17:36:33, dmichael wrote: > On 2012/08/16 04:31:52, ddorwin wrote: > > Pending scherkus's response in the other CL. > 64 bits seems really huge for milliseconds... are you sure that's really the > resolution you want? Did you mean microseconds, or int32_t? duration is removed as per offline discussion w/ scherkus@. http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:42: * For decryption, all of the cipher bytes in a buffer should be concatenated On 2012/08/16 17:36:33, dmichael wrote: > Do you really have to concatenate the "cipher bytes"? I think you just mean that > they should be considered as a contiguous sequence of bytes during decryption, > ignoring the "clear" portion. The way you word it seems to dictate an > implementation detail, and I think the implementation would actually just want > to read the cipher text as-is from the subsamples without any intermediate step > of concatenating the encrypted data. Make sense? I'm not sure how best to word > that, though. Done. http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:57: struct PP_DecryptSubsampleEntry { On 2012/08/16 17:36:33, dmichael wrote: > Maybe a different name, since this isn't really a Subsample Entry so much as a > "Subsample Description"? Done. http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:70: * The data structure that contains all information needed to decrypt a buffer. On 2012/08/16 04:31:52, ddorwin wrote: > Is "The data structure that" wording common in PPAPI? Sounds funny. Done. http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:77: PP_DecryptTrackingInfo tracking_info; On 2012/08/16 04:31:52, ddorwin wrote: > Not really decryption configuration. Can we use a separate parameter or are we > out of parameter space in the calls? Added comment about this. http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:87: uint8_t[68] key_id; On 2012/08/16 17:36:33, dmichael wrote: > On 2012/08/16 03:14:41, xhwang wrote: > > As per dmichael@: > > 1, 8-byte data need to be on 8-byte boundary. > Well, it's not *our* rule :-). The compiler will align it on an 8-byte boundary > for you on 64-bit platforms, and sometimes 4-byte boundaries on 32-bit (e.g., > double aligns to 32-bit on NaCl and Windows). We try to keep consistent sizes to > make the proxy easier. > > 2, if a struct has 8byte data (e.g. timestamp in tracking_info), then the size > > of the struct needs to be a multiple of 8 (through padding if necessary). > That is, the compiler will enforce that (sometimes only on 64-bit, though), so > that if you put those types in an array, all the structs and their > 8-byte-aligned members remain 8-byte-aligned. To keep consistent size across > platforms, we usually pad. (Admittedly, this only really matters when we talk to > NaCl from 64-bit trusted code, and only when we do cheap memcpy serialization as > we do here). > > > > That's why I have "68" here just to avoid extra padding. > > > > Maybe I should avoid these magic numbers and use something like > "kMaxKeyIdSize"? > A constant might be better, yeah. There's no easy way to define and use a constant in IDL file. So keep the numbers hard coded here for now. And I would just use the correct size for your > data, whatever it is. Add padding as a separate field with a nice explanatory > comment (see some of our other padding comments on structs for examples). > > > > > As per fgalligan@, the KeyId can be as large as 2048bytes in WebM. If we do > need > > to use that many, maybe we should move this out as a separate PP_Var > parameter. > Thanks for the more detailed explanation! I double checked and adjusted the size for IV and checksum. Now it's a multiple of 8 w/o padding :) http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:103: * Subsamples of the buffer to be decrypted. On 2012/08/16 04:31:52, ddorwin wrote: > Subsample information/map of... Done. http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:105: PP_DecryptSubsampleEntry[16] subsamples; On 2012/08/16 04:31:52, ddorwin wrote: > Strange to always have 1 KB of data appended. May have similar problems if we > have to support large Key IDs. It's 16*8=128bytes, so I suppose it's fine here. Will keep it here for now due to the limit on num of parameters in IPC message. http://codereview.chromium.org/10857027/diff/1/ppapi/api/private/pp_decrypt_c... ppapi/api/private/pp_decrypt_config.idl:107: }; On 2012/08/16 17:36:33, dmichael wrote: > This is pretty big... you *could* consider making it a PP_Resource. > - Is it likely to change a lot (like, adding new fields, changing their size)? > - Do you have to worry about backwards compatibility, like working with older > CDM modules that were built for an older version of PPAPI? > > If yes on those questions, you should probably use PP_Resource. If not, then the > way you have it is probably simpler (at least to start). That's a good suggestion. It may change in size and we'll need backwards compatibility later (but not anytime soon). But currently we need something simpler to start with and we don't have resource to do it that way. I'll keep it as is for now and we can fix it in the future.
lgtm http://codereview.chromium.org/10857027/diff/5001/ppapi/api/private/pp_decryp... File ppapi/api/private/pp_decrypt_config.idl (right): http://codereview.chromium.org/10857027/diff/5001/ppapi/api/private/pp_decryp... ppapi/api/private/pp_decrypt_config.idl:42: * continuous (in the subsample order) logical stream. The clear bytes should continuous->contiguous?
On 2012/08/16 22:26:51, dmichael wrote: > lgtm > > http://codereview.chromium.org/10857027/diff/5001/ppapi/api/private/pp_decryp... > File ppapi/api/private/pp_decrypt_config.idl (right): > > http://codereview.chromium.org/10857027/diff/5001/ppapi/api/private/pp_decryp... > ppapi/api/private/pp_decrypt_config.idl:42: * continuous (in the subsample > order) logical stream. The clear bytes should > continuous->contiguous? lgtm
lgtm % comments. I wrote these before our discussion. Not sure whether you want to land this before addressing that discussion. http://codereview.chromium.org/10857027/diff/5001/ppapi/api/private/pp_decryp... File ppapi/api/private/pp_decrypt_config.idl (right): http://codereview.chromium.org/10857027/diff/5001/ppapi/api/private/pp_decryp... ppapi/api/private/pp_decrypt_config.idl:18: uint64_t request_id; This is only used for decrypt (no decode), right? If so, couldn't we just use timestamp in all cases? All blocks have a timestamp, right? The fact that I had to write "and not in response to the request_id they were provided with" below makes me question again the use of a request_id instead of an enum. Fine for now I guess. http://codereview.chromium.org/10857027/diff/5001/ppapi/api/private/pp_decryp... ppapi/api/private/pp_decrypt_config.idl:23: * buffer. This is needed because the decoded buffers can be out of the input ... buffers may be delivered out of order and not in response to the request_id they were provided with.
Sorry for another change :) I just realized that we need the decrpytion (and/or decoding) result to be passed back with the decrypted_block(frame, samples). So I added PP_DecryptResult and PP_DecryptedBlockInfo. Also I renamed PP_DecryptConfig to PP_EncryptedBlockInfo. After this CL is reviewed, I'd like to rename pp_decrypt_config.idl to pp_content_decryptor.idl to better reflect the content of this file. PTAL again! xhwang http://codereview.chromium.org/10857027/diff/5001/ppapi/api/private/pp_decryp... File ppapi/api/private/pp_decrypt_config.idl (right): http://codereview.chromium.org/10857027/diff/5001/ppapi/api/private/pp_decryp... ppapi/api/private/pp_decrypt_config.idl:18: uint64_t request_id; On 2012/08/16 23:56:05, ddorwin wrote: > This is only used for decrypt (no decode), right? If so, couldn't we just use > timestamp in all cases? All blocks have a timestamp, right? > The fact that I had to write "and not in response to the request_id they were > provided with" below makes me question again the use of a request_id instead of > an enum. Fine for now I guess. Even with decode, we still need to find the right callback to fire. I'll think about enum more but I'd land this first. http://codereview.chromium.org/10857027/diff/5001/ppapi/api/private/pp_decryp... ppapi/api/private/pp_decrypt_config.idl:23: * buffer. This is needed because the decoded buffers can be out of the input On 2012/08/16 23:56:05, ddorwin wrote: > ... buffers may be delivered out of order and not in response to the request_id > they were provided with. Done. http://codereview.chromium.org/10857027/diff/5001/ppapi/api/private/pp_decryp... ppapi/api/private/pp_decrypt_config.idl:42: * continuous (in the subsample order) logical stream. The clear bytes should On 2012/08/16 22:26:51, dmichael wrote: > continuous->contiguous? Done.
lgtm + 1 nit https://chromiumcodereview.appspot.com/10857027/diff/6005/ppapi/api/private/p... File ppapi/api/private/pp_decrypt_config.idl (right): https://chromiumcodereview.appspot.com/10857027/diff/6005/ppapi/api/private/p... ppapi/api/private/pp_decrypt_config.idl:127: /** Unexpected error happened during decocding. */ s/decocding/decoding/
lgtm just nits http://codereview.chromium.org/10857027/diff/6005/ppapi/api/private/pp_decryp... File ppapi/api/private/pp_decrypt_config.idl (right): http://codereview.chromium.org/10857027/diff/6005/ppapi/api/private/pp_decryp... ppapi/api/private/pp_decrypt_config.idl:69: * The <code>PP_EncryptedBlockInfo</code> struct contains all information nit: all +the+ information http://codereview.chromium.org/10857027/diff/6005/ppapi/api/private/pp_decryp... ppapi/api/private/pp_decrypt_config.idl:125: /** Unexpected error happened during decryption. */ nit: +An+ unexpected http://codereview.chromium.org/10857027/diff/6005/ppapi/api/private/pp_decryp... ppapi/api/private/pp_decrypt_config.idl:127: /** Unexpected error happened during decocding. */ nit: +An+ unexpected
http://codereview.chromium.org/10857027/diff/6005/ppapi/api/private/pp_decryp... File ppapi/api/private/pp_decrypt_config.idl (right): http://codereview.chromium.org/10857027/diff/6005/ppapi/api/private/pp_decryp... ppapi/api/private/pp_decrypt_config.idl:69: * The <code>PP_EncryptedBlockInfo</code> struct contains all information On 2012/08/17 02:24:06, dmichael wrote: > nit: all +the+ information Done. http://codereview.chromium.org/10857027/diff/6005/ppapi/api/private/pp_decryp... ppapi/api/private/pp_decrypt_config.idl:125: /** Unexpected error happened during decryption. */ On 2012/08/17 02:24:06, dmichael wrote: > nit: +An+ unexpected Done. http://codereview.chromium.org/10857027/diff/6005/ppapi/api/private/pp_decryp... ppapi/api/private/pp_decrypt_config.idl:127: /** Unexpected error happened during decocding. */ On 2012/08/17 01:41:55, tomf wrote: > s/decocding/decoding/ Done. http://codereview.chromium.org/10857027/diff/6005/ppapi/api/private/pp_decryp... ppapi/api/private/pp_decrypt_config.idl:127: /** Unexpected error happened during decocding. */ On 2012/08/17 02:24:06, dmichael wrote: > nit: +An+ unexpected Done.
This CL will be committed as part of tomf@'s change: http://codereview.chromium.org/10854209/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
