Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
905023005:
Pepper: PPB_VideoEncoder implementation (Closed)
|
Created:
5 years, 10 months ago by llandwerlin-old Modified:
5 years, 10 months ago CC:
chromium-reviews, mkwst+moarreviews-renderer_chromium.org, darin-cc_chromium.org, mlamouri+watch-content_chromium.org, piman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPepper: PPB_VideoEncoder implementation
BUG=455409
TEST=run ./ppapi_unittests --gtest_filter=VideoEncoderResourceTest
Committed: https://crrev.com/c3f15126607423812eaefe48ecdcb0ffd39c5e05
Cr-Commit-Position: refs/heads/master@{#317938}
Patch Set 1 #
Total comments: 57
Patch Set 2 : Update after bbudge's review #
Total comments: 94
Patch Set 3 : Update after bbudge's review of the renderer #Patch Set 4 : Update after bbudge's review of the proxy #
Total comments: 36
Patch Set 5 : Deal with Encode() callbacks in the proxy #
Total comments: 89
Patch Set 6 : Split initialization and bitstream buffers messages #Patch Set 7 : Split error notification from error state #
Total comments: 4
Patch Set 8 : Send bitstream buffers in their own ppapi ipc message #
Total comments: 44
Patch Set 9 : Update after bbudge's review #
Total comments: 31
Patch Set 10 : Update after bbudge's review #
Total comments: 7
Patch Set 11 : Final nits #
Total comments: 7
Patch Set 12 : tsepez's nits #Patch Set 13 : Fix build issues #Patch Set 14 : Fix unit tests with dchecks enabled #Patch Set 15 : Fix remaining win compile errors #Patch Set 16 : Fix unittest on Windows #Patch Set 17 : Fix issue with gpu process communication not yet established when requesting supported codecs #Patch Set 18 : Fix size_t->uint32_t conversion on Windows #Patch Set 19 : Fix size_t->uint32_t conversion on Windows #
Total comments: 7
Patch Set 20 : Do not create more than one gpu channel #Patch Set 21 : Fix size_t to uint32_t conversion on Windows #Patch Set 22 : fix unittest on win_chromium_x64_rel_ng #Messages
Total messages: 59 (12 generated)
lionel.g.landwerlin@intel.com changed reviewers: + bbudge@chromium.org, jam@chromium.org, wfh@chromium.org
wfh@ bbudge@ jam@: Please review this to implement the PPB_VideoEncoder API, Thanks.
I forgot to mention it, but this depends on https://codereview.chromium.org/877353002/
Partial review. Mostly of VideoEncoderResource. https://codereview.chromium.org/905023005/diff/1/content/common/gpu/gpu_proce... File content/common/gpu/gpu_process_launch_causes.h (right): https://codereview.chromium.org/905023005/diff/1/content/common/gpu/gpu_proce... content/common/gpu/gpu_process_launch_causes.h:20: CAUSE_FOR_GPU_LAUNCH_PEPPERVIDEOENCODERACCELERATOR_INITIALIZE, Are you sure this should go here, and not after _CANVAS_2D? I'm not familiar with how this syncs with histograms. https://codereview.chromium.org/905023005/diff/1/content/renderer/pepper/pepp... File content/renderer/pepper/pepper_video_encoder_host.cc (right): https://codereview.chromium.org/905023005/diff/1/content/renderer/pepper/pepp... content/renderer/pepper/pepper_video_encoder_host.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: remove the (c) and 2015 https://codereview.chromium.org/905023005/diff/1/content/renderer/pepper/pepp... content/renderer/pepper/pepper_video_encoder_host.cc:37: ~BufferManagerDelegate() override {} You could make the host implement the Delegate interface, and eliminate this class. https://codereview.chromium.org/905023005/diff/1/content/renderer/pepper/pepp... File content/renderer/pepper/pepper_video_encoder_host.h (right): https://codereview.chromium.org/905023005/diff/1/content/renderer/pepper/pepp... content/renderer/pepper/pepper_video_encoder_host.h:112: buffer_manager_delegate_; Why not make the host implement the Delegate interface, especially since we don't override any methods? https://codereview.chromium.org/905023005/diff/1/content/renderer/pepper/pepp... content/renderer/pepper/pepper_video_encoder_host.h:113: scoped_ptr<ppapi::MediaStreamBufferManager> buffer_manager_; Since this is owned by the host and not shared, you could make this a simple member, i.e.: ppapi::MediaStreamBufferManager buffer_manager_; Then initialize it in the ctor's initializer list with 'this'. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... File ppapi/proxy/video_encoder_resource.cc (right): https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.cc:30: }; Same comment as on PepperVideoEncoderHost. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.cc:47: if (shm) You can assume shm is non-null here. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.cc:77: encoder_frame_count_(0), initialize encoder_coded_size_. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.cc:86: encoder_last_error_ = PP_ERROR_ABORTED; I think it's clearer if encoder_last_error_ represents the last error received from the host side (in response to NotifyError from the VEA). This seems contrary to that. Just pass PP_ERROR_ABORTED directly below. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.cc:90: ReleaseFrames(); I think Close() should be called somewhere in here. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.cc:100: // TODO(llandwerlin): should we prevent concurrent calls? The IDL leaves it unspecified but I don't think concurrent calls are useful. We will probably make another pass over the IDL file as the implementation raises issues like this so I would hold off on updating it. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.cc:150: // Lazily ask for video frames buffers. The terminology here is a little confusing. You're really asking for a single block of shared memory. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.cc:266: callback->Run(PP_ERROR_BADARGUMENT); The plugin is allowed to return null, so I think PP_ERROR_FAILED is a better return value here. See: https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/file_i... https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.cc:270: memcpy(ptr, &profiles[0], profiles could be empty in theory, in which case &profiles[0] is undefined. You should check for size. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.cc:287: std::vector<base::SharedMemoryHandle> buffer_handles; shm_handles to match shm_buffers_? https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.cc:290: CHECK(buffer_handles.size() >= buffer_count); This implies that buffer_handles is the source of truth for the buffer count. Why not eliminate the 'buffer_count' parameter and use buffer_handles.size()? https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.cc:293: bitstream_buffers_map_.clear(); I don't see how these two collections could be non-empty at this point. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.cc:302: encoder_coded_size_ = input_coded_size; Looking at this, I think 'input_frame_count_' and 'input_coded_size_' would be better names for these. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... File ppapi/proxy/video_encoder_resource.h (right): https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.h:47: uint32_t id; There should be a comment somewhere that this is the index of the ShmBuffer in the ScopedVector. See: https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/video_... https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.h:56: uint32_t id; I think this id has the same meaning as above, right? https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.h:102: void OnPluginMsgGetVideoFramesReply(const ResourceMessageReplyParams& params, This name is a little confusing to me, since it sounds like it's getting video frames. How about 'GetVideoFrameInfo'? https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.h:122: void WriteBitstreamerBuffer(PP_BitstreamBuffer* bitstream_buffer, s/WriteBitstreamerBuffer/WriteBitstreamBuffer https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.h:135: scoped_ptr<MediaStreamBufferManager> buffer_manager_; Same comments as on PepperVideoEncoderHost to eliminate a separate Delegate, and use of scoped_ptrs here. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.h:137: typedef std::map<PP_Resource, scoped_refptr<VideoFrameResource> > Since we only use C++11 toolchains when we build the proxy, I think we can use >> without parsing problems. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.h:142: get_video_frame_cbs_; get_video_frame_cbs_ sounds like a function or method name. How about 'pending_get_video_frame_calls_' or something similar? https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.h:144: ScopedVector<ShmBuffer> bitstream_buffers_; Could we name this shm_buffers_? https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.h:151: scoped_refptr<TrackedCallback> get_bitstreamer_buffer_callback_; s/get_bitstreamer_buffer_callback_/get_bitstream_buffer_callback_ https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.h:152: PP_BitstreamBuffer* get_bitstreamer_buffer_data_; s/get_bitstreamer_buffer_data_/get_bitstream_buffer_data_
PTAL, Thanks. https://codereview.chromium.org/905023005/diff/1/content/common/gpu/gpu_proce... File content/common/gpu/gpu_process_launch_causes.h (right): https://codereview.chromium.org/905023005/diff/1/content/common/gpu/gpu_proce... content/common/gpu/gpu_process_launch_causes.h:20: CAUSE_FOR_GPU_LAUNCH_PEPPERVIDEOENCODERACCELERATOR_INITIALIZE, On 2015/02/10 01:31:07, bbudge wrote: > Are you sure this should go here, and not after _CANVAS_2D? I'm not familiar > with how this syncs with histograms. Right, fixing this. The comment instruct to update tools/histograms/histograms.xml, but it doesn't seem to contain this enum anymore. https://codereview.chromium.org/905023005/diff/1/content/renderer/pepper/pepp... File content/renderer/pepper/pepper_video_encoder_host.cc (right): https://codereview.chromium.org/905023005/diff/1/content/renderer/pepper/pepp... content/renderer/pepper/pepper_video_encoder_host.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2015/02/10 01:31:07, bbudge wrote: > nit: remove the (c) and 2015 Done. https://codereview.chromium.org/905023005/diff/1/content/renderer/pepper/pepp... content/renderer/pepper/pepper_video_encoder_host.cc:37: ~BufferManagerDelegate() override {} On 2015/02/10 01:31:07, bbudge wrote: > You could make the host implement the Delegate interface, and eliminate this > class. Done. https://codereview.chromium.org/905023005/diff/1/content/renderer/pepper/pepp... File content/renderer/pepper/pepper_video_encoder_host.h (right): https://codereview.chromium.org/905023005/diff/1/content/renderer/pepper/pepp... content/renderer/pepper/pepper_video_encoder_host.h:112: buffer_manager_delegate_; On 2015/02/10 01:31:07, bbudge wrote: > Why not make the host implement the Delegate interface, especially since we > don't override any methods? Done. https://codereview.chromium.org/905023005/diff/1/content/renderer/pepper/pepp... content/renderer/pepper/pepper_video_encoder_host.h:113: scoped_ptr<ppapi::MediaStreamBufferManager> buffer_manager_; On 2015/02/10 01:31:07, bbudge wrote: > Since this is owned by the host and not shared, you could make this a simple > member, i.e.: > ppapi::MediaStreamBufferManager buffer_manager_; > Then initialize it in the ctor's initializer list with 'this'. Done. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... File ppapi/proxy/video_encoder_resource.cc (right): https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.cc:30: }; On 2015/02/10 01:31:08, bbudge wrote: > Same comment as on PepperVideoEncoderHost. Done. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.cc:47: if (shm) On 2015/02/10 01:31:08, bbudge wrote: > You can assume shm is non-null here. Done. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.cc:77: encoder_frame_count_(0), On 2015/02/10 01:31:08, bbudge wrote: > initialize encoder_coded_size_. Done. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.cc:86: encoder_last_error_ = PP_ERROR_ABORTED; On 2015/02/10 01:31:08, bbudge wrote: > I think it's clearer if encoder_last_error_ represents the last error received > from the host side (in response to NotifyError from the VEA). This seems > contrary to that. Just pass PP_ERROR_ABORTED directly below. Done. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.cc:90: ReleaseFrames(); On 2015/02/10 01:31:07, bbudge wrote: > I think Close() should be called somewhere in here. Done. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.cc:100: // TODO(llandwerlin): should we prevent concurrent calls? On 2015/02/10 01:31:08, bbudge wrote: > The IDL leaves it unspecified but I don't think concurrent calls are useful. We > will probably make another pass over the IDL file as the implementation raises > issues like this so I would hold off on updating it. Acknowledged. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.cc:150: // Lazily ask for video frames buffers. On 2015/02/10 01:31:08, bbudge wrote: > The terminology here is a little confusing. You're really asking for a single > block of shared memory. Done. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.cc:266: callback->Run(PP_ERROR_BADARGUMENT); On 2015/02/10 01:31:07, bbudge wrote: > The plugin is allowed to return null, so I think PP_ERROR_FAILED is a better > return value here. > See: > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/file_i... Done. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.cc:270: memcpy(ptr, &profiles[0], On 2015/02/10 01:31:08, bbudge wrote: > profiles could be empty in theory, in which case &profiles[0] is undefined. You > should check for size. Thanks for spotting this. Done. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.cc:287: std::vector<base::SharedMemoryHandle> buffer_handles; On 2015/02/10 01:31:08, bbudge wrote: > shm_handles to match shm_buffers_? Done. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.cc:290: CHECK(buffer_handles.size() >= buffer_count); On 2015/02/10 01:31:08, bbudge wrote: > This implies that buffer_handles is the source of truth for the buffer count. > Why not eliminate the 'buffer_count' parameter and use buffer_handles.size()? Make sense. Done. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.cc:293: bitstream_buffers_map_.clear(); On 2015/02/10 01:31:08, bbudge wrote: > I don't see how these two collections could be non-empty at this point. I was thinking that maybe after the encoder is closed, it could be reinitialized. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.cc:302: encoder_coded_size_ = input_coded_size; On 2015/02/10 01:31:08, bbudge wrote: > Looking at this, I think 'input_frame_count_' and 'input_coded_size_' would be > better names for these. Done. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... File ppapi/proxy/video_encoder_resource.h (right): https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.h:47: uint32_t id; On 2015/02/10 01:31:08, bbudge wrote: > There should be a comment somewhere that this is the index of the ShmBuffer in > the ScopedVector. > See: > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/video_... Done. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.h:56: uint32_t id; On 2015/02/10 01:31:08, bbudge wrote: > I think this id has the same meaning as above, right? Adding a comment too. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.h:102: void OnPluginMsgGetVideoFramesReply(const ResourceMessageReplyParams& params, On 2015/02/10 01:31:08, bbudge wrote: > This name is a little confusing to me, since it sounds like it's getting video > frames. How about 'GetVideoFrameInfo'? We're actually getting the shm buffer too. It feels easier to understand when the name match the ipc message name. Or maybe I could rename the message? https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.h:122: void WriteBitstreamerBuffer(PP_BitstreamBuffer* bitstream_buffer, On 2015/02/10 01:31:08, bbudge wrote: > s/WriteBitstreamerBuffer/WriteBitstreamBuffer Oh dear... Done. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.h:135: scoped_ptr<MediaStreamBufferManager> buffer_manager_; On 2015/02/10 01:31:08, bbudge wrote: > Same comments as on PepperVideoEncoderHost to eliminate a separate Delegate, and > use of scoped_ptrs here. Done. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.h:137: typedef std::map<PP_Resource, scoped_refptr<VideoFrameResource> > On 2015/02/10 01:31:08, bbudge wrote: > Since we only use C++11 toolchains when we build the proxy, I think we can use > >> without parsing problems. I might be wrong, but I think the GCC version used on ChromeOS doesn't support this. Last time I triggered a try job with the API containing a ">>" the job failed. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.h:142: get_video_frame_cbs_; On 2015/02/10 01:31:08, bbudge wrote: > get_video_frame_cbs_ sounds like a function or method name. > How about 'pending_get_video_frame_calls_' or something similar? Done. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.h:144: ScopedVector<ShmBuffer> bitstream_buffers_; On 2015/02/10 01:31:08, bbudge wrote: > Could we name this shm_buffers_? Sure. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.h:151: scoped_refptr<TrackedCallback> get_bitstreamer_buffer_callback_; On 2015/02/10 01:31:08, bbudge wrote: > s/get_bitstreamer_buffer_callback_/get_bitstream_buffer_callback_ Done. https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.h:152: PP_BitstreamBuffer* get_bitstreamer_buffer_data_; On 2015/02/10 01:31:08, bbudge wrote: > s/get_bitstreamer_buffer_data_/get_bitstream_buffer_data_ Done.
On 2015/02/09 15:00:54, llandwerlin wrote: > wfh@ bbudge@ jam@: Please review this to implement the PPB_VideoEncoder API, > Thanks. what do you want me to look at exactly?
On 2015/02/10 17:23:39, jam wrote: > On 2015/02/09 15:00:54, llandwerlin wrote: > > wfh@ bbudge@ jam@: Please review this to implement the PPB_VideoEncoder API, > > Thanks. > > what do you want me to look at exactly? We just needed someone for the build system stuff in content/. Sorry for the noise :/
On 2015/02/10 17:26:08, llandwerlin wrote: > On 2015/02/10 17:23:39, jam wrote: > > On 2015/02/09 15:00:54, llandwerlin wrote: > > > wfh@ bbudge@ jam@: Please review this to implement the PPB_VideoEncoder API, > > > Thanks. > > > > what do you want me to look at exactly? > > We just needed someone for the build system stuff in content/. > Sorry for the noise :/ gypi lgtm
https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... File ppapi/proxy/video_encoder_resource.cc (right): https://codereview.chromium.org/905023005/diff/1/ppapi/proxy/video_encoder_re... ppapi/proxy/video_encoder_resource.cc:293: bitstream_buffers_map_.clear(); On 2015/02/10 14:28:13, llandwerlin wrote: > On 2015/02/10 01:31:08, bbudge wrote: > > I don't see how these two collections could be non-empty at this point. > > I was thinking that maybe after the encoder is closed, it could be > reinitialized. In Pepper the convention is that calling Close is permanent, i.e. no other methods can be called. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_encoder_host.cc (right): https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:53: // No default case, to catch unhandled enum values. Indentation should match 'case', here and below. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:125: return PP_VIDEOPROFILE_VP9_ANY; add a NOTREACHED and return -1, e.g. https://code.google.com/p/chromium/codesearch#chromium/src/net/spdy/spdy_sess... https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:129: media::VideoFrame::Format PepperToMediaVideoFormat( s/MediaVideo/VideoFrame or MediaVideoFrame https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:164: //////////////////////////////////////////////////////////////////////////////// Chrome doesn't really use this commenting convention https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:189: encoder_last_error_(PP_ERROR_FAILED), initialize input_coded_size_, frame_count_, and media_format_. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:199: } Shouldn't we Destroy the encoder, here or where we handle Close()? https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:261: PP_HardwareAcceleration acceleration) { You should use this parameter to fail when it's not hardware for now. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:262: scoped_ptr<media::VideoEncodeAccelerator> encoder; Unused. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:266: media_format_ = PepperToMediaVideoFormat(input_format); Could you make the locals that are translated from input params follow the name so it's easier to follow? In this case, s/media_format_/media_input_format_ And below. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:268: return PP_ERROR_NOTSUPPORTED; PP_ERROR_BADARGUMENT https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:277: // we create a dump command buffer to communicate with the gpu s/garantee/guarantee s/dump/dummy ? https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:303: if (encoder_.get() && I don't think you need the .get() https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:319: media::VideoFrame::AllocationSize(media_format_, input_coded_size_); Since this returns size_t and we don't range limit input_coded_size_, a malicious plugin could force an overflow. Use size_t here and below. I think it's still possible to overflow, so you should also limit input_coded_size_ in OnHostMsgInitialize above (i.e. the product of width * height should be < some number so size_t can handle all of this arithmetic without overflow. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:333: // synchronous and should be avoided. I don't think this TODO will ever get todone. I'd remove it. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:380: Range check 'frame_id' here. This is an attack surface. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:396: return PP_ERROR_BADARGUMENT; use brackets. Also PP_ERROR_FAILED would be better, as this could only happen if there's an internal error or it's a malicious plugin. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:418: NotifyPepperError(PP_ERROR_ABORTED); I think you should just call encoder_->Destroy() here, after checking that encoder_ != null. Let's reserve NotifyError for the special class of errors that encoders report. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:429: buffers_.clear(); I think I asked before, but is it possible to receive multiple RequireBitstreamBuffers calls from the encoder? If not, I'd remove this, or at most have a DCHECK. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:451: NotifyPepperError(PP_ERROR_FAILED); Don't use NotifyPepperError as a generic error reporter. Just return the error code as a normal initialize reply. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:460: // Notify the plugins of the buffers and the input size. s/plugins/plugin https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:461: PP_Size size; PP_MakeSize would be more concise. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:495: const ppapi::host::ReplyMessageContext& reply_context) { You might want to DCHECK that frame_id is in range. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_encoder_host.h (right): https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.h:80: // void* BufferIdToAddress(int32_t buffer_id); remove https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.h:84: void EncodeVideoFrameDone( 'EncodeDone' might be better. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.h:94: // Shared memory buffers. Child class definitions are done as the first private: declarations. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.h:108: ScopedVector<ShmBuffer> buffers_; Rename this shm_buffers_ for clarity. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.h:109: size_t buffer_count_hint_; Unused. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.h:116: // Global state Not really global. Why not remove this and document individual fields instead? https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... File ppapi/proxy/video_encoder_resource.cc (right): https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:70: buffer_manager_(this) { Initialize get_bitstream_buffer_data_ to nullptr here. https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:75: Close(); I think you'll need to check if the plugin also called Close() so it doesn't happen twice. https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:76: ReleaseFrames(); Could ReleaseFrames just be part of Close()? https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:86: // TODO(llandwerlin): should we prevent concurrent calls? Yes, we should prevent that. https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:133: You'll need some way to throttle the plugin. As it stands, the plugin can call this method an unlimited number of times without an error, growing the queue without bound. I'd recommend returning PP_ERROR_INPROGRESS if we're waiting for the initial set of frames (waiting_for_video_frames_ == true) or if we are waiting for an existing frame to become available. https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:138: if (buffer_manager_.number_of_buffers() < 1) { Condition is clearer as == 0. https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:139: // Check that we haven't ask for the buffers already. nit: s/ask/asked https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:161: We're not throttling the plugin here, but I don't think the plugin would ever be able to get enough VideoFrames to spam the proxy, so it's OK. https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:164: // TODO(llandwerlin): deal MediaStreamVideoTrack's video frames. s/deal/accept ? https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:207: if (iter != bitstream_buffers_map_.end()) Need brackets around multiline block here. https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:217: bitrate, framerate)); Is this indentation the clang formatter? https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:226: SyncCall<PpapiPluginMsg_VideoEncoder_CloseReply>( We avoid sync calls whenever possible. I don't understand why this is needed, since you don't handle the reply message. I expect the host to eventually reply to every message that you sent, so all callbacks will eventually be called. Is there something I'm missing? https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:278: bitstream_buffers_map_.clear(); These should be empty at this point. Perhaps DCHECKs if you think it likely that this could break in the future. But then I'd probably DCHECK(!initialized_) at the beginning instead. https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:304: params.TakeAllSharedMemoryHandles(&buffer_handles); Since this is for a single handle, you could use TakeSharedMemoryHandleAtIndex instead. https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/gamepa... https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:305: CHECK(buffer_handles.size() == 1); I don't think you need to CHECK for the correct number of handles. This code runs in a less privileged process than the host. https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:315: waiting_for_video_frames_ = false; You might want to make this the first line so it isn't stuck at 'true' after a failure. https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:336: BitstreamBuffer(buffer_id, buffer_size, PP_FromBool(key_frame))); PP_FromBool isn't needed here. https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:382: void VideoEncoderResource::NotifyGetVideoFrameCallbacksError(int32_t error) { You could just move this loop into NotifyError. Is there a case where it would be called separately? https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... File ppapi/proxy/video_encoder_resource.h (right): https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.h:150: BitstreamBufferMap bitstream_buffers_map_; nit: s/bitstream_buffers_map_/bitstream_buffer_map_
New patchsets have been uploaded after l-g-t-m from jam@chromium.org
Hopefully, I haven't left anything unanswered. There is still one issue : SyncCall on Close(). I'll try to tackle this soon. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_encoder_host.cc (right): https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:53: // No default case, to catch unhandled enum values. On 2015/02/10 22:47:37, bbudge wrote: > Indentation should match 'case', here and below. Done. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:125: return PP_VIDEOPROFILE_VP9_ANY; On 2015/02/10 22:47:37, bbudge wrote: > add a NOTREACHED and return -1, e.g. > https://code.google.com/p/chromium/codesearch#chromium/src/net/spdy/spdy_sess... Done. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:129: media::VideoFrame::Format PepperToMediaVideoFormat( On 2015/02/10 22:47:36, bbudge wrote: > s/MediaVideo/VideoFrame or MediaVideoFrame Renamed all these functions to match the PP_From*/PP_To* pattern. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:164: //////////////////////////////////////////////////////////////////////////////// On 2015/02/10 22:47:37, bbudge wrote: > Chrome doesn't really use this commenting convention Done. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:189: encoder_last_error_(PP_ERROR_FAILED), On 2015/02/10 22:47:36, bbudge wrote: > initialize input_coded_size_, frame_count_, and media_format_. Sure, can I leave the input_coded_size_ initialized with the default constructor though? https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:199: } On 2015/02/10 22:47:37, bbudge wrote: > Shouldn't we Destroy the encoder, here or where we handle Close()? I guess it should be destroyed before the command buffer is destroyed. Thanks. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:261: PP_HardwareAcceleration acceleration) { On 2015/02/10 22:47:36, bbudge wrote: > You should use this parameter to fail when it's not hardware for now. Done. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:262: scoped_ptr<media::VideoEncodeAccelerator> encoder; On 2015/02/10 22:47:37, bbudge wrote: > Unused. Done. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:266: media_format_ = PepperToMediaVideoFormat(input_format); On 2015/02/10 22:47:37, bbudge wrote: > Could you make the locals that are translated from input params follow the name > so it's easier to follow? In this case, > s/media_format_/media_input_format_ > And below. Done. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:268: return PP_ERROR_NOTSUPPORTED; On 2015/02/10 22:47:36, bbudge wrote: > PP_ERROR_BADARGUMENT Done. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:277: // we create a dump command buffer to communicate with the gpu On 2015/02/10 22:47:37, bbudge wrote: > s/garantee/guarantee > s/dump/dummy ? Done. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:303: if (encoder_.get() && On 2015/02/10 22:47:36, bbudge wrote: > I don't think you need the .get() Done. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:319: media::VideoFrame::AllocationSize(media_format_, input_coded_size_); On 2015/02/10 22:47:37, bbudge wrote: > Since this returns size_t and we don't range limit input_coded_size_, a > malicious plugin could force an overflow. Use size_t here and below. I think > it's still possible to overflow, so you should also limit input_coded_size_ in > OnHostMsgInitialize above (i.e. the product of width * height should be < some > number so size_t can handle all of this arithmetic without overflow. Added a method to check against supported format instead. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:333: // synchronous and should be avoided. On 2015/02/10 22:47:36, bbudge wrote: > I don't think this TODO will ever get todone. I'd remove it. Done. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:380: On 2015/02/10 22:47:36, bbudge wrote: > Range check 'frame_id' here. This is an attack surface. Done. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:396: return PP_ERROR_BADARGUMENT; On 2015/02/10 22:47:37, bbudge wrote: > use brackets. Also PP_ERROR_FAILED would be better, as this could only happen if > there's an internal error or it's a malicious plugin. Done. Not sure where you would like me to put brackets? Around the statement or around the conditions? https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:418: NotifyPepperError(PP_ERROR_ABORTED); Ok, I'll keep setting the encoder_last_error_ variable to aborted though. I need to know whether an error should be reported in EncodeVideoFrameDone(). https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:429: buffers_.clear(); On 2015/02/10 22:47:36, bbudge wrote: > I think I asked before, but is it possible to receive multiple > RequireBitstreamBuffers calls from the encoder? If not, I'd remove this, or at > most have a DCHECK. No, not with the current implementations. That could change though, it seems changing the bitrate is not really well tested. I wonder if that could require bigger bitstream buffers at some point. Removing this for now. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:451: NotifyPepperError(PP_ERROR_FAILED); On 2015/02/10 22:47:37, bbudge wrote: > Don't use NotifyPepperError as a generic error reporter. Just return the error > code as a normal initialize reply. Done. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:460: // Notify the plugins of the buffers and the input size. On 2015/02/10 22:47:36, bbudge wrote: > s/plugins/plugin Done. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:461: PP_Size size; On 2015/02/10 22:47:37, bbudge wrote: > PP_MakeSize would be more concise. Done. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:495: const ppapi::host::ReplyMessageContext& reply_context) { On 2015/02/10 22:47:37, bbudge wrote: > You might want to DCHECK that frame_id is in range. Done. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_encoder_host.h (right): https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.h:80: // void* BufferIdToAddress(int32_t buffer_id); On 2015/02/10 22:47:37, bbudge wrote: > remove Done. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.h:94: // Shared memory buffers. On 2015/02/10 22:47:38, bbudge wrote: > Child class definitions are done as the first private: declarations. Using struct instead. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.h:109: size_t buffer_count_hint_; On 2015/02/10 22:47:38, bbudge wrote: > Unused. Done. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.h:116: // Global state On 2015/02/10 22:47:37, bbudge wrote: > Not really global. Why not remove this and document individual fields instead? Done. https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... File ppapi/proxy/video_encoder_resource.cc (right): https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:70: buffer_manager_(this) { On 2015/02/10 22:47:38, bbudge wrote: > Initialize get_bitstream_buffer_data_ to nullptr here. Done. https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:75: Close(); On 2015/02/10 22:47:38, bbudge wrote: > I think you'll need to check if the plugin also called Close() so it doesn't > happen twice. Done. https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:76: ReleaseFrames(); On 2015/02/10 22:47:39, bbudge wrote: > Could ReleaseFrames just be part of Close()? Done. https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:86: // TODO(llandwerlin): should we prevent concurrent calls? On 2015/02/10 22:47:38, bbudge wrote: > Yes, we should prevent that. Done. https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:133: On 2015/02/10 22:47:38, bbudge wrote: > You'll need some way to throttle the plugin. As it stands, the plugin can call > this method an unlimited number of times without an error, growing the queue > without bound. > > I'd recommend returning PP_ERROR_INPROGRESS if we're waiting for the initial set > of frames (waiting_for_video_frames_ == true) or if we are waiting for an > existing frame to become available. Done. https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:138: if (buffer_manager_.number_of_buffers() < 1) { On 2015/02/10 22:47:38, bbudge wrote: > Condition is clearer as == 0. Done. https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:139: // Check that we haven't ask for the buffers already. On 2015/02/10 22:47:38, bbudge wrote: > nit: s/ask/asked Done. https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:161: On 2015/02/10 22:47:38, bbudge wrote: > We're not throttling the plugin here, but I don't think the plugin would ever be > able to get enough VideoFrames to spam the proxy, so it's OK. I guess it could be good not to resend the same frame to the encoder until it comes back as done. https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:164: // TODO(llandwerlin): deal MediaStreamVideoTrack's video frames. On 2015/02/10 22:47:38, bbudge wrote: > s/deal/accept ? Done. https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:207: if (iter != bitstream_buffers_map_.end()) On 2015/02/10 22:47:38, bbudge wrote: > Need brackets around multiline block here. Done. https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:217: bitrate, framerate)); On 2015/02/10 22:47:38, bbudge wrote: > Is this indentation the clang formatter? Oops... https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:226: SyncCall<PpapiPluginMsg_VideoEncoder_CloseReply>( On 2015/02/10 22:47:38, bbudge wrote: > We avoid sync calls whenever possible. I don't understand why this is needed, > since you don't handle the reply message. I expect the host to eventually reply > to every message that you sent, so all callbacks will eventually be called. > > Is there something I'm missing? Yes, just so it's convenient, I'm using Call<>(), so I don't have to keep track of all Encode(), but there is no guarantee these will be called before Close() returns. I'll switching to a map to manage this here. https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:278: bitstream_buffers_map_.clear(); On 2015/02/10 22:47:38, bbudge wrote: > These should be empty at this point. Perhaps DCHECKs if you think it likely that > this could break in the future. But then I'd probably DCHECK(!initialized_) at > the beginning instead. Done. https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:304: params.TakeAllSharedMemoryHandles(&buffer_handles); On 2015/02/10 22:47:38, bbudge wrote: > Since this is for a single handle, you could use TakeSharedMemoryHandleAtIndex > instead. > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/gamepa... Done. https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:305: CHECK(buffer_handles.size() == 1); On 2015/02/10 22:47:38, bbudge wrote: > I don't think you need to CHECK for the correct number of handles. This code > runs in a less privileged process than the host. Done. https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:315: waiting_for_video_frames_ = false; On 2015/02/10 22:47:38, bbudge wrote: > You might want to make this the first line so it isn't stuck at 'true' after a > failure. Done. https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:336: BitstreamBuffer(buffer_id, buffer_size, PP_FromBool(key_frame))); On 2015/02/10 22:47:38, bbudge wrote: > PP_FromBool isn't needed here. What's the purpose of PP_FromBool then? https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:382: void VideoEncoderResource::NotifyGetVideoFrameCallbacksError(int32_t error) { On 2015/02/10 22:47:38, bbudge wrote: > You could just move this loop into NotifyError. Is there a case where it would > be called separately? Done. https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... File ppapi/proxy/video_encoder_resource.h (right): https://codereview.chromium.org/905023005/diff/20001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.h:150: BitstreamBufferMap bitstream_buffers_map_; On 2015/02/10 22:47:39, bbudge wrote: > nit: s/bitstream_buffers_map_/bitstream_buffer_map_ Done.
https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_encoder_host.cc (right): https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:189: encoder_last_error_(PP_ERROR_FAILED), On 2015/02/11 18:42:23, llandwerlin wrote: > On 2015/02/10 22:47:36, bbudge wrote: > > initialize input_coded_size_, frame_count_, and media_format_. > > Sure, can I leave the input_coded_size_ initialized with the default constructor > though? Oh, right, that's fine. https://codereview.chromium.org/905023005/diff/20001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:396: return PP_ERROR_BADARGUMENT; On 2015/02/11 18:42:24, llandwerlin wrote: > On 2015/02/10 22:47:37, bbudge wrote: > > use brackets. Also PP_ERROR_FAILED would be better, as this could only happen > if > > there's an internal error or it's a malicious plugin. > > Done. > Not sure where you would like me to put brackets? Around the statement or around > the conditions? On second thought, the style guide leaves this open, so your preference here: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Conditionals https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_encoder_host.cc (right): https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:26: using ppapi::proxy::SerializedHandle; nit: you only use this once, and use the fully qualified name once below. https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:136: // No default case, to catch unhandled PP_VideoFrame_Format values. indent like above https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:217: DCHECK(channel_.get()); nit: .get() not needed. https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:221: channel_ = nullptr; nit: this destructor is about to do this anyway. https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:227: PPAPI_BEGIN_MESSAGE_MAP(PepperVideoEncoderHost, msg) handlers in the map should be indented 2 spaces. clang format does the wrong thing here. https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:276: PP_ToMediaVideoProfile(output_profile); Check that media_profile is valid, and if not, return PP_ERROR_BADARGUMENT. That's clearer than returning PP_ERROR_NOTSUPPORTED (below). https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:294: attribs.push_back(PP_GRAPHICS3DATTRIB_NONE); nit: Could be: std::vector<int32> attribs(1, PP_GRAPHICS3DATTRIB_NONE); https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:314: return PP_OK_COMPLETIONPENDING; If Initialize() fails encoder_ is in a weird state. It's probably better to do it this way: if (encoder_ && !encoder.Initialize(...)) { encoder_ = nullptr; return PP_ERROR_FAILED; // it's supported but failed for some reason. } initialized_ = true; return PP_OK_COMPLETIONPENDING; ... https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:320: } It would be best if initialized_ was true before exiting this function. VEA's return synchronously, and I bet software encoders do too. See my related comments in RequireBitstreamBuffers. https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:432: base::Unretained(encoder_.release()); if (encoder_) encoder_.release()->Destroy(); I think encoder_ = nullptr; will have the same effect though. https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:441: DCHECK(RenderThreadImpl::current()); Rereading the comments on VEA, it implies that this could be called more than once during encoding, not just after initialization: https://code.google.com/p/chromium/codesearch#chromium/src/media/video/video_... I'll ask tomorrow for clarification on this. In the meantime, we should think about that possibility which implies: 1) Removing initialization-related code here. I think Initialize() is synchronous for both hardware and software, so this could be handled by OnHostMsgInitialize(). This would also prevent malicious plugins from calling OnHostMsgInitialize more than once. 2) Add an unsolicited reply message to pass handles and values to the plugin instead of doing it in InitializeReply. 3) Allow buffers and frames to be reallocated. This is tricky, since the plugin may holding frames and bitstream buffers. PepperVideoDecoderHost does something like this. https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_encoder_host.h (right): https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.h:57: // Plugin's IPC calls. nit: These are message handlers, but I don't think a comment adds much here. https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.h:97: // Shared memory buffers. Child type definitions should be the first private definitions. Move this up to line 43. https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.h:109: // Bitstream buffers. Thanks for adding comments, but it isn't necessary to comment each field. In general, add a comment when there are things future maintainers should know about the field. For an example: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p... https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.h:112: // Video frames. nit: // Buffer manager for shared memory that holds video frames. https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.h:115: // Gpu process communication. nit: I think you could omit the comment here. https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.h:124: // Last error encountered. This is too vague given the complexity of this.
https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_encoder_host.cc (right): https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:441: DCHECK(RenderThreadImpl::current()); On 2015/02/11 23:27:45, bbudge wrote: > Rereading the comments on VEA, it implies that this could be called more than > once during encoding, not just after initialization: > https://code.google.com/p/chromium/codesearch#chromium/src/media/video/video_... > > I'll ask tomorrow for clarification on this. In the meantime, we should think > about that possibility which implies: > > 1) Removing initialization-related code here. I think Initialize() is > synchronous for both hardware and software, so this could be handled by > OnHostMsgInitialize(). This would also prevent malicious plugins from calling > OnHostMsgInitialize more than once. > > 2) Add an unsolicited reply message to pass handles and values to the plugin > instead of doing it in InitializeReply. > > 3) Allow buffers and frames to be reallocated. This is tricky, since the plugin > may holding frames and bitstream buffers. PepperVideoDecoderHost does something > like this. As discussed on email, with the current VEA code, this will only be called once. I think it would still be good to consider initialization done when OnHostMsgInitialize returns, and to add a separate unsolicited reply message to return the shm buffers and parameters to the resource.
PTAL, Thanks. https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_encoder_host.cc (right): https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:26: using ppapi::proxy::SerializedHandle; On 2015/02/11 23:27:45, bbudge wrote: > nit: you only use this once, and use the fully qualified name once below. Done. https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:136: // No default case, to catch unhandled PP_VideoFrame_Format values. On 2015/02/11 23:27:45, bbudge wrote: > indent like above Sorry for that, cl format does the wrong thing. https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:217: DCHECK(channel_.get()); On 2015/02/11 23:27:45, bbudge wrote: > nit: .get() not needed. Done. https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:221: channel_ = nullptr; On 2015/02/11 23:27:45, bbudge wrote: > nit: this destructor is about to do this anyway. Done. https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:227: PPAPI_BEGIN_MESSAGE_MAP(PepperVideoEncoderHost, msg) On 2015/02/11 23:27:45, bbudge wrote: > handlers in the map should be indented 2 spaces. > clang format does the wrong thing here. Done. https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:276: PP_ToMediaVideoProfile(output_profile); On 2015/02/11 23:27:44, bbudge wrote: > Check that media_profile is valid, and if not, return PP_ERROR_BADARGUMENT. > That's clearer than returning PP_ERROR_NOTSUPPORTED (below). Thanks, adding a check on the input_size too (it should not be empty). https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:294: attribs.push_back(PP_GRAPHICS3DATTRIB_NONE); On 2015/02/11 23:27:45, bbudge wrote: > nit: Could be: > std::vector<int32> attribs(1, PP_GRAPHICS3DATTRIB_NONE); Done. https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:432: base::Unretained(encoder_.release()); On 2015/02/11 23:27:45, bbudge wrote: > if (encoder_) > encoder_.release()->Destroy(); > > I think encoder_ = nullptr; will have the same effect though. Yeah, looking again at the receiving end of the VEA ipc, I can't see any code actually calling Destroy()... Switching to = nullptr; https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:441: DCHECK(RenderThreadImpl::current()); On 2015/02/12 02:25:36, bbudge wrote: > On 2015/02/11 23:27:45, bbudge wrote: > > Rereading the comments on VEA, it implies that this could be called more than > > once during encoding, not just after initialization: > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/video/video_... > > > > I'll ask tomorrow for clarification on this. In the meantime, we should think > > about that possibility which implies: > > > > 1) Removing initialization-related code here. I think Initialize() is > > synchronous for both hardware and software, so this could be handled by > > OnHostMsgInitialize(). This would also prevent malicious plugins from calling > > OnHostMsgInitialize more than once. > > > > 2) Add an unsolicited reply message to pass handles and values to the plugin > > instead of doing it in InitializeReply. > > > > 3) Allow buffers and frames to be reallocated. This is tricky, since the > plugin > > may holding frames and bitstream buffers. PepperVideoDecoderHost does > something > > like this. > > As discussed on email, with the current VEA code, this will only be called once. > I think it would still be good to consider initialization done when > OnHostMsgInitialize returns, and to add a separate unsolicited reply message to > return the shm buffers and parameters to the resource. Ok. But from the Resource point of view, the initialization won't finish until we've received the bitstream buffers, right? https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_encoder_host.h (right): https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.h:57: // Plugin's IPC calls. On 2015/02/11 23:27:46, bbudge wrote: > nit: These are message handlers, but I don't think a comment adds much here. Done. https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.h:97: // Shared memory buffers. On 2015/02/11 23:27:45, bbudge wrote: > Child type definitions should be the first private definitions. Move this up to > line 43. Done. https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.h:109: // Bitstream buffers. On 2015/02/11 23:27:45, bbudge wrote: > Thanks for adding comments, but it isn't necessary to comment each field. In > general, add a comment when there are things future maintainers should know > about the field. For an example: > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p... Done. https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.h:109: // Bitstream buffers. On 2015/02/11 23:27:45, bbudge wrote: > Thanks for adding comments, but it isn't necessary to comment each field. In > general, add a comment when there are things future maintainers should know > about the field. For an example: > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p... Acknowledged. https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.h:112: // Video frames. On 2015/02/11 23:27:46, bbudge wrote: > nit: // Buffer manager for shared memory that holds video frames. Done. https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.h:112: // Video frames. On 2015/02/11 23:27:46, bbudge wrote: > nit: // Buffer manager for shared memory that holds video frames. Done. https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.h:115: // Gpu process communication. On 2015/02/11 23:27:45, bbudge wrote: > nit: I think you could omit the comment here. Done. https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.h:124: // Last error encountered. On 2015/02/11 23:27:46, bbudge wrote: > This is too vague given the complexity of this. Done.
Question answered, I'll review this today. https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_encoder_host.cc (right): https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:441: DCHECK(RenderThreadImpl::current()); On 2015/02/12 15:59:39, llandwerlin wrote: > On 2015/02/12 02:25:36, bbudge wrote: > > On 2015/02/11 23:27:45, bbudge wrote: > > > Rereading the comments on VEA, it implies that this could be called more > than > > > once during encoding, not just after initialization: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/video/video_... > > > > > > I'll ask tomorrow for clarification on this. In the meantime, we should > think > > > about that possibility which implies: > > > > > > 1) Removing initialization-related code here. I think Initialize() is > > > synchronous for both hardware and software, so this could be handled by > > > OnHostMsgInitialize(). This would also prevent malicious plugins from > calling > > > OnHostMsgInitialize more than once. > > > > > > 2) Add an unsolicited reply message to pass handles and values to the plugin > > > instead of doing it in InitializeReply. > > > > > > 3) Allow buffers and frames to be reallocated. This is tricky, since the > > plugin > > > may holding frames and bitstream buffers. PepperVideoDecoderHost does > > something > > > like this. > > > > As discussed on email, with the current VEA code, this will only be called > once. > > I think it would still be good to consider initialization done when > > OnHostMsgInitialize returns, and to add a separate unsolicited reply message > to > > return the shm buffers and parameters to the resource. > > Ok. > But from the Resource point of view, the initialization won't finish until we've > received the bitstream buffers, right? I don't see why it has to be that way. The plugin could call Encode with an external video frame before we have any buffers. Of course, GetVideoFrame can't complete until the VE's frames are passed back to the resource, and GetBitstreamBuffer can't complete until we've received the bitstream buffers.
On 2015/02/12 18:18:11, bbudge wrote: > Question answered, I'll review this today. > > https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... > File content/renderer/pepper/pepper_video_encoder_host.cc (right): > > https://codereview.chromium.org/905023005/diff/60001/content/renderer/pepper/... > content/renderer/pepper/pepper_video_encoder_host.cc:441: > DCHECK(RenderThreadImpl::current()); > On 2015/02/12 15:59:39, llandwerlin wrote: > > On 2015/02/12 02:25:36, bbudge wrote: > > > On 2015/02/11 23:27:45, bbudge wrote: > > > > Rereading the comments on VEA, it implies that this could be called more > > than > > > > once during encoding, not just after initialization: > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/video/video_... > > > > > > > > I'll ask tomorrow for clarification on this. In the meantime, we should > > think > > > > about that possibility which implies: > > > > > > > > 1) Removing initialization-related code here. I think Initialize() is > > > > synchronous for both hardware and software, so this could be handled by > > > > OnHostMsgInitialize(). This would also prevent malicious plugins from > > calling > > > > OnHostMsgInitialize more than once. > > > > > > > > 2) Add an unsolicited reply message to pass handles and values to the > plugin > > > > instead of doing it in InitializeReply. > > > > > > > > 3) Allow buffers and frames to be reallocated. This is tricky, since the > > > plugin > > > > may holding frames and bitstream buffers. PepperVideoDecoderHost does > > > something > > > > like this. > > > > > > As discussed on email, with the current VEA code, this will only be called > > once. > > > I think it would still be good to consider initialization done when > > > OnHostMsgInitialize returns, and to add a separate unsolicited reply message > > to > > > return the shm buffers and parameters to the resource. > > > > Ok. > > But from the Resource point of view, the initialization won't finish until > we've > > received the bitstream buffers, right? > > I don't see why it has to be that way. The plugin could call Encode with an > external video frame before we have any buffers. Of course, GetVideoFrame can't > complete until the VE's frames are passed back to the resource, and > GetBitstreamBuffer can't complete until we've received the bitstream buffers. Fair, I'll change that after your review. Thanks.
This is looking good. Mostly minor issues. I want to unblock you, so I still haven't reviewed the unit tests. https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_encoder_host.cc (right): https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:186: scoped_ptr<base::SharedMemory> memory, nit: s/memory/shm ? https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:189: if (shm) Since it's only called inside this file, shm should always be non-null. Maybe a DCHECK? https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:215: if (encoder_) nit: 'if' isn't necessary. https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:222: } Suggestion: Add a new utility method, Close() that does all of this. Then call it here, and in OnHostMsgClose, which currently doesn't release the command_buffer_. https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:325: encoder_ = nullptr; You probably should Close() this instance here. See my comments on the dtor. https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:340: media::VideoFrame::AllocationSize(media_input_format_, input_coded_size_); This doesn't overflow because of your input validation, but it's not obvious from looking here, and could be broken by a future maintainer. Also, the following statement could in theory overflow. Could you do all checked math like this? base::CheckedNumeric<uint32_t> size = media::VideoFrame::AllocationSize(media_input_format_, input_coded_size_); uint32_t frame_size = size.ValueOrDie(); size += sizeof(ppapi::MediaStreamBuffer::Video) - sizeof(ppapi::MediaStreamBuffer::Video::data); uint32_t buffer_size = size.ValueOrDie(); size += (4 - buffer_size % 4); base::CheckedNumeric<int32_t> buffer_size_aligned = size.ValueOrDie(); size *= frame_count_; base::CheckedNumeric<int32_t> total_size = size.valueOrDie(); https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:358: return PP_ERROR_FAILED; PP_ERROR_NOMEMORY is more consistent. https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:400: if (frame_id >= static_cast<uint32_t>(buffer_manager_.number_of_buffers())) Could you use frame_count_ instead of buffer_manager_.number_of_buffers()? Here and below. https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:416: if (buffer_id < 0 || buffer_id is unsigned https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:442: encoder_last_error_ = PP_ERROR_ABORTED; This error code just seems odd to me here, as it's usually generated on the resource side before calling back into the plugin (though I see some precedent in content/renderer/pepper.) Could a generic PP_ERROR_FAILED also work here? https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:471: output_buffer_size)); You could save a loop by creating the SerializedHandles when you feed them to the encoder (thus not needing to Close() them if you can't get all the shm buffers.) https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:477: NotifyPepperError(PP_ERROR_FAILED); PP_ERROR_NOMEMORY https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:486: // Notify the plugin of the buffers and the input size. nit: s/input size/encoding parameters because there's output_buffer_size, frame_count, and size. nit: s/size/pp_input_coded_size https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:491: encoder_last_error_ = PP_OK; I think this should be set in OnHostMsgInitialize() by my earlier comments about when the encoder is ready. https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:495: PpapiPluginMsg_VideoEncoder_BitstreamBuffers(output_buffer_size, output_buffer_size is size_t but param is uint32_t. https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:554: static_cast<uint32_t>(buffer_manager_.number_of_buffers())); frame_count_? https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:556: ppapi::MediaStreamBuffer* buffer = buffer_manager_.GetBufferPointer(frame_id); This returns NULL if frame_id is out of range, so you could DCHECK that instead of your above range check. https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:576: context.params.set_result(encoder_last_error_); EncodeDone is a confusing to me. Could we rename this FrameReleased or something that indicates the encoder has returned the frame to us? https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:585: encoder_ = nullptr; Close() here instead? https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... File ppapi/proxy/video_encoder_resource.cc (right): https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:152: if (!WriteVideoFrame(video_frame)) { Suggestion: Move the next two statements to above this 'if' statement and change WriteVideoFrame to run the callback and clear the data pointer (and it won't take a parameter.) That will make it easier to do the right thing at the other two call sites for WriteVideoFrame. Right now, you sometimes leave the pointer non-null afterwards. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:200: BitstreamBuffer buffer(available_bitstream_buffers_.front()); Could you make this const BitstreamBuffer& instead, and not pop_front() until after WriteBitstreamBuffer returns? Here and below. If you can, you can also eliminate BitstreamBuffer's copy constructor. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:205: } Suggestion: similarly to GetVideoFrame, you could move all of this into WriteBitstreamBuffer. In other words, always set the get_bitstream_xxx fields, then check for empty in WriteBitstreamBuffer. Probably better names for these functions would be TryWriteBitstreamBuffer and TryGetVideoFrame. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:275: initialized_ = true; Here, not below. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:277: if (error) We should always run the callback here, not below. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:415: } Maybe clear video_frames_ for hygiene? https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... File ppapi/proxy/video_encoder_resource.h (right): https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.h:48: // Index of the buffer in the ScopedVector. Buffers have the id in s/id/same id/
Make sure to run unit tests in DEBUG mode to catch leaked resources etc. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... File ppapi/proxy/video_encoder_resource_unittest.cc (right): https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:31: const uint32_t kDecodeId = 5; I don't think you need these definitions. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:70: const uint32_t kVideoFrameNb = 3; It took me a while to get that Nb is 'Count'. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:73: const PP_Size kSize = {640, 480}; kFrameSize? https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:366: bool CheckRequestEncodeingParametersChangeMsg( s/Encodeing/Encoding https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:386: bool CheckMsg(ResourceMessageCallParams* params, int id) { Not used https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:399: char decode_buffer_[kDecodeBufferSize]; Not used. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:415: std::vector<PP_VideoProfileDescription> profiles; This could just be an array, i.e. PP_VideoProfileDescription profiles[2]; https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:520: } As discussed, the Initialize callback should always call back. Things to test are calling Initialize before previous call completes, and weird enum values like e.g. casted -1. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:554: SendInitializeReply(params, kBitstreamBufferSize, kVideoFrameNb, size); Again, Initialize completes without getting buffers. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:642: // Unlock the GetVideoFrame callback. // Unblock the GetVideoFrame callback by freeing up a frame. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:655: video_frames[i]); Could you call encoder.Close() instead? https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:665: ; empty statement https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:732: Test a second call gets INPROGRESS. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:784: // Check an error from the encoder calls back GetVideoFrame and s/calls back/aborts https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:809: // Check an error from the encoder calls back Encode and GetBitstreamBuffer s/calls back/aborts https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:853: // Check closing the encoder callbacks back GetVideoFrame and s/callbacks back/aborts https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:877: // Check closing the encoder calls back Encode and GetBitstreamBuffer s/calls back/aborts https://codereview.chromium.org/905023005/diff/80001/ppapi/tests/test_video_e... File ppapi/tests/test_video_encoder.h (right): https://codereview.chromium.org/905023005/diff/80001/ppapi/tests/test_video_e... ppapi/tests/test_video_encoder.h:29: #endif // PPAPI_TESTS_TEST_VIDEO_ENCODER_H_ These tests won't be run unless you add them in chrome/test/ppapi/ppapi_browsertest.cc. See that file for examples. This test should run like video decoder, that is, out of process and NaCl only.
On 2015/02/13 01:45:55, bbudge wrote: > Make sure to run unit tests in DEBUG mode to catch leaked resources etc. > > https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... > File ppapi/proxy/video_encoder_resource_unittest.cc (right): > > https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... > ppapi/proxy/video_encoder_resource_unittest.cc:31: const uint32_t kDecodeId = 5; > I don't think you need these definitions. > > https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... > ppapi/proxy/video_encoder_resource_unittest.cc:70: const uint32_t kVideoFrameNb > = 3; > It took me a while to get that Nb is 'Count'. > > https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... > ppapi/proxy/video_encoder_resource_unittest.cc:73: const PP_Size kSize = {640, > 480}; > kFrameSize? > > https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... > ppapi/proxy/video_encoder_resource_unittest.cc:366: bool > CheckRequestEncodeingParametersChangeMsg( > s/Encodeing/Encoding > > https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... > ppapi/proxy/video_encoder_resource_unittest.cc:386: bool > CheckMsg(ResourceMessageCallParams* params, int id) { > Not used > > https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... > ppapi/proxy/video_encoder_resource_unittest.cc:399: char > decode_buffer_[kDecodeBufferSize]; > Not used. > > https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... > ppapi/proxy/video_encoder_resource_unittest.cc:415: > std::vector<PP_VideoProfileDescription> profiles; > This could just be an array, i.e. PP_VideoProfileDescription profiles[2]; > > https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... > ppapi/proxy/video_encoder_resource_unittest.cc:520: } > As discussed, the Initialize callback should always call back. Things to test > are calling Initialize before previous call completes, and weird enum values > like e.g. casted -1. > > https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... > ppapi/proxy/video_encoder_resource_unittest.cc:554: SendInitializeReply(params, > kBitstreamBufferSize, kVideoFrameNb, size); > Again, Initialize completes without getting buffers. > > https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... > ppapi/proxy/video_encoder_resource_unittest.cc:642: // Unlock the GetVideoFrame > callback. > // Unblock the GetVideoFrame callback by freeing up a frame. > > https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... > ppapi/proxy/video_encoder_resource_unittest.cc:655: video_frames[i]); > Could you call encoder.Close() instead? > > https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... > ppapi/proxy/video_encoder_resource_unittest.cc:665: ; > empty statement > > https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... > ppapi/proxy/video_encoder_resource_unittest.cc:732: > Test a second call gets INPROGRESS. > > https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... > ppapi/proxy/video_encoder_resource_unittest.cc:784: // Check an error from the > encoder calls back GetVideoFrame and > s/calls back/aborts > > https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... > ppapi/proxy/video_encoder_resource_unittest.cc:809: // Check an error from the > encoder calls back Encode and GetBitstreamBuffer > s/calls back/aborts > > https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... > ppapi/proxy/video_encoder_resource_unittest.cc:853: // Check closing the encoder > callbacks back GetVideoFrame and > s/callbacks back/aborts > > https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... > ppapi/proxy/video_encoder_resource_unittest.cc:877: // Check closing the encoder > calls back Encode and GetBitstreamBuffer > s/calls back/aborts > > https://codereview.chromium.org/905023005/diff/80001/ppapi/tests/test_video_e... > File ppapi/tests/test_video_encoder.h (right): > > https://codereview.chromium.org/905023005/diff/80001/ppapi/tests/test_video_e... > ppapi/tests/test_video_encoder.h:29: #endif // > PPAPI_TESTS_TEST_VIDEO_ENCODER_H_ > These tests won't be run unless you add them in > chrome/test/ppapi/ppapi_browsertest.cc. See that file for examples. This test > should run like video decoder, that is, out of process and NaCl only. I've completed a pass of the entire patch. You need to modify ppapi_browsertest.cc.
PTAL, Thanks. Ended up changing more stuff than I wanted to. Splitting initialize and bitstream buffer message have some consequences. We might need to update the documentation for new error codes in GetFrameCodedSize() and GetFramesRequired(). https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... File content/renderer/pepper/pepper_video_encoder_host.cc (right): https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:186: scoped_ptr<base::SharedMemory> memory, On 2015/02/12 22:22:09, bbudge wrote: > nit: s/memory/shm ? Done. https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:189: if (shm) On 2015/02/12 22:22:09, bbudge wrote: > Since it's only called inside this file, shm should always be non-null. Maybe a > DCHECK? Done. https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:215: if (encoder_) On 2015/02/12 22:22:09, bbudge wrote: > nit: 'if' isn't necessary. Done. https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:222: } On 2015/02/12 22:22:10, bbudge wrote: > Suggestion: Add a new utility method, Close() that does all of this. Then call > it here, and in OnHostMsgClose, which currently doesn't release the > command_buffer_. Done. https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:325: encoder_ = nullptr; On 2015/02/12 22:22:09, bbudge wrote: > You probably should Close() this instance here. See my comments on the dtor. Done. https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:340: media::VideoFrame::AllocationSize(media_input_format_, input_coded_size_); On 2015/02/12 22:22:09, bbudge wrote: > This doesn't overflow because of your input validation, but it's not obvious > from looking here, and could be broken by a future maintainer. Also, the > following statement could in theory overflow. Could you do all checked math like > this? > > base::CheckedNumeric<uint32_t> size = > media::VideoFrame::AllocationSize(media_input_format_, input_coded_size_); > uint32_t frame_size = size.ValueOrDie(); > size += sizeof(ppapi::MediaStreamBuffer::Video) - > sizeof(ppapi::MediaStreamBuffer::Video::data); > uint32_t buffer_size = size.ValueOrDie(); > size += (4 - buffer_size % 4); > base::CheckedNumeric<int32_t> buffer_size_aligned = size.ValueOrDie(); > size *= frame_count_; > base::CheckedNumeric<int32_t> total_size = size.valueOrDie(); > Done. https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:358: return PP_ERROR_FAILED; On 2015/02/12 22:22:10, bbudge wrote: > PP_ERROR_NOMEMORY is more consistent. Done. https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:400: if (frame_id >= static_cast<uint32_t>(buffer_manager_.number_of_buffers())) On 2015/02/12 22:22:09, bbudge wrote: > Could you use frame_count_ instead of buffer_manager_.number_of_buffers()? > Here and below. Done. https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:416: if (buffer_id < 0 || On 2015/02/12 22:22:09, bbudge wrote: > buffer_id is unsigned Done. https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:442: encoder_last_error_ = PP_ERROR_ABORTED; On 2015/02/12 22:22:09, bbudge wrote: > This error code just seems odd to me here, as it's usually generated on the > resource side before calling back into the plugin (though I see some precedent > in content/renderer/pepper.) Could a generic PP_ERROR_FAILED also work here? Sure. https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:471: output_buffer_size)); On 2015/02/12 22:22:10, bbudge wrote: > You could save a loop by creating the SerializedHandles when you feed them to > the encoder (thus not needing to Close() them if you can't get all the shm > buffers.) Nicer indeed. https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:477: NotifyPepperError(PP_ERROR_FAILED); On 2015/02/12 22:22:09, bbudge wrote: > PP_ERROR_NOMEMORY Done. https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:486: // Notify the plugin of the buffers and the input size. On 2015/02/12 22:22:09, bbudge wrote: > nit: s/input size/encoding parameters > because there's output_buffer_size, frame_count, and size. > nit: s/size/pp_input_coded_size Done. https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:491: encoder_last_error_ = PP_OK; On 2015/02/12 22:22:09, bbudge wrote: > I think this should be set in OnHostMsgInitialize() by my earlier comments about > when the encoder is ready. Done. https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:495: PpapiPluginMsg_VideoEncoder_BitstreamBuffers(output_buffer_size, On 2015/02/12 22:22:09, bbudge wrote: > output_buffer_size is size_t but param is uint32_t. Done. https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:554: static_cast<uint32_t>(buffer_manager_.number_of_buffers())); On 2015/02/12 22:22:09, bbudge wrote: > frame_count_? Done. https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:556: ppapi::MediaStreamBuffer* buffer = buffer_manager_.GetBufferPointer(frame_id); On 2015/02/12 22:22:10, bbudge wrote: > This returns NULL if frame_id is out of range, so you could DCHECK that instead > of your above range check. Done. https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:576: context.params.set_result(encoder_last_error_); On 2015/02/12 22:22:09, bbudge wrote: > EncodeDone is a confusing to me. Could we rename this FrameReleased or something > that indicates the encoder has returned the frame to us? Sure. https://codereview.chromium.org/905023005/diff/80001/content/renderer/pepper/... content/renderer/pepper/pepper_video_encoder_host.cc:585: encoder_ = nullptr; On 2015/02/12 22:22:10, bbudge wrote: > Close() here instead? Done. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... File ppapi/proxy/video_encoder_resource.cc (right): https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:152: if (!WriteVideoFrame(video_frame)) { On 2015/02/12 22:22:10, bbudge wrote: > Suggestion: Move the next two statements to above this 'if' statement and change > WriteVideoFrame to run the callback and clear the data pointer (and it won't > take a parameter.) That will make it easier to do the right thing at the other > two call sites for WriteVideoFrame. Right now, you sometimes leave the pointer > non-null afterwards. Done. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:200: BitstreamBuffer buffer(available_bitstream_buffers_.front()); On 2015/02/12 22:22:10, bbudge wrote: > Could you make this const BitstreamBuffer& instead, and not pop_front() until > after WriteBitstreamBuffer returns? Here and below. If you can, you can also > eliminate BitstreamBuffer's copy constructor. Can't actually do that, because of reentrancy. We want the bitstream buffer out of the available list before we notify the plugin. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:205: } On 2015/02/12 22:22:10, bbudge wrote: > Suggestion: similarly to GetVideoFrame, you could move all of this into > WriteBitstreamBuffer. In other words, always set the get_bitstream_xxx fields, > then check for empty in WriteBitstreamBuffer. > > Probably better names for these functions would be TryWriteBitstreamBuffer and > TryGetVideoFrame. Done. Didn't rename WriteBitstreamBuffer as we're sure to write something. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:275: On 2015/02/12 22:22:10, bbudge wrote: > initialized_ = true; > Here, not below. Done. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:277: if (error) On 2015/02/12 22:22:10, bbudge wrote: > We should always run the callback here, not below. Done. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.cc:415: } On 2015/02/12 22:22:10, bbudge wrote: > Maybe clear video_frames_ for hygiene? Done. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... File ppapi/proxy/video_encoder_resource.h (right): https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource.h:48: // Index of the buffer in the ScopedVector. Buffers have the id in On 2015/02/12 22:22:10, bbudge wrote: > s/id/same id/ Done. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... File ppapi/proxy/video_encoder_resource_unittest.cc (right): https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:31: const uint32_t kDecodeId = 5; On 2015/02/13 01:45:54, bbudge wrote: > I don't think you need these definitions. Done. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:70: const uint32_t kVideoFrameNb = 3; On 2015/02/13 01:45:54, bbudge wrote: > It took me a while to get that Nb is 'Count'. Done. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:73: const PP_Size kSize = {640, 480}; On 2015/02/13 01:45:54, bbudge wrote: > kFrameSize? Done. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:366: bool CheckRequestEncodeingParametersChangeMsg( On 2015/02/13 01:45:53, bbudge wrote: > s/Encodeing/Encoding Done. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:386: bool CheckMsg(ResourceMessageCallParams* params, int id) { On 2015/02/13 01:45:53, bbudge wrote: > Not used Done. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:399: char decode_buffer_[kDecodeBufferSize]; On 2015/02/13 01:45:54, bbudge wrote: > Not used. Done. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:415: std::vector<PP_VideoProfileDescription> profiles; On 2015/02/13 01:45:54, bbudge wrote: > This could just be an array, i.e. PP_VideoProfileDescription profiles[2]; Done. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:520: } On 2015/02/13 01:45:54, bbudge wrote: > As discussed, the Initialize callback should always call back. Things to test > are calling Initialize before previous call completes, and weird enum values > like e.g. casted -1. Done. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:554: SendInitializeReply(params, kBitstreamBufferSize, kVideoFrameNb, size); On 2015/02/13 01:45:54, bbudge wrote: > Again, Initialize completes without getting buffers. Done. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:642: // Unlock the GetVideoFrame callback. On 2015/02/13 01:45:53, bbudge wrote: > // Unblock the GetVideoFrame callback by freeing up a frame. Done. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:655: video_frames[i]); On 2015/02/13 01:45:54, bbudge wrote: > Could you call encoder.Close() instead? Done. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:665: ; On 2015/02/13 01:45:54, bbudge wrote: > empty statement Done. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:732: On 2015/02/13 01:45:54, bbudge wrote: > Test a second call gets INPROGRESS. Done. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:809: // Check an error from the encoder calls back Encode and GetBitstreamBuffer On 2015/02/13 01:45:54, bbudge wrote: > s/calls back/aborts Done. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:853: // Check closing the encoder callbacks back GetVideoFrame and On 2015/02/13 01:45:54, bbudge wrote: > s/callbacks back/aborts Done. https://codereview.chromium.org/905023005/diff/80001/ppapi/proxy/video_encode... ppapi/proxy/video_encoder_resource_unittest.cc:877: // Check closing the encoder calls back Encode and GetBitstreamBuffer On 2015/02/13 01:45:53, bbudge wrote: > s/calls back/aborts Done. https://codereview.chromium.org/905023005/diff/80001/ppapi/tests/test_video_e... File ppapi/tests/test_video_encoder.h (right): https://codereview.chromium.org/905023005/diff/80001/ppapi/tests/test_video_e... ppapi/tests/test_video_encoder.h:29: #endif // PPAPI_TESTS_TEST_VIDEO_ENCODER_H_ On 2015/02/13 01:45:54, bbudge wrote: > These tests won't be run unless you add them in > chrome/test/ppapi/ppapi_browsertest.cc. See that file for examples. This test > should run like video decoder, that is, out of process and NaCl only. Reading this file, it looks like the VideoDecoder is run in process. Am I missing something?
https://codereview.chromium.org/905023005/diff/80001/ppapi/tests/test_video_e... File ppapi/tests/test_video_encoder.h (right): https://codereview.chromium.org/905023005/diff/80001/ppapi/tests/test_video_e... ppapi/tests/test_video_encoder.h:29: #endif // PPAPI_TESTS_TEST_VIDEO_ENCODER_H_ On 2015/02/13 20:12:26, llandwerlin wrote: > On 2015/02/13 01:45:54, bbudge wrote: > > These tests won't be run unless you add them in > > chrome/test/ppapi/ppapi_browsertest.cc. See that file for examples. This test > > should run like video decoder, that is, out of process and NaCl only. > > Reading this file, it looks like the VideoDecoder is run in process. > Am I missing something? OK, just do the same thing as VideoDecoder does. https://codereview.chromium.org/905023005/diff/120001/content/renderer/pepper... File content/renderer/pepper/pepper_video_encoder_host.cc (right): https://codereview.chromium.org/905023005/diff/120001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:315: Close(); Close() should be outside the 'if'. As it stands, if the plugin doesn't specify hardware only, Close isn't called. I think it might be clearer if you reverse the sense of the previous 'if' from if (success) to if (failure). https://codereview.chromium.org/905023005/diff/120001/ppapi/proxy/video_encod... File ppapi/proxy/video_encoder_resource.cc (right): https://codereview.chromium.org/905023005/diff/120001/ppapi/proxy/video_encod... ppapi/proxy/video_encoder_resource.cc:95: return PP_ERROR_INPROGRESS; Returning PP_ERROR_INPROGRESS here is strange, since the GetFramesRequired function doesn't take a callback. I'm sorry for pushing you in this direction. This method should return PP_ERROR_FAILED if Initialize hasn't completed, or if there has been an encoder error, PP_OK otherwise. I think the host should defer the Initialize reply until it has the buffer information, similarly to how you had it before. However, the RequireBitstreamBuffers function should still be callable more than once for future proofing. I don't think this is hard - just add logic in RequireBitstreamBuffers to check if Initialize is pending and send the reply from there, and skip that if initialize has already completed. Same applies to GetFrameCodedSize below. I think there's a potential API problem if someday RequireBitstreamBuffers is called multiple times, in that the values returned by these two functions might change and there is no way to notify the plugin of this event. It seems such a change isn't likely any time soon, so let's not worry about that.
PTAL Your suggestion of have an initialize message with the number/sizes of frames and another one for sending the bitstream buffers looks much better. Thanks. https://codereview.chromium.org/905023005/diff/80001/ppapi/tests/test_video_e... File ppapi/tests/test_video_encoder.h (right): https://codereview.chromium.org/905023005/diff/80001/ppapi/tests/test_video_e... ppapi/tests/test_video_encoder.h:29: #endif // PPAPI_TESTS_TEST_VIDEO_ENCODER_H_ On 2015/02/17 18:38:04, bbudge wrote: > On 2015/02/13 20:12:26, llandwerlin wrote: > > On 2015/02/13 01:45:54, bbudge wrote: > > > These tests won't be run unless you add them in > > > chrome/test/ppapi/ppapi_browsertest.cc. See that file for examples. This > test > > > should run like video decoder, that is, out of process and NaCl only. > > > > Reading this file, it looks like the VideoDecoder is run in process. > > Am I missing something? > > OK, just do the same thing as VideoDecoder does. Done. https://codereview.chromium.org/905023005/diff/120001/content/renderer/pepper... File content/renderer/pepper/pepper_video_encoder_host.cc (right): https://codereview.chromium.org/905023005/diff/120001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:315: Close(); On 2015/02/17 18:38:04, bbudge wrote: > Close() should be outside the 'if'. As it stands, if the plugin doesn't specify > hardware only, Close isn't called. > I think it might be clearer if you reverse the sense of the previous 'if' from > if (success) to if (failure). Done. https://codereview.chromium.org/905023005/diff/120001/ppapi/proxy/video_encod... File ppapi/proxy/video_encoder_resource.cc (right): https://codereview.chromium.org/905023005/diff/120001/ppapi/proxy/video_encod... ppapi/proxy/video_encoder_resource.cc:95: return PP_ERROR_INPROGRESS; On 2015/02/17 18:38:04, bbudge wrote: > Returning PP_ERROR_INPROGRESS here is strange, since the GetFramesRequired > function doesn't take a callback. I'm sorry for pushing you in this direction. > This method should return PP_ERROR_FAILED if Initialize hasn't completed, or if > there has been an encoder error, PP_OK otherwise. > > I think the host should defer the Initialize reply until it has the buffer > information, similarly to how you had it before. However, the > RequireBitstreamBuffers function should still be callable more than once for > future proofing. I don't think this is hard - just add logic in > RequireBitstreamBuffers to check if Initialize is pending and send the reply > from there, and skip that if initialize has already completed. > > Same applies to GetFrameCodedSize below. > > I think there's a potential API problem if someday RequireBitstreamBuffers is > called multiple times, in that the values returned by these two functions might > change and there is no way to notify the plugin of this event. It seems such a > change isn't likely any time soon, so let's not worry about that. Done.
https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... File content/renderer/pepper/pepper_video_encoder_host.cc (right): https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:177: // No default case, to catch unhandled PP_HardwareAcceleration values. nit:indent https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:187: DCHECK(shm); I think shm here refers to the parameter. DCHECK(this->shm)? https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:294: return PP_ERROR_FAILED; We should Close() here before returning. It might be helpful to split this code into an Initialize or InitializeHardware method that returns an error code. Then you can Close() and return that code if it's not PP_OK. https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:301: return PP_ERROR_FAILED; Close() here too. https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:332: get_video_frames_context_ = ppapi::host::ReplyMessageContext(); This should happen in AllocateVideoFrames. https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:395: DCHECK(RenderThreadImpl::current()); DCHECK(!initialized_) so we find out if/when RequireBitstreamBuffers gets called more than once. This deserves a comment so anyone who breaks it knows why, e.g. // We assume RequireBitstreamBuffers is only called once. https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:408: } It seems to me that the frame count and input coded size should be sent with the handles. If someday RequireBitstreamBuffers is called multiple times, we should send all of these together each time. So can we do all the work to create the shared memory, and send the unsolicited PpapiPluginMsg_VideoEncoder_BitstreamBuffers reply first, followed by the Initialize reply and finally any GetVideoFrames reply? Comments on how to do this below... https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:427: } Do this check after replying to Initialize. https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:443: Initialization reply PP_OK here, then test for handles: 424 if (shm_buffers_.empty()) { 425 NotifyPepperError(PP_ERROR_NOMEMORY); 426 return; 427 } https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:508: 0, 0, PP_MakeSize(0, 0))); This code is repeated 4 times in this method. I think a SendGetFramesErrorReply method is in order. Something like this: void PepperVideoEncoderHost::SendGetFramesErrorReply(int32_t error) { DCHECK(get_video_frames_context_.is_valid()); get_video_frames_context_.params.set_result(error); host()->SendReply(get_video_frames_context_, PpapiPluginMsg_VideoEncoder_GetVideoFramesReply( 0, 0, PP_MakeSize(0, 0))); get_video_frames_context_ = ppapi::host::ReplyMessageContext(); } https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:575: PP_FromGfxSize(input_coded_size_))); Clear the reply context here: get_video_frames_context_ = ppapi::host::ReplyMessageContext(); https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:581: DCHECK_LT(frame_id, frame_count_); nit: This check is repeated by MediaStrreamBufferManager, so your second DCHECK is sufficient. https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... File content/renderer/pepper/pepper_video_encoder_host.h (right): https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.h:12: #include "gpu/command_buffer/common/mailbox_holder.h" This doesn't appear to get used. https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.h:30: class RenderViewImpl; Not used. https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.h:49: I think it would be helpful to repeat the comment in the resource: // Index of the buffer in the ScopedVector. Buffers have the same id in // the plugin and the host. https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.h:122: ppapi::host::ReplyMessageContext initialize_context_; nit: s/initialize_context_/initialize_reply_context_ is more consistent with other host impls. Also, get_video_frames_reply_context_ below. https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.h:130: // on the encoder should proceed or fail. Could we reword this a bit? e.g. // This represents the current error state of the encoder, i.e. PP_OK // normally, or a Pepper error code if the encoder is uninitialized, // has been notified of an encoder error, has encountered some // other unrecoverable error, or has been closed by the plugin. // This field is checked in most message handlers to decide whether // operations should proceed or fail. https://codereview.chromium.org/905023005/diff/140001/ppapi/proxy/video_encod... File ppapi/proxy/video_encoder_resource.cc (right): https://codereview.chromium.org/905023005/diff/140001/ppapi/proxy/video_encod... ppapi/proxy/video_encoder_resource.cc:302: TryWriteVideoFrame(); Shouldn't we check the callback here? i.e. if (TrackedCallback::IsPending(get_video_frame_callback_)) The plugin could have called Close() after calling GetVideoFrame. https://codereview.chromium.org/905023005/diff/140001/ppapi/proxy/video_encod... ppapi/proxy/video_encoder_resource.cc:311: scoped_refptr<TrackedCallback> callback = encode_callbacks_.front(); Do we know that the encoder releases video frames in the order in which they're submitted via Encode()? If that's not true, then we may call the plugin's callbacks in the wrong order here. In that case, we should use a map instead of a queue, with the frame_id being the key. https://codereview.chromium.org/905023005/diff/140001/ppapi/proxy/video_encod... File ppapi/proxy/video_encoder_resource.h (right): https://codereview.chromium.org/905023005/diff/140001/ppapi/proxy/video_encod... ppapi/proxy/video_encoder_resource.h:126: int32_t encoder_last_error_; Need a comment similar to the one in the host about this member.
https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... File content/renderer/pepper/pepper_video_encoder_host.cc (right): https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:177: // No default case, to catch unhandled PP_HardwareAcceleration values. On 2015/02/18 22:53:00, bbudge wrote: > nit:indent Done. https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:187: DCHECK(shm); On 2015/02/18 22:53:00, bbudge wrote: > I think shm here refers to the parameter. > DCHECK(this->shm)? Done. https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:294: return PP_ERROR_FAILED; On 2015/02/18 22:53:01, bbudge wrote: > We should Close() here before returning. It might be helpful to split this code > into an Initialize or InitializeHardware method that returns an error code. Then > you can Close() and return that code if it's not PP_OK. Done. https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:301: return PP_ERROR_FAILED; On 2015/02/18 22:53:00, bbudge wrote: > Close() here too. Done. https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:332: get_video_frames_context_ = ppapi::host::ReplyMessageContext(); On 2015/02/18 22:53:00, bbudge wrote: > This should happen in AllocateVideoFrames. Done. https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:395: DCHECK(RenderThreadImpl::current()); On 2015/02/18 22:53:00, bbudge wrote: > DCHECK(!initialized_) so we find out if/when RequireBitstreamBuffers gets called > more than once. This deserves a comment so anyone who breaks it knows why, e.g. > // We assume RequireBitstreamBuffers is only called once. Done. https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:408: } On 2015/02/18 22:53:00, bbudge wrote: > It seems to me that the frame count and input coded size should be sent with the > handles. If someday RequireBitstreamBuffers is called multiple times, we should > send all of these together each time. So can we do all the work to create the > shared memory, and send the unsolicited > PpapiPluginMsg_VideoEncoder_BitstreamBuffers reply first, followed by the > Initialize reply and finally any GetVideoFrames reply? Comments on how to do > this below... It seems to me that input coded size is a hardware constraint computed using the input visible size given at initialization. In other words it shouldn't change for a given set of initialization parameters. I believe this is the same thing for frame_count which is a number of made of the maximum number of reference frame the hardware needs to work with at point in time. Based on given parameters passed at Initialize(), this shouldn't change either. The output buffer size and number is the only thing that could actually change, based on the bitrate (which is the only parameter we change with the current VEA api without creating a new encoder). I could add checks to trigger an error in case this parameters changes. https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:427: } On 2015/02/18 22:53:01, bbudge wrote: > Do this check after replying to Initialize. Done. https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:443: On 2015/02/18 22:53:00, bbudge wrote: > Initialization reply PP_OK here, then test for handles: > > 424 if (shm_buffers_.empty()) { > 425 NotifyPepperError(PP_ERROR_NOMEMORY); > 426 return; > 427 } Done. https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:508: 0, 0, PP_MakeSize(0, 0))); On 2015/02/18 22:53:00, bbudge wrote: > This code is repeated 4 times in this method. I think a SendGetFramesErrorReply > method is in order. Something like this: > > void PepperVideoEncoderHost::SendGetFramesErrorReply(int32_t error) { > DCHECK(get_video_frames_context_.is_valid()); > get_video_frames_context_.params.set_result(error); > host()->SendReply(get_video_frames_context_, > PpapiPluginMsg_VideoEncoder_GetVideoFramesReply( > 0, 0, PP_MakeSize(0, 0))); > get_video_frames_context_ = ppapi::host::ReplyMessageContext(); > } Done. https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:575: PP_FromGfxSize(input_coded_size_))); On 2015/02/18 22:53:00, bbudge wrote: > Clear the reply context here: > get_video_frames_context_ = ppapi::host::ReplyMessageContext(); Done. https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:581: DCHECK_LT(frame_id, frame_count_); On 2015/02/18 22:53:01, bbudge wrote: > nit: This check is repeated by MediaStrreamBufferManager, so your second DCHECK > is sufficient. Done. https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... File content/renderer/pepper/pepper_video_encoder_host.h (right): https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.h:12: #include "gpu/command_buffer/common/mailbox_holder.h" On 2015/02/18 22:53:01, bbudge wrote: > This doesn't appear to get used. Done. https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.h:30: class RenderViewImpl; On 2015/02/18 22:53:01, bbudge wrote: > Not used. Done. https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.h:49: On 2015/02/18 22:53:01, bbudge wrote: > I think it would be helpful to repeat the comment in the resource: > // Index of the buffer in the ScopedVector. Buffers have the same id in > // the plugin and the host. Done. https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.h:122: ppapi::host::ReplyMessageContext initialize_context_; On 2015/02/18 22:53:01, bbudge wrote: > nit: s/initialize_context_/initialize_reply_context_ is more consistent with > other host impls. > Also, get_video_frames_reply_context_ below. Done. https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.h:130: // on the encoder should proceed or fail. On 2015/02/18 22:53:01, bbudge wrote: > Could we reword this a bit? e.g. > > // This represents the current error state of the encoder, i.e. PP_OK > // normally, or a Pepper error code if the encoder is uninitialized, > // has been notified of an encoder error, has encountered some > // other unrecoverable error, or has been closed by the plugin. > // This field is checked in most message handlers to decide whether > // operations should proceed or fail. Done. https://codereview.chromium.org/905023005/diff/140001/ppapi/proxy/video_encod... File ppapi/proxy/video_encoder_resource.cc (right): https://codereview.chromium.org/905023005/diff/140001/ppapi/proxy/video_encod... ppapi/proxy/video_encoder_resource.cc:302: TryWriteVideoFrame(); On 2015/02/18 22:53:01, bbudge wrote: > Shouldn't we check the callback here? i.e. > if (TrackedCallback::IsPending(get_video_frame_callback_)) > > The plugin could have called Close() after calling GetVideoFrame. I will add the check, but given this is done lazily, it means we at least have a request from the plugin. https://codereview.chromium.org/905023005/diff/140001/ppapi/proxy/video_encod... ppapi/proxy/video_encoder_resource.cc:311: scoped_refptr<TrackedCallback> callback = encode_callbacks_.front(); On 2015/02/18 22:53:01, bbudge wrote: > Do we know that the encoder releases video frames in the order in which they're > submitted via Encode()? If that's not true, then we may call the plugin's > callbacks in the wrong order here. In that case, we should use a map instead of > a queue, with the frame_id being the key. The current implementations release the frame in order. I can't imagine a different way. Switching to a map anyway.
Really close now. https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... File content/renderer/pepper/pepper_video_encoder_host.cc (right): https://codereview.chromium.org/905023005/diff/140001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:408: } On 2015/02/19 15:55:45, llandwerlin wrote: > On 2015/02/18 22:53:00, bbudge wrote: > > It seems to me that the frame count and input coded size should be sent with > the > > handles. If someday RequireBitstreamBuffers is called multiple times, we > should > > send all of these together each time. So can we do all the work to create the > > shared memory, and send the unsolicited > > PpapiPluginMsg_VideoEncoder_BitstreamBuffers reply first, followed by the > > Initialize reply and finally any GetVideoFrames reply? Comments on how to do > > this below... > > It seems to me that input coded size is a hardware constraint computed using the > input visible size given at initialization. In other words it shouldn't change > for a given set of initialization parameters. > > I believe this is the same thing for frame_count which is a number of made of > the maximum number of reference frame the hardware needs to work with at point > in time. Based on given parameters passed at Initialize(), this shouldn't change > either. > > The output buffer size and number is the only thing that could actually change, > based on the bitrate (which is the only parameter we change with the current VEA > api without creating a new encoder). > > I could add checks to trigger an error in case this parameters changes. I agree that RequireBitstreamBuffers is an odd part of the VEA Client API, since those values should never change. On further thought, I think it's fine that you return these in the Initialize reply message. The plugin shouldn't really be concerned about bitstream buffer sizes, as we manage these internally. https://codereview.chromium.org/905023005/diff/140001/ppapi/proxy/video_encod... File ppapi/proxy/video_encoder_resource.cc (right): https://codereview.chromium.org/905023005/diff/140001/ppapi/proxy/video_encod... ppapi/proxy/video_encoder_resource.cc:302: TryWriteVideoFrame(); On 2015/02/19 15:55:45, llandwerlin wrote: > On 2015/02/18 22:53:01, bbudge wrote: > > Shouldn't we check the callback here? i.e. > > if (TrackedCallback::IsPending(get_video_frame_callback_)) > > > > The plugin could have called Close() after calling GetVideoFrame. > > I will add the check, but given this is done lazily, it means we at least have a > request from the plugin. The plugin could call GetVideoFrame and immediately call Close() before we get here. In that case I think the callback would have been aborted already. https://codereview.chromium.org/905023005/diff/140001/ppapi/proxy/video_encod... ppapi/proxy/video_encoder_resource.cc:311: scoped_refptr<TrackedCallback> callback = encode_callbacks_.front(); On 2015/02/19 15:55:45, llandwerlin wrote: > On 2015/02/18 22:53:01, bbudge wrote: > > Do we know that the encoder releases video frames in the order in which > they're > > submitted via Encode()? If that's not true, then we may call the plugin's > > callbacks in the wrong order here. In that case, we should use a map instead > of > > a queue, with the frame_id being the key. > > The current implementations release the frame in order. I can't imagine a > different way. > Switching to a map anyway. I wasn't sure about this. If it ever wasn't true, the bug would be subtle and hard to diagnose. https://codereview.chromium.org/905023005/diff/160001/content/renderer/pepper... File content/renderer/pepper/pepper_video_encoder_host.cc (right): https://codereview.chromium.org/905023005/diff/160001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:307: // the context and answer when we're ready. This comment is confusing to me. The test implies that we *have* gotten the frames and *can* allocate the frames. https://codereview.chromium.org/905023005/diff/160001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:422: // If the plugin already ask for video frames, we can now answer nit: s/ask for/requested https://codereview.chromium.org/905023005/diff/160001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:487: if (!channel_.get()) nit: get() isn't necessary. https://codereview.chromium.org/905023005/diff/160001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:500: weak_ptr_factory_.GetWeakPtr(), PP_ERROR_FAILED))); nit: PP_ERROR_RESOURCE_FAILED might better indicate that this was a platform failure and the plugin did nothing wrong. https://codereview.chromium.org/905023005/diff/160001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:517: return; nit: Perhaps a NOTREACHED() before return to indicate this shouldn't normally happen. https://codereview.chromium.org/905023005/diff/160001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:528: base::CheckedNumeric<uint32_t> buffer_size_aligned = size.ValueOrDie(); This can just be uint32_t, since we do no further calculations with it, right? https://codereview.chromium.org/905023005/diff/160001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:530: base::CheckedNumeric<uint32_t> total_size = size.ValueOrDie(); This can also simply be uint32_t. If you want to keep the IsValid check below, it could be done just before taking this value: size *= frame_count_; it (!size.IsValid()) ... https://codereview.chromium.org/905023005/diff/160001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:565: nit: DCHECK(get_video_frames_reply_context_.is_valid()); https://codereview.chromium.org/905023005/diff/160001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:613: host()->SendReply(reply_context, This needs to be context, your local copy. Do you even need to worry about returning an error result here? If the encoder has been closed or otherwise failed, the callback on the resource side will have been aborted. https://codereview.chromium.org/905023005/diff/160001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:627: void PepperVideoEncoderHost::Close() { This seems to belong together with InitializeHardware (and someday InitializeSoftware). Could you move it up (and change the header to stay in the same order?) https://codereview.chromium.org/905023005/diff/160001/ppapi/proxy/ppapi_messa... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/905023005/diff/160001/ppapi/proxy/ppapi_messa... ppapi/proxy/ppapi_messages.h:2023: IPC_MESSAGE_CONTROL0(PpapiPluginMsg_VideoEncoder_CloseReply) This message isn't needed any more. https://codereview.chromium.org/905023005/diff/160001/ppapi/proxy/video_encod... File ppapi/proxy/video_encoder_resource.cc (right): https://codereview.chromium.org/905023005/diff/160001/ppapi/proxy/video_encod... ppapi/proxy/video_encoder_resource.cc:303: DCHECK(TrackedCallback::IsPending(get_video_frame_callback_)); I think this needs to be 'if' DCHECK in TryWriteVideoFrame? https://codereview.chromium.org/905023005/diff/160001/ppapi/proxy/video_encod... ppapi/proxy/video_encoder_resource.cc:332: params.TakeAllSharedMemoryHandles(&shm_handles); You might consider testing for 0 and considering that a platform error as there was a failure to allocate shared memory. https://codereview.chromium.org/905023005/diff/160001/ppapi/proxy/video_encod... ppapi/proxy/video_encoder_resource.cc:373: get_bitstream_buffer_data_ = nullptr; get_video_frame_data_ = nullptr;
Thanks for your time on the review. https://codereview.chromium.org/905023005/diff/140001/ppapi/proxy/video_encod... File ppapi/proxy/video_encoder_resource.cc (right): https://codereview.chromium.org/905023005/diff/140001/ppapi/proxy/video_encod... ppapi/proxy/video_encoder_resource.cc:302: TryWriteVideoFrame(); On 2015/02/19 20:30:54, bbudge wrote: > On 2015/02/19 15:55:45, llandwerlin wrote: > > On 2015/02/18 22:53:01, bbudge wrote: > > > Shouldn't we check the callback here? i.e. > > > if (TrackedCallback::IsPending(get_video_frame_callback_)) > > > > > > The plugin could have called Close() after calling GetVideoFrame. > > > > I will add the check, but given this is done lazily, it means we at least have > a > > request from the plugin. > > The plugin could call GetVideoFrame and immediately call Close() before we get > here. In that case I think the callback would have been aborted already. Ah fair. Thanks! https://codereview.chromium.org/905023005/diff/140001/ppapi/proxy/video_encod... ppapi/proxy/video_encoder_resource.cc:311: scoped_refptr<TrackedCallback> callback = encode_callbacks_.front(); On 2015/02/19 20:30:54, bbudge wrote: > On 2015/02/19 15:55:45, llandwerlin wrote: > > On 2015/02/18 22:53:01, bbudge wrote: > > > Do we know that the encoder releases video frames in the order in which > > they're > > > submitted via Encode()? If that's not true, then we may call the plugin's > > > callbacks in the wrong order here. In that case, we should use a map instead > > of > > > a queue, with the frame_id being the key. > > > > The current implementations release the frame in order. I can't imagine a > > different way. > > Switching to a map anyway. > > I wasn't sure about this. If it ever wasn't true, the bug would be subtle and > hard to diagnose. Acknowledged. https://codereview.chromium.org/905023005/diff/160001/content/renderer/pepper... File content/renderer/pepper/pepper_video_encoder_host.cc (right): https://codereview.chromium.org/905023005/diff/160001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:307: // the context and answer when we're ready. On 2015/02/19 20:30:55, bbudge wrote: > This comment is confusing to me. The test implies that we *have* gotten the > frames and *can* allocate the frames. Reformulating : If the encoder is not initialized yet, we need to wait for that to happen. Until then, we don't know what is the allocate frames at the right size. With the previous changes on the initialization sequence, I don't think this test is required now, as test on encoder_last_error_ will still fail (we only position encoder_last_error_ to PP_OK once the bitstream buffers have been required, after the initialization has succeeded). https://codereview.chromium.org/905023005/diff/160001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:422: // If the plugin already ask for video frames, we can now answer On 2015/02/19 20:30:55, bbudge wrote: > nit: s/ask for/requested Done. https://codereview.chromium.org/905023005/diff/160001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:487: if (!channel_.get()) On 2015/02/19 20:30:55, bbudge wrote: > nit: get() isn't necessary. Done. https://codereview.chromium.org/905023005/diff/160001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:500: weak_ptr_factory_.GetWeakPtr(), PP_ERROR_FAILED))); On 2015/02/19 20:30:55, bbudge wrote: > nit: PP_ERROR_RESOURCE_FAILED might better indicate that this was a platform > failure and the plugin did nothing wrong. Done. https://codereview.chromium.org/905023005/diff/160001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:517: return; On 2015/02/19 20:30:55, bbudge wrote: > nit: Perhaps a NOTREACHED() before return to indicate this shouldn't normally > happen. I was actually wondering whether we should send the same buffers again. What do you think? https://codereview.chromium.org/905023005/diff/160001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:528: base::CheckedNumeric<uint32_t> buffer_size_aligned = size.ValueOrDie(); On 2015/02/19 20:30:55, bbudge wrote: > This can just be uint32_t, since we do no further calculations with it, right? Yes, Done. https://codereview.chromium.org/905023005/diff/160001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:530: base::CheckedNumeric<uint32_t> total_size = size.ValueOrDie(); On 2015/02/19 20:30:55, bbudge wrote: > This can also simply be uint32_t. If you want to keep the IsValid check below, > it could be done just before taking this value: > > size *= frame_count_; > it (!size.IsValid()) ... Done. https://codereview.chromium.org/905023005/diff/160001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:565: On 2015/02/19 20:30:55, bbudge wrote: > nit: DCHECK(get_video_frames_reply_context_.is_valid()); Done. https://codereview.chromium.org/905023005/diff/160001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:613: host()->SendReply(reply_context, On 2015/02/19 20:30:55, bbudge wrote: > This needs to be context, your local copy. > Do you even need to worry about returning an error result here? If the encoder > has been closed or otherwise failed, the callback on the resource side will have > been aborted. Oh thanks! I wasn't sure about the policy regarding this case. Should we avoid sending messages when we know the plugin will not care or is already gone? https://codereview.chromium.org/905023005/diff/160001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:627: void PepperVideoEncoderHost::Close() { On 2015/02/19 20:30:55, bbudge wrote: > This seems to belong together with InitializeHardware (and someday > InitializeSoftware). Could you move it up (and change the header to stay in the > same order?) Done. https://codereview.chromium.org/905023005/diff/160001/ppapi/proxy/ppapi_messa... File ppapi/proxy/ppapi_messages.h (right): https://codereview.chromium.org/905023005/diff/160001/ppapi/proxy/ppapi_messa... ppapi/proxy/ppapi_messages.h:2023: IPC_MESSAGE_CONTROL0(PpapiPluginMsg_VideoEncoder_CloseReply) On 2015/02/19 20:30:55, bbudge wrote: > This message isn't needed any more. Done. https://codereview.chromium.org/905023005/diff/160001/ppapi/proxy/video_encod... File ppapi/proxy/video_encoder_resource.cc (right): https://codereview.chromium.org/905023005/diff/160001/ppapi/proxy/video_encod... ppapi/proxy/video_encoder_resource.cc:303: DCHECK(TrackedCallback::IsPending(get_video_frame_callback_)); On 2015/02/19 20:30:55, bbudge wrote: > I think this needs to be 'if' > DCHECK in TryWriteVideoFrame? Done. https://codereview.chromium.org/905023005/diff/160001/ppapi/proxy/video_encod... ppapi/proxy/video_encoder_resource.cc:332: params.TakeAllSharedMemoryHandles(&shm_handles); On 2015/02/19 20:30:55, bbudge wrote: > You might consider testing for 0 and considering that a platform error as there > was a failure to allocate shared memory. Done. https://codereview.chromium.org/905023005/diff/160001/ppapi/proxy/video_encod... ppapi/proxy/video_encoder_resource.cc:373: get_bitstream_buffer_data_ = nullptr; On 2015/02/19 20:30:55, bbudge wrote: > get_video_frame_data_ = nullptr; Thanks, done.
A few minor things, otherwise LGTM https://codereview.chromium.org/905023005/diff/160001/content/renderer/pepper... File content/renderer/pepper/pepper_video_encoder_host.cc (right): https://codereview.chromium.org/905023005/diff/160001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:517: return; On 2015/02/20 12:19:16, llandwerlin wrote: > On 2015/02/19 20:30:55, bbudge wrote: > > nit: Perhaps a NOTREACHED() before return to indicate this shouldn't normally > > happen. > > I was actually wondering whether we should send the same buffers again. What do > you think? I recommend doing the simplest thing, which is failing. Otherwise, you have to make sure the old handle (created whenever we share a handle across processes) is properly released, which will probably be tricky since the plugin may hold some of the video frames. https://codereview.chromium.org/905023005/diff/160001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:613: host()->SendReply(reply_context, On 2015/02/20 12:19:16, llandwerlin wrote: > On 2015/02/19 20:30:55, bbudge wrote: > > This needs to be context, your local copy. > > Do you even need to worry about returning an error result here? If the encoder > > has been closed or otherwise failed, the callback on the resource side will > have > > been aborted. > > Oh thanks! > I wasn't sure about the policy regarding this case. > Should we avoid sending messages when we know the plugin will not care or is > already gone? I don't think there's a real policy, but in general I would do whatever is simplest and assumes the least about the state on the other side. https://codereview.chromium.org/905023005/diff/180001/content/renderer/pepper... File content/renderer/pepper/pepper_video_encoder_host.cc (right): https://codereview.chromium.org/905023005/diff/180001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:316: if (frame_id >= frame_count_) In theory a malicious plugin could try to get us to encode a frame that is already at the encoder. Since these are just raw images, I don't there's any concern. https://codereview.chromium.org/905023005/diff/180001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:539: if (!size.IsValid()) { Since we didn't 'Die' above, size must be valid, so you don't have to check. I was thinking you would put this above the previous line, but on further thought, this is to protect against attack and we don't owe a nice error message to an attacker. https://codereview.chromium.org/905023005/diff/180001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:560: } If you remove the first SendGetFramesErrorReply, then you could also combine the above two tests into a single 'if': if (!shm || !buffer_manager_.SetBuffers...) in which case you could just inline the body of SendGetFramesErrorReply into that. Also, it seems like the VLOG belongs after everything has succeeded. https://codereview.chromium.org/905023005/diff/180001/content/renderer/pepper... File content/renderer/pepper/pepper_video_encoder_host.h (right): https://codereview.chromium.org/905023005/diff/180001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.h:126: // Saved context to answer a Initialize message from the plugin. super nit: s/a/an
https://codereview.chromium.org/905023005/diff/160001/content/renderer/pepper... File content/renderer/pepper/pepper_video_encoder_host.cc (right): https://codereview.chromium.org/905023005/diff/160001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:517: return; On 2015/02/20 15:56:13, bbudge wrote: > On 2015/02/20 12:19:16, llandwerlin wrote: > > On 2015/02/19 20:30:55, bbudge wrote: > > > nit: Perhaps a NOTREACHED() before return to indicate this shouldn't > normally > > > happen. > > > > I was actually wondering whether we should send the same buffers again. What > do > > you think? > > I recommend doing the simplest thing, which is failing. Otherwise, you have to > make sure the old handle (created whenever we share a handle across processes) > is properly released, which will probably be tricky since the plugin may hold > some of the video frames. Acknowledged. https://codereview.chromium.org/905023005/diff/180001/content/renderer/pepper... File content/renderer/pepper/pepper_video_encoder_host.cc (right): https://codereview.chromium.org/905023005/diff/180001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:539: if (!size.IsValid()) { On 2015/02/20 15:56:13, bbudge wrote: > Since we didn't 'Die' above, size must be valid, so you don't have to check. I > was thinking you would put this above the previous line, but on further thought, > this is to protect against attack and we don't owe a nice error message to an > attacker. Done. https://codereview.chromium.org/905023005/diff/180001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:560: } On 2015/02/20 15:56:13, bbudge wrote: > If you remove the first SendGetFramesErrorReply, then you could also combine the > above two tests into a single 'if': > > if (!shm || !buffer_manager_.SetBuffers...) > > in which case you could just inline the body of SendGetFramesErrorReply into > that. Also, it seems like the VLOG belongs after everything has succeeded. Should I drop the very first SendGetFramesErrorReply() then? (just before NOTREACHED) https://codereview.chromium.org/905023005/diff/180001/content/renderer/pepper... File content/renderer/pepper/pepper_video_encoder_host.h (right): https://codereview.chromium.org/905023005/diff/180001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.h:126: // Saved context to answer a Initialize message from the plugin. On 2015/02/20 15:56:13, bbudge wrote: > super nit: s/a/an Done.
New patchsets have been uploaded after l-g-t-m from bbudge@chromium.org
@wfh: Could review the ppapi_messages.h file? Thanks
bbudge@chromium.org changed reviewers: + tsepez@chromium.org
+tsepez for IPC review
Messages themselves are OK, but one question and some nits. Thanks. https://codereview.chromium.org/905023005/diff/200001/content/renderer/pepper... File content/renderer/pepper/pepper_video_encoder_host.h (right): https://codereview.chromium.org/905023005/diff/200001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.h:43: ShmBuffer(int32_t id, scoped_ptr<base::SharedMemory> shm); nit: id should probably be uint32_t or some other unsigned type. I was confused as there is also a similarly-namedShmBuffer struct in video_encoder_resource.h which already has a uint32_t id. https://codereview.chromium.org/905023005/diff/200001/ppapi/proxy/video_encod... File ppapi/proxy/video_encoder_resource.cc (right): https://codereview.chromium.org/905023005/diff/200001/ppapi/proxy/video_encod... ppapi/proxy/video_encoder_resource.cc:315: DCHECK(encode_callbacks_.end() != it); Need a stronger check here than a DCHECK, right? Or is the video_frame coming directly from the browser process and is trustworthy? (I'm not sure which way this message is going). https://codereview.chromium.org/905023005/diff/200001/ppapi/proxy/video_encod... File ppapi/proxy/video_encoder_resource_unittest.cc (right): https://codereview.chromium.org/905023005/diff/200001/ppapi/proxy/video_encod... ppapi/proxy/video_encoder_resource_unittest.cc:313: if (!sink().GetFirstResourceCallMatching( nit: wouldn't this be just return sink().GetFirstResourceCallMatching(...);
On 2015/02/20 16:28:04, llandwerlin wrote: > @wfh: Could review the ppapi_messages.h file? Thanks Yup, was waiting for the interfaces to stabalize. Sounds like tsepez has you covered though.
Whoops @wfh, I didn't realize you were already on for IPC review. https://codereview.chromium.org/905023005/diff/200001/ppapi/proxy/video_encod... File ppapi/proxy/video_encoder_resource.cc (right): https://codereview.chromium.org/905023005/diff/200001/ppapi/proxy/video_encod... ppapi/proxy/video_encoder_resource.cc:315: DCHECK(encode_callbacks_.end() != it); On 2015/02/20 17:48:02, Tom Sepez wrote: > Need a stronger check here than a DCHECK, right? Or is the video_frame coming > directly from the browser process and is trustworthy? (I'm not sure which way > this message is going). It's coming from the renderer, and this is in the plugin process.
https://codereview.chromium.org/905023005/diff/200001/content/renderer/pepper... File content/renderer/pepper/pepper_video_encoder_host.h (right): https://codereview.chromium.org/905023005/diff/200001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.h:43: ShmBuffer(int32_t id, scoped_ptr<base::SharedMemory> shm); On 2015/02/20 17:48:02, Tom Sepez wrote: > nit: id should probably be uint32_t or some other unsigned type. I was confused > as there is also a similarly-namedShmBuffer struct in video_encoder_resource.h > which already has a uint32_t id. Done. https://codereview.chromium.org/905023005/diff/200001/ppapi/proxy/video_encod... File ppapi/proxy/video_encoder_resource.cc (right): https://codereview.chromium.org/905023005/diff/200001/ppapi/proxy/video_encod... ppapi/proxy/video_encoder_resource.cc:315: DCHECK(encode_callbacks_.end() != it); On 2015/02/20 17:48:02, Tom Sepez wrote: > Need a stronger check here than a DCHECK, right? Or is the video_frame coming > directly from the browser process and is trustworthy? (I'm not sure which way > this message is going). It's coming from PepperVideoEncoderHost (renderer process) from the browser indeed. If that's broken we've got bigger problems that reporting a correct error to the plugin, right? :) https://codereview.chromium.org/905023005/diff/200001/ppapi/proxy/video_encod... File ppapi/proxy/video_encoder_resource_unittest.cc (right): https://codereview.chromium.org/905023005/diff/200001/ppapi/proxy/video_encod... ppapi/proxy/video_encoder_resource_unittest.cc:313: if (!sink().GetFirstResourceCallMatching( On 2015/02/20 17:48:02, Tom Sepez wrote: > nit: wouldn't this be just > return sink().GetFirstResourceCallMatching(...); Done.
tsepez@: Are you reviewing the CL more in depth?
On 2015/02/21 00:58:10, llandwerlin wrote: > tsepez@: Are you reviewing the CL more in depth? No, just needed an answer for that one question. LGTM.
The CQ bit was checked by lionel.g.landwerlin@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/905023005/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
New patchsets have been uploaded after l-g-t-m from tsepez@chromium.org
Fix unittest on Windows
bbudge@, I think this last upload should be mostly green (apart from the ios_dbg_simulator). If you could have another look at the host and proxy code. Some code had to be changed for the Windows compiler. Also if you think we need graphics people to look at the gpu communication part of the CL, maybe some of code could be factorized somewhere into content/common/gpu/client? Thanks for your time!
bbudge@chromium.org changed reviewers: + piman@chromium.org
+piman for GpuChannelHost / CommandBufferProxyImpl creation and lifetime in: content/renderer/pepper/video_encoder_host.cc Antoine, feel free to reroute. https://codereview.chromium.org/905023005/diff/360001/content/renderer/pepper... File content/renderer/pepper/pepper_video_encoder_host.cc (right): https://codereview.chromium.org/905023005/diff/360001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:476: bool PepperVideoEncoderHost::EnsureGpuChannel() { Shouldn't we check if the channel has already been set up before allocating a new one? https://codereview.chromium.org/905023005/diff/360001/ppapi/proxy/video_encod... File ppapi/proxy/video_encoder_resource.cc (right): https://codereview.chromium.org/905023005/diff/360001/ppapi/proxy/video_encod... ppapi/proxy/video_encoder_resource.cc:254: base::checked_cast<uint32_t>(sizeof(PP_VideoProfileDescription))); static_cast is fine here, since the input is a compile time constant which we know will not overflow.
https://codereview.chromium.org/905023005/diff/360001/content/renderer/pepper... File content/renderer/pepper/pepper_video_encoder_host.cc (right): https://codereview.chromium.org/905023005/diff/360001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:476: bool PepperVideoEncoderHost::EnsureGpuChannel() { On 2015/02/23 23:41:31, bbudge wrote: > Shouldn't we check if the channel has already been set up before allocating a > new one? Yes, thanks. https://codereview.chromium.org/905023005/diff/360001/ppapi/proxy/video_encod... File ppapi/proxy/video_encoder_resource.cc (right): https://codereview.chromium.org/905023005/diff/360001/ppapi/proxy/video_encod... ppapi/proxy/video_encoder_resource.cc:254: base::checked_cast<uint32_t>(sizeof(PP_VideoProfileDescription))); On 2015/02/23 23:41:31, bbudge wrote: > static_cast is fine here, since the input is a compile time constant which we > know will not overflow. Weird, that's how it was initially, but that doesn't pass the win_chromium_x64_rel_ng bot : http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...
https://codereview.chromium.org/905023005/diff/360001/content/renderer/pepper... File content/renderer/pepper/pepper_video_encoder_host.cc (right): https://codereview.chromium.org/905023005/diff/360001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:451: for (media::VideoEncodeAccelerator::SupportedProfile profile : profiles) nit: need {} by style guide because body > 1 line. https://codereview.chromium.org/905023005/diff/360001/content/renderer/pepper... content/renderer/pepper/pepper_video_encoder_host.cc:476: bool PepperVideoEncoderHost::EnsureGpuChannel() { On 2015/02/23 23:49:12, llandwerlin wrote: > On 2015/02/23 23:41:31, bbudge wrote: > > Shouldn't we check if the channel has already been set up before allocating a > > new one? > > Yes, thanks. Actually, current code is fine wrt channel. EstablishGpuChannelSync is lazy. Part of reusing a channel means checking if it's lost, and EstablishGpuChannelSync does that. That said, you definitely need to check for command_buffer_ having been created already, because you call this method multiple times.
https://codereview.chromium.org/905023005/diff/360001/ppapi/proxy/video_encod... File ppapi/proxy/video_encoder_resource.cc (right): https://codereview.chromium.org/905023005/diff/360001/ppapi/proxy/video_encod... ppapi/proxy/video_encoder_resource.cc:254: base::checked_cast<uint32_t>(sizeof(PP_VideoProfileDescription))); On 2015/02/23 23:49:12, llandwerlin wrote: > On 2015/02/23 23:41:31, bbudge wrote: > > static_cast is fine here, since the input is a compile time constant which we > > know will not overflow. > > Weird, that's how it was initially, but that doesn't pass the > win_chromium_x64_rel_ng bot : > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_... I see the compile issues. I'm wondering what's going on since I see lots of static_cast<uint32_t> in the existing code (code search "static_cast<uint32_t>". Here's one for example: https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/video_... I talked with a security reviewer and checked_cast is preferred here. Since sizeof is known at compile time, the runtime check is supposedly optimized out. So disregard my comment.
The CQ bit was checked by lionel.g.landwerlin@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, bbudge@chromium.org, jam@chromium.org Link to the patchset: https://codereview.chromium.org/905023005/#ps420001 (title: "fix unittest on win_chromium_x64_rel_ng")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/905023005/420001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)
The CQ bit was checked by bbudge@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/905023005/420001
Message was sent while issue was closed.
Committed patchset #22 (id:420001)
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/c3f15126607423812eaefe48ecdcb0ffd39c5e05 Cr-Commit-Position: refs/heads/master@{#317938} |
