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

Issue 2738763002: [Mojo Video Capture] Introduce abstraction BuildableVideoCaptureDevice (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 3 weeks ago by chfremer
Modified:
1 month ago
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 Review-Url: https://codereview.chromium.org/2738763002 Cr-Commit-Position: refs/heads/master@{#461217} Committed: https://chromium.googlesource.com/chromium/src/+/fb141a46523161bc47659b88dd1e76e36a727d6c

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 #

Patch Set 10 : Rebase to March 28 #

Patch Set 11 : Rebase to March 30 #

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 9 10 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 9 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
Commit queue not available (can’t edit this change).

Depends on Patchset:

Dependent Patchsets:

Messages

Total messages: 78 (46 generated)
chfremer
miu@: PTAL emircan@: PTAL mcasas@: FYI (let's finish up with https://codereview.chromium.org/2735083002 first)
1 month, 3 weeks 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 ...
1 month, 3 weeks 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 ...
1 month, 3 weeks 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; ...
1 month, 2 weeks 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 month, 2 weeks 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 month, 2 weeks 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 month, 2 weeks 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 month, 2 weeks 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 month, 2 weeks 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 month, 2 weeks 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 month, 1 week 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 month, 1 week 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 month, 1 week 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 ...
1 month, 1 week 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 ...
1 month, 1 week 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 ...
1 month, 1 week 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 ...
1 month, 1 week 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 ...
1 month, 1 week 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 ...
1 month, 1 week ago (2017-03-21 18:47:53 UTC) #51
Avi (ping after 24h)
lgtm build stamp
1 month, 1 week ago (2017-03-21 19:46:23 UTC) #52
mcasas
Sorry for the delay and, can you plz reupload with a different --similarity ? Now ...
1 month, 1 week 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, ...
1 month, 1 week 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 ...
1 month, 1 week 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 ...
1 month, 1 week 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 ...
1 month, 1 week 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 ...
1 month, 1 week ago (2017-03-22 23:20:38 UTC) #60
mcasas
On 2017/03/22 23:20:38, chfremer wrote: > Incorporated suggestions from PatchSet 8. > > mcasas@: Please ...
1 month, 1 week ago (2017-03-24 20:43:04 UTC) #61
miu
PS10 and still lgtm
1 month ago (2017-03-29 23:03:14 UTC) #62
chfremer
On 2017/03/29 23:03:14, miu wrote: > PS10 and still lgtm Just in case you are ...
1 month ago (2017-03-29 23:19:37 UTC) #63
miu
On 2017/03/29 23:19:37, chfremer wrote: > On 2017/03/29 23:03:14, miu wrote: > > PS10 and ...
1 month ago (2017-03-30 01:12:27 UTC) #64
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2738763002/240001
1 month ago (2017-03-31 20:38:08 UTC) #75
commit-bot: I haz the power
1 month ago (2017-03-31 20:49:22 UTC) #78
Message was sent while issue was closed.
Committed patchset #11 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/fb141a46523161bc47659b88dd1e...
Sign in to reply to this message.

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