Chromium Code Reviews
Help | Chromium Project | Sign in
(10)

Issue 2738763002: [Mojo Video Capture] Introduce abstraction BuildableVideoCaptureDevice

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks, 4 days ago by chfremer
Modified:
1 day, 22 hours ago
Reviewers:
emircan, Avi, mcasas, miu
CC:
chromium-reviews, posciak+watch_chromium.org, chfremer+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/heads/master
Project:
chromium
Visibility:
Public.

Description

[Mojo Video Capture] Introduce abstraction BuildableVideoCaptureDevice This CL is supposed to be a pure refactoring. There should be no changes to existing behavior. This is part of a series of CLs with the goal of separating the class VideoCaptureManager from knowledge of things that are going to be private implementation details of the video capture service. The end goal is to have VideoCaptureManager talk to an abstraction that can be implemented by either the video capture Mojo service or the existing (legacy) in-process implementation. Changes in this CL: * Add abstraction (interface) BuildableVideoCaptureDevice to be operated by class VideoCaptureManager. Add a callback interface BuildableDeviceCallbacks for BuildableVideoCaptureDevice to send messages back to VideoCaptureManager. * Create an implementation InProcessBuildableVideoCaptureDevice and move the logic for building and releasing devices from VideoCaptureManager into there. * Promote struct VideoCaptureManager::DeviceEntry to class VideoCaptureDeviceEntry and move it to a separate file. * In VideoCaptureManager, changed ownership of DeviceEntries from std::unique_ptr<> to scoped_refptr<>. The reason for this is that VideoCaptureManager wants to temporarily pass ownership of it to an asynchronous operation. Before this CL, this asynchronous operation took shared ownership of the whole VideoCaptureManager instead. This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL15. BUG=584797 TEST= content_browsertests --gtest_filter="VideoCaptureBrowserTest.*" content_unittests --gtest_filter="*Video*" [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxsxWslJ_X-zpOhijvI8/edit?usp=sharing

Patch Set 1 #

Patch Set 2 : Rebase to Feb 10th and change similarity to 10 #

Total comments: 36

Patch Set 3 : Incorporated miu@'s suggestions from PatchSet 2 #

Total comments: 4

Patch Set 4 : Incorporated miu@'s suggestions from PatchSet 3 #

Total comments: 12

Patch Set 5 : Incorporated emircan@'s suggestions from Patch Set 4 #

Patch Set 6 : Use base::OnceClosure for ownership, use DeviceBuildContext #

Total comments: 15

Patch Set 7 : Use BuildableDeviceCallbacks* plus OnceClosure instead of DeviceBuildContext #

Total comments: 8

Patch Set 8 : Incorporated mcasas@'s suggestions from PatchSet 7 and Similarity 50 #

Total comments: 16

Patch Set 9 : Incorporated suggestions from Patch Set 8 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1019 lines, -632 lines) Patch
M content/browser/BUILD.gn View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -0 lines 0 comments Download
A content/browser/renderer_host/media/buildable_video_capture_device.h View 1 2 3 4 5 6 7 1 chunk +73 lines, -0 lines 0 comments Download
A content/browser/renderer_host/media/in_process_buildable_video_capture_device.h View 1 2 3 4 5 6 7 1 chunk +95 lines, -0 lines 0 comments Download
A content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc View 1 2 3 4 5 6 7 8 1 chunk +438 lines, -0 lines 0 comments Download
M content/browser/renderer_host/media/video_capture_controller.h View 1 2 3 4 5 6 7 8 5 chunks +49 lines, -3 lines 0 comments Download
M content/browser/renderer_host/media/video_capture_controller.cc View 1 2 3 4 5 6 7 8 3 chunks +65 lines, -2 lines 0 comments Download
M content/browser/renderer_host/media/video_capture_controller_unittest.cc View 1 2 3 4 5 6 7 3 chunks +34 lines, -2 lines 0 comments Download
M content/browser/renderer_host/media/video_capture_manager.h View 1 2 3 4 5 6 7 8 chunks +37 lines, -70 lines 0 comments Download
M content/browser/renderer_host/media/video_capture_manager.cc View 1 2 3 4 5 6 7 31 chunks +225 lines, -555 lines 0 comments Download
Trybot results: Sign in to try more bots
Commit queue not available (can’t edit this change).

Depends on Patchset:

Messages

Total messages: 61 (34 generated)
chfremer
miu@: PTAL emircan@: PTAL mcasas@: FYI (let's finish up with https://codereview.chromium.org/2735083002 first)
2 weeks, 4 days ago (2017-03-07 23:32:49 UTC) #16
chfremer
On 2017/03/07 23:32:49, chfremer wrote: > miu@: PTAL > emircan@: PTAL > mcasas@: FYI (let's ...
2 weeks, 2 days ago (2017-03-10 17:27:34 UTC) #19
chfremer
On 2017/03/10 17:27:34, chfremer wrote: > On 2017/03/07 23:32:49, chfremer wrote: > > miu@: PTAL ...
2 weeks, 1 day ago (2017-03-10 19:11:26 UTC) #20
miu
Comments for PS2: https://codereview.chromium.org/2738763002/diff/60001/content/browser/renderer_host/media/buildable_video_capture_device.h File content/browser/renderer_host/media/buildable_video_capture_device.h (right): https://codereview.chromium.org/2738763002/diff/60001/content/browser/renderer_host/media/buildable_video_capture_device.h#newcode27 content/browser/renderer_host/media/buildable_video_capture_device.h:27: virtual void OnDeviceAboutToStart(media::VideoFacingMode facing_mode) = 0; ...
2 weeks, 1 day ago (2017-03-11 01:13:22 UTC) #21
chfremer
miu@: PTAL https://codereview.chromium.org/2738763002/diff/60001/content/browser/renderer_host/media/buildable_video_capture_device.h File content/browser/renderer_host/media/buildable_video_capture_device.h (right): https://codereview.chromium.org/2738763002/diff/60001/content/browser/renderer_host/media/buildable_video_capture_device.h#newcode27 content/browser/renderer_host/media/buildable_video_capture_device.h:27: virtual void OnDeviceAboutToStart(media::VideoFacingMode facing_mode) = 0; On ...
1 week, 5 days ago (2017-03-13 22:02:03 UTC) #24
miu
lgtm https://codereview.chromium.org/2738763002/diff/60001/content/browser/renderer_host/media/buildable_video_capture_device.h File content/browser/renderer_host/media/buildable_video_capture_device.h (right): https://codereview.chromium.org/2738763002/diff/60001/content/browser/renderer_host/media/buildable_video_capture_device.h#newcode43 content/browser/renderer_host/media/buildable_video_capture_device.h:43: class BuildableVideoCaptureDevice { On 2017/03/13 22:02:03, chfremer wrote: ...
1 week, 3 days ago (2017-03-16 00:01:36 UTC) #27
chfremer
emicran@: PTAL content/browser/renderer_host/media/{!video}_* https://codereview.chromium.org/2738763002/diff/60001/content/browser/renderer_host/media/buildable_video_capture_device.h File content/browser/renderer_host/media/buildable_video_capture_device.h (right): https://codereview.chromium.org/2738763002/diff/60001/content/browser/renderer_host/media/buildable_video_capture_device.h#newcode43 content/browser/renderer_host/media/buildable_video_capture_device.h:43: class BuildableVideoCaptureDevice { On 2017/03/16 00:01:35, ...
1 week, 3 days ago (2017-03-16 17:39:40 UTC) #28
emircan
content/browser/renderer_host/media/{!video}_* lgtm % nits https://codereview.chromium.org/2738763002/diff/100001/content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc File content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc (right): https://codereview.chromium.org/2738763002/diff/100001/content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc#newcode1 content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:1: // Copyright 2016 The Chromium ...
1 week, 2 days ago (2017-03-16 20:54:04 UTC) #29
chfremer
avi@: Please RS content/browser/BUILD.gn https://codereview.chromium.org/2738763002/diff/100001/content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc File content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc (right): https://codereview.chromium.org/2738763002/diff/100001/content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc#newcode1 content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:1: // Copyright 2016 The Chromium ...
1 week, 2 days ago (2017-03-16 22:02:30 UTC) #31
miu
Revisiting prior discussion: https://codereview.chromium.org/2738763002/diff/60001/content/browser/renderer_host/media/buildable_video_capture_device.h File content/browser/renderer_host/media/buildable_video_capture_device.h (right): https://codereview.chromium.org/2738763002/diff/60001/content/browser/renderer_host/media/buildable_video_capture_device.h#newcode48 content/browser/renderer_host/media/buildable_video_capture_device.h:48: // The passed-in |context_reference| must guarantee ...
1 week, 1 day ago (2017-03-17 21:18:15 UTC) #36
chfremer
Thanks for the detailed explanations. I was hoping for some good reasons to not have ...
1 week, 1 day ago (2017-03-17 22:44:53 UTC) #37
miu
On 2017/03/17 22:44:53, chfremer wrote: > But something still bugs me. In our example > ...
1 week, 1 day ago (2017-03-17 23:11:26 UTC) #38
chfremer
On 2017/03/17 23:11:26, miu wrote: > On 2017/03/17 22:44:53, chfremer wrote: > > But something ...
1 week, 1 day ago (2017-03-18 02:24:12 UTC) #39
chfremer
miu@: PTAL For PatchSet 6 I did the following two things. 1.) Use base::OnceCallback instead ...
5 days, 21 hours ago (2017-03-20 21:32:25 UTC) #42
miu
On 2017/03/20 21:32:25, chfremer wrote: > miu@: PTAL Will take a look shortly... > Using ...
5 days, 20 hours ago (2017-03-20 22:17:25 UTC) #45
miu
Comments on PS6: https://codereview.chromium.org/2738763002/diff/140001/content/browser/renderer_host/media/buildable_video_capture_device.h File content/browser/renderer_host/media/buildable_video_capture_device.h (right): https://codereview.chromium.org/2738763002/diff/140001/content/browser/renderer_host/media/buildable_video_capture_device.h#newcode66 content/browser/renderer_host/media/buildable_video_capture_device.h:66: callbacks_->OnDeviceStartFailed(controller); This is an interesting way ...
5 days, 19 hours ago (2017-03-20 23:02:16 UTC) #46
chfremer
Just replies without a new patch set to keep the discussion going. Will post a ...
5 days, 18 hours ago (2017-03-20 23:58:27 UTC) #47
chfremer
On 2017/03/20 22:17:25, miu wrote: > On 2017/03/20 21:32:25, chfremer wrote: > > miu@: PTAL ...
5 days, 18 hours ago (2017-03-21 00:09:33 UTC) #48
chfremer
miu@: PTAL avi@: Please RS content/browser/BUILD.gn In order to be able to move forward with ...
5 days ago (2017-03-21 18:47:53 UTC) #51
Avi
lgtm build stamp
4 days, 23 hours ago (2017-03-21 19:46:23 UTC) #52
mcasas
Sorry for the delay and, can you plz reupload with a different --similarity ? Now ...
4 days, 18 hours ago (2017-03-22 00:33:43 UTC) #55
chfremer
mcasas@: PTAL https://codereview.chromium.org/2738763002/diff/160001/content/browser/renderer_host/media/buildable_video_capture_device.h File content/browser/renderer_host/media/buildable_video_capture_device.h (right): https://codereview.chromium.org/2738763002/diff/160001/content/browser/renderer_host/media/buildable_video_capture_device.h#newcode18 content/browser/renderer_host/media/buildable_video_capture_device.h:18: class CONTENT_EXPORT BuildableDeviceCallbacks { On 2017/03/22 00:33:42, ...
4 days, 1 hour ago (2017-03-22 17:26:52 UTC) #56
miu
PS8 lgtm % nit: https://codereview.chromium.org/2738763002/diff/140001/content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc File content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc (right): https://codereview.chromium.org/2738763002/diff/140001/content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc#newcode118 content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:118: context.LookupDeviceDescriptor(controller->device_id()); On 2017/03/20 23:58:26, chfremer ...
3 days, 20 hours ago (2017-03-22 22:09:50 UTC) #57
miu
https://codereview.chromium.org/2738763002/diff/180001/content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc File content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc (right): https://codereview.chromium.org/2738763002/diff/180001/content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc#newcode145 content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:145: // Use of Unretained() is safe, because |context_reference| guarantees ...
3 days, 20 hours ago (2017-03-22 22:16:52 UTC) #58
mcasas
Sending a few comments seeing that miu@ has also commented in this PS. https://codereview.chromium.org/2738763002/diff/180001/content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc File ...
3 days, 20 hours ago (2017-03-22 22:43:16 UTC) #59
chfremer
Incorporated suggestions from PatchSet 8. mcasas@: Please let me know if you want to take ...
3 days, 19 hours ago (2017-03-22 23:20:38 UTC) #60
mcasas
1 day, 22 hours ago (2017-03-24 20:43:04 UTC) #61
On 2017/03/22 23:20:38, chfremer wrote:
> Incorporated suggestions from PatchSet 8.
> 
> mcasas@: Please let me know if you want to take another pass on this. I want
to
> figure out the test failure on Chromium OS on CL14 anyway before I land this
> one.
> 
>
https://codereview.chromium.org/2738763002/diff/180001/content/browser/render...
> File
>
content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc
> (right):
> 
>
https://codereview.chromium.org/2738763002/diff/180001/content/browser/render...
>
content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:68:
> std::move(done_cb).Run();
> On 2017/03/22 22:09:50, miu wrote:
> > Oh, I mentioned this in the other CL, but here is where it's introduced: Is
it
> > necessary to std::move() here (and elsewhere, where done_cb.Run() is the
last
> > thing a method does before returning)?
> 
> Yes. The OnceCallback must be moved in order to invoke Run().
> However, I just learned from mcasas@, that
base::ResetAndReturn(&done_cb).Run()
> now also works on OnceCallbacks (as of Feb 22nd).
> 
>
https://codereview.chromium.org/2738763002/diff/180001/content/browser/render...
>
content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:124:
> // Use of Unretained() is safe, because |context_reference| guarantees
> On 2017/03/22 22:43:16, mcasas wrote:
> > Seems like |context_reference| doesn't exist around here.
> > (Also in l. 145, 161).
> 
> Done.
> 
>
https://codereview.chromium.org/2738763002/diff/180001/content/browser/render...
>
content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:141:
> }
> On 2017/03/22 22:43:16, mcasas wrote:
> > This probably predatesthis CL, but I suggest we move this else{}
> > upwards:
> > 
> > if (!descriptor) {
> >   callbacks->OnDeviceStartFailed(controller);
> >   base::ResetAndReturn(&done_cb)->Run();
> >   return;
> > }
> > // and here continue knowing that |descriptor| verifies, l.116.
> 
> Done.
> 
>
https://codereview.chromium.org/2738763002/diff/180001/content/browser/render...
>
content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:145:
> // Use of Unretained() is safe, because |context_reference| guarantees
> On 2017/03/22 22:16:51, miu wrote:
> > s/context_reference/done_cb/
> > 
> > ...and elsewhere...
> 
> Done.
> 
>
https://codereview.chromium.org/2738763002/diff/180001/content/browser/render...
>
content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:204:
> std::move(done_cb).Run();
> On 2017/03/22 22:43:16, mcasas wrote:
> > Same as in l.139, I'd say here
> >   base::ResetAndReturn(&done_cb)->Run();
> > 
> > because it's more explicit what you want to do.
> > 
> > Also to consider around all this methods and interfaces
> > is to use ScopedClosureRunner [1] which guarantees in
> > prose and code that the a given closure is either 
> > std::move()d or Run().  Keep it in mind for future 
> > refactorings ;)
> > 
> > [1]
> >
>
https://cs.chromium.org/chromium/src/base/callback_helpers.h?type=cs&q=ResetA...,
> > 
> 
> Thanks. I didn't know that base::ResetAndReturn() now supports OnceCallbacks
(it
> did not last time I tried, seems it was fixed recently). ScopedClosureRunner
> currently does not support OnceCallbacks.
> 
>
https://codereview.chromium.org/2738763002/diff/180001/content/browser/render...
>
content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:284:
> // device is destroyed on the device_task_runner_ and |context_reference|
> On 2017/03/22 22:43:16, mcasas wrote:
> > s/device_task_runner_/|device_task_runner_|/
> > 
> > |context_reference| ?
> 
> Done.
> 
>
https://codereview.chromium.org/2738763002/diff/180001/content/browser/render...
>
content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:400:
> }
> On 2017/03/22 22:43:16, mcasas wrote:
> > nit: no {} for one line bodies.  Probably this predates you,
> > but can't harm.
> 
> Done.
> 
>
https://codereview.chromium.org/2738763002/diff/180001/content/browser/render...
> File content/browser/renderer_host/media/video_capture_controller.cc (right):
> 
>
https://codereview.chromium.org/2738763002/diff/180001/content/browser/render...
> content/browser/renderer_host/media/video_capture_controller.cc:518: }
> On 2017/03/22 22:43:16, mcasas wrote:
> > How is this method supposed to be used? 
> > (It isn't used in this CL).
> 
> Good catch. With the merge of VideoCaptureDeviceEntry into
> VideoCaptureController, this is now no longer needed.
> 
> Removed.

lgtm to unblock this patch.

We should find a better way of having a series of
interface/class methods just to forward to an inner
class/interface methods in another thread.  It's
essentially bloating the code and leaking abstractions.

base::Bind() is not too friendly to this but perhaps 
some template  magic...?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld d1a128a62