Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2551193002:
[Mojo Video Capture] Decouple VCController from VCBufferPool and VCDeviceClient (Closed)
|
Created:
4 years ago by chfremer Modified:
4 years ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mcasas+watch+vc_chromium.org, xjz+watch_chromium.org, miu+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Mojo Video Capture] Decouple VideoCaptureController from VideoCaptureBufferPool and VideoCaptureDeviceClient
Decouple VideoCaptureController from VideoCaptureBufferPool and
VideoCaptureDeviceClient. This is needed in order to prepare
VideoCaptureController for working with both the (legacy) in-process video
capture and the new video_capture Mojo service.
List of changes:
* Introduce an abstraction FrameBufferHost for the VideoCaptureController to
talk to the VideoCaptureBufferPool.
* Move VideoFrameReceiverOnIOThread to separate file.
* Move factory logic for creating and connecting
VideoCaptureBufferPoolImpl and VideoCaptureDeviceClient out of
VideoCaptureController and into VideoCaptureManager as well as the tests.
* Update OWNERS to include new file and include chfremer as owner.
* In VideoCaptureDeviceClient: Rename |frame_format| to |foramt|.
This CL is part of the Mojo Video Capture work. For the bigger picture,
see [1] CL1.9.8
BUG=584797
TEST=
content_unittests,
video_capture_unittests,
Apprtc loopback on Debug,
Desktop Capture Example extension on Release
[1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxsxWslJ_X-zpOhijvI8/edit?usp=sharing
Committed: https://crrev.com/0b129bdbdd3099a492bcf014c22239c2b35071a2
Cr-Commit-Position: refs/heads/master@{#438604}
Patch Set 1 #
Total comments: 20
Patch Set 2 : mcasas comments #
Total comments: 20
Patch Set 3 : miu's comments #Patch Set 4 : Added TODO comment #Depends on Patchset: Dependent Patchsets: Messages
Total messages: 43 (28 generated)
Description was changed from ========== presubmit Cleanups for review Merge branch '1.9.7' into 1.9.8 Cleaned up VideoCaptureDeviceClient. Fixed content_unittests. Merge branch '1.9.7' into 1.9.8 Merge branch '1.9.7' into 1.9.8 Merge branch '1.9.7' into 1.9.8 Fixed compile Merge branch '1.9.7' into 1.9.8 Fixed merge errors Merge branch '1.9.7' into 1.9.8 Merge branch '1.9.7' into 1.9.8 Merge branch '1.9.7' into 1.9.8 Merge branch '1.9.7' into 1.9.8 Merge branch '1.9.7' into 1.9.8 Merge branch '1.9.7' into 1.9.8 Setting FrameReceiverObserver is mandatory Merge branch '1.9.7' into 1.9.8 Merge branch '1.9.7' into 1.9.8 Merge branch '1.9.7' into 1.9.8 Apply patch to 1.9.8 BUG= ========== to ========== [Mojo Video Capture] Decouple VideoCaptureController from VideoCaptureBufferPool and VideoCaptureDeviceClient Decouple VideoCaptureController from VideoCaptureBufferPool and VideoCaptureDeviceClient. This is needed in order to prepare VideoCaptureController for working with both the (legacy) in-process video capture and the new video_capture Mojo service. List of changes: * Introduce an abstraction FrameBufferHost for the VideoCaptureController to talk to the VideoCaptureBufferPool. * Move VideoFrameReceiverOnIOThread to separate file. * Move factory logic for creating and connecting VideoCaptureBufferPoolImpl and VideoCaptureDeviceClient out of VideoCaptureController and into VideoCaptureManager as well as the tests. This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL1.9.8 BUG=584797 TEST= content_unittests, video_capture_unittests, Apprtc loopback on Debug, Desktop Capture Example extension on Release [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... ==========
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...
Patchset #1 (id:1) has been deleted
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...
Description was changed from ========== [Mojo Video Capture] Decouple VideoCaptureController from VideoCaptureBufferPool and VideoCaptureDeviceClient Decouple VideoCaptureController from VideoCaptureBufferPool and VideoCaptureDeviceClient. This is needed in order to prepare VideoCaptureController for working with both the (legacy) in-process video capture and the new video_capture Mojo service. List of changes: * Introduce an abstraction FrameBufferHost for the VideoCaptureController to talk to the VideoCaptureBufferPool. * Move VideoFrameReceiverOnIOThread to separate file. * Move factory logic for creating and connecting VideoCaptureBufferPoolImpl and VideoCaptureDeviceClient out of VideoCaptureController and into VideoCaptureManager as well as the tests. This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL1.9.8 BUG=584797 TEST= content_unittests, video_capture_unittests, Apprtc loopback on Debug, Desktop Capture Example extension on Release [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... ========== to ========== [Mojo Video Capture] Decouple VideoCaptureController from VideoCaptureBufferPool and VideoCaptureDeviceClient Decouple VideoCaptureController from VideoCaptureBufferPool and VideoCaptureDeviceClient. This is needed in order to prepare VideoCaptureController for working with both the (legacy) in-process video capture and the new video_capture Mojo service. List of changes: * Introduce an abstraction FrameBufferHost for the VideoCaptureController to talk to the VideoCaptureBufferPool. * Move VideoFrameReceiverOnIOThread to separate file. * Move factory logic for creating and connecting VideoCaptureBufferPoolImpl and VideoCaptureDeviceClient out of VideoCaptureController and into VideoCaptureManager as well as the tests. * Update OWNERS to include new file and include chfremer as owner. * In VideoCaptureDeviceClient: Rename |frame_format| to |foramt|. This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL1.9.8 BUG=584797 TEST= content_unittests, video_capture_unittests, Apprtc loopback on Debug, Desktop Capture Example extension on Release [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... ==========
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
chfremer@chromium.org changed reviewers: + mcasas@chromium.org
mcasas@: PTAL
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.
Looking good. Would like miu@ to take a quick look too. https://codereview.chromium.org/2551193002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/OWNERS (right): https://codereview.chromium.org/2551193002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/OWNERS:10: per-file video_*=chfremer@chromium.org Maybe we could wrap the video_* files, or, equivalently, the *capture* files, to a subfolder |video| or |capture| (but not |video_capture| please), and just add a normal OWNERS file there with miu@, emircan@, you and me (note that these file would propagate down). Can be done subsequently. https://codereview.chromium.org/2551193002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/2551193002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:25: #include "media/capture/video/video_capture_buffer_pool_impl.h" Not needed anymore? https://codereview.chromium.org/2551193002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.h (right): https://codereview.chromium.org/2551193002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.h:150: class BufferState { Hmm we should move this full declaration to the .cc, file, right? And leave here just a forward declaration. Can be done subsequently too. https://codereview.chromium.org/2551193002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/2551193002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:225: scoped_refptr<media::VideoCaptureBufferPool> buffer_pool_; media::FrameBufferHost (and BufferPoolFrameBufferHost) are not ref counted whereas media::VideoCaptureBufferPool is. This forces you to use naked pointers in VCC::BufferState, is it because you envision the mojo-based interface to not be ref-counted...? https://codereview.chromium.org/2551193002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:476: base::Passed(&entry->video_capture_device))); std::move(entry->video_capture_device()) https://codereview.chromium.org/2551193002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_frame_receiver_on_io_thread.cc (right): https://codereview.chromium.org/2551193002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_frame_receiver_on_io_thread.cc:21: content::BrowserThread::IO, FROM_HERE, Instead of using content::BrowserThread::IO here, consider: - not adding the suffix OnIOThread, but perhaps just OnTaskRunner, - adding a member, const scoped_refptr<base::SingleThreadTaskRunner> task_runner_; - which is initialized on ctor with BrowserThread::GetTaskRunnerForThread(BrowserThread::IO); Note that you don't need content:: in these files. https://codereview.chromium.org/2551193002/diff/40001/media/capture/video/vid... File media/capture/video/video_capture_device.h (right): https://codereview.chromium.org/2551193002/diff/40001/media/capture/video/vid... media/capture/video/video_capture_device.h:44: class CAPTURE_EXPORT FrameBufferHost { In Chromium, Hosts are usually sitting at one end of an IPC/mojo channel. To avoid confusion, we could consider calling this class e.g. FrameBufferPool? https://codereview.chromium.org/2551193002/diff/40001/media/capture/video/vid... media/capture/video/video_capture_device.h:51: }; Move it to a file on its own plz. https://codereview.chromium.org/2551193002/diff/40001/media/capture/video/vid... media/capture/video/video_capture_device.h:53: class CAPTURE_EXPORT VideoFrameConsumerFeedbackObserver { We should move this fella to a file on its own too. And, incidentally, we should move these interface definitions to media/capture ISO being buried in media/capture/video. This can be done after this CL.
Addressed mcasas@ comments https://codereview.chromium.org/2551193002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/OWNERS (right): https://codereview.chromium.org/2551193002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/OWNERS:10: per-file video_*=chfremer@chromium.org On 2016/12/06 20:02:24, mcasas wrote: > Maybe we could wrap the video_* files, or, > equivalently, the *capture* files, to a > subfolder |video| or |capture| (but not > |video_capture| please), and just add a normal > OWNERS file there with miu@, emircan@, you > and me (note that these file would propagate > down). Can be done subsequently. I am in favor of this. Adding a task to the design doc. To avoid merge conflicts, I would like to "schedule" the CLs that move files around to the tip of my local branches. https://codereview.chromium.org/2551193002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/2551193002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:25: #include "media/capture/video/video_capture_buffer_pool_impl.h" On 2016/12/06 20:02:24, mcasas wrote: > Not needed anymore? Good catch! Hmm, seems we still need at least video_capture_buffer_pool.h, since there is a DCHECK that checks against VideoCaptureBufferPool::kInvalidId. Done. https://codereview.chromium.org/2551193002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.h (right): https://codereview.chromium.org/2551193002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.h:150: class BufferState { On 2016/12/06 20:02:24, mcasas wrote: > Hmm we should move this full declaration to the .cc, > file, right? And leave here just a forward declaration. > Can be done subsequently too. Not sure this can be done with Buffer being used as a non-pointer type inside |buffer_id_to_state_map_|. Added as task to the design doc. https://codereview.chromium.org/2551193002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/2551193002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:225: scoped_refptr<media::VideoCaptureBufferPool> buffer_pool_; On 2016/12/06 20:02:24, mcasas wrote: > media::FrameBufferHost (and BufferPoolFrameBufferHost) > are not ref counted whereas media::VideoCaptureBufferPool > is. This forces you to use naked pointers in VCC::BufferState, > is it because you envision the mojo-based interface to not > be ref-counted...? The reason is that I am trying to avoid shared ownership in favor of clear ownership rules. Since shared ownership is not needed here, I opted for one unique_ptr<> for the owner and raw pointers for non-owning users. https://codereview.chromium.org/2551193002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:476: base::Passed(&entry->video_capture_device))); On 2016/12/06 20:02:24, mcasas wrote: > std::move(entry->video_capture_device()) Hmm, I don't fully understand the documentation of base::Bind, but when trying to use std::move() here, I get the following compile error: ../../base/bind_internal.h:214:31: error: call to deleted constructor of 'std::unique_ptr<media::VideoCaptureDevice, std::default_delete<media::VideoCaptureDevice> >' return (receiver.*method)(std::forward<RunArgs>(args)...); ^~~~~~~~~~~~~~~~~~~~~~~~~~~ https://codereview.chromium.org/2551193002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_frame_receiver_on_io_thread.cc (right): https://codereview.chromium.org/2551193002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_frame_receiver_on_io_thread.cc:21: content::BrowserThread::IO, FROM_HERE, On 2016/12/06 20:02:24, mcasas wrote: > Instead of using content::BrowserThread::IO here, > consider: > - not adding the suffix OnIOThread, but perhaps > just OnTaskRunner, > - adding a member, > const scoped_refptr<base::SingleThreadTaskRunner> task_runner_; > - which is initialized on ctor with > BrowserThread::GetTaskRunnerForThread(BrowserThread::IO); > > Note that you don't need content:: in these files. Good suggestion. Agreed. The ...OnTaskRunner seems like a good general pattern and is much more flexible than the hard-coded hop to the Browser thread. Added this to the cleanup tasks in the design doc (trying to avoid the file rename at this point). https://codereview.chromium.org/2551193002/diff/40001/media/capture/video/vid... File media/capture/video/video_capture_device.h (right): https://codereview.chromium.org/2551193002/diff/40001/media/capture/video/vid... media/capture/video/video_capture_device.h:44: class CAPTURE_EXPORT FrameBufferHost { On 2016/12/06 20:02:24, mcasas wrote: > In Chromium, Hosts are usually sitting at one end > of an IPC/mojo channel. To avoid confusion, we > could consider calling this class e.g. FrameBufferPool? Done. https://codereview.chromium.org/2551193002/diff/40001/media/capture/video/vid... media/capture/video/video_capture_device.h:51: }; On 2016/12/06 20:02:24, mcasas wrote: > Move it to a file on its own plz. Please allow me to defer this as cleanup task by adding it to the design doc. Actually, this interface will be eliminated anyway in CL1.9.10. https://codereview.chromium.org/2551193002/diff/40001/media/capture/video/vid... media/capture/video/video_capture_device.h:53: class CAPTURE_EXPORT VideoFrameConsumerFeedbackObserver { On 2016/12/06 20:02:24, mcasas wrote: > We should move this fella to a file on its own too. > > And, incidentally, we should move these interface > definitions to media/capture ISO being buried > in media/capture/video. This can be done after > this CL. Agreed. Please allow me to defer this as cleanup task by adding it to the design doc.
chfremer@chromium.org changed reviewers: + miu@chromium.org
miu@: PTAL
On 2016/12/06 21:08:01, chfremer wrote: > miu@: PTAL ping
On 2016/12/13 17:26:28, chfremer wrote: > On 2016/12/06 21:08:01, chfremer wrote: > > miu@: PTAL > > ping Looking now. Thanks for being patient with me. :)
PS2 lgtm. Comments below are mostly polish, suggestions, or sanity-check questions: https://codereview.chromium.org/2551193002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/2551193002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:124: if (frame_buffer_pool_ != nullptr) Instead of nesting if-statements, consider consolidating into one expression: if (frame_buffer_pool_ && consumer_hold_count_ == 0) ... https://codereview.chromium.org/2551193002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:505: CHECK(frame_buffer_pool_); DCHECK instead of CHECK? Actually, the since |frame_buffer_pool_| is a std::unique_ptr; I believe dereferencing it in the method call below will execute an assertion that checks for not-null. So, you probably don't need this. https://codereview.chromium.org/2551193002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.h (right): https://codereview.chromium.org/2551193002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.h:65: explicit VideoCaptureController(); No longer need 'explicit' here. https://codereview.chromium.org/2551193002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.h:192: std::unique_ptr<media::FrameBufferPool> frame_buffer_pool_; Nice! Removing the ref-counting is a good sign the object graph has just become more "untangled." :) I'm aware the VCBufferPool is still ref-counted, but that's fine since it's representing a shared data resource, and the ref-counting has been limited to internal implementation (in the VCManager module). https://codereview.chromium.org/2551193002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (left): https://codereview.chromium.org/2551193002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:237: DCHECK(thread_checker_.CalledOnValidThread()); Is it okay that any of the objects DeviceEntry owns (such as VCController) could be destroyed on any thread? If so, maybe comment that this point should be reached only if there are no external references to the objects on any thread? If not, you may need to post tasks to other threads to delete them... https://codereview.chromium.org/2551193002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/2551193002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:208: BufferPoolFrameBufferPool( 'explicit' https://codereview.chromium.org/2551193002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:274: const int max_buffers = stream_type == MEDIA_TAB_VIDEO_CAPTURE Note: At some point, we should probably have the VideoCaptureDevice impl provide this number (maybe specify it via a new method in VCDClient?), since different capture pipelines will need something different. For example, there are two different desktop capture impls: One that probably would work best with the tab capture # of buffers, and one with the smaller #. https://codereview.chromium.org/2551193002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:559: // Passing raw pointer |device_client_ptr| is safe, because it will Did you mean "device_task_runner_" instead of "device_client_ptr" here? https://codereview.chromium.org/2551193002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:591: if (device) { Can |device| ever be null? Is this new if-statement necessary? (If it can now be null sometimes, does that mean the code earlier in this method should null-check it too?) https://codereview.chromium.org/2551193002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:1211: auto video_capture_controller = base::MakeUnique<VideoCaptureController>(); nit: Instead of creating separate locals and then just std::move'ing their values, IMHO, it's simpler to: devices_.emplace_back(new DeviceEntry(..., base::MakeUnique<VideoCaptureController>(), params)); return devices_.back(); Note: You might even simplify further and put the construction of the VCController in the DeviceEntry ctor instead. Note 2: And then, in the DeviceEntry class, maybe use composition instead of a std::unique_ptr for the VCController member?
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...
chfremer@chromium.org changed reviewers: + avi@chromium.org
mcasas@: PTAL miu@: PTAL avi@: Please RS https://codereview.chromium.org/2551193002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/2551193002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:124: if (frame_buffer_pool_ != nullptr) On 2016/12/13 21:11:10, miu wrote: > Instead of nesting if-statements, consider consolidating into one expression: > > if (frame_buffer_pool_ && consumer_hold_count_ == 0) > ... In this case, we have the statement in l.125 to be conditioned only by the first "if", that's why I went for the nested ifs. Are there better alternatives? https://codereview.chromium.org/2551193002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.cc:505: CHECK(frame_buffer_pool_); On 2016/12/13 21:11:10, miu wrote: > DCHECK instead of CHECK? > > Actually, the since |frame_buffer_pool_| is a std::unique_ptr; I believe > dereferencing it in the method call below will execute an assertion that checks > for not-null. So, you probably don't need this. Done. https://codereview.chromium.org/2551193002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_controller.h (right): https://codereview.chromium.org/2551193002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.h:65: explicit VideoCaptureController(); On 2016/12/13 21:11:10, miu wrote: > No longer need 'explicit' here. Done. https://codereview.chromium.org/2551193002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_controller.h:192: std::unique_ptr<media::FrameBufferPool> frame_buffer_pool_; On 2016/12/13 21:11:10, miu wrote: > Nice! Removing the ref-counting is a good sign the object graph has just become > more "untangled." :) > > I'm aware the VCBufferPool is still ref-counted, but that's fine since it's > representing a shared data resource, and the ref-counting has been limited to > internal implementation (in the VCManager module). Acknowledged. https://codereview.chromium.org/2551193002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (left): https://codereview.chromium.org/2551193002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:237: DCHECK(thread_checker_.CalledOnValidThread()); On 2016/12/13 21:11:10, miu wrote: > Is it okay that any of the objects DeviceEntry owns (such as VCController) could > be destroyed on any thread? If so, maybe comment that this point should be > reached only if there are no external references to the objects on any thread? > If not, you may need to post tasks to other threads to delete them... That would be not okay, and should never happen. DeviceEntry instances are privately owned by the VideoCaptureManager instance and are deleted on the IO thread. Added a DCHECK_CURRENTLY_ON(BrowserThread::IO) to make it clear. https://codereview.chromium.org/2551193002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/2551193002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:208: BufferPoolFrameBufferPool( On 2016/12/13 21:11:10, miu wrote: > 'explicit' Done. https://codereview.chromium.org/2551193002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:274: const int max_buffers = stream_type == MEDIA_TAB_VIDEO_CAPTURE On 2016/12/13 21:11:10, miu wrote: > Note: At some point, we should probably have the VideoCaptureDevice impl provide > this number (maybe specify it via a new method in VCDClient?), since different > capture pipelines will need something different. For example, there are two > different desktop capture impls: One that probably would work best with the tab > capture # of buffers, and one with the smaller #. Added this as a point for future discussion to the design doc. I am not 100% sure the VideoCaptureDevice impl always has all the information required to make this decision. The ideal buffer size probably depends not just on the VideoCaptureDevice implementation itself, but also on the consumers, e.g. how fast and reliable clients are able to consume. https://codereview.chromium.org/2551193002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:559: // Passing raw pointer |device_client_ptr| is safe, because it will On 2016/12/13 21:11:10, miu wrote: > Did you mean "device_task_runner_" instead of "device_client_ptr" here? Nope. Sorry, this comment was just a leftover from a previous version of this CL and I missed deleting it during merging. Deleted. https://codereview.chromium.org/2551193002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:591: if (device) { On 2016/12/13 21:11:10, miu wrote: > Can |device| ever be null? Is this new if-statement necessary? > > (If it can now be null sometimes, does that mean the code earlier in this method > should null-check it too?) Thanks for catching this. Yes, |device| can be null (see l.574). The code earlier in this method does check it, see l.583 where it is checked as |device_ptr|. Actually, the newly introduced if-statement should have already been present in the previous CL to prevent that we call SetConsumerFeedbackObserver with a VideoFrameConsumerFeedbackObserverOnTaskRunner forwarding to a nullptr. https://codereview.chromium.org/2551193002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/video_capture_manager.cc:1211: auto video_capture_controller = base::MakeUnique<VideoCaptureController>(); On 2016/12/13 21:11:10, miu wrote: > nit: Instead of creating separate locals and then just std::move'ing their > values, IMHO, it's simpler to: > > devices_.emplace_back(new DeviceEntry(..., > base::MakeUnique<VideoCaptureController>(), params)); > return devices_.back(); > > Note: You might even simplify further and put the construction of the > VCController in the DeviceEntry ctor instead. > > Note 2: And then, in the DeviceEntry class, maybe use composition instead of a > std::unique_ptr for the VCController member? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM stampity stamp
lgtm Please add TODOs for tasks in the future, so other developers can see that there are some moving parts (besides adding them to the Doc). https://codereview.chromium.org/2551193002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_frame_receiver_on_io_thread.cc (right): https://codereview.chromium.org/2551193002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_frame_receiver_on_io_thread.cc:21: content::BrowserThread::IO, FROM_HERE, On 2016/12/06 21:07:41, chfremer wrote: > On 2016/12/06 20:02:24, mcasas wrote: > > Instead of using content::BrowserThread::IO here, > > consider: > > - not adding the suffix OnIOThread, but perhaps > > just OnTaskRunner, > > - adding a member, > > const scoped_refptr<base::SingleThreadTaskRunner> task_runner_; > > - which is initialized on ctor with > > BrowserThread::GetTaskRunnerForThread(BrowserThread::IO); > > > > Note that you don't need content:: in these files. > > Good suggestion. Agreed. The ...OnTaskRunner seems like a good general pattern > and is much more flexible than the hard-coded hop to the Browser thread. Added > this to the cleanup tasks in the design doc (trying to avoid the file rename at > this point). Could you please add a TODO() ?
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...
Thanks for the reviews! https://codereview.chromium.org/2551193002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/video_frame_receiver_on_io_thread.cc (right): https://codereview.chromium.org/2551193002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/video_frame_receiver_on_io_thread.cc:21: content::BrowserThread::IO, FROM_HERE, On 2016/12/14 17:50:49, mcasas wrote: > On 2016/12/06 21:07:41, chfremer wrote: > > On 2016/12/06 20:02:24, mcasas wrote: > > > Instead of using content::BrowserThread::IO here, > > > consider: > > > - not adding the suffix OnIOThread, but perhaps > > > just OnTaskRunner, > > > - adding a member, > > > const scoped_refptr<base::SingleThreadTaskRunner> task_runner_; > > > - which is initialized on ctor with > > > BrowserThread::GetTaskRunnerForThread(BrowserThread::IO); > > > > > > Note that you don't need content:: in these files. > > > > Good suggestion. Agreed. The ...OnTaskRunner seems like a good general pattern > > and is much more flexible than the hard-coded hop to the Browser thread. Added > > this to the cleanup tasks in the design doc (trying to avoid the file rename > at > > this point). > > Could you please add a TODO() ? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chfremer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org, avi@chromium.org, mcasas@chromium.org Link to the patchset: https://codereview.chromium.org/2551193002/#ps100001 (title: "Added TODO comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481746071582810,
"parent_rev": "46b59fbf2751b69ecab2d717bb3172eefdffbce5", "commit_rev":
"877927aa9108be0bacf27f9de08386a9ada21133"}
Message was sent while issue was closed.
Description was changed from ========== [Mojo Video Capture] Decouple VideoCaptureController from VideoCaptureBufferPool and VideoCaptureDeviceClient Decouple VideoCaptureController from VideoCaptureBufferPool and VideoCaptureDeviceClient. This is needed in order to prepare VideoCaptureController for working with both the (legacy) in-process video capture and the new video_capture Mojo service. List of changes: * Introduce an abstraction FrameBufferHost for the VideoCaptureController to talk to the VideoCaptureBufferPool. * Move VideoFrameReceiverOnIOThread to separate file. * Move factory logic for creating and connecting VideoCaptureBufferPoolImpl and VideoCaptureDeviceClient out of VideoCaptureController and into VideoCaptureManager as well as the tests. * Update OWNERS to include new file and include chfremer as owner. * In VideoCaptureDeviceClient: Rename |frame_format| to |foramt|. This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL1.9.8 BUG=584797 TEST= content_unittests, video_capture_unittests, Apprtc loopback on Debug, Desktop Capture Example extension on Release [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... ========== to ========== [Mojo Video Capture] Decouple VideoCaptureController from VideoCaptureBufferPool and VideoCaptureDeviceClient Decouple VideoCaptureController from VideoCaptureBufferPool and VideoCaptureDeviceClient. This is needed in order to prepare VideoCaptureController for working with both the (legacy) in-process video capture and the new video_capture Mojo service. List of changes: * Introduce an abstraction FrameBufferHost for the VideoCaptureController to talk to the VideoCaptureBufferPool. * Move VideoFrameReceiverOnIOThread to separate file. * Move factory logic for creating and connecting VideoCaptureBufferPoolImpl and VideoCaptureDeviceClient out of VideoCaptureController and into VideoCaptureManager as well as the tests. * Update OWNERS to include new file and include chfremer as owner. * In VideoCaptureDeviceClient: Rename |frame_format| to |foramt|. This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL1.9.8 BUG=584797 TEST= content_unittests, video_capture_unittests, Apprtc loopback on Debug, Desktop Capture Example extension on Release [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... Review-Url: https://codereview.chromium.org/2551193002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [Mojo Video Capture] Decouple VideoCaptureController from VideoCaptureBufferPool and VideoCaptureDeviceClient Decouple VideoCaptureController from VideoCaptureBufferPool and VideoCaptureDeviceClient. This is needed in order to prepare VideoCaptureController for working with both the (legacy) in-process video capture and the new video_capture Mojo service. List of changes: * Introduce an abstraction FrameBufferHost for the VideoCaptureController to talk to the VideoCaptureBufferPool. * Move VideoFrameReceiverOnIOThread to separate file. * Move factory logic for creating and connecting VideoCaptureBufferPoolImpl and VideoCaptureDeviceClient out of VideoCaptureController and into VideoCaptureManager as well as the tests. * Update OWNERS to include new file and include chfremer as owner. * In VideoCaptureDeviceClient: Rename |frame_format| to |foramt|. This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL1.9.8 BUG=584797 TEST= content_unittests, video_capture_unittests, Apprtc loopback on Debug, Desktop Capture Example extension on Release [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... Review-Url: https://codereview.chromium.org/2551193002 ========== to ========== [Mojo Video Capture] Decouple VideoCaptureController from VideoCaptureBufferPool and VideoCaptureDeviceClient Decouple VideoCaptureController from VideoCaptureBufferPool and VideoCaptureDeviceClient. This is needed in order to prepare VideoCaptureController for working with both the (legacy) in-process video capture and the new video_capture Mojo service. List of changes: * Introduce an abstraction FrameBufferHost for the VideoCaptureController to talk to the VideoCaptureBufferPool. * Move VideoFrameReceiverOnIOThread to separate file. * Move factory logic for creating and connecting VideoCaptureBufferPoolImpl and VideoCaptureDeviceClient out of VideoCaptureController and into VideoCaptureManager as well as the tests. * Update OWNERS to include new file and include chfremer as owner. * In VideoCaptureDeviceClient: Rename |frame_format| to |foramt|. This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL1.9.8 BUG=584797 TEST= content_unittests, video_capture_unittests, Apprtc loopback on Debug, Desktop Capture Example extension on Release [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... Committed: https://crrev.com/0b129bdbdd3099a492bcf014c22239c2b35071a2 Cr-Commit-Position: refs/heads/master@{#438604} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0b129bdbdd3099a492bcf014c22239c2b35071a2 Cr-Commit-Position: refs/heads/master@{#438604}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:100001) has been created in https://codereview.chromium.org/2595853003/ by chfremer@chromium.org. The reason for reverting is: This caused a regression where video capture on Android would get stuck in a loop when putting the browser to the background and restoring. See crbug.com/675528 for details.. |
