Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11091005:
Update PluginInstance for decrypt-and-decode video. (Closed)
|
Created:
8 years, 2 months ago by xhwang Modified:
8 years, 2 months ago CC:
chromium-reviews, darin-cc_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionUpdate PluginInstance for decrypt-and-decode video.
- Hook up PpapiDecryptor and PluginInstance in terms of decrypt-and-decode calls.
- Enable empty input buffer and empty video frame to be passed to/from the plugin. This is used for end-of-stream buffer/frames.
- Add logs in DecryptingVideoDecoder.
BUG=141780, 141784
TEST=Decrypt-and-decode of video is working with a fake video decoder in clearkey CDM.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162657
Patch Set 1 #Patch Set 2 : #
Total comments: 21
Patch Set 3 : Add todo about decrypt_cb_map. #
Total comments: 1
Patch Set 4 : resolve comments #Patch Set 5 : a lot of change, need to be reviewed again #
Total comments: 27
Patch Set 6 : resolve ddorwin's comments #
Total comments: 10
Patch Set 7 : resolve comments #
Total comments: 2
Patch Set 8 : fix check on empty resource #
Total comments: 8
Patch Set 9 : resolve comments #Patch Set 10 : fix a bug in ppp_content_decryptor_private_proxy.cc #
Total comments: 1
Messages
Total messages: 29 (0 generated)
I haven't added video decoder initialization because it's added in tomf's CL. PTAL! http://codereview.chromium.org/11091005/diff/2001/webkit/plugins/ppapi/ppapi_... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/11091005/diff/2001/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2375: DCHECK(frame_info->format == PP_DECRYPTEDFRAMEFORMAT_YV12); +tomf: can we control what the output format is? Or do we need to support multiple formats here?
I will review DeliverFrame() after discussing comments. http://codereview.chromium.org/11091005/diff/2001/webkit/plugins/ppapi/ppapi_... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/11091005/diff/2001/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.cc:460: pending_video_decode_request_id_(0) { Why not avoid 0 like we do above? http://codereview.chromium.org/11091005/diff/2001/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1533: if (next_decryption_request_id_ == 0) What is the purpose of this? Fixing wrapping? http://codereview.chromium.org/11091005/diff/2001/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1591: pending_video_decode_request_id_ = request_id; What purpose do these IDs serve? For decrypt, it was to coordinate the audio and video requests, but I'm not sure that the decode requests need them. (We may even want to get rid of them for decrypt since it's just A or V. Another option (and related to my comment in the header) is to have a flag for audio and a flag for video and only allow decrypt or decode once for each. This requires adding DecryptAudio/Video. http://codereview.chromium.org/11091005/diff/2001/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2377: scoped_refptr<media::VideoFrame> decoded_frame( I hate to add all this media code to ppapi code. Can we just pass some raw ptrs in the callback and require the callback to copy them by stating the lifetime may end after the callback returns? Longer term, we need to solve tomfinegan's TODO above? How hard would it be to create an interface whose impl holds the PPB_Buffer_Dev and frees it when the interface is deleted/released? brettw@: Any thoughts on retaining the shared memory behind the PPB_Buffer_Dev? http://codereview.chromium.org/11091005/diff/2001/webkit/plugins/ppapi/ppapi_... File webkit/plugins/ppapi/ppapi_plugin_instance.h (right): http://codereview.chromium.org/11091005/diff/2001/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.h:767: media::Decryptor::VideoDecodeCB pending_video_decode_cb_; We have a list for decrypt-only and one for decode? This is fine for now (getting decode working), but we should revisit this after adding audio.
http://codereview.chromium.org/11091005/diff/2001/webkit/plugins/ppapi/ppapi_... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/11091005/diff/2001/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.cc:460: pending_video_decode_request_id_(0) { On 2012/10/08 19:05:57, ddorwin wrote: > Why not avoid 0 like we do above? I'd like it to be initialized to an invalid request ID. http://codereview.chromium.org/11091005/diff/2001/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1533: if (next_decryption_request_id_ == 0) On 2012/10/08 19:05:57, ddorwin wrote: > What is the purpose of this? Fixing wrapping? So that the request_id can never be invalid. Please see DeliverFrame how the pending request id is cleared when there's no pending callback. http://codereview.chromium.org/11091005/diff/2001/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1591: pending_video_decode_request_id_ = request_id; On 2012/10/08 19:05:57, ddorwin wrote: > What purpose do these IDs serve? For decrypt, it was to coordinate the audio and > video requests, but I'm not sure that the decode requests need them. (We may > even want to get rid of them for decrypt since it's just A or V. > Another option (and related to my comment in the header) is to have a flag for > audio and a flag for video and only allow decrypt or decode once for each. This > requires adding DecryptAudio/Video. As discussed offline and replied here (http://codereview.chromium.org/11013052/diff/2001/ppapi/api/private/pp_conten...), the request id will be used for tracking callbacks so that we can cancel operations without waiting. http://codereview.chromium.org/11091005/diff/2001/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2377: scoped_refptr<media::VideoFrame> decoded_frame( On 2012/10/08 19:05:57, ddorwin wrote: > I hate to add all this media code to ppapi code. Can we just pass some raw ptrs > in the callback and require the callback to copy them by stating the lifetime > may end after the callback returns? Agreed. For now the callback will get posted onto the decoder's thread and executed asynchronously. I can modify the PpapiDecryptor to do this. Actually if it's just adding too much code into this class/file, I think we have too many methods added to the PluginInstance already. dmichael@ mentioned that we need to have some delegate to do this for PluginInstance. Any concrete idea how to do this? Is there any example to follow? > Longer term, we need to solve tomfinegan's TODO above? How hard would it be to > create an interface whose impl holds the PPB_Buffer_Dev and frees it when the > interface is deleted/released? > > brettw@: Any thoughts on retaining the shared memory behind the PPB_Buffer_Dev? http://codereview.chromium.org/11091005/diff/2001/webkit/plugins/ppapi/ppapi_... File webkit/plugins/ppapi/ppapi_plugin_instance.h (right): http://codereview.chromium.org/11091005/diff/2001/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.h:767: media::Decryptor::VideoDecodeCB pending_video_decode_cb_; On 2012/10/08 19:05:57, ddorwin wrote: > We have a list for decrypt-only and one for decode? This is fine for now > (getting decode working), but we should revisit this after adding audio. TODO added.
I thought I had already sent this. Sorry. http://codereview.chromium.org/11091005/diff/2001/webkit/plugins/ppapi/ppapi_... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/11091005/diff/2001/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.cc:460: pending_video_decode_request_id_(0) { On 2012/10/08 20:50:51, xhwang wrote: > On 2012/10/08 19:05:57, ddorwin wrote: > > Why not avoid 0 like we do above? > > I'd like it to be initialized to an invalid request ID. We should try to initialize these in a consistent way. http://codereview.chromium.org/11091005/diff/2001/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1533: if (next_decryption_request_id_ == 0) On 2012/10/08 20:50:51, xhwang wrote: > On 2012/10/08 19:05:57, ddorwin wrote: > > What is the purpose of this? Fixing wrapping? > > So that the request_id can never be invalid. Please see DeliverFrame how the > pending request id is cleared when there's no pending callback. DeliverFrame only deals with pending_video_decode_request_id_, not this value. http://codereview.chromium.org/11091005/diff/2001/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1591: pending_video_decode_request_id_ = request_id; On 2012/10/08 20:50:51, xhwang wrote: > On 2012/10/08 19:05:57, ddorwin wrote: > > What purpose do these IDs serve? For decrypt, it was to coordinate the audio > and > > video requests, but I'm not sure that the decode requests need them. (We may > > even want to get rid of them for decrypt since it's just A or V. > > Another option (and related to my comment in the header) is to have a flag for > > audio and a flag for video and only allow decrypt or decode once for each. > This > > requires adding DecryptAudio/Video. > > As discussed offline and replied here > (http://codereview.chromium.org/11013052/diff/2001/ppapi/api/private/pp_conten...), > the request id will be used for tracking callbacks so that we can cancel > operations without waiting. As discussed offline (and in fischman's comment), we should eventually eliminate this. Add TODO to header or somewhere. http://codereview.chromium.org/11091005/diff/2001/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2377: scoped_refptr<media::VideoFrame> decoded_frame( On 2012/10/08 20:50:51, xhwang wrote: > On 2012/10/08 19:05:57, ddorwin wrote: > > I hate to add all this media code to ppapi code. Can we just pass some raw > ptrs > > in the callback and require the callback to copy them by stating the lifetime > > may end after the callback returns? > > Agreed. For now the callback will get posted onto the decoder's thread and > executed asynchronously. I can modify the PpapiDecryptor to do this. > > Actually if it's just adding too much code into this class/file, I think we have > too many methods added to the PluginInstance already. dmichael@ mentioned that > we need to have some delegate to do this for PluginInstance. Any concrete idea > how to do this? Is there any example to follow? > > > Longer term, we need to solve tomfinegan's TODO above? How hard would it be to > > create an interface whose impl holds the PPB_Buffer_Dev and frees it when the > > interface is deleted/released? > > > > brettw@: Any thoughts on retaining the shared memory behind the > PPB_Buffer_Dev? > I think that's a separate issue, but we should avoid adding these media calls here. We at least need a TODO to eliminate the video_util.h include. This is worse than just lots of code IMO.
Have not reviewed. I just wanted to answer xhwang's question. http://codereview.chromium.org/11091005/diff/2001/webkit/plugins/ppapi/ppapi_... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/11091005/diff/2001/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2375: DCHECK(frame_info->format == PP_DECRYPTEDFRAMEFORMAT_YV12); On 2012/10/08 17:50:36, xhwang wrote: > +tomf: can we control what the output format is? Or do we need to support > multiple formats here? Since we support only VP8, and it outputs only I420 and YV12, I think it would be best to handle swapping the plane offsets here if YV12 is required.
http://codereview.chromium.org/11091005/diff/2001/webkit/plugins/ppapi/ppapi_... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/11091005/diff/2001/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.cc:460: pending_video_decode_request_id_(0) { On 2012/10/09 02:10:48, ddorwin wrote: > On 2012/10/08 20:50:51, xhwang wrote: > > On 2012/10/08 19:05:57, ddorwin wrote: > > > Why not avoid 0 like we do above? > > > > I'd like it to be initialized to an invalid request ID. > > We should try to initialize these in a consistent way. As discussed offline, we need the pending_video_decode_request_id to be invalid because there's no pending request yet. http://codereview.chromium.org/11091005/diff/2001/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1533: if (next_decryption_request_id_ == 0) On 2012/10/09 02:10:48, ddorwin wrote: > On 2012/10/08 20:50:51, xhwang wrote: > > On 2012/10/08 19:05:57, ddorwin wrote: > > > What is the purpose of this? Fixing wrapping? > > > > So that the request_id can never be invalid. Please see DeliverFrame how the > > pending request id is cleared when there's no pending callback. > > DeliverFrame only deals with pending_video_decode_request_id_, not this value. Removed this as it's not possible to hit. http://codereview.chromium.org/11091005/diff/2001/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1591: pending_video_decode_request_id_ = request_id; On 2012/10/09 02:10:48, ddorwin wrote: > On 2012/10/08 20:50:51, xhwang wrote: > > On 2012/10/08 19:05:57, ddorwin wrote: > > > What purpose do these IDs serve? For decrypt, it was to coordinate the audio > > and > > > video requests, but I'm not sure that the decode requests need them. (We may > > > even want to get rid of them for decrypt since it's just A or V. > > > Another option (and related to my comment in the header) is to have a flag > for > > > audio and a flag for video and only allow decrypt or decode once for each. > > This > > > requires adding DecryptAudio/Video. > > > > As discussed offline and replied here > > > (http://codereview.chromium.org/11013052/diff/2001/ppapi/api/private/pp_conten...), > > the request id will be used for tracking callbacks so that we can cancel > > operations without waiting. > > As discussed offline (and in fischman's comment), we should eventually eliminate > this. Add TODO to header or somewhere. Comments added in header file. http://codereview.chromium.org/11091005/diff/2001/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2375: DCHECK(frame_info->format == PP_DECRYPTEDFRAMEFORMAT_YV12); On 2012/10/09 06:25:34, Tom Finegan wrote: > On 2012/10/08 17:50:36, xhwang wrote: > > +tomf: can we control what the output format is? Or do we need to support > > multiple formats here? > > Since we support only VP8, and it outputs only I420 and YV12, I think it would > be best to handle swapping the plane offsets here if YV12 is required. I am asking because VideoFrame::CreateFrame only supports RGB32/YV12 right now. Since VP8 supports YV12 we are good here (by enforcing using YV12 by the vp8 decoder)? http://codereview.chromium.org/11091005/diff/2001/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2377: scoped_refptr<media::VideoFrame> decoded_frame( On 2012/10/09 02:10:48, ddorwin wrote: > On 2012/10/08 20:50:51, xhwang wrote: > > On 2012/10/08 19:05:57, ddorwin wrote: > > > I hate to add all this media code to ppapi code. Can we just pass some raw > > ptrs > > > in the callback and require the callback to copy them by stating the > lifetime > > > may end after the callback returns? > > > > Agreed. For now the callback will get posted onto the decoder's thread and > > executed asynchronously. I can modify the PpapiDecryptor to do this. > > > > Actually if it's just adding too much code into this class/file, I think we > have > > too many methods added to the PluginInstance already. dmichael@ mentioned that > > we need to have some delegate to do this for PluginInstance. Any concrete idea > > how to do this? Is there any example to follow? > > > > > Longer term, we need to solve tomfinegan's TODO above? How hard would it be > to > > > create an interface whose impl holds the PPB_Buffer_Dev and frees it when > the > > > interface is deleted/released? > > > > > > brettw@: Any thoughts on retaining the shared memory behind the > > PPB_Buffer_Dev? > > > > I think that's a separate issue, but we should avoid adding these media calls > here. We at least need a TODO to eliminate the video_util.h include. This is > worse than just lots of code IMO. TODO added before #include "media/base/video_util.h" http://codereview.chromium.org/11091005/diff/7001/webkit/plugins/ppapi/ppapi_... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/11091005/diff/7001/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1534: next_decryption_request_id_++; Okay, it took a user to watch a 60fps movie for 2 years non-stop to hit this :P Removed.
lgtm
David/Tom, This patch + my local fake clearkey video decoder = decrypt-and-decode video working Sorry for the rebase noise, but I have to :) I probably need to doc about how we pass empty buffers through. But I want to get your opinions about the current implementation first. PTAL! brettw: to save your time, please wait until I got lgtm from Tom and David to start review.
Thanks, I'm removing myself to avoid inbox spam for now. When you're ready please just add me.
Nits and questions. http://codereview.chromium.org/11091005/diff/18010/media/filters/decrypting_v... File media/filters/decrypting_video_decoder.cc (right): http://codereview.chromium.org/11091005/diff/18010/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:165: DVLOG(2) << "DoInitialize()"; Suggest eliminating the extra line below. One way to combine all these debug lines and get log statements before a DCHECK is to put this as the first line in the function. http://codereview.chromium.org/11091005/diff/18010/ppapi/cpp/private/content_... File ppapi/cpp/private/content_decryptor_private.h (right): http://codereview.chromium.org/11091005/diff/18010/ppapi/cpp/private/content_... ppapi/cpp/private/content_decryptor_private.h:46: // NULL |encrypted_frame| means end-of-stream buffer. s/NULL/Null/ It's more of a concept of a null resource than NULL the value. http://codereview.chromium.org/11091005/diff/18010/webkit/media/crypto/ppapi_... File webkit/media/crypto/ppapi_decryptor.cc (right): http://codereview.chromium.org/11091005/diff/18010/webkit/media/crypto/ppapi_... webkit/media/crypto/ppapi_decryptor.cc:41: DVLOG(2) << "GenerateKeyRequest()"; Do we have level 1 logs in Proxydecryptor or something? http://codereview.chromium.org/11091005/diff/18010/webkit/media/crypto/ppapi_... webkit/media/crypto/ppapi_decryptor.cc:158: void PpapiDecryptor::CancelDecryptAndDecodeVideo() { Should this be CancelVideoDecode() or eventually Cancel(type)? http://codereview.chromium.org/11091005/diff/18010/webkit/media/crypto/ppapi_... webkit/media/crypto/ppapi_decryptor.cc:168: cdm_plugin_->ResetDecoder(); When we have audio, this will reset both. Is that what we want? http://codereview.chromium.org/11091005/diff/18010/webkit/media/crypto/ppapi_... webkit/media/crypto/ppapi_decryptor.cc:171: void PpapiDecryptor::StopVideoDecoder() { same here for both of the above. http://codereview.chromium.org/11091005/diff/18010/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/11091005/diff/18010/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1568: uint32_t request_id = next_decryption_request_id_++; const like below. http://codereview.chromium.org/11091005/diff/18010/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2449: uint32_t request_id = frame_info->tracking_info.request_id; nit: Since this is just an alias, I think it should be const. I wonder if we should just avoid it altogether since it's only used in 2452. http://codereview.chromium.org/11091005/diff/18010/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2452: if (request_id == 0 || request_id != pending_video_decode_request_id_) DCHECK? http://codereview.chromium.org/11091005/diff/18010/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2489: // TODO(tomfinegan): Find a way to take ownership of the shared memory Should this be before 2494 instead? http://codereview.chromium.org/11091005/diff/18010/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2491: DCHECK(frame_info->format == PP_DECRYPTEDFRAMEFORMAT_YV12); This should be closer to the use of the YV12 enum. Maybe const Fromat = YV12 below the DCHECK. This makes it clear that we are choosing to only support YV12 rather than hiding it amongst all the params in the call below. I guess the whole Copy*Plane is related too, but hopefully that will be eliminated here. http://codereview.chromium.org/11091005/diff/18010/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.h (right): http://codereview.chromium.org/11091005/diff/18010/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.h:273: bool DecryptAndDecode( "// Note: This method can be used with an unencrypted frame." was removed. http://codereview.chromium.org/11091005/diff/18010/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.h:275: const media::Decryptor::VideoDecodeCB& video_decode_cb); What's the plan for audio? Will we have two CBs or rename it?
ddorwin's comments resolved. PTAL! Sorry for the rebase noise. http://codereview.chromium.org/11091005/diff/18010/media/filters/decrypting_v... File media/filters/decrypting_video_decoder.cc (right): http://codereview.chromium.org/11091005/diff/18010/media/filters/decrypting_v... media/filters/decrypting_video_decoder.cc:165: DVLOG(2) << "DoInitialize()"; On 2012/10/13 01:40:28, ddorwin wrote: > Suggest eliminating the extra line below. > One way to combine all these debug lines and get log statements before a DCHECK > is to put this as the first line in the function. Done. http://codereview.chromium.org/11091005/diff/18010/ppapi/cpp/private/content_... File ppapi/cpp/private/content_decryptor_private.h (right): http://codereview.chromium.org/11091005/diff/18010/ppapi/cpp/private/content_... ppapi/cpp/private/content_decryptor_private.h:46: // NULL |encrypted_frame| means end-of-stream buffer. On 2012/10/13 01:40:28, ddorwin wrote: > s/NULL/Null/ > It's more of a concept of a null resource than NULL the value. Done. http://codereview.chromium.org/11091005/diff/18010/webkit/media/crypto/ppapi_... File webkit/media/crypto/ppapi_decryptor.cc (right): http://codereview.chromium.org/11091005/diff/18010/webkit/media/crypto/ppapi_... webkit/media/crypto/ppapi_decryptor.cc:41: DVLOG(2) << "GenerateKeyRequest()"; On 2012/10/13 01:40:28, ddorwin wrote: > Do we have level 1 logs in Proxydecryptor or something? We have a level 1 log in ReportFailureToCallPlugin. I use level 1 log for abnormal situations. But I agree that we don't have consistent rule about the use of log levels :) http://codereview.chromium.org/11091005/diff/18010/webkit/media/crypto/ppapi_... webkit/media/crypto/ppapi_decryptor.cc:158: void PpapiDecryptor::CancelDecryptAndDecodeVideo() { On 2012/10/13 01:40:28, ddorwin wrote: > Should this be CancelVideoDecode() or eventually Cancel(type)? Yes, I plan to rename it to ResetDecoder(type) when I add audio support. http://codereview.chromium.org/11091005/diff/18010/webkit/media/crypto/ppapi_... webkit/media/crypto/ppapi_decryptor.cc:168: cdm_plugin_->ResetDecoder(); On 2012/10/13 01:40:28, ddorwin wrote: > When we have audio, this will reset both. Is that what we want? ResetDecoder will take a StreamType param later. http://codereview.chromium.org/11091005/diff/18010/webkit/media/crypto/ppapi_... webkit/media/crypto/ppapi_decryptor.cc:171: void PpapiDecryptor::StopVideoDecoder() { On 2012/10/13 01:40:28, ddorwin wrote: > same here for both of the above. ditto http://codereview.chromium.org/11091005/diff/18010/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/11091005/diff/18010/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1568: uint32_t request_id = next_decryption_request_id_++; On 2012/10/13 01:40:28, ddorwin wrote: > const like below. Done. http://codereview.chromium.org/11091005/diff/18010/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2449: uint32_t request_id = frame_info->tracking_info.request_id; On 2012/10/13 01:40:28, ddorwin wrote: > nit: Since this is just an alias, I think it should be const. I wonder if we > should just avoid it altogether since it's only used in 2452. Done. http://codereview.chromium.org/11091005/diff/18010/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2452: if (request_id == 0 || request_id != pending_video_decode_request_id_) On 2012/10/13 01:40:28, ddorwin wrote: > DCHECK? My whole point is that I treat CDM as a third party module that I don't fully trust. I don't want the renderer to crash if the CDM returns some garbage. WDYT? http://codereview.chromium.org/11091005/diff/18010/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2489: // TODO(tomfinegan): Find a way to take ownership of the shared memory On 2012/10/13 01:40:28, ddorwin wrote: > Should this be before 2494 instead? Done. http://codereview.chromium.org/11091005/diff/18010/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2491: DCHECK(frame_info->format == PP_DECRYPTEDFRAMEFORMAT_YV12); On 2012/10/13 01:40:28, ddorwin wrote: > This should be closer to the use of the YV12 enum. Maybe const Fromat = YV12 > below the DCHECK. > This makes it clear that we are choosing to only support YV12 rather than hiding > it amongst all the params in the call below. I guess the whole Copy*Plane is > related too, but hopefully that will be eliminated here. Done. http://codereview.chromium.org/11091005/diff/18010/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.h (right): http://codereview.chromium.org/11091005/diff/18010/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.h:273: bool DecryptAndDecode( On 2012/10/13 01:40:28, ddorwin wrote: > "// Note: This method can be used with an unencrypted frame." was removed. Done. http://codereview.chromium.org/11091005/diff/18010/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.h:275: const media::Decryptor::VideoDecodeCB& video_decode_cb); On 2012/10/13 01:40:28, ddorwin wrote: > What's the plan for audio? Will we have two CBs or rename it? I guess I'll just rename so we have DecryptAndDecodeVideo/DecryptAndDecodeAudio or DecryptAndDecodeFrame/DecryptAndDecodeSamples. Either one is okay for me but I prefer the former. Frame/Samples is what's used in CDM now. But it may not be obvious for people not familiar w/ this code that frame is for video and samples is for audio.
Two nits, one issue. http://codereview.chromium.org/11091005/diff/24001/webkit/media/crypto/ppapi_... File webkit/media/crypto/ppapi_decryptor.cc (right): http://codereview.chromium.org/11091005/diff/24001/webkit/media/crypto/ppapi_... webkit/media/crypto/ppapi_decryptor.cc:139: return; nit: you don't need this return anymore. http://codereview.chromium.org/11091005/diff/24001/webkit/media/crypto/ppapi_... File webkit/media/crypto/ppapi_decryptor.h (right): http://codereview.chromium.org/11091005/diff/24001/webkit/media/crypto/ppapi_... webkit/media/crypto/ppapi_decryptor.h:68: nit: don't really need this blank line http://codereview.chromium.org/11091005/diff/24001/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/11091005/diff/24001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1659: encrypted_buffer->GetDataSize()); I think this leaks one ref on the resource. Decrypt() uses the ScopedPPResource PassRef constructor to avoid the problem: MakeBufferResource returns resource with one ref Scoper adds one, and then removes one upon destruction We want the count to be zero when this method returns, and not 1. I think it's safe to use MakeBufferResource with a NULL pointer and a size of 0. It'll just return 0. Using the PassRef ScopedPPResource with a resource = 0 won't hurt anything. You can probably do away with the if (!IsEndOfStream) check.
I have some comments in both patch versions. It sounds like there is a ref counting issue for you and Tom to work out. LG otherwise. http://codereview.chromium.org/11091005/diff/18010/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/11091005/diff/18010/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2452: if (request_id == 0 || request_id != pending_video_decode_request_id_) On 2012/10/15 19:53:01, xhwang wrote: > On 2012/10/13 01:40:28, ddorwin wrote: > > DCHECK? > > My whole point is that I treat CDM as a third party module that I don't fully > trust. I don't want the renderer to crash if the CDM returns some garbage. WDYT? Well, we want to catch bugs in these third-party modules, so a _D_CHECK seems fine. Still handle the condition, just NOTREACHED() << "Invalid request ID from the CDM"; or something like that. Up to you, though. http://codereview.chromium.org/11091005/diff/24001/webkit/media/crypto/ppapi_... File webkit/media/crypto/ppapi_decryptor.cc (right): http://codereview.chromium.org/11091005/diff/24001/webkit/media/crypto/ppapi_... webkit/media/crypto/ppapi_decryptor.cc:139: return; On 2012/10/15 21:26:30, Tom Finegan wrote: > nit: you don't need this return anymore. Might be safer to leave it, though. Someone adding code below later might not add it back. Either way, though. http://codereview.chromium.org/11091005/diff/24001/webkit/media/crypto/ppapi_... File webkit/media/crypto/ppapi_decryptor.h (right): http://codereview.chromium.org/11091005/diff/24001/webkit/media/crypto/ppapi_... webkit/media/crypto/ppapi_decryptor.h:68: On 2012/10/15 21:26:30, Tom Finegan wrote: > nit: don't really need this blank line These are dissimilar functions I think, so spacing seems fine. http://codereview.chromium.org/11091005/diff/24001/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/11091005/diff/24001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2499: // TODO(tomfinegan): Find a way to take ownership of the shared memory FWIW, this is what I was proposing in the previous revision: DCHECK(frame_info->format == PP_DECRYPTEDFRAMEFORMAT_YV12); const media::VideoFrame::Format format = media::VideoFrame::YV12; // TODO(tomfinegan): Find a way to take ownership of the shared memory // managed by the PPB_Buffer_Dev, and avoid the extra copy. scoped_refptr<media::VideoFrame> decoded_frame( media::VideoFrame::CreateFrame( format, frame_size, frame_size, ...
http://codereview.chromium.org/11091005/diff/24001/webkit/media/crypto/ppapi_... File webkit/media/crypto/ppapi_decryptor.cc (right): http://codereview.chromium.org/11091005/diff/24001/webkit/media/crypto/ppapi_... webkit/media/crypto/ppapi_decryptor.cc:139: return; On 2012/10/15 23:31:38, ddorwin wrote: > On 2012/10/15 21:26:30, Tom Finegan wrote: > > nit: you don't need this return anymore. > > Might be safer to leave it, though. Someone adding code below later might not > add it back. Either way, though. Will keep the return then :) http://codereview.chromium.org/11091005/diff/24001/webkit/media/crypto/ppapi_... File webkit/media/crypto/ppapi_decryptor.h (right): http://codereview.chromium.org/11091005/diff/24001/webkit/media/crypto/ppapi_... webkit/media/crypto/ppapi_decryptor.h:68: On 2012/10/15 23:31:38, ddorwin wrote: > On 2012/10/15 21:26:30, Tom Finegan wrote: > > nit: don't really need this blank line > > These are dissimilar functions I think, so spacing seems fine. Will keep the white line then. http://codereview.chromium.org/11091005/diff/24001/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/11091005/diff/24001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1659: encrypted_buffer->GetDataSize()); On 2012/10/15 21:26:30, Tom Finegan wrote: > I think this leaks one ref on the resource. Decrypt() uses the ScopedPPResource > PassRef constructor to avoid the problem: > > MakeBufferResource returns resource with one ref > Scoper adds one, and then removes one upon destruction > > We want the count to be zero when this method returns, and not 1. > > I think it's safe to use MakeBufferResource with a NULL pointer and a size of 0. > It'll just return 0. Using the PassRef ScopedPPResource with a resource = 0 > won't hurt anything. You can probably do away with the if (!IsEndOfStream) > check. > Thank you so much for the pro-tip! Done. http://codereview.chromium.org/11091005/diff/24001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2499: // TODO(tomfinegan): Find a way to take ownership of the shared memory On 2012/10/15 23:31:38, ddorwin wrote: > FWIW, this is what I was proposing in the previous revision: > > DCHECK(frame_info->format == PP_DECRYPTEDFRAMEFORMAT_YV12); > const media::VideoFrame::Format format = media::VideoFrame::YV12; > > // TODO(tomfinegan): Find a way to take ownership of the shared memory > // managed by the PPB_Buffer_Dev, and avoid the extra copy. > scoped_refptr<media::VideoFrame> decoded_frame( > media::VideoFrame::CreateFrame( > format, frame_size, frame_size, > ... Done.
LGTM, but wait for Tom to review the refcount change. You can request OWNERS review now.
Hello brettw, Looks mostly good now. Could you please do an OWNERS review? Thanks! xhwang
lgtm after one more fix... http://codereview.chromium.org/11091005/diff/18020/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/11091005/diff/18020/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1660: if (!encrypted_resource.get()) This check should only be made when IsEndOfStream() is false. ScopedPPResource::get() returns PP_Resource, and will always be 0 when the resource is empty.
Good catch! Thanks. http://codereview.chromium.org/11091005/diff/18020/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/11091005/diff/18020/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1660: if (!encrypted_resource.get()) On 2012/10/16 00:38:05, Tom Finegan wrote: > This check should only be made when IsEndOfStream() is false. > ScopedPPResource::get() returns PP_Resource, and will always be 0 when the > resource is empty. Done.
Hello viettrungluu, brettw@ is super busy. Could you please do an OWNER's review on those ppapi_plugin_instance.* files? Thanks!
http://codereview.chromium.org/11091005/diff/20011/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/11091005/diff/20011/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1610: DCHECK_EQ(pending_video_decoder_init_request_id_, 0u); InitializeVideoDecoder() is called by Chrome, right? (If this DCHECK fails, it really must be a bug in Chrome, right?) http://codereview.chromium.org/11091005/diff/20011/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2388: return; Should there be a NOTREACHED() here? http://codereview.chromium.org/11091005/diff/20011/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2499: const uint8* frame_data = reinterpret_cast<uint8*>(mapper.data()); static_cast http://codereview.chromium.org/11091005/diff/20011/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2929: } // namespace webkit This file is too big. Please factor your changes (and possibly all the decryptor stuff) out to some helper (or whatever) in another file.
Thanks for the review! PTAL again. http://codereview.chromium.org/11091005/diff/20011/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/11091005/diff/20011/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1610: DCHECK_EQ(pending_video_decoder_init_request_id_, 0u); On 2012/10/17 20:47:59, viettrungluu wrote: > InitializeVideoDecoder() is called by Chrome, right? (If this DCHECK fails, it > really must be a bug in Chrome, right?) Correct. It's called in PpapiDecryptor which is in chrome. http://codereview.chromium.org/11091005/diff/20011/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2388: return; On 2012/10/17 20:47:59, viettrungluu wrote: > Should there be a NOTREACHED() here? This function is called by the plugin/CDM which could in theory be created by third party. We return here to prevent bad plugin/cdm from causing crash in the renderer. http://codereview.chromium.org/11091005/diff/20011/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2499: const uint8* frame_data = reinterpret_cast<uint8*>(mapper.data()); On 2012/10/17 20:47:59, viettrungluu wrote: > static_cast Done. http://codereview.chromium.org/11091005/diff/20011/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2929: } // namespace webkit On 2012/10/17 20:47:59, viettrungluu wrote: > This file is too big. Please factor your changes (and possibly all the decryptor > stuff) out to some helper (or whatever) in another file. Totally agreed. I have a proposal for this and will share with you offline. But I am afraid that we don't have time to do this right now...
Okay, let's put off moving stuff out for the moment (though let's get that done as soon as possible). LGTM on webkit/plugins/ppapi/* and ppapi/*. On 2012/10/17 21:11:54, xhwang wrote: > Thanks for the review! PTAL again. > > http://codereview.chromium.org/11091005/diff/20011/webkit/plugins/ppapi/ppapi... > File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): > > http://codereview.chromium.org/11091005/diff/20011/webkit/plugins/ppapi/ppapi... > webkit/plugins/ppapi/ppapi_plugin_instance.cc:1610: > DCHECK_EQ(pending_video_decoder_init_request_id_, 0u); > On 2012/10/17 20:47:59, viettrungluu wrote: > > InitializeVideoDecoder() is called by Chrome, right? (If this DCHECK fails, it > > really must be a bug in Chrome, right?) > > Correct. It's called in PpapiDecryptor which is in chrome. > > http://codereview.chromium.org/11091005/diff/20011/webkit/plugins/ppapi/ppapi... > webkit/plugins/ppapi/ppapi_plugin_instance.cc:2388: return; > On 2012/10/17 20:47:59, viettrungluu wrote: > > Should there be a NOTREACHED() here? > > This function is called by the plugin/CDM which could in theory be created by > third party. We return here to prevent bad plugin/cdm from causing crash in the > renderer. > > http://codereview.chromium.org/11091005/diff/20011/webkit/plugins/ppapi/ppapi... > webkit/plugins/ppapi/ppapi_plugin_instance.cc:2499: const uint8* frame_data = > reinterpret_cast<uint8*>(mapper.data()); > On 2012/10/17 20:47:59, viettrungluu wrote: > > static_cast > > Done. > > http://codereview.chromium.org/11091005/diff/20011/webkit/plugins/ppapi/ppapi... > webkit/plugins/ppapi/ppapi_plugin_instance.cc:2929: } // namespace webkit > On 2012/10/17 20:47:59, viettrungluu wrote: > > This file is too big. Please factor your changes (and possibly all the > decryptor > > stuff) out to some helper (or whatever) in another file. > > Totally agreed. I have a proposal for this and will share with you offline. But > I am afraid that we don't have time to do this right now...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/11091005/29001
Retried try job too often for step(s) content_browsertests
Fixed a bug in ppp_content_decryptor_private_proxy.cc to allow 0 resource. PTAL! http://codereview.chromium.org/11091005/diff/33003/ppapi/proxy/ppp_content_de... File ppapi/proxy/ppp_content_decryptor_private_proxy.cc (right): http://codereview.chromium.org/11091005/diff/33003/ppapi/proxy/ppp_content_de... ppapi/proxy/ppp_content_decryptor_private_proxy.cc:109: Sorry for the rebase noise. Add this line since the resource could be 0, e.g. for empty extra_data, empty input buffer etc.
lgtm
(still lgtm)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/11091005/33003
Change committed as 162657 |
