Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10900007:
Add video decoding support in the CDM interface. (Closed)
|
Created:
8 years, 3 months ago by xhwang Modified:
8 years, 3 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd video decoding support in the CDM interface.
BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155174
Patch Set 1 #Patch Set 2 : Added more video decoding methods. #Patch Set 3 : Replace uint8 with uint8_t. #
Total comments: 8
Patch Set 4 : Resolve comments. #
Total comments: 11
Patch Set 5 : Resolve comments. #
Total comments: 1
Patch Set 6 : Merge issue 10899021 on CDM interface. #
Total comments: 28
Patch Set 7 : Resolve ddorwin's comments. #
Total comments: 28
Patch Set 8 : Resolve comments. #
Messages
Total messages: 18 (0 generated)
Hello, I added video decoding methods in the CDM interface. Please review. xhwang
http://codereview.chromium.org/10900007/diff/4004/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/10900007/diff/4004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.cc:221: return cdm::kErrorUnknown; add NOTIMPLEMENTED() so we get some logging http://codereview.chromium.org/10900007/diff/4004/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10900007/diff/4004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:133: struct VideoFrame { this looks copied from media/base/video_frame.h can you trim this down to the absolute minimal set required for webm/vp8? http://codereview.chromium.org/10900007/diff/4004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:188: kCodecTheora, ditto http://codereview.chromium.org/10900007/diff/4004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:271: // function must be called before DecryptAndDecode is called. append () to function names in comments
Resolved comments and also fixed some others issues I found. PTAL! http://codereview.chromium.org/10900007/diff/4004/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/10900007/diff/4004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.cc:221: return cdm::kErrorUnknown; On 2012/09/01 12:33:08, scherkus wrote: > add NOTIMPLEMENTED() so we get some logging Done. http://codereview.chromium.org/10900007/diff/4004/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10900007/diff/4004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:133: struct VideoFrame { On 2012/09/01 12:33:08, scherkus wrote: > this looks copied from media/base/video_frame.h > > can you trim this down to the absolute minimal set required for webm/vp8? Done. http://codereview.chromium.org/10900007/diff/4004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:188: kCodecTheora, On 2012/09/01 12:33:08, scherkus wrote: > ditto Done. http://codereview.chromium.org/10900007/diff/4004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:271: // function must be called before DecryptAndDecode is called. On 2012/09/01 12:33:08, scherkus wrote: > append () to function names in comments Done.
http://codereview.chromium.org/10900007/diff/10001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10900007/diff/10001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:138: kI420 // 12bpp YVU planar 1x1 Y, 2x2 UV samples. Tom: in vp8, we use I420, not YV12, correct?
http://codereview.chromium.org/10900007/diff/10001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10900007/diff/10001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:176: struct VideoDeocderConfig { typo http://codereview.chromium.org/10900007/diff/10001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:265: // only empty (|format| being EMPTY) |video_frame| can be produced. what's EMPTY? I only see kInvalidFormat http://codereview.chromium.org/10900007/diff/10001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:268: // |data| in |video_frame| to the caller. what about decoders with delay? the first few calls may return empty frames what's the error code when flushing end of the stream? is it kSuccess + kInvalidFormat? http://codereview.chromium.org/10900007/diff/10001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:286: // that a VideoDecoder should/could not be re-initialized after it has been should/could not -> cannot http://codereview.chromium.org/10900007/diff/10001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:290: virtual Status StopVideoDecoder() = 0; what does Stop() + kErrorUnknown mean?
http://codereview.chromium.org/10900007/diff/10001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10900007/diff/10001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:176: struct VideoDeocderConfig { On 2012/09/02 09:13:24, scherkus wrote: > typo Done. http://codereview.chromium.org/10900007/diff/10001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:265: // only empty (|format| being EMPTY) |video_frame| can be produced. On 2012/09/02 09:13:24, scherkus wrote: > what's EMPTY? I only see kInvalidFormat Done. http://codereview.chromium.org/10900007/diff/10001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:268: // |data| in |video_frame| to the caller. On 2012/09/02 09:13:24, scherkus wrote: > what about decoders with delay? the first few calls may return empty frames > > what's the error code when flushing end of the stream? is it kSuccess + > kInvalidFormat? Done. http://codereview.chromium.org/10900007/diff/10001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:286: // that a VideoDecoder should/could not be re-initialized after it has been On 2012/09/02 09:13:24, scherkus wrote: > should/could not -> cannot Done. http://codereview.chromium.org/10900007/diff/10001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:290: virtual Status StopVideoDecoder() = 0; On 2012/09/02 09:13:24, scherkus wrote: > what does Stop() + kErrorUnknown mean? Right, in media code the callback is a closure. Update this and ResetVideoDecoder() to return void.
Need to reconcile CDM.h changes with 10899021. I'm waiting for that before reviewing most of CDM.h. http://codereview.chromium.org/10900007/diff/11001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10900007/diff/11001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:32: kDecryptOnly, // The CDM does not support to DecryptAndDecode() requested "to" does not seem like the correct grammar.
IIRC the plan is for xhwang@ to merge in tomf@'s changes -- if that is indeed the plan I'm going to wait until that's done before reviewing anything else (please re-publish / ping this CR when that's done)
Merged part of the changes and resolved comments in 10899021. Also resolved comments in this CL. PTAL! http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:142: kI420 // 12bpp YVU planar 1x1 Y, 2x2 UV samples. I am not totally clear about what formats we will use. I know VP8 decoder supports Yv12 and I420 output so keep them here. I guess later on when people adds decoder implementations they can always update this if needed. http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:168: VideoSize data_size; scherkus@: my understanding is that this is the same as coded_size on line 205 (both are imported from media). Is that correct? If so, probably should name it coded_size to avoid unnecessary confusion. http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:296: virtual void ResetVideoDecoder() = 0; In 10899021 it's changed to Flush*. AudioDecoder/VideoDecoder uses Reset. VideoRenderer and ffmpeg use Flush. I'll keep Reset here just to be consistent with AudioDecoder/VideoDecoder, but would be totally ok if we want to use Flush. http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:299: // that a VideoDecoder cannot be re-initialized after it has been stopped. This will not be true in the future if we need to Stop() then Initialize() to support kConfigChanged.
http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:36: kNoKey, // The decryption key is not available for decryption. The required decryption key is not available. http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:124: struct OutputBuffer { // Represents an output buffer. // Does not own the data it represents. http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:165: VideoFormat format; As discussed in 10899021, why do we need format and data_size in both the frame and the config? How does src/media use these two copies? http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:254: // If the return value is not kSuccess, |decrypted_buffer| should be discarded What does "discarded" mean exactly? Ignored or destroy the data buffer? http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:263: // Initializes the CDM video decoder with |video_decoder_config|. This In 10899021, I suggested moving this up so that Decrypt*() are together. Thoughts? http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:270: // CDM can not be used (or is not allowed) to Decrypt() the video stream. Can "allowed" really be determined during initialization? Do you expect to only call this AFTER AddKey()? (That would seem to prevent pre-initializing decoders.) http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:271: virtual Status InitializeVideoDecoder( Nothing to do now, but something to consider when we add audio: Video/Audio labels work now because we only support 1 of each. However, at some point in the future, we will support multiple. Thus, we may need stream IDs. I guess we could still have separate methods for video and audio since it's not possible to use a single value/enum for stream IDs. http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:276: // repeatedly with empty |encrypted_buffer| (|data| being NULL) until Suggestion: The following might be easier to read. s/being/=/ http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:286: // If the return value is not kSuccess, |video_frame| should be discarded and Same wrt "discarded". http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:295: // Resets the CDM video decoder to an initialized clean state. ... Any in-process frames are flushed. ^ or something to that effect. http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:298: // Stops the CDM video decoder and set it to an uninitialized state. Note s/set/sets/ http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:299: // that a VideoDecoder cannot be re-initialized after it has been stopped. On 2012/09/04 15:08:18, xhwang wrote: > This will not be true in the future if we need to Stop() then Initialize() to > support kConfigChanged. We do. I think this is likely to be a V1 requirement. What additional complexity does this involve? IOW, why add this limitation in the comment? Why would we stop in this case rather than just reset and initialize, though?
http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:36: kNoKey, // The decryption key is not available for decryption. On 2012/09/05 09:37:58, ddorwin wrote: > The required decryption key is not available. Done. http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:124: struct OutputBuffer { On 2012/09/05 09:37:58, ddorwin wrote: > // Represents an output buffer. > // Does not own the data it represents. Done. http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:165: VideoFormat format; On 2012/09/05 09:37:58, ddorwin wrote: > As discussed in 10899021, why do we need format and data_size in both the frame > and the config? How does src/media use these two copies? In src/media, video decoder config is only used in initialization, so each video frame has all information needed for rendering and the renderer doesn't need to store the config. You are right that we don't need to duplicate them here. I actually removed natural size etc from VideoFrame because it's only needed for rendering. It's also safe to remove format and size here (given data_size is equivalent to coded_size) because the caller already have all the info. Removed format and data_size. http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:254: // If the return value is not kSuccess, |decrypted_buffer| should be discarded On 2012/09/05 09:37:58, ddorwin wrote: > What does "discarded" mean exactly? Ignored or destroy the data buffer? Done. http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:263: // Initializes the CDM video decoder with |video_decoder_config|. This On 2012/09/05 09:37:58, ddorwin wrote: > In 10899021, I suggested moving this up so that Decrypt*() are together. > Thoughts? I am grouping *Video* together. Later will also group *Audio* together. I feel it awkward to interleave video and audio calls. What do you think? http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:270: // CDM can not be used (or is not allowed) to Decrypt() the video stream. On 2012/09/05 09:37:58, ddorwin wrote: > Can "allowed" really be determined during initialization? Do you expect to only > call this AFTER AddKey()? (That would seem to prevent pre-initializing > decoders.) Removed kDecryptOnly. If InitializeVideoDecoder() returns kError, the pipeline will always try to use the CDM as a Decryptor. If the CDM's license doesn't allow this, we'll get decryption error, which is okay. http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:271: virtual Status InitializeVideoDecoder( On 2012/09/05 09:37:58, ddorwin wrote: > Nothing to do now, but something to consider when we add audio: > Video/Audio labels work now because we only support 1 of each. However, at some > point in the future, we will support multiple. Thus, we may need stream IDs. I > guess we could still have separate methods for video and audio since it's not > possible to use a single value/enum for stream IDs. Good point. Added a todo. http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:276: // repeatedly with empty |encrypted_buffer| (|data| being NULL) until On 2012/09/05 09:37:58, ddorwin wrote: > Suggestion: The following might be easier to read. > s/being/=/ Done. http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:286: // If the return value is not kSuccess, |video_frame| should be discarded and On 2012/09/05 09:37:58, ddorwin wrote: > Same wrt "discarded". Done. http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:295: // Resets the CDM video decoder to an initialized clean state. On 2012/09/05 09:37:58, ddorwin wrote: > ... Any in-process frames are flushed. > ^ or something to that effect. Done. http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:298: // Stops the CDM video decoder and set it to an uninitialized state. Note On 2012/09/05 09:37:58, ddorwin wrote: > s/set/sets/ Done. http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:299: // that a VideoDecoder cannot be re-initialized after it has been stopped. On 2012/09/05 09:37:58, ddorwin wrote: > On 2012/09/04 15:08:18, xhwang wrote: > > This will not be true in the future if we need to Stop() then Initialize() to > > support kConfigChanged. > > We do. I think this is likely to be a V1 requirement. What additional complexity > does this involve? IOW, why add this limitation in the comment? Why would we > stop in this case rather than just reset and initialize, though? I chatted w/ acolwell@ offline. We are not doing stop/reinitialize right now in FFmpegVideoDecoder when kConfigChanged. But this may change since for GpuVideoDecoder it's more complicated. In all cases, the bottom line is that we can always stop then initialize. The reason that we have this comment is because we have this limit in src/media. But you are right that we dont' need to have this restriction here. Comments removed. We can't do reset and initialize since resetting only flushes internal buffers and doesn't release internal resources. Stop() will release internal resources so we should do Stop() then Initialize().
LGTM % 1 comment http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:292: // Resets the CDM video decoder to an initialized clean state. All internally s/internally/internal/
http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:10: typedef unsigned int uint32_t; Will add typedef for int32_t here.
mostly nits and Q about Stop vs destructor http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:147: struct VideoSize { this looks like a general purpose size struct to me considering it's namespaced lets go w/ Size http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:211: // Generates a |key_request| given the |init_data|. add blank line below to separate function summary from return statements http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:226: // Adds the |key| to the CDM to be associated with |key_id|. add blank line below to separate function summary from return statements http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:228: // Returns kError otherwise. this isn't a sentence -- I'd rewrite this as: Returns kSuccess if the key was successfully added, kError otherwise. http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:236: // Cancels any pending key request made to the CDM for |session_id|. add blank line below to separate function summary from return statements http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:239: // Returns kError otherwise. ditto http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:243: // Decrypts the |encrypted_buffer|. add blank line below to separate function summary from return statements http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:249: // Returns kError if any other error happened. Returns kSuccess if the key was successfully added, kError otherwise. http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:260: // function must be called before DecryptAndDecodeVideo() is called. add blank line below to separate function summary from return statements http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:274: // |video_frame| (|format| == kEmptyVideoFrame) is produced. add blank line below to separate function summary from return statements http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:275: // Returns kSuccess if decryption and decoding both succeeded, in which case ditto for vertical whitespacing http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:297: // caller can call InitializeVideoDecoder() again after this call to do you have a use case for this? if not I'd recommend simply deleting the object to signal stop
http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:10: typedef unsigned int uint32_t; On 2012/09/05 15:36:50, xhwang wrote: > Will add typedef for int32_t here. Done. http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:147: struct VideoSize { On 2012/09/05 16:43:43, scherkus wrote: > this looks like a general purpose size struct to me > > considering it's namespaced lets go w/ Size Done. http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:211: // Generates a |key_request| given the |init_data|. On 2012/09/05 16:43:43, scherkus wrote: > add blank line below to separate function summary from return statements Done. http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:226: // Adds the |key| to the CDM to be associated with |key_id|. On 2012/09/05 16:43:43, scherkus wrote: > add blank line below to separate function summary from return statements Done. http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:228: // Returns kError otherwise. On 2012/09/05 16:43:43, scherkus wrote: > this isn't a sentence -- I'd rewrite this as: > > Returns kSuccess if the key was successfully added, kError otherwise. Done. http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:236: // Cancels any pending key request made to the CDM for |session_id|. On 2012/09/05 16:43:43, scherkus wrote: > add blank line below to separate function summary from return statements Done. http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:239: // Returns kError otherwise. On 2012/09/05 16:43:43, scherkus wrote: > ditto Done. http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:243: // Decrypts the |encrypted_buffer|. On 2012/09/05 16:43:43, scherkus wrote: > add blank line below to separate function summary from return statements Done. http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:249: // Returns kError if any other error happened. On 2012/09/05 16:43:43, scherkus wrote: > Returns kSuccess if the key was successfully added, kError otherwise. This doesn't apply directly. I'll keep it as is. http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:260: // function must be called before DecryptAndDecodeVideo() is called. On 2012/09/05 16:43:43, scherkus wrote: > add blank line below to separate function summary from return statements Done. http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:274: // |video_frame| (|format| == kEmptyVideoFrame) is produced. On 2012/09/05 16:43:43, scherkus wrote: > add blank line below to separate function summary from return statements Done. http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:275: // Returns kSuccess if decryption and decoding both succeeded, in which case On 2012/09/05 16:43:43, scherkus wrote: > ditto for vertical whitespacing ? http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:292: // Resets the CDM video decoder to an initialized clean state. All internally On 2012/09/05 14:52:29, ddorwin wrote: > s/internally/internal/ Done. http://codereview.chromium.org/10900007/diff/10004/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:297: // caller can call InitializeVideoDecoder() again after this call to On 2012/09/05 16:43:43, scherkus wrote: > do you have a use case for this? > > if not I'd recommend simply deleting the object to signal stop This will be used for config change, in which case we want to do Stop then Initialize with new config.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/10900007/8016
Change committed as 155174 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
