Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1040)

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

Created:
1 year, 1 month ago by chfremer
Modified:
1 year 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 #

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 year, 1 month 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 year, 1 month 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 year, 1 month 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 year, 1 month 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 year, 1 month 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 year, 1 month 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 year, 1 month 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 year, 1 month 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 year, 1 month 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 year, 1 month 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 year, 1 month 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 year, 1 month 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 year, 1 month 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 year, 1 month 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 year, 1 month 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 year, 1 month 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 year, 1 month 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 year, 1 month 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 year, 1 month ago (2017-03-21 18:47:53 UTC) #51
Avi (use Gerrit)
lgtm build stamp
1 year, 1 month 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 year 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 year 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 year 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 year 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 year 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 year 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 year ago (2017-03-24 20:43:04 UTC) #61
miu
PS10 and still lgtm
1 year 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 year 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 year 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 year ago (2017-03-31 20:38:08 UTC) #75
commit-bot: I haz the power
1 year 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...

Powered by Google App Engine
This is Rietveld 408576698