Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2308533003:
Break tight coupling of VideoCaptureDeviceClient to renderer_host (Closed)
|
Created:
4 years, 3 months ago by chfremer Modified:
4 years, 3 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mcasas+watch+vc_chromium.org, miu+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBreak tight coupling of VideoCaptureDeviceClient to renderer_host
This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL1.6.
This CL is a step towards being able to reuse the classes VideoCaptureDeviceClient
and VideoCaptureBufferPool as part of the Video Capture Mojo Service, because they
implement functionality that is needed there. Expressed in class-diagram form, we
want to go from [2] to [3].
Currently, the classes VideoCaptureDeviceClient and VideoCaptureBufferPool live in
content/browser/renderer_host/media. Even though their functionality is not specific
to renderer_host, their dependencies create a tight coupling to code that is. In order
to use them in services/video_capture, we need to move them out of there (and probably
into media/capture/video).
In this CL we break the tight coupling by extracting interfaces and having the two
classes depend on the abstractions instead of the implementations.
The class VideoCaptureDeviceClient has a second type of tight coupling, which is that
it knows about BrowserThread and wants to post tasks to the Browser IO thread. We
break this dependency by moving the logic to forward calls to the Browser IO Thread
into a decorator "PostOnBrowserIoThreadDecorator" (using the Decorator Pattern).
No new files are created in this CL. Interfaces will be moved to new files in a new
location in a follow-up CL.
BUG=584797
TEST=This is a pure refactoring. No new functionality. All tests still pass.
[1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxsxWslJ_X-zpOhijvI8/edit?usp=sharing
[2] https://docs.google.com/a/chromium.org/drawings/d/1jgDkxl1WvQCO-zeg87eLh5XkB4RP0wt46xlsB_XFfP8/edit?usp=sharing
[3] https://docs.google.com/a/chromium.org/drawings/d/12yOdum03TP1dL7DmUjPxtAfSo16K8La6GJovSisqyRk/edit?usp=sharing
Committed: https://crrev.com/42b7da0b82c0f67702f3f0f0033d611ff44916f5
Cr-Commit-Position: refs/heads/master@{#419565}
Patch Set 1 #Patch Set 2 : Merge branch 'master' into DeviceClientRefactor #Patch Set 3 : Apply method name changes to Mock #
Total comments: 24
Patch Set 4 : mcasas' comments plus rebase #
Total comments: 16
Patch Set 5 : mcasas' & emircan's comments #
Total comments: 16
Patch Set 6 : mcasas' comments #
Total comments: 34
Patch Set 7 : Rename interface/impl, fix tests #Patch Set 8 : miu's comments #Patch Set 9 : miu's comments #Dependent Patchsets: Messages
Total messages: 52 (28 generated)
Description was changed from ========== Merge branch 'master' into DeviceClientRefactor Merge branch 'master' into DeviceClientRefactor Extracted logic of posting on Browser IO thread from VideoCaptureDeviceClient in order to break dependency on browser_thread.h/cc. Use new interface ThreadAccessor to break dependency of VideoCaptureDeviceClient from browser_thread.h/cc. Extract interface VideoCaptureJpegDecoder from class VideoCaptureGpuJpegDecoder. Add interface VideoCaptureJpegDecoderFactory. Extract methods from class class VideoCaptureController called by VideoCaptureDeviceClient into a new interface VideoFrameReceiver. Extract methods from class class VideoCaptureBufferPool called by VideoCaptureDeviceClient into a new interface VideoCaptureBufferPoolInterface. BUG= ========== to ========== Break tight coupling of VideoCaptureDeviceClient to renderer_host This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL1.6. This CL is a step towards being able to reuse the classes VideoCaptureDeviceClient and VideoCaptureBufferPool as part of the Video Capture Mojo Service, because they implement functionality that is needed there. Expressed in class-diagram form, we want to go from [2] to [3]. Currently, the classes VideoCaptureDeviceClient and VideoCaptureBufferPool live in content/browser/renderer_host/media. Even though their functionality is not specific to renderer_host, their dependencies create a tight coupling to code that is. In order to use them in services/video_capture, we need to move them out of there (and probably into media/capture/video). In this CL we break the tight coupling by extracting interfaces and having the two classes depend on the abstractions instead of the implementations. The class VideoCaptureDeviceClient has a second type of tight coupling, which is that it knows about BrowserThread and wants to post tasks to the Browser IO thread. We break this dependency by moving the logic to forward calls to the Browser IO Thread into a decorator "PostOnBrowserIoThreadDecorator" (using the Decorator Pattern). No new files are created in this CL. Interfaces will be moved to new files in a new location in a follow-up CL. BUG=584797 TEST=This is a pure refactoring. No new functionality. All tests still pass. [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... [2] https://docs.google.com/a/chromium.org/drawings/d/1jgDkxl1WvQCO-zeg87eLh5XkB4... [3] https://docs.google.com/a/chromium.org/drawings/d/12yOdum03TP1dL7DmUjPxtAfSo1... ==========
Description was changed from ========== Break tight coupling of VideoCaptureDeviceClient to renderer_host This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL1.6. This CL is a step towards being able to reuse the classes VideoCaptureDeviceClient and VideoCaptureBufferPool as part of the Video Capture Mojo Service, because they implement functionality that is needed there. Expressed in class-diagram form, we want to go from [2] to [3]. Currently, the classes VideoCaptureDeviceClient and VideoCaptureBufferPool live in content/browser/renderer_host/media. Even though their functionality is not specific to renderer_host, their dependencies create a tight coupling to code that is. In order to use them in services/video_capture, we need to move them out of there (and probably into media/capture/video). In this CL we break the tight coupling by extracting interfaces and having the two classes depend on the abstractions instead of the implementations. The class VideoCaptureDeviceClient has a second type of tight coupling, which is that it knows about BrowserThread and wants to post tasks to the Browser IO thread. We break this dependency by moving the logic to forward calls to the Browser IO Thread into a decorator "PostOnBrowserIoThreadDecorator" (using the Decorator Pattern). No new files are created in this CL. Interfaces will be moved to new files in a new location in a follow-up CL. BUG=584797 TEST=This is a pure refactoring. No new functionality. All tests still pass. [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... [2] https://docs.google.com/a/chromium.org/drawings/d/1jgDkxl1WvQCO-zeg87eLh5XkB4... [3] https://docs.google.com/a/chromium.org/drawings/d/12yOdum03TP1dL7DmUjPxtAfSo1... ========== to ========== Break tight coupling of VideoCaptureDeviceClient to renderer_host This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL1.6. This CL is a step towards being able to reuse the classes VideoCaptureDeviceClient and VideoCaptureBufferPool as part of the Video Capture Mojo Service, because they implement functionality that is needed there. Expressed in class-diagram form, we want to go from [2] to [3]. Currently, the classes VideoCaptureDeviceClient and VideoCaptureBufferPool live in content/browser/renderer_host/media. Even though their functionality is not specific to renderer_host, their dependencies create a tight coupling to code that is. In order to use them in services/video_capture, we need to move them out of there (and probably into media/capture/video). In this CL we break the tight coupling by extracting interfaces and having the two classes depend on the abstractions instead of the implementations. The class VideoCaptureDeviceClient has a second type of tight coupling, which is that it knows about BrowserThread and wants to post tasks to the Browser IO thread. We break this dependency by moving the logic to forward calls to the Browser IO Thread into a decorator "PostOnBrowserIoThreadDecorator" (using the Decorator Pattern). No new files are created in this CL. Interfaces will be moved to new files in a new location in a follow-up CL. BUG=584797 TEST=This is a pure refactoring. No new functionality. All tests still pass. [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... [2] https://docs.google.com/a/chromium.org/drawings/d/1jgDkxl1WvQCO-zeg87eLh5XkB4... [3] https://docs.google.com/a/chromium.org/drawings/d/12yOdum03TP1dL7DmUjPxtAfSo1... ==========
chfremer@chromium.org changed reviewers: + emircan@chromium.org, mcasas@chromium.org
mcasas@: PTAL emircan@: FYI
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
A couple of comments https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_buffer_pool.h (right): https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_buffer_pool.h:41: class CONTENT_EXPORT VideoCaptureBufferPoolInterface Using Base as suffix is more commonplace than Interface (49 versus 14, [1,2]). [1] find content/ chrome media | grep _base.h | wc -l [2] find content/ chrome media | grep _interface.h | wc -l https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:96: }; I don't think this deserves a full class, a callback signature should be enough, e.g. using VideoCaptureGpuJpegDecoderCreateCB = base::Callback<std::unique_ptr<VideoCaptureJpegDecoder>( VideoCaptureJpegDecoder::DecodeDoneCB)>; defined in video_capture_device_client.h, and base::Bind()ed in l.209 to a static function. https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:76: check_thread_closure.Run(); I'd rather pass a const SingleThreadTaskRunner& where this class is supposed to operate, cache it and then DCHECK(task_runner_->BelongsToCurrentThread()) in all methods where it makes sense. https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:102: external_jpeg_decoder_initialized_ = true; I think we can remove this flag and test |external_jpeg_decoder_| instead. https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:284: } nit: Remove {} https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.h (right): https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.h:98: std::unique_ptr<VideoFrameReceiver> receiver_; maybe const? Also, there's a subtle change here in that |controller_| is only used on IO thread whereas |receiver_| is used on the thread where video frames are delievered. The whole structure might be racy on destruction, right? https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.h:100: std::unique_ptr<VideoCaptureJpegDecoderFactory> jpeg_decoder_factory_; const
chfremer@chromium.org changed reviewers: + miu@chromium.org
https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_buffer_pool.h (right): https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_buffer_pool.h:41: class CONTENT_EXPORT VideoCaptureBufferPoolInterface On 2016/09/07 22:43:40, mcasas wrote: > Using Base as suffix is more commonplace than > Interface (49 versus 14, [1,2]). > > > > [1] find content/ chrome media | grep _base.h | wc -l > [2] find content/ chrome media | grep _interface.h | wc -l I agree that VideoCaptureBufferPoolInterface is probably not the best name for this interface. I don't think calling it "VideoCaptureBufferPoolBase" would be appropriate, because it is not a base class in the sense that it inherits functionality to child classes. What it is, is the subset of method signatures from class VideoCaptureBufferPool that class VideoCaptureDeviceClient depends on (i.e. want to invoke). In terms of the buffer pool, clients interacting with that set of methods are probably considered "producers", so maybe a good name would be "ProducerVideoCaptureBufferPool". Please let me know what you think about it. https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:96: }; On 2016/09/07 22:43:40, mcasas wrote: > I don't think this deserves a full class, a callback signature should > be enough, e.g. > > using VideoCaptureGpuJpegDecoderCreateCB = > base::Callback<std::unique_ptr<VideoCaptureJpegDecoder>( > VideoCaptureJpegDecoder::DecodeDoneCB)>; > > defined in video_capture_device_client.h, and base::Bind()ed > in l.209 to a static function. I believe what you are proposing is to replace the explicit interface class VideoCaptureJpegDecoderFactory with a callback interface using VideoCaptureJpegDecoderFactoryCB = base::Callback<std::unique_ptr<VideoCaptureJpegDecoder>()>; and then using base::Bind() to create an instance by binding to a static function static std::unique_ptr<VideoCaptureJpegDecoder> CreateGpuJpegDecoder( const VideoCaptureJpegDecoder::DecodeDoneCB& decode_done_cb) { return base::MakeUnique<VideoCaptureGpuJpegDecoder>(decode_done_cb); } with the |decode_done_cb| provided at bind-time at l.209: base::Bind(&CreateGpuJpegDecoder, base::Bind(&VideoFrameReceiver::OnIncomingCapturedVideoFrame, this->GetWeakPtrForIOThread())), Both approaches are equivalent from an Object-Oriented design perspective. But as a reader of the code, I find the explicit interface class plus explicit implementation class much easier to understand than the ad-hoc binding. Therefore I would like to keep using the existing approach unless there is any other reason why the callback approach is superior that I missed. https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:76: check_thread_closure.Run(); On 2016/09/07 22:43:40, mcasas wrote: > I'd rather pass a const SingleThreadTaskRunner& where > this class is supposed to operate, cache it and then > DCHECK(task_runner_->BelongsToCurrentThread()) > in all methods where it makes sense. Please explain why you would prefer doing that. I don't see any advantage, but I see a disadvantage. The disadvantage is that it would create a dependency on a rather complex concrete class SingleThreadTaskRunner where none is needed. The VideoCaptureDeviceClient class should not need to know or care about threads. All it needs to know is that there is a method it is supposed to call at a certain time. https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:102: external_jpeg_decoder_initialized_ = true; On 2016/09/07 22:43:40, mcasas wrote: > I think we can remove this flag and test > |external_jpeg_decoder_| instead. Not sure we can do that without changing behavior. Note that there is a call external_jpeg_decoder_.reset(); at line 228, which means it is possible that external_jpeg_decoder_ is null while external_jpeg_decoder_initialized_ == true. https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:284: } On 2016/09/07 22:43:40, mcasas wrote: > nit: Remove {} Done. https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.h (right): https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.h:98: std::unique_ptr<VideoFrameReceiver> receiver_; On 2016/09/07 22:43:40, mcasas wrote: > maybe const? > > Also, there's a subtle change here in that |controller_| > is only used on IO thread whereas |receiver_| is used > on the thread where video frames are delievered. > The whole structure might be racy on destruction, > right? Not sure if my understanding of the threading issues is correct here. But I believe that the refactoring does not change anything regarding the threading. Calls from the VideoCaptureDeviceClient instance to the instance formerly referred to as |controller_| are still being issued on the Browser IO Thread, just as before. The only difference is that after the refactoring, the class VideoCaptureDeviceClient no longer has the responsibility of posting to that thread. The responsibility of posting calls to the correct thread is now with |receiver_|, which in our usage is an instance of the new class PostOnBrowserIoThreadDecorator. I'll ping you for an offline discussion to check if I am missing anything here. https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.h:100: std::unique_ptr<VideoCaptureJpegDecoderFactory> jpeg_decoder_factory_; On 2016/09/07 22:43:40, mcasas wrote: > const Done.
https://codereview.chromium.org/2308533003/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_buffer_pool.h (right): https://codereview.chromium.org/2308533003/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_buffer_pool.h:28: // TODO(emircan): See https://crbug.com/521059, refactor this class. You can remove this line now. That bug is closed. https://codereview.chromium.org/2308533003/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_buffer_pool.h:143: double GetBufferPoolUtilization() override; We can keep the function marked as const here. https://codereview.chromium.org/2308533003/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/2308533003/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:100: class PostOnBrowserIoThreadDecorator : public VideoFrameReceiver { VideoFrameReceiverOnIOThread? https://codereview.chromium.org/2308533003/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:211: this->GetWeakPtrForIOThread())), Passing a callback would be easier to follow here than passing the factory class. I would rather avoid another layer of class hierarchy for readability here. https://codereview.chromium.org/2308533003/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.h (right): https://codereview.chromium.org/2308533003/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.h:98: std::unique_ptr<VideoFrameReceiver> receiver_; const https://codereview.chromium.org/2308533003/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.h (right): https://codereview.chromium.org/2308533003/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.h:68: class CONTENT_EXPORT VideoCaptureJpegDecoderFactory { Can you look if there is test coverage for this class? AFAIK, this only kicks in for some CrOS devices when there is MJPEG camera input. They might not be running on the waterfall.
https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:96: }; On 2016/09/08 17:28:51, chfremer wrote: > On 2016/09/07 22:43:40, mcasas wrote: > > I don't think this deserves a full class, a callback signature should > > be enough, e.g. > > > > using VideoCaptureGpuJpegDecoderCreateCB = > > base::Callback<std::unique_ptr<VideoCaptureJpegDecoder>( > > VideoCaptureJpegDecoder::DecodeDoneCB)>; > > > > defined in video_capture_device_client.h, and base::Bind()ed > > in l.209 to a static function. > > I believe what you are proposing is to replace the explicit interface class > VideoCaptureJpegDecoderFactory with a callback interface > > using VideoCaptureJpegDecoderFactoryCB = > base::Callback<std::unique_ptr<VideoCaptureJpegDecoder>()>; > > and then using base::Bind() to create an instance by binding to a static > function > > static std::unique_ptr<VideoCaptureJpegDecoder> CreateGpuJpegDecoder( > const VideoCaptureJpegDecoder::DecodeDoneCB& decode_done_cb) { > return base::MakeUnique<VideoCaptureGpuJpegDecoder>(decode_done_cb); > } > > with the |decode_done_cb| provided at bind-time at l.209: > > base::Bind(&CreateGpuJpegDecoder, > base::Bind(&VideoFrameReceiver::OnIncomingCapturedVideoFrame, > this->GetWeakPtrForIOThread())), > > Both approaches are equivalent from an Object-Oriented design perspective. > But as a reader of the code, I find the explicit interface class plus explicit > implementation class much easier to understand than the ad-hoc binding. > Therefore I would like to keep using the existing approach unless there is any > other reason why the callback approach is superior that I missed. emircan@ thinks the same in his comment in this PS line 211 :-) https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:76: check_thread_closure.Run(); On 2016/09/08 17:28:51, chfremer wrote: > On 2016/09/07 22:43:40, mcasas wrote: > > I'd rather pass a const SingleThreadTaskRunner& where > > this class is supposed to operate, cache it and then > > DCHECK(task_runner_->BelongsToCurrentThread()) > > in all methods where it makes sense. > > Please explain why you would prefer doing that. > I don't see any advantage, but I see a disadvantage. The disadvantage is that it > would create a dependency on a rather complex concrete class > SingleThreadTaskRunner where none is needed. The VideoCaptureDeviceClient class > should not need to know or care about threads. All it needs to know is that > there is a method it is supposed to call at a certain time. But SingleThreadTaskRunners are pervasively passed around the codebase to check for thread affinity of methods and/or PostTask()s ([1] gives 160); OTOH, a bound base::Callback is more opaque to me, since here I can't tell what's exactly being done. [1] grep -rn "const scoped_refptr<base::SingleThreadTaskRunner>&" content/ | wc -l https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:102: external_jpeg_decoder_initialized_ = true; On 2016/09/08 17:28:51, chfremer wrote: > On 2016/09/07 22:43:40, mcasas wrote: > > I think we can remove this flag and test > > |external_jpeg_decoder_| instead. > > Not sure we can do that without changing behavior. > Note that there is a call > > external_jpeg_decoder_.reset(); > > at line 228, which means it is possible that external_jpeg_decoder_ is null > while external_jpeg_decoder_initialized_ == true. I see, forget it then. https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.h (right): https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.h:98: std::unique_ptr<VideoFrameReceiver> receiver_; On 2016/09/08 17:28:51, chfremer wrote: > On 2016/09/07 22:43:40, mcasas wrote: > > maybe const? > > > > Also, there's a subtle change here in that |controller_| > > is only used on IO thread whereas |receiver_| is used > > on the thread where video frames are delievered. > > The whole structure might be racy on destruction, > > right? > > Not sure if my understanding of the threading issues is correct here. > But I believe that the refactoring does not change anything regarding the > threading. > > Calls from the VideoCaptureDeviceClient instance to the instance formerly > referred to as |controller_| are still being issued on the Browser IO Thread, > just as before. The only difference is that after the refactoring, the class > VideoCaptureDeviceClient no longer has the responsibility of posting to that > thread. The responsibility of posting calls to the correct thread is now with > |receiver_|, which in our usage is an instance of the new class > PostOnBrowserIoThreadDecorator. > > I'll ping you for an offline discussion to check if I am missing anything here. Notwithstanding any offline discussions, I see this sequence as potentially racy: - |receiver_| is created on the thread where VCDClient is created (which we know is IO thread, but that's irrelevant for the purpose of this conversation). - |receiver_| is used from the thread where OnIncomingCapturedData(), which we cannot assume to be the same of where the class is constructed. - VCDClient can still use |receiver_| in said OnIncomingCapturedData() after being destructed on IO thread. This scenario wasn't possible before because of the PostTask(), that forced using |controller_| (weak) explicitly, which was invalidated synchronously in its destructor. https://codereview.chromium.org/2308533003/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/2308533003/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:100: class PostOnBrowserIoThreadDecorator : public VideoFrameReceiver { IOThread is written with IO in uppercase. (grep -rn "IoThread" content/ | wc -l says 0)
I am posting my replies to the open comments at this time without uploading a corresponding patch set. This is because I am OOO until tomorrow morning, but don't want to stall the discussion with a delayed reply. I'll upload a patch set with the agreed changes tomorrow. Until then, please keep the discussion points going. https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:96: }; On 2016/09/08 20:25:45, mcasas wrote: > On 2016/09/08 17:28:51, chfremer wrote: > > On 2016/09/07 22:43:40, mcasas wrote: > > > I don't think this deserves a full class, a callback signature should > > > be enough, e.g. > > > > > > using VideoCaptureGpuJpegDecoderCreateCB = > > > base::Callback<std::unique_ptr<VideoCaptureJpegDecoder>( > > > VideoCaptureJpegDecoder::DecodeDoneCB)>; > > > > > > defined in video_capture_device_client.h, and base::Bind()ed > > > in l.209 to a static function. > > > > I believe what you are proposing is to replace the explicit interface class > > VideoCaptureJpegDecoderFactory with a callback interface > > > > using VideoCaptureJpegDecoderFactoryCB = > > base::Callback<std::unique_ptr<VideoCaptureJpegDecoder>()>; > > > > and then using base::Bind() to create an instance by binding to a static > > function > > > > static std::unique_ptr<VideoCaptureJpegDecoder> CreateGpuJpegDecoder( > > const VideoCaptureJpegDecoder::DecodeDoneCB& decode_done_cb) { > > return base::MakeUnique<VideoCaptureGpuJpegDecoder>(decode_done_cb); > > } > > > > with the |decode_done_cb| provided at bind-time at l.209: > > > > base::Bind(&CreateGpuJpegDecoder, > > base::Bind(&VideoFrameReceiver::OnIncomingCapturedVideoFrame, > > this->GetWeakPtrForIOThread())), > > > > Both approaches are equivalent from an Object-Oriented design perspective. > > But as a reader of the code, I find the explicit interface class plus explicit > > implementation class much easier to understand than the ad-hoc binding. > > Therefore I would like to keep using the existing approach unless there is any > > other reason why the callback approach is superior that I missed. > > emircan@ thinks the same in his comment in this PS > line 211 > :-) Done. https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:76: check_thread_closure.Run(); On 2016/09/08 20:25:45, mcasas wrote: > On 2016/09/08 17:28:51, chfremer wrote: > > On 2016/09/07 22:43:40, mcasas wrote: > > > I'd rather pass a const SingleThreadTaskRunner& where > > > this class is supposed to operate, cache it and then > > > DCHECK(task_runner_->BelongsToCurrentThread()) > > > in all methods where it makes sense. > > > > Please explain why you would prefer doing that. > > I don't see any advantage, but I see a disadvantage. The disadvantage is that > it > > would create a dependency on a rather complex concrete class > > SingleThreadTaskRunner where none is needed. The VideoCaptureDeviceClient > class > > should not need to know or care about threads. All it needs to know is that > > there is a method it is supposed to call at a certain time. > > But SingleThreadTaskRunners are pervasively passed > around the codebase to check for thread > affinity of methods and/or PostTask()s ([1] gives 160); > OTOH, a bound base::Callback is more opaque to me, > since here I can't tell what's exactly being done. > > [1] grep -rn "const scoped_refptr<base::SingleThreadTaskRunner>&" content/ | wc > -l Being more opaque here is actually a good thing, since as a reader of the code for this class you should not have to know or care what exactly this call is doing. Using a name like |thread_checker| or |check_thread_closure| for the variable should reveal enough about the intended purpose of the call. Of course I am willing to conform with the rest of the code base for consistency purposes, so I'll make the change. But I still think it is bad to depend on more than needed. https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:102: external_jpeg_decoder_initialized_ = true; On 2016/09/08 20:25:45, mcasas wrote: > On 2016/09/08 17:28:51, chfremer wrote: > > On 2016/09/07 22:43:40, mcasas wrote: > > > I think we can remove this flag and test > > > |external_jpeg_decoder_| instead. > > > > Not sure we can do that without changing behavior. > > Note that there is a call > > > > external_jpeg_decoder_.reset(); > > > > at line 228, which means it is possible that external_jpeg_decoder_ is null > > while external_jpeg_decoder_initialized_ == true. > > I see, forget it then. Acknowledged. https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.h (right): https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.h:98: std::unique_ptr<VideoFrameReceiver> receiver_; On 2016/09/08 20:25:45, mcasas wrote: > On 2016/09/08 17:28:51, chfremer wrote: > > On 2016/09/07 22:43:40, mcasas wrote: > > > maybe const? > > > > > > Also, there's a subtle change here in that |controller_| > > > is only used on IO thread whereas |receiver_| is used > > > on the thread where video frames are delievered. > > > The whole structure might be racy on destruction, > > > right? > > > > Not sure if my understanding of the threading issues is correct here. > > But I believe that the refactoring does not change anything regarding the > > threading. > > > > Calls from the VideoCaptureDeviceClient instance to the instance formerly > > referred to as |controller_| are still being issued on the Browser IO Thread, > > just as before. The only difference is that after the refactoring, the class > > VideoCaptureDeviceClient no longer has the responsibility of posting to that > > thread. The responsibility of posting calls to the correct thread is now with > > |receiver_|, which in our usage is an instance of the new class > > PostOnBrowserIoThreadDecorator. > > > > I'll ping you for an offline discussion to check if I am missing anything > here. > > Notwithstanding any offline discussions, I see this > sequence as potentially racy: > > - |receiver_| is created on the thread where VCDClient is > created (which we know is IO thread, but that's irrelevant > for the purpose of this conversation). > - |receiver_| is used from the thread where > OnIncomingCapturedData(), which we cannot assume to > be the same of where the class is constructed. > - VCDClient can still use |receiver_| in said > OnIncomingCapturedData() after being destructed > on IO thread. > > This scenario wasn't possible before because of the > PostTask(), that forced using |controller_| (weak) > explicitly, which was invalidated synchronously in its > destructor. Discussed offline. mcasas@ found out that after construction on the IO thread, ownership of the VCDClient is passed to a media::VideoCaptureDevice instance, where it is used and eventually destroyed on the Device Thread. We agreed to make it clear in the description of VCDClient that it is legal to construct on one thread and then pass to another thread for usage and ownership. https://codereview.chromium.org/2308533003/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_buffer_pool.h (right): https://codereview.chromium.org/2308533003/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_buffer_pool.h:28: // TODO(emircan): See https://crbug.com/521059, refactor this class. On 2016/09/08 19:57:56, emircan wrote: > You can remove this line now. That bug is closed. Done. https://codereview.chromium.org/2308533003/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_buffer_pool.h:143: double GetBufferPoolUtilization() override; On 2016/09/08 19:57:56, emircan wrote: > We can keep the function marked as const here. Thanks for noticing and bringing up the removed const here. I removed the const qualifier from the method on purpose as part of extracting these methods into the interface "ProducerVideoCaptureBufferPool". The rationale behind this is as follows. When working with pure interfaces in C++ (i.e. classes with all purely virtual methods), we can have concrete classes expose any number of different interfaces simultaneously. The const qualifier on methods is basically a short-hand notation for exposing exactly two interfaces, one const and one non-const. Using pure interfaces, we can do the same thing by exposing two interfaces, e.g. "ProducerVideoCaptureBufferPool" and "ConstProducerVideoCaptureBufferPool". Now there are some observations to make: 1.) We have one client that wants to consume the methods in ProducerVideoCaptureBufferPool. We don't have any clients that would instead want to just consume GetBufferPoolUtilization(). Therefore we don't need to expose two different interfaces. 2.) It is of no concern to clients how an implementation realizes an exposed interfaces. Clients have no need to know whether an implementation is given access to "this" or "const this". Therefore, the const qualifier should not be part of the exposed interface. 3.) It is true that by removing the const from the method, the compiler no longer enforces that the implementation can only interface with "const this". That is acceptable, because it does not effectively prevent program state from being modified anyway. In conclusion: When working with pure interfaces, the const qualifier for methods is not useful. Please let me know if this explanation makes sense. https://codereview.chromium.org/2308533003/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/2308533003/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:100: class PostOnBrowserIoThreadDecorator : public VideoFrameReceiver { On 2016/09/08 20:25:45, mcasas wrote: > IOThread is written with IO in uppercase. > > (grep -rn "IoThread" content/ | wc -l says 0) Thanks for both of the suggestions. I hope you are okay with "PostOnIOThreadVideoFrameReceiver", since I try to be consistent in putting the specialization aspect of a name in front of the thing it specializes. https://codereview.chromium.org/2308533003/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:211: this->GetWeakPtrForIOThread())), On 2016/09/08 19:57:56, emircan wrote: > Passing a callback would be easier to follow here than passing the factory > class. I would rather avoid another layer of class hierarchy for readability > here. Interesting. I feel the exact opposite way about the readability. With mcasas@ already having expressed the same feeling as you, I'll go ahead and make the change :-). https://codereview.chromium.org/2308533003/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.h (right): https://codereview.chromium.org/2308533003/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.h:98: std::unique_ptr<VideoFrameReceiver> receiver_; On 2016/09/08 19:57:56, emircan wrote: > const Done. https://codereview.chromium.org/2308533003/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.h (right): https://codereview.chromium.org/2308533003/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.h:68: class CONTENT_EXPORT VideoCaptureJpegDecoderFactory { On 2016/09/08 19:57:56, emircan wrote: > Can you look if there is test coverage for this class? AFAIK, this only kicks in > for some CrOS devices when there is MJPEG camera input. They might not be > running on the waterfall. For this CL I am not too worried (yet) about breaking anything, since extracting the interfaces seems relatively safe. I will look into the test coverage question before moving on with the next CL. If I find that there is no tests exercising this, I will consider adding some before moving on. Added this task as CL1.6.1.1 to the design doc [1]. [1] https://docs.google.com/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxsxWslJ_X-zpOhijv...
mcasas@: PTAL emircan@: PTAL https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:76: check_thread_closure.Run(); On 2016/09/09 00:55:37, chfremer wrote: > On 2016/09/08 20:25:45, mcasas wrote: > > On 2016/09/08 17:28:51, chfremer wrote: > > > On 2016/09/07 22:43:40, mcasas wrote: > > > > I'd rather pass a const SingleThreadTaskRunner& where > > > > this class is supposed to operate, cache it and then > > > > DCHECK(task_runner_->BelongsToCurrentThread()) > > > > in all methods where it makes sense. > > > > > > Please explain why you would prefer doing that. > > > I don't see any advantage, but I see a disadvantage. The disadvantage is > that > > it > > > would create a dependency on a rather complex concrete class > > > SingleThreadTaskRunner where none is needed. The VideoCaptureDeviceClient > > class > > > should not need to know or care about threads. All it needs to know is that > > > there is a method it is supposed to call at a certain time. > > > > But SingleThreadTaskRunners are pervasively passed > > around the codebase to check for thread > > affinity of methods and/or PostTask()s ([1] gives 160); > > OTOH, a bound base::Callback is more opaque to me, > > since here I can't tell what's exactly being done. > > > > [1] grep -rn "const scoped_refptr<base::SingleThreadTaskRunner>&" content/ | > wc > > -l > > Being more opaque here is actually a good thing, since as a reader of the code > for this class you should not have to know or care what exactly this call is > doing. Using a name like |thread_checker| or |check_thread_closure| for the > variable should reveal enough about the intended purpose of the call. > > Of course I am willing to conform with the rest of the code base for consistency > purposes, so I'll make the change. But I still think it is bad to depend on more > than needed. After looking at it one more time, I decided to simply remove the thread check call in this constructor. This makes sense, because the VCDClient class does not care on which thread it is created, since it is legal to use it on other threads than the creating one. And if we really would care that construction happens on the Browser IO Thread, we still have a check in the method that calls the constructor in VideoCaptureController.
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Almost there. A few nits and naming suggestion. https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_buffer_pool.h (right): https://codereview.chromium.org/2308533003/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_buffer_pool.h:41: class CONTENT_EXPORT VideoCaptureBufferPoolInterface On 2016/09/08 17:28:51, chfremer wrote: > On 2016/09/07 22:43:40, mcasas wrote: > > Using Base as suffix is more commonplace than > > Interface (49 versus 14, [1,2]). > > > > > > > > [1] find content/ chrome media | grep _base.h | wc -l > > [2] find content/ chrome media | grep _interface.h | wc -l > > I agree that VideoCaptureBufferPoolInterface is probably not the best name for > this interface. > I don't think calling it "VideoCaptureBufferPoolBase" would be appropriate, > because it is not a base class in the sense that it inherits functionality to > child classes. > > What it is, is the subset of method signatures from class VideoCaptureBufferPool > that class VideoCaptureDeviceClient depends on (i.e. want to invoke). In terms > of the buffer pool, clients interacting with that set of methods are probably > considered "producers", so maybe a good name would be > "ProducerVideoCaptureBufferPool". > > Please let me know what you think about it. I see, in that case I'd say the usual naming convention is VideoCaptureBufferPool and VideoCaptureBufferPoolImpl, for base and derived class, resp. https://codereview.chromium.org/2308533003/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_buffer_pool.h (right): https://codereview.chromium.org/2308533003/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_buffer_pool.h:92: }; nit : add here a DISALLOW_IMPLICIT_CONSTRUCTORS(ClassName); actually, it should be added to all new base classes, either that macro or DISALLOW_COPY_AND_ASSIGN, whichever makes sense https://codereview.chromium.org/2308533003/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/2308533003/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:366: // ProducerVideoCaptureBufferPool::GetBufferPoolUtilization() is thread-safe. This comment always throws me off. If it is thread safe, it should be called GetBufferPoolUtillizationThreadSafe(), but the reality is that all VideoCaptureBufferPool methods are thread-safe. So could you please remove the comment, please? https://codereview.chromium.org/2308533003/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.h (right): https://codereview.chromium.org/2308533003/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.h:27: // Implementation of media::VideoCaptureDevice::Client that uses a buffer pool nit: only one space between "of" and "media" https://codereview.chromium.org/2308533003/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.h:35: // The owner is repsonsible for making sure that the instance outlives these nit: s/repsonsible/responsible/ https://codereview.chromium.org/2308533003/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.h:102: const VideoCaptureJpegDecoderFactoryCB jpeg_decoder_factory_; nit : s/jpeg_decoder_factory_/jpeg_decoder_factory_callback_/ ? https://codereview.chromium.org/2308533003/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.h (right): https://codereview.chromium.org/2308533003/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.h:81: class CONTENT_EXPORT VideoCaptureGpuJpegDecoder Following my recommendation before, this class should be called VideoCaptureJpegDecoderImpl. https://codereview.chromium.org/2308533003/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.h:86: public: Also, if we're creating abstract base classes and Impls, shouldn't we extract the base classes into their own files, and rename the appropriate ones to _impl.{cc,h} ? I'm more than OK with doing that in a subsequent, single, specific CL :-)
lgtm % nits below. https://codereview.chromium.org/2308533003/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_buffer_pool.h (right): https://codereview.chromium.org/2308533003/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_buffer_pool.h:27: // Abstraction of a pool's buffer data buffer and size for clients. I was referring to removing only TODO line :) The other should stay. https://codereview.chromium.org/2308533003/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_buffer_pool.h:143: double GetBufferPoolUtilization() override; On 2016/09/09 00:55:37, chfremer wrote: > On 2016/09/08 19:57:56, emircan wrote: > > We can keep the function marked as const here. > > Thanks for noticing and bringing up the removed const here. > I removed the const qualifier from the method on purpose as part of extracting > these methods into the interface "ProducerVideoCaptureBufferPool". The rationale > behind this is as follows. > > When working with pure interfaces in C++ (i.e. classes with all purely virtual > methods), we can have concrete classes expose any number of different interfaces > simultaneously. The const qualifier on methods is basically a short-hand > notation for exposing exactly two interfaces, one const and one non-const. Using > pure interfaces, we can do the same thing by exposing two interfaces, e.g. > "ProducerVideoCaptureBufferPool" and "ConstProducerVideoCaptureBufferPool". > > Now there are some observations to make: > 1.) We have one client that wants to consume the methods in > ProducerVideoCaptureBufferPool. We don't have any clients that would instead > want to just consume GetBufferPoolUtilization(). Therefore we don't need to > expose two different interfaces. > 2.) It is of no concern to clients how an implementation realizes an exposed > interfaces. Clients have no need to know whether an implementation is given > access to "this" or "const this". Therefore, the const qualifier should not be > part of the exposed interface. > 3.) It is true that by removing the const from the method, the compiler no > longer enforces that the implementation can only interface with "const this". > That is acceptable, because it does not effectively prevent program state from > being modified anyway. > > In conclusion: When working with pure interfaces, the const qualifier for > methods is not useful. > > Please let me know if this explanation makes sense. Sgtm. https://codereview.chromium.org/2308533003/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.h (right): https://codereview.chromium.org/2308533003/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.h:68: class CONTENT_EXPORT VideoCaptureJpegDecoderFactory { On 2016/09/09 00:55:38, chfremer wrote: > On 2016/09/08 19:57:56, emircan wrote: > > Can you look if there is test coverage for this class? AFAIK, this only kicks > in > > for some CrOS devices when there is MJPEG camera input. They might not be > > running on the waterfall. > > For this CL I am not too worried (yet) about breaking anything, since extracting > the interfaces seems relatively safe. > > I will look into the test coverage question before moving on with the next CL. > If I find that there is no tests exercising this, I will consider adding some > before moving on. > > Added this task as CL1.6.1.1 to the design doc [1]. > > [1] > https://docs.google.com/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxsxWslJ_X-zpOhijv... Thanks for adding the task. For now, we can test at least AppRTC loopback on Cros with HW MJPEG. This CL actually touches the production code and most CrOS testing is done manually AFAIK. https://codereview.chromium.org/2308533003/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.h (right): https://codereview.chromium.org/2308533003/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.h:102: const VideoCaptureJpegDecoderFactoryCB jpeg_decoder_factory_; On 2016/09/09 22:28:02, mcasas wrote: > nit : s/jpeg_decoder_factory_/jpeg_decoder_factory_callback_/ ? +1
mcasas@: PTAL miu@: PTAL RS https://codereview.chromium.org/2308533003/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_buffer_pool.h (right): https://codereview.chromium.org/2308533003/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_buffer_pool.h:92: }; On 2016/09/09 22:28:01, mcasas wrote: > nit : add here a > DISALLOW_IMPLICIT_CONSTRUCTORS(ClassName); > > actually, it should be added to all new base classes, > either that macro or DISALLOW_COPY_AND_ASSIGN, > whichever makes sense Done, but I have a question about it. Why is this needed? Interface classed cannot be instantiated anyway, because they consist of purely virtual methods only. https://codereview.chromium.org/2308533003/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/2308533003/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.cc:366: // ProducerVideoCaptureBufferPool::GetBufferPoolUtilization() is thread-safe. On 2016/09/09 22:28:01, mcasas wrote: > This comment always throws me off. If it is thread safe, > it should be called GetBufferPoolUtillizationThreadSafe(), > but the reality is that all VideoCaptureBufferPool methods > are thread-safe. So could you please remove the > comment, please? Done. https://codereview.chromium.org/2308533003/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_device_client.h (right): https://codereview.chromium.org/2308533003/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.h:27: // Implementation of media::VideoCaptureDevice::Client that uses a buffer pool On 2016/09/09 22:28:02, mcasas wrote: > nit: only one space between "of" and "media" Done. https://codereview.chromium.org/2308533003/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.h:35: // The owner is repsonsible for making sure that the instance outlives these On 2016/09/09 22:28:02, mcasas wrote: > nit: s/repsonsible/responsible/ Done. https://codereview.chromium.org/2308533003/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_device_client.h:102: const VideoCaptureJpegDecoderFactoryCB jpeg_decoder_factory_; On 2016/09/09 22:28:02, mcasas wrote: > nit : s/jpeg_decoder_factory_/jpeg_decoder_factory_callback_/ ? Done. https://codereview.chromium.org/2308533003/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.h (right): https://codereview.chromium.org/2308533003/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.h:81: class CONTENT_EXPORT VideoCaptureGpuJpegDecoder On 2016/09/09 22:28:02, mcasas wrote: > Following my recommendation before, this class > should be called VideoCaptureJpegDecoderImpl. Despite this being done a lot in the Chromium code base, naming an implementation of an interface just <InterfaceName>Impl has several downsides: 1.) It assumes that there is only ever going to be a single implementation. 2.) It does not express what it is that makes the implementation more concrete than the interface. I highly recommend naming implementations of an interface specification as follows: <WhatMakesThisImplementationDifferentFromOthers><InterfaceName> In this example case, I think a good name would be GpuVideoCaptureJpegDecoder. This nicely distinguishes it from a second potential implementation which would be called SoftwareVideoCaptureJpegDecoder. Since the name of the existing class already was "VideoCaptureGpuJpegDecoder" I thought it would be good to not touch it in order to not add to the amount of change of the CL. Please let me know if you still prefer to rename this to VideoCaptureJpeDecoderImpl, and I'll go ahead and make the change. https://codereview.chromium.org/2308533003/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.h:86: public: On 2016/09/09 22:28:02, mcasas wrote: > Also, if we're creating abstract base classes and Impls, > shouldn't we extract the base classes into their own > files, and rename the appropriate ones to _impl.{cc,h} ? > I'm more than OK with doing that in a subsequent, single, > specific CL :-) Agreed. As stated in the CL description: "Interfaces will be moved to new files in a new location in a follow-up CL.".
On 2016/09/12 18:31:22, chfremer wrote: > miu@: PTAL RS Looking now...
https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_buffer_pool.h (right): https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_buffer_pool.h:39: class CONTENT_EXPORT ProducerVideoCaptureBufferPool The naming feels reversed to me: Shouldn't the interface be named VideoCaptureBufferPool, and the concrete implementation be named ProducerVideoCaptureBufferPool? https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_buffer_pool.h:40: : public base::RefCountedThreadSafe<ProducerVideoCaptureBufferPool> { I looked in the design doc, but I could not find an answer to this question: What are the lifetime semantics of the buffer pool? Before your changes, there was a single buffer pool that is shared by all video capture devices, and it lived in the browser process (memory shared to render processes as needed). Are any of these properties changing in this change? How about in future changes? https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_buffer_pool.h:85: virtual double GetBufferPoolUtilization() = 0; Why was 'const' removed from the method signature? https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.cc:96: void OnIncomingCapturedVideoFrame( Please use base::ThreadChecker to ensure all these methods are being called by the same thread. This ensures the sequence of method calls is preserved when tasks are run on the IO thread. https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.cc:118: BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, This makes me wonder whether receiver_ needs to be a scoped_refptr<> instead. The buffer must not be destroyed until the client is done using it, but what you have here with WeakPtr seems to imply the buffer can be destroyed out from underneath a client. Or, are you depending on the operating system to keep the shared memory around in the render process for as long as it's needed, and we shouldn't be worried about this? If so, will this scheme work for GPU memory buffers and other types? We won't be leaking memory in the GPU or GPU process? https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller.h (right): https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.h:66: const scoped_refptr<media::VideoFrame>& frame) = 0; Since this code is being modified, please use the new style for passing ref-counted pointers when the method will add a reference: virtual void OnIncomingCapturedVideoFrame( std::unique_ptr<media::VideoCaptureDevice::Client::Buffer> buffer, scoped_refptr<media::VideoFrame> frame) = 0; If the caller of this method no longer needs its reference to |frame|, make sure to use std::move(). And, inside the method, use std::move() to pass the pointer around until it reaches its final "owner" member. See "calling conventions" section here: https://www.chromium.org/developers/smart-pointer-guidelines (what applies to unique_ptr also applies to scoped_refptr, per e-mail discussion on chromium-dev@). There are several of these in this change, so please be sure to fix them too. :) https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:39: pool_(pool), For example, when passing scoped_refptr by value, you would use std::move() here: pool_(std::move(pool)), https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.h (right): https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.h:55: virtual STATUS GetStatus() = 0; Why was 'const' removed here too?
miu@: PTAL my responses https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_buffer_pool.h (right): https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_buffer_pool.h:39: class CONTENT_EXPORT ProducerVideoCaptureBufferPool On 2016/09/12 20:16:46, miu wrote: > The naming feels reversed to me: Shouldn't the interface be named > VideoCaptureBufferPool, and the concrete implementation be named > ProducerVideoCaptureBufferPool? I understand why it feels reversed. The reason I chose the names like this is as follows: Instances of VideoCaptureBufferPool seem to have more than one type of client. One such client is the class media::VideoCaptureDeviceClient. This client calls only a subset of the public methods of VideoCaptureBufferPool. These are the methods I extracted into this interface. Based on the method names, I believed that this client, from the perspective of the pool, is called a "Producer" (but I may be off with this). Without having it analyzed too much, I am assuming that there is at least a second type of client that calls the remaining public methods, and I am assuming that, from the perspective of the pool, is called a "Consumer". To properly model this, we should have the concrete class VideoCaptureBufferPool expose two separate interfaces (one for each type of client). To distinguish them, my best idea for naming is to call one "ProducerVideoCaptureBufferPool" and the other "ConsumerVideoCaptureBufferPool", even though I see that these names are not great at explaining themselves. I have extracted the methods for the "Producer" client, but not yet the ones for the "Consumer" client (since it was not yet needed). But maybe the change would be clearer if I would do that second interface in this CL as well? Please let me know if you have any good suggestions for this situation. https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_buffer_pool.h:40: : public base::RefCountedThreadSafe<ProducerVideoCaptureBufferPool> { On 2016/09/12 20:16:46, miu wrote: > I looked in the design doc, but I could not find an answer to this question: > What are the lifetime semantics of the buffer pool? Before your changes, there > was a single buffer pool that is shared by all video capture devices, and it > lived in the browser process (memory shared to render processes as needed). > > Are any of these properties changing in this change? How about in future > changes? My understanding of how things currently work is different. To me it seems that there is one buffer pool per device. This CL does not change anything about that. All it does it extract some interfaces to break dependencies. I am planning to keep things the way they are now, i.e. one buffer pool per device, but instead of "Chromium IPC shared memory" I will switch to "Mojo shared buffers" down the line. If it is helpful, I could go and add a some diagrams and explanations to the design doc describing how things are working currently, and how I think they will work when the transition to Mojo is complete. https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_buffer_pool.h:85: virtual double GetBufferPoolUtilization() = 0; On 2016/09/12 20:16:46, miu wrote: > Why was 'const' removed from the method signature? This may be a topic worth discussing with a larger audience. The rationale behind removing const qualifiers from methods that are extracted to a pure interface is as follows. When working with pure interfaces in C++ (i.e. classes with all purely virtual methods), we can have concrete classes expose any number of different interfaces simultaneously. The const qualifier on methods is basically a short-hand notation for exposing exactly two interfaces, one const and one non-const. Using pure interfaces, we can do the same thing by exposing two interfaces and naming them, for example, "ProducerVideoCaptureBufferPool" and "ConstProducerVideoCaptureBufferPool". Now there are some observations to make: 1.) We have one client that wants to consume the methods in ProducerVideoCaptureBufferPool. We don't have any clients that would instead want to just consume GetBufferPoolUtilization(). Therefore we don't need to expose two different interfaces. 2.) It is of no concern to clients how an implementation realizes an exposed interfaces. Clients have no need to know whether an implementation is given access to "this" or "const this". Therefore, the const qualifier should not be part of the exposed interface. 3.) It is true that by removing the const from the method, the compiler no longer enforces that the implementation can only interface with "const this". That is acceptable, because it does not effectively prevent program state from being modified anyway. In conclusion: When working with pure interfaces, the const qualifier for methods is not useful. Please let me know if this explanation makes sense. https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.cc:96: void OnIncomingCapturedVideoFrame( On 2016/09/12 20:16:46, miu wrote: > Please use base::ThreadChecker to ensure all these methods are being called by > the same thread. This ensures the sequence of method calls is preserved when > tasks are run on the IO thread. Hmm, I am not sure I agree that this would be an improvement. If clients make calls to this class from multiple threads, they already cannot expect the calls to be processed in-order. And if clients want to call from multiple threads while also making sure that no overlapping calls happen, they should be allowed to and they are already guaranteed to have the order preserved. Am I missing something here? https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.cc:118: BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, On 2016/09/12 20:16:46, miu wrote: > This makes me wonder whether receiver_ needs to be a scoped_refptr<> instead. > The buffer must not be destroyed until the client is done using it, but what you > have here with WeakPtr seems to imply the buffer can be destroyed out from > underneath a client. > > Or, are you depending on the operating system to keep the shared memory around > in the render process for as long as it's needed, and we shouldn't be worried > about this? If so, will this scheme work for GPU memory buffers and other types? > We won't be leaking memory in the GPU or GPU process? Thanks. This is an interesting question. I haven't thought about this, because I am attempting to do a refactoring that I believe does not change any of the existing behavior. Please let me read up a bit more on how this mechanism is supposed to work currently, and then I will respond back. https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller.h (right): https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.h:66: const scoped_refptr<media::VideoFrame>& frame) = 0; On 2016/09/12 20:16:46, miu wrote: > Since this code is being modified, please use the new style for passing > ref-counted pointers when the method will add a reference: > > virtual void OnIncomingCapturedVideoFrame( > std::unique_ptr<media::VideoCaptureDevice::Client::Buffer> buffer, > scoped_refptr<media::VideoFrame> frame) = 0; > > If the caller of this method no longer needs its reference to |frame|, make sure > to use std::move(). And, inside the method, use std::move() to pass the pointer > around until it reaches its final "owner" member. > > See "calling conventions" section here: > https://www.chromium.org/developers/smart-pointer-guidelines (what applies to > unique_ptr also applies to scoped_refptr, per e-mail discussion on > chromium-dev@). > > There are several of these in this change, so please be sure to fix them too. :) Thanks for making me aware of this. I would like to keep the scope of this CL limited to breaking the dependencies. Changing these signatures would require reviewing all call sites as well and deciding on whether or not to use std::move(). I added a new planned CL (1.6.1.2) to the design doc to do this work in a separate CL. https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_device_client.cc:39: pool_(pool), On 2016/09/12 20:16:46, miu wrote: > For example, when passing scoped_refptr by value, you would use std::move() > here: > > pool_(std::move(pool)), Acknowledged. https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.h (right): https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.h:55: virtual STATUS GetStatus() = 0; On 2016/09/12 20:16:46, miu wrote: > Why was 'const' removed here too? Same as other case.
Updated comments after offline discussion. https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_buffer_pool.h (right): https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_buffer_pool.h:39: class CONTENT_EXPORT ProducerVideoCaptureBufferPool On 2016/09/12 22:23:33, chfremer wrote: > On 2016/09/12 20:16:46, miu wrote: > > The naming feels reversed to me: Shouldn't the interface be named > > VideoCaptureBufferPool, and the concrete implementation be named > > ProducerVideoCaptureBufferPool? > > I understand why it feels reversed. > The reason I chose the names like this is as follows: > Instances of VideoCaptureBufferPool seem to have more than one type of client. > One such client is the class media::VideoCaptureDeviceClient. This client calls > only a subset of the public methods of VideoCaptureBufferPool. These are the > methods I extracted into this interface. Based on the method names, I believed > that this client, from the perspective of the pool, is called a "Producer" (but > I may be off with this). > > Without having it analyzed too much, I am assuming that there is at least a > second type of client that calls the remaining public methods, and I am assuming > that, from the perspective of the pool, is called a "Consumer". > > To properly model this, we should have the concrete class VideoCaptureBufferPool > expose two separate interfaces (one for each type of client). To distinguish > them, my best idea for naming is to call one "ProducerVideoCaptureBufferPool" > and the other "ConsumerVideoCaptureBufferPool", even though I see that these > names are not great at explaining themselves. > > I have extracted the methods for the "Producer" client, but not yet the ones for > the "Consumer" client (since it was not yet needed). But maybe the change would > be clearer if I would do that second interface in this CL as well? > > Please let me know if you have any good suggestions for this situation. > Had an offline discussion with mcasas@ and emircan@. We agreed on doing the following. Use an interface "VideoCaptureBufferPool" that contains _all_ public methods (as opposed to just the ones that are called by VideoCaptureDeviceClient). Then rename the implementation to "VideoCaptureBufferPoolImpl". The downside of this solution is that it does not separate the interfaces required by different client, which make them depend on more than they need. But it makes it possible to conform to the simple scheme of having one interface and one implementation.
* Renamed VideoCaptureBufferPool -> VideoCaptureBufferPoolImpl * Fixed missing type rename in test code. (Not sure why not caught by bots on earlier Patch Set). https://codereview.chromium.org/2308533003/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_buffer_pool.h (right): https://codereview.chromium.org/2308533003/diff/80001/content/browser/rendere... content/browser/renderer_host/media/video_capture_buffer_pool.h:92: }; On 2016/09/12 18:31:22, chfremer wrote: > On 2016/09/09 22:28:01, mcasas wrote: > > nit : add here a > > DISALLOW_IMPLICIT_CONSTRUCTORS(ClassName); > > > > actually, it should be added to all new base classes, > > either that macro or DISALLOW_COPY_AND_ASSIGN, > > whichever makes sense > > Done, but I have a question about it. > Why is this needed? Interface classed cannot be instantiated anyway, because > they consist of purely virtual methods only. Hmm. I tried both, but neither of the macros seems to be suitable here. Since they remove the default constructor, derived classes can no longer instantiate, and we get a compiler error. https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_buffer_pool.h (right): https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_buffer_pool.h:39: class CONTENT_EXPORT ProducerVideoCaptureBufferPool On 2016/09/12 23:57:10, chfremer wrote: > On 2016/09/12 22:23:33, chfremer wrote: > > On 2016/09/12 20:16:46, miu wrote: > > > The naming feels reversed to me: Shouldn't the interface be named > > > VideoCaptureBufferPool, and the concrete implementation be named > > > ProducerVideoCaptureBufferPool? > > > > I understand why it feels reversed. > > The reason I chose the names like this is as follows: > > Instances of VideoCaptureBufferPool seem to have more than one type of client. > > One such client is the class media::VideoCaptureDeviceClient. This client > calls > > only a subset of the public methods of VideoCaptureBufferPool. These are the > > methods I extracted into this interface. Based on the method names, I believed > > that this client, from the perspective of the pool, is called a "Producer" > (but > > I may be off with this). > > > > Without having it analyzed too much, I am assuming that there is at least a > > second type of client that calls the remaining public methods, and I am > assuming > > that, from the perspective of the pool, is called a "Consumer". > > > > To properly model this, we should have the concrete class > VideoCaptureBufferPool > > expose two separate interfaces (one for each type of client). To distinguish > > them, my best idea for naming is to call one "ProducerVideoCaptureBufferPool" > > and the other "ConsumerVideoCaptureBufferPool", even though I see that these > > names are not great at explaining themselves. > > > > I have extracted the methods for the "Producer" client, but not yet the ones > for > > the "Consumer" client (since it was not yet needed). But maybe the change > would > > be clearer if I would do that second interface in this CL as well? > > > > Please let me know if you have any good suggestions for this situation. > > > > Had an offline discussion with mcasas@ and emircan@. We agreed on doing the > following. Use an interface "VideoCaptureBufferPool" that contains _all_ public > methods (as opposed to just the ones that are called by > VideoCaptureDeviceClient). Then rename the implementation to > "VideoCaptureBufferPoolImpl". > > The downside of this solution is that it does not separate the interfaces > required by different client, which make them depend on more than they need. But > it makes it possible to conform to the simple scheme of having one interface and > one implementation. One more thing maybe as a link to the "literature" explaining my initial motivation to split the interfaces. This is the I in SOLID. https://en.wikipedia.org/wiki/Interface_segregation_principle
https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_buffer_pool.h (right): https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_buffer_pool.h:39: class CONTENT_EXPORT ProducerVideoCaptureBufferPool On 2016/09/12 22:23:33, chfremer wrote: > On 2016/09/12 20:16:46, miu wrote: > > The naming feels reversed to me: Shouldn't the interface be named > > VideoCaptureBufferPool, and the concrete implementation be named > > ProducerVideoCaptureBufferPool? > > I understand why it feels reversed. > The reason I chose the names like this is as follows: > Instances of VideoCaptureBufferPool seem to have more than one type of client. > One such client is the class media::VideoCaptureDeviceClient. This client calls > only a subset of the public methods of VideoCaptureBufferPool. These are the > methods I extracted into this interface. Based on the method names, I believed > that this client, from the perspective of the pool, is called a "Producer" (but > I may be off with this). OIC now. Hmm...any other names I can think of sound stranger than this. Perhaps, later in the refactoring process (a future change), it'll become more obvious what to call this? ;-) ProducerVCBufferPool is fine for now. > Without having it analyzed too much, I am assuming that there is at least a > second type of client that calls the remaining public methods, and I am assuming > that, from the perspective of the pool, is called a "Consumer". Yep. > To properly model this, we should have the concrete class VideoCaptureBufferPool > expose two separate interfaces (one for each type of client). To distinguish > them, my best idea for naming is to call one "ProducerVideoCaptureBufferPool" > and the other "ConsumerVideoCaptureBufferPool", even though I see that these > names are not great at explaining themselves. > > I have extracted the methods for the "Producer" client, but not yet the ones for > the "Consumer" client (since it was not yet needed). But maybe the change would > be clearer if I would do that second interface in this CL as well? It's fine to leave this for a later change. BTW--You will have conflicts if the ConsumerVCBufferPool also implements base::RefCountedThreadSafe<CVCBPool>, since then VideoCaptureBufferPool would be inheriting two different base::RefCountedThreadSafe<> types, each of which thinks it can "delete this". Thus, if you really don't need the ref-counting, because object ownership and lifetime is well-defined across threads, by all means *do* try to remove it. This is likely easier to do in earlier changes (per my experience w/ refactoring, anyways). https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_buffer_pool.h:40: : public base::RefCountedThreadSafe<ProducerVideoCaptureBufferPool> { On 2016/09/12 22:23:33, chfremer wrote: > On 2016/09/12 20:16:46, miu wrote: > > I looked in the design doc, but I could not find an answer to this question: > > What are the lifetime semantics of the buffer pool? Before your changes, there > > was a single buffer pool that is shared by all video capture devices, and it > > lived in the browser process (memory shared to render processes as needed). > > > > Are any of these properties changing in this change? How about in future > > changes? > > My understanding of how things currently work is different. To me it seems that > there is one buffer pool per device. This CL does not change anything about > that. My mistake. I must've been remembering the design from a long time ago. > I am planning to keep things the way they are now, i.e. one buffer pool per > device, but instead of "Chromium IPC shared memory" I will switch to "Mojo > shared buffers" down the line. SGTM. You will need the same fine-grained control over the shared memory to support things like the "resurrect" feature; and hopefully porting the code to use the Mojo shmem will be trivial (IIUC). https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_buffer_pool.h:85: virtual double GetBufferPoolUtilization() = 0; On 2016/09/12 22:23:33, chfremer wrote: > On 2016/09/12 20:16:46, miu wrote: > > Why was 'const' removed from the method signature? > > This may be a topic worth discussing with a larger audience. I do have counter-arguments on this topic, but while they would be strong arguments in many situations, they are not very strong in this particular situation here. So, in short, practical considerations trump theoretical ones; which is why I'm fine if you want to remove the const. But my original question was: Why? I was curious whether some assumption changed, because of the refactoring, that required the method to be non-const. https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.cc:92: PostOnIOThreadVideoFrameReceiver( Sorry, didn't see this the last time around: Please add 'explicit' keyword for one-arg ctors to avoid implicit construction. https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.cc:96: void OnIncomingCapturedVideoFrame( On 2016/09/12 22:23:33, chfremer wrote: > On 2016/09/12 20:16:46, miu wrote: > > Please use base::ThreadChecker to ensure all these methods are being called by > > the same thread. This ensures the sequence of method calls is preserved when > > tasks are run on the IO thread. > > Hmm, I am not sure I agree that this would be an improvement. > > If clients make calls to this class from multiple threads, they already cannot > expect the calls to be processed in-order. And if clients want to call from > multiple threads while also making sure that no overlapping calls happen, they > should be allowed to and they are already guaranteed to have the order > preserved. > > Am I missing something here? Generally, in Chromium code, we try to adhere to the "one thread <--> one class" design guideline as much as possible. (Granted, there are lots of situations where it is not possible and/or not desirable.) There's actually no problem with the use of this class, in this change, they way it is. The problem stems from how this code will evolve over the years: For example, it could be that, several years from now, another developer comes along without all the context, takes a quick look, and makes a change that breaks some assumption about the multi-threaded access to this interface and/or which sequences of method calls were legal. They introduce a change, it is reviewed by another developer who also doesn't have all the context, and it introduces a subtle bug that only surfaces once released into the wild to millions of users. Often, it's a good idea to restrict the semantics of an interface/impl as much as possible, only to relax those restrictions later when a need arises. In this case, using a ThreadChecker restricts the potential for new bugs to those that would only exist for single-threaded method calling. Such bugs are much easier to find during development and/or via unit testing. Again, most code review comments are suggestions. So, take it or leave it. :)
miu@: PTAL mcasas@: PTAL * Renamed class PostOnIOThreadVideoFrameReceiver to VideoFrameReceiverOnIOThread in order to adhere to naming schema of putting the name of the implemented interface before a postfix expressing the specialization. * Make constructor explicit. https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_buffer_pool.h (right): https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_buffer_pool.h:85: virtual double GetBufferPoolUtilization() = 0; On 2016/09/13 00:33:29, miu wrote: > On 2016/09/12 22:23:33, chfremer wrote: > > On 2016/09/12 20:16:46, miu wrote: > > > Why was 'const' removed from the method signature? > > > > This may be a topic worth discussing with a larger audience. > > I do have counter-arguments on this topic, but while they would be strong > arguments in many situations, they are not very strong in this particular > situation here. So, in short, practical considerations trump theoretical ones; > which is why I'm fine if you want to remove the const. > > But my original question was: Why? I was curious whether some assumption > changed, because of the refactoring, that required the method to be non-const. Hehe, the answer to the "why?" was supposed to be that I tried to apply something I believed to be a general good practice. There is no change in semantics or behavior that would require the method to be non-const. I would love to learn more about the counter-arguments and also if there are any existing guidelines on how to handle this in Chromium. I feel there should be an agreed upon practice for Chromium so that we can apply it consistently, but I found nothing in the coding style docs. I don't think it makes sense if I am the only dev on the project that avoids the use of const in interfaces while everyone else wants to see it be used. Is this something that has been discussed before on the chromium-dev list? If not, I could start a discussion thread there to see if this practice would get any buy-in. Would that make sense? https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.cc:92: PostOnIOThreadVideoFrameReceiver( On 2016/09/13 00:33:29, miu wrote: > Sorry, didn't see this the last time around: Please add 'explicit' keyword for > one-arg ctors to avoid implicit construction. Done. https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.cc:96: void OnIncomingCapturedVideoFrame( On 2016/09/13 00:33:29, miu wrote: > On 2016/09/12 22:23:33, chfremer wrote: > > On 2016/09/12 20:16:46, miu wrote: > > > Please use base::ThreadChecker to ensure all these methods are being called > by > > > the same thread. This ensures the sequence of method calls is preserved when > > > tasks are run on the IO thread. > > > > Hmm, I am not sure I agree that this would be an improvement. > > > > If clients make calls to this class from multiple threads, they already cannot > > expect the calls to be processed in-order. And if clients want to call from > > multiple threads while also making sure that no overlapping calls happen, they > > should be allowed to and they are already guaranteed to have the order > > preserved. > > > > Am I missing something here? > > Generally, in Chromium code, we try to adhere to the "one thread <--> one class" > design guideline as much as possible. (Granted, there are lots of situations > where it is not possible and/or not desirable.) > > There's actually no problem with the use of this class, in this change, they way > it is. The problem stems from how this code will evolve over the years: For > example, it could be that, several years from now, another developer comes along > without all the context, takes a quick look, and makes a change that breaks some > assumption about the multi-threaded access to this interface and/or which > sequences of method calls were legal. They introduce a change, it is reviewed by > another developer who also doesn't have all the context, and it introduces a > subtle bug that only surfaces once released into the wild to millions of users. > > Often, it's a good idea to restrict the semantics of an interface/impl as much > as possible, only to relax those restrictions later when a need arises. In this > case, using a ThreadChecker restricts the potential for new bugs to those that > would only exist for single-threaded method calling. Such bugs are much easier > to find during development and/or via unit testing. > > Again, most code review comments are suggestions. So, take it or leave it. :) Thanks. This insight is super useful for me learning the ways of Chromium. Since I am still new to the project, I am trying to find a good set of practices that can be applied in most situations, so please allow me to dig into this a little more. I agree that it is probably a good idea to restrict classes to be used from a single thread by default and only relax this restriction where needed, for the benefit of being able to add sanity checks that verify the single-thread usage and protect us from subtle and hard-to-find bugs. Since I am looking for practices that can/should be applied in most situations, I am wondering if using a ThreadChecker to enforce single-threaded usage should then be something I apply to _all_ classes that do not explicitly require multi-threaded (not necessarily concurrent) operation. I do not see any particular reason why this practice would apply to this class more than to any other, so consistency would guide me towards applying this practice everywhere. Would that be okay from your perspective? In the particular case of this class here, there is one concern that I have about doing it as part of this CL: It would introduce a change of behavior. Before this CL, there is no enforcement of these calls to arrive from a single thread. Calls are initiated from the platform-specific implementations of media::VideoCaptureDevice (and are passing through VideoCaptureDeviceClient). Without checking every implementation carefully, I feel it is very well possible that for some of those implementations calls are issued from different threads. If we want to make this change in behavior, I would prefer to do this in a separate step. https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.cc:118: BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, On 2016/09/12 22:23:33, chfremer wrote: > On 2016/09/12 20:16:46, miu wrote: > > This makes me wonder whether receiver_ needs to be a scoped_refptr<> instead. > > The buffer must not be destroyed until the client is done using it, but what > you > > have here with WeakPtr seems to imply the buffer can be destroyed out from > > underneath a client. > > > > Or, are you depending on the operating system to keep the shared memory around > > in the render process for as long as it's needed, and we shouldn't be worried > > about this? If so, will this scheme work for GPU memory buffers and other > types? > > We won't be leaking memory in the GPU or GPU process? > > Thanks. This is an interesting question. > I haven't thought about this, because I am attempting to do a refactoring that I > believe does not change any of the existing behavior. > > Please let me read up a bit more on how this mechanism is supposed to work > currently, and then I will respond back. After thinking about it more carefully, I don't think I fully understand what you mean. I understand that a buffer must not be destroyed while someone is still using it. What I don't understand is how you infer a concern based on the fact that |receiver_| is a WeakPtr<> (as opposed to a scoped_refptr<>). After following the calls in the code, my current understanding is as follows. 1.) VideoCaptureDeviceClient calls VideoCaptureBufferPool::ReserveForProducer() on behalf of a "Producer", e.g. a capture device. 2.) The VideoCaptureBufferPool may decide that in order to fulfill the request, it needs to drop an existing buffer. If that is the case it returns the id of the buffer it has dropped back to the VideoCaptureDeviceClient. 3.) The VideoCaptureDeviceClient calls OnBufferDestroyed() to notify the "Consumer" that the buffer with the id is no longer valid. Without having analyzed exhaustively, I assume that the VideoCaptureBufferPool only decides to drop buffers that are known to no longer be in use by both Producers and Consumers. The "hold" and "relinquish hold" methods are probably offered to allow those clients to tell the VideoCaptureBufferPool when they are done with using a buffer. This CL does not change anything about how this works.
Responses here (rather lengthy, sorry). Let me know when you are ready for me to take another look. https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_buffer_pool.h (right): https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_buffer_pool.h:85: virtual double GetBufferPoolUtilization() = 0; On 2016/09/14 16:26:01, chfremer wrote: > On 2016/09/13 00:33:29, miu wrote: > > On 2016/09/12 22:23:33, chfremer wrote: > > > On 2016/09/12 20:16:46, miu wrote: > > > > Why was 'const' removed from the method signature? > > > > > > This may be a topic worth discussing with a larger audience. > > > I would love to learn more about the counter-arguments and also if there are any > existing guidelines on how to handle this in Chromium. I feel there should be an > agreed upon practice for Chromium so that we can apply it consistently, but I > found nothing in the coding style docs. First of all, let me say: THANK YOU so much for caring about interface design, naming, and code structure! :) I really wish more developers paid attention to details like this because making poor decisions in these areas costs us *all* dearly in terms of future code changes/maintenance. As for general guidance, the Chromium C++ style guide starts with the Google C++ style guide, which discusses the use of const: https://google.github.io/styleguide/cppguide.html#Use_of_const Granted, in the case here, the discussion in the style guide could be interpreted as either "make this const" or "it doesn't make sense to make this const." Here's how I look at this particular case: 1. Your argument (IIUC): The client of a "pure interface" shouldn't care whether the implementation of the interface actually mutates state or not, so the interface should not restrict that. 2. My support for your argument: The more "distant" an interface is from its implementation and/or client code modules, the more I would tend to agree. The reason here is that the cost of tighter-coupling of the client code and the implementation--via the interface--is high. Meaning, making any interface changes (such as removing the const), would take a lot more effort (e.g., more code churn, more code reviewers, OWNERS stamps, etc.). What I mean by "distant" interfaces: Those defined in src/content/public, third-party library includes, etc. 3. My counter-arguments sum up to: Start restrictively, then relax; as a matter of safety (and other "pros" listed in the style guide). As of today, we know all implementations of this "getter" will not mutate state. Also, changing this interface later (to remove the const) is very low cost because basically it involves changing a small number of files, with code review from a small number of developers around you. One other note: If there ever was a reason the implementation needed to have the const removed, a reviewer would be presented with a code change that shows a clear reason that a "getter" operation needs to mutate state. The developer may actually chose to re-name the method when making such a change. > I don't think it makes sense if I am the only dev on the project that avoids the > use of const in interfaces while everyone else wants to see it be used. Is this > something that has been discussed before on the chromium-dev list? If not, I > could start a discussion thread there to see if this practice would get any > buy-in. Would that make sense? If you start one, I'm sure you're gonna get 100 different answers/opinions. ;-) FWIW, I drank this cool-aid long ago: http://gamesfromwithin.com/the-const-nazi There have been several cases where I cleaned-up code as part of my project efforts, added more consts, and actually found hidden bugs as a result. I've also been told by others I can sometimes be too excessive in my use of const. This is mainly influenced by my past experience with certain functional programming languages, where the focus was on instantiating immutable values and transforming them, rather than imperatively mutating variables. https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.cc:96: void OnIncomingCapturedVideoFrame( On 2016/09/14 16:26:01, chfremer wrote: > Since I am looking for practices that can/should be applied in most situations, > I am wondering if using a ThreadChecker to enforce single-threaded usage should > then be something I apply to _all_ classes that do not explicitly require > multi-threaded (not necessarily concurrent) operation. There is variance in styles/attitudes, depending on which part of the code base you're in. AFAICT, the most general consensus is: 1. If a code module clearly never makes any references to threads, task runners, etc., it should be assumed to be non-thread-safe. 2. In situations where a code module is not thread safe, but seems to be used by a lot of code that cares about the threading strategy, people will often either: a) use base::ThreadChecker anyway; or b) use base::NonThreadSafe. > I do not see any > particular reason why this practice would apply to this class more than to any > other, so consistency would guide me towards applying this practice everywhere. > Would that be okay from your perspective? Probably. Unfortunately, I'm struggling to explain more concretely. Umm...let experience and intuition be your guide? ;-) I lean towards safety, but also practicality. So, I wouldn't add a whole bunch of code to do thread DCHECKing if I feel its fairly obvious to other developers that the code I wrote wasn't intended to be thread-safe. After all, the reason to do all this is to avoid unpleasantly surprising yourself and other developers down the road. The ROI here is roughly: investment="additional effort of writing checks and in sometimes being wrong and having to fix them", reward="eliminate time spent resolving bugs + peace of mind" So, your own judgement is needed to quantify these terms. > In the particular case of this class here, there is one concern that I have > about doing it as part of this CL: It would introduce a change of behavior. > Before this CL, there is no enforcement of these calls to arrive from a single > thread. Calls are initiated from the platform-specific implementations of > media::VideoCaptureDevice (and are passing through VideoCaptureDeviceClient). > Without checking every implementation carefully, I feel it is very well possible > that for some of those implementations calls are issued from different threads. Yeah, it's possible they might be called from different threads. With this interface, FWICT, there *might* be a sequence relationship between OnIncomingCapturedVideoFrame() and OnBufferDestroyed(), but as for the other two methods, call sequence shouldn't really matter. One possibility here is to use ThreadChecker only on those two methods (the ones where sequence matters). However, if you think it's unlikely there's a sequence relationship, maybe no ThreadCheck'ing is appropriate here. As for checking all the platform-specific implementations: Yeah, that can be tedious. One suggestion: Add the checks and then do a `git cl try` to do a CQ dry-run, which will runs tests on all platforms. If things are green, go ahead and commit the change. After that, there are plenty of other things to catch problems (e.g., more build configurations, and the ASAN/TSAN builds to test thread-safety). > If we want to make this change in behavior, I would prefer to do this in a > separate step. That sounds reasonable to me. https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.cc:118: BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, On 2016/09/14 16:26:01, chfremer wrote: > On 2016/09/12 22:23:33, chfremer wrote: > > On 2016/09/12 20:16:46, miu wrote: > > > This makes me wonder whether receiver_ needs to be a scoped_refptr<> > instead. > > > The buffer must not be destroyed until the client is done using it, but what > > you > > > have here with WeakPtr seems to imply the buffer can be destroyed out from > > > underneath a client. > > What I don't understand is how you infer a concern based on the fact that > |receiver_| is a WeakPtr<> (as opposed to a scoped_refptr<>). What I was thinking (I now realize is not quite right): I was assuming the memory backing the buffer had to remain valid until OnBufferDestroyed() was called. In fact, the naming suggests I already was wrong about that. ;-) So, I thought that VideoCaptureController (the impl of VideoFrameReceiver), which owns the VideoCaptureBufferPool, could destroy the buffer pool while the buffer memory was still in use. > This CL does not change anything about how this works. I took another look, and agree. The thing I didn't notice originally was that VCDeviceClient held a WeakPtr to VCController. With your change, this becomes: VCDeviceClient holds a strong ptr to VideoFrameReceiver, and this impl of VideoFrameReceiver then holds the WeakPtr to VCController. So, the ownership of objects and pointer validity should be exactly the same.
miu@: PTAL mcasas@: PTAL https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_buffer_pool.h (right): https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_buffer_pool.h:85: virtual double GetBufferPoolUtilization() = 0; On 2016/09/16 21:10:25, miu wrote: > On 2016/09/14 16:26:01, chfremer wrote: > > On 2016/09/13 00:33:29, miu wrote: > > > On 2016/09/12 22:23:33, chfremer wrote: > > > > On 2016/09/12 20:16:46, miu wrote: > > > > > Why was 'const' removed from the method signature? > > > > > > > > This may be a topic worth discussing with a larger audience. > > > > > I would love to learn more about the counter-arguments and also if there are > any > > existing guidelines on how to handle this in Chromium. I feel there should be > an > > agreed upon practice for Chromium so that we can apply it consistently, but I > > found nothing in the coding style docs. > > First of all, let me say: THANK YOU so much for caring about interface design, > naming, and code structure! :) I really wish more developers paid attention to > details like this because making poor decisions in these areas costs us *all* > dearly in terms of future code changes/maintenance. Thanks so much for these kind words and for spending the effort of presenting such a clear and detailed response. > As for general guidance, the Chromium C++ style guide starts with the Google C++ > style guide, which discusses the use of const: > https://google.github.io/styleguide/cppguide.html#Use_of_const > > Granted, in the case here, the discussion in the style guide could be > interpreted as either "make this const" or "it doesn't make sense to make this > const." Here's how I look at this particular case: Agreed. I reviewed the guidelines and they are really quite vague on this topic. > 1. Your argument (IIUC): The client of a "pure interface" shouldn't care whether > the implementation of the interface actually mutates state or not, so the > interface should not restrict that. Correct. And I think I would even go a bit further and say that a pure interface should not dictate whether or not implementations are allowed to mutate state. One example would be a Mock implementation of a getter method that (legitimately) wants to increase a call counter. > 2. My support for your argument: The more "distant" an interface is from its > implementation and/or client code modules, the more I would tend to agree. The > reason here is that the cost of tighter-coupling of the client code and the > implementation--via the interface--is high. Meaning, making any interface > changes (such as removing the const), would take a lot more effort (e.g., more > code churn, more code reviewers, OWNERS stamps, etc.). What I mean by "distant" > interfaces: Those defined in src/content/public, third-party library includes, > etc. Makes sense. The "distant" probably can mean the same as "widely used", which in turn makes changes expensive. > 3. My counter-arguments sum up to: Start restrictively, then relax; as a matter > of safety (and other "pros" listed in the style guide). As of today, we know all > implementations of this "getter" will not mutate state. Also, changing this > interface later (to remove the const) is very low cost because basically it > involves changing a small number of files, with code review from a small number > of developers around you. One other note: If there ever was a reason the > implementation needed to have the const removed, a reviewer would be presented > with a code change that shows a clear reason that a "getter" operation needs to > mutate state. The developer may actually chose to re-name the method when making > such a change. "Start restrictively, then relax" sounds like a good principle to follow. A downside of this could be that the change may already have become quite expensive at the time where we decide to relax a restriction. But I realize that facing the realities of a large project like this one, the advantages of following this principle may outweigh the disadvantages by a wide margin. > > I don't think it makes sense if I am the only dev on the project that avoids > the > > use of const in interfaces while everyone else wants to see it be used. Is > this > > something that has been discussed before on the chromium-dev list? If not, I > > could start a discussion thread there to see if this practice would get any > > buy-in. Would that make sense? > > If you start one, I'm sure you're gonna get 100 different answers/opinions. ;-) > > FWIW, I drank this cool-aid long ago: http://gamesfromwithin.com/the-const-nazi > There have been several cases where I cleaned-up code as part of my project > efforts, added more consts, and actually found hidden bugs as a result. > > I've also been told by others I can sometimes be too excessive in my use of > const. This is mainly influenced by my past experience with certain functional > programming languages, where the focus was on instantiating immutable values and > transforming them, rather than imperatively mutating variables. To me it feels like the usefulness of const scales with the complexity of the "units" we author. Idealistically, I would like to believe that, in theory, we could make our units (such as methods, classes, and interfaces) small enough such that const loses its usefulness. Using the example of a "getter" method, if it is small enough, it should become completely obvious to both authors and readers of the implementation that it does not mutate state. As a result of this consideration, I feel that a need for relying on mechanisms such as const is a warning sign that our "units" are too large and complex. I realize that in an open-source project of this scale we are probably not going to get to a point where const loses its usefulness. Considering that, I will go with embracing const for now :-). https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.cc:96: void OnIncomingCapturedVideoFrame( On 2016/09/16 21:10:25, miu wrote: > On 2016/09/14 16:26:01, chfremer wrote: > > Since I am looking for practices that can/should be applied in most > situations, > > I am wondering if using a ThreadChecker to enforce single-threaded usage > should > > then be something I apply to _all_ classes that do not explicitly require > > multi-threaded (not necessarily concurrent) operation. > > There is variance in styles/attitudes, depending on which part of the code base > you're in. AFAICT, the most general consensus is: > > 1. If a code module clearly never makes any references to threads, task runners, > etc., it should be assumed to be non-thread-safe. > > 2. In situations where a code module is not thread safe, but seems to be used by > a lot of code that cares about the threading strategy, people will often either: > a) use base::ThreadChecker anyway; or b) use base::NonThreadSafe. > > > I do not see any > > particular reason why this practice would apply to this class more than to any > > other, so consistency would guide me towards applying this practice > everywhere. > > Would that be okay from your perspective? > > Probably. Unfortunately, I'm struggling to explain more concretely. Umm...let > experience and intuition be your guide? ;-) > > I lean towards safety, but also practicality. So, I wouldn't add a whole bunch > of code to do thread DCHECKing if I feel its fairly obvious to other developers > that the code I wrote wasn't intended to be thread-safe. After all, the reason > to do all this is to avoid unpleasantly surprising yourself and other developers > down the road. The ROI here is roughly: investment="additional effort of writing > checks and in sometimes being wrong and having to fix them", reward="eliminate > time spent resolving bugs + peace of mind" So, your own judgement is needed to > quantify these terms. > Thanks for this explanation, which I find helpful. It really seems that a case-by-case judgement call is needed. > > In the particular case of this class here, there is one concern that I have > > about doing it as part of this CL: It would introduce a change of behavior. > > Before this CL, there is no enforcement of these calls to arrive from a single > > thread. Calls are initiated from the platform-specific implementations of > > media::VideoCaptureDevice (and are passing through VideoCaptureDeviceClient). > > Without checking every implementation carefully, I feel it is very well > possible > > that for some of those implementations calls are issued from different > threads. > > Yeah, it's possible they might be called from different threads. With this > interface, FWICT, there *might* be a sequence relationship between > OnIncomingCapturedVideoFrame() and OnBufferDestroyed(), but as for the other two > methods, call sequence shouldn't really matter. One possibility here is to use > ThreadChecker only on those two methods (the ones where sequence matters). > However, if you think it's unlikely there's a sequence relationship, maybe no > ThreadCheck'ing is appropriate here. > > As for checking all the platform-specific implementations: Yeah, that can be > tedious. One suggestion: Add the checks and then do a `git cl try` to do a CQ > dry-run, which will runs tests on all platforms. If things are green, go ahead > and commit the change. After that, there are plenty of other things to catch > problems (e.g., more build configurations, and the ASAN/TSAN builds to test > thread-safety). > > > If we want to make this change in behavior, I would prefer to do this in a > > separate step. > > That sounds reasonable to me. Excellent suggestion. I added this as planned CL1.6.1.3 to the design doc. https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_controller.cc:118: BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, On 2016/09/16 21:10:25, miu wrote: > On 2016/09/14 16:26:01, chfremer wrote: > > On 2016/09/12 22:23:33, chfremer wrote: > > > On 2016/09/12 20:16:46, miu wrote: > > > > This makes me wonder whether receiver_ needs to be a scoped_refptr<> > > instead. > > > > The buffer must not be destroyed until the client is done using it, but > what > > > you > > > > have here with WeakPtr seems to imply the buffer can be destroyed out from > > > > underneath a client. > > > > What I don't understand is how you infer a concern based on the fact that > > |receiver_| is a WeakPtr<> (as opposed to a scoped_refptr<>). > > What I was thinking (I now realize is not quite right): I was assuming the > memory backing the buffer had to remain valid until OnBufferDestroyed() was > called. In fact, the naming suggests I already was wrong about that. ;-) So, I > thought that VideoCaptureController (the impl of VideoFrameReceiver), which owns > the VideoCaptureBufferPool, could destroy the buffer pool while the buffer > memory was still in use. > > > This CL does not change anything about how this works. > > I took another look, and agree. The thing I didn't notice originally was that > VCDeviceClient held a WeakPtr to VCController. With your change, this becomes: > VCDeviceClient holds a strong ptr to VideoFrameReceiver, and this impl of > VideoFrameReceiver then holds the WeakPtr to VCController. So, the ownership of > objects and pointer validity should be exactly the same. Yes, that is exactly right. Adding the decorator between the original two instances made it a bit hard to see at first. https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... File content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.h (right): https://codereview.chromium.org/2308533003/diff/100001/content/browser/render... content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.h:55: virtual STATUS GetStatus() = 0; On 2016/09/12 22:23:33, chfremer wrote: > On 2016/09/12 20:16:46, miu wrote: > > Why was 'const' removed here too? > > Same as other case. Reverted as per decision from our discussion at the other case.
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/17 00:18:37, miu wrote: > lgtm lgtm, ship it!
The CQ bit was checked by chfremer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from emircan@chromium.org Link to the patchset: https://codereview.chromium.org/2308533003/#ps160001 (title: "miu's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Break tight coupling of VideoCaptureDeviceClient to renderer_host This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL1.6. This CL is a step towards being able to reuse the classes VideoCaptureDeviceClient and VideoCaptureBufferPool as part of the Video Capture Mojo Service, because they implement functionality that is needed there. Expressed in class-diagram form, we want to go from [2] to [3]. Currently, the classes VideoCaptureDeviceClient and VideoCaptureBufferPool live in content/browser/renderer_host/media. Even though their functionality is not specific to renderer_host, their dependencies create a tight coupling to code that is. In order to use them in services/video_capture, we need to move them out of there (and probably into media/capture/video). In this CL we break the tight coupling by extracting interfaces and having the two classes depend on the abstractions instead of the implementations. The class VideoCaptureDeviceClient has a second type of tight coupling, which is that it knows about BrowserThread and wants to post tasks to the Browser IO thread. We break this dependency by moving the logic to forward calls to the Browser IO Thread into a decorator "PostOnBrowserIoThreadDecorator" (using the Decorator Pattern). No new files are created in this CL. Interfaces will be moved to new files in a new location in a follow-up CL. BUG=584797 TEST=This is a pure refactoring. No new functionality. All tests still pass. [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... [2] https://docs.google.com/a/chromium.org/drawings/d/1jgDkxl1WvQCO-zeg87eLh5XkB4... [3] https://docs.google.com/a/chromium.org/drawings/d/12yOdum03TP1dL7DmUjPxtAfSo1... ========== to ========== Break tight coupling of VideoCaptureDeviceClient to renderer_host This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL1.6. This CL is a step towards being able to reuse the classes VideoCaptureDeviceClient and VideoCaptureBufferPool as part of the Video Capture Mojo Service, because they implement functionality that is needed there. Expressed in class-diagram form, we want to go from [2] to [3]. Currently, the classes VideoCaptureDeviceClient and VideoCaptureBufferPool live in content/browser/renderer_host/media. Even though their functionality is not specific to renderer_host, their dependencies create a tight coupling to code that is. In order to use them in services/video_capture, we need to move them out of there (and probably into media/capture/video). In this CL we break the tight coupling by extracting interfaces and having the two classes depend on the abstractions instead of the implementations. The class VideoCaptureDeviceClient has a second type of tight coupling, which is that it knows about BrowserThread and wants to post tasks to the Browser IO thread. We break this dependency by moving the logic to forward calls to the Browser IO Thread into a decorator "PostOnBrowserIoThreadDecorator" (using the Decorator Pattern). No new files are created in this CL. Interfaces will be moved to new files in a new location in a follow-up CL. BUG=584797 TEST=This is a pure refactoring. No new functionality. All tests still pass. [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... [2] https://docs.google.com/a/chromium.org/drawings/d/1jgDkxl1WvQCO-zeg87eLh5XkB4... [3] https://docs.google.com/a/chromium.org/drawings/d/12yOdum03TP1dL7DmUjPxtAfSo1... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Break tight coupling of VideoCaptureDeviceClient to renderer_host This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL1.6. This CL is a step towards being able to reuse the classes VideoCaptureDeviceClient and VideoCaptureBufferPool as part of the Video Capture Mojo Service, because they implement functionality that is needed there. Expressed in class-diagram form, we want to go from [2] to [3]. Currently, the classes VideoCaptureDeviceClient and VideoCaptureBufferPool live in content/browser/renderer_host/media. Even though their functionality is not specific to renderer_host, their dependencies create a tight coupling to code that is. In order to use them in services/video_capture, we need to move them out of there (and probably into media/capture/video). In this CL we break the tight coupling by extracting interfaces and having the two classes depend on the abstractions instead of the implementations. The class VideoCaptureDeviceClient has a second type of tight coupling, which is that it knows about BrowserThread and wants to post tasks to the Browser IO thread. We break this dependency by moving the logic to forward calls to the Browser IO Thread into a decorator "PostOnBrowserIoThreadDecorator" (using the Decorator Pattern). No new files are created in this CL. Interfaces will be moved to new files in a new location in a follow-up CL. BUG=584797 TEST=This is a pure refactoring. No new functionality. All tests still pass. [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... [2] https://docs.google.com/a/chromium.org/drawings/d/1jgDkxl1WvQCO-zeg87eLh5XkB4... [3] https://docs.google.com/a/chromium.org/drawings/d/12yOdum03TP1dL7DmUjPxtAfSo1... ========== to ========== Break tight coupling of VideoCaptureDeviceClient to renderer_host This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL1.6. This CL is a step towards being able to reuse the classes VideoCaptureDeviceClient and VideoCaptureBufferPool as part of the Video Capture Mojo Service, because they implement functionality that is needed there. Expressed in class-diagram form, we want to go from [2] to [3]. Currently, the classes VideoCaptureDeviceClient and VideoCaptureBufferPool live in content/browser/renderer_host/media. Even though their functionality is not specific to renderer_host, their dependencies create a tight coupling to code that is. In order to use them in services/video_capture, we need to move them out of there (and probably into media/capture/video). In this CL we break the tight coupling by extracting interfaces and having the two classes depend on the abstractions instead of the implementations. The class VideoCaptureDeviceClient has a second type of tight coupling, which is that it knows about BrowserThread and wants to post tasks to the Browser IO thread. We break this dependency by moving the logic to forward calls to the Browser IO Thread into a decorator "PostOnBrowserIoThreadDecorator" (using the Decorator Pattern). No new files are created in this CL. Interfaces will be moved to new files in a new location in a follow-up CL. BUG=584797 TEST=This is a pure refactoring. No new functionality. All tests still pass. [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... [2] https://docs.google.com/a/chromium.org/drawings/d/1jgDkxl1WvQCO-zeg87eLh5XkB4... [3] https://docs.google.com/a/chromium.org/drawings/d/12yOdum03TP1dL7DmUjPxtAfSo1... Committed: https://crrev.com/42b7da0b82c0f67702f3f0f0033d611ff44916f5 Cr-Commit-Position: refs/heads/master@{#419565} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/42b7da0b82c0f67702f3f0f0033d611ff44916f5 Cr-Commit-Position: refs/heads/master@{#419565} |
