Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(31)

Issue 2857303002: [Mojo Video Capture] Implement a VideoCaptureProvider using the Mojo service (part 2) (Closed)

Created:
3 years, 7 months ago by chfremer
Modified:
3 years, 7 months ago
CC:
chromium-reviews, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, chfremer+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[Mojo Video Capture] Implement a VideoCaptureProvider using the Mojo service (part 2) This CL is part of the Mojo Video Capture work. For the bigger picture, see [1] CL24_part2. Goal of CL24: The interface content::VideoCaptureProvider currently has one implementation called InProcessVideoCaptureProvider, which is essentially a factory for the legacy in-process video capture stack. This CL adds a second implementation called MojoServiceVideoCaptureProvider which is essentially a wrapper for connecting to and communicating with the new video capture service. Changes in part2: * Add implementation for class ServiceVideoCaptureDeviceLauncher * Add basic implementation for class ServiceLaunchedVideoCaptureDevice BUG=584797 TEST= service_unittests --gtest_filter="*Video*" content_unittests --gtest_filter="*Video*" content_browsertests --gtest_filter="VideoCaptureBrowserTest.*" [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxsxWslJ_X-zpOhijvI8/edit?usp=sharing Review-Url: https://codereview.chromium.org/2857303002 Cr-Commit-Position: refs/heads/master@{#470645} Committed: https://chromium.googlesource.com/chromium/src/+/46083e3783e7a71d7eef4aeb5c5f6b3cc4c0338d

Patch Set 1 #

Total comments: 13

Patch Set 2 : Rebase to May 5th #

Total comments: 9

Patch Set 3 : Incorporated suggestions from PatchSets 1 and 2 #

Total comments: 1

Patch Set 4 : Add const #

Patch Set 5 : Rebase to May 9th #

Patch Set 6 : Use base::SequenceChecker #

Patch Set 7 : Added back update to |state_| which was accidentally dropped in PatchSet 3 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+166 lines, -20 lines) Patch
M content/browser/renderer_host/media/in_process_video_capture_device_launcher.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M content/browser/renderer_host/media/service_launched_video_capture_device.h View 1 2 3 4 5 1 chunk +4 lines, -1 line 0 comments Download
M content/browser/renderer_host/media/service_launched_video_capture_device.cc View 1 2 3 4 5 1 chunk +28 lines, -6 lines 0 comments Download
M content/browser/renderer_host/media/service_video_capture_device_launcher.h View 1 2 3 4 5 1 chunk +16 lines, -0 lines 0 comments Download
M content/browser/renderer_host/media/service_video_capture_device_launcher.cc View 1 2 3 4 5 6 2 chunks +108 lines, -4 lines 0 comments Download
M content/browser/renderer_host/media/service_video_capture_provider.h View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M content/browser/renderer_host/media/service_video_capture_provider.cc View 1 2 3 4 5 3 chunks +6 lines, -6 lines 0 comments Download
M services/video_capture/service_impl.h View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M services/video_capture/service_impl.cc View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 39 (20 generated)
chfremer
mcasas@: PTAL non-Mojo parts rockot@: PTAL Mojo parts emircan@, miu@: FYI
3 years, 7 months ago (2017-05-03 22:31:46 UTC) #4
chfremer
ping
3 years, 7 months ago (2017-05-05 18:40:01 UTC) #7
mcasas
Some minor comments https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_host/media/service_video_capture_device_launcher.cc File content/browser/renderer_host/media/service_video_capture_device_launcher.cc (right): https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_host/media/service_video_capture_device_launcher.cc#newcode95 content/browser/renderer_host/media/service_video_capture_device_launcher.cc:95: state_ = State::READY_TO_LAUNCH; If we move ...
3 years, 7 months ago (2017-05-05 22:41:02 UTC) #8
chfremer
PTAL https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_host/media/service_video_capture_device_launcher.cc File content/browser/renderer_host/media/service_video_capture_device_launcher.cc (right): https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_host/media/service_video_capture_device_launcher.cc#newcode95 content/browser/renderer_host/media/service_video_capture_device_launcher.cc:95: state_ = State::READY_TO_LAUNCH; On 2017/05/05 22:41:02, mcasas wrote: ...
3 years, 7 months ago (2017-05-08 17:01:54 UTC) #9
miu
https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_host/media/service_video_capture_device_launcher.h File content/browser/renderer_host/media/service_video_capture_device_launcher.h (right): https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_host/media/service_video_capture_device_launcher.h#newcode53 content/browser/renderer_host/media/service_video_capture_device_launcher.h:53: void OnDeviceCreationFailed(Callbacks* callbacks, base::OnceClosure done_cb); On 2017/05/08 17:01:54, chfremer ...
3 years, 7 months ago (2017-05-08 20:21:11 UTC) #10
chfremer
https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_host/media/service_video_capture_device_launcher.h File content/browser/renderer_host/media/service_video_capture_device_launcher.h (right): https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_host/media/service_video_capture_device_launcher.h#newcode53 content/browser/renderer_host/media/service_video_capture_device_launcher.h:53: void OnDeviceCreationFailed(Callbacks* callbacks, base::OnceClosure done_cb); On 2017/05/08 20:21:11, miu ...
3 years, 7 months ago (2017-05-08 20:51:46 UTC) #11
mcasas
https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_host/media/service_video_capture_device_launcher.cc File content/browser/renderer_host/media/service_video_capture_device_launcher.cc (right): https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_host/media/service_video_capture_device_launcher.cc#newcode95 content/browser/renderer_host/media/service_video_capture_device_launcher.cc:95: state_ = State::READY_TO_LAUNCH; On 2017/05/08 17:01:54, chfremer wrote: > ...
3 years, 7 months ago (2017-05-08 21:08:04 UTC) #12
chfremer
https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_host/media/service_video_capture_device_launcher.cc File content/browser/renderer_host/media/service_video_capture_device_launcher.cc (right): https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_host/media/service_video_capture_device_launcher.cc#newcode95 content/browser/renderer_host/media/service_video_capture_device_launcher.cc:95: state_ = State::READY_TO_LAUNCH; On 2017/05/08 21:08:04, mcasas wrote: > ...
3 years, 7 months ago (2017-05-08 21:56:31 UTC) #13
mcasas
On 2017/05/08 21:56:31, chfremer wrote: > https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_host/media/service_video_capture_device_launcher.cc > File > content/browser/renderer_host/media/service_video_capture_device_launcher.cc > (right): > > ...
3 years, 7 months ago (2017-05-09 18:13:29 UTC) #14
chfremer
PTAL Incorporated suggestions from PS1 and PS2 https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_host/media/service_video_capture_device_launcher.cc File content/browser/renderer_host/media/service_video_capture_device_launcher.cc (right): https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_host/media/service_video_capture_device_launcher.cc#newcode95 content/browser/renderer_host/media/service_video_capture_device_launcher.cc:95: state_ = ...
3 years, 7 months ago (2017-05-09 18:41:33 UTC) #15
mcasas
lgtm, thanks! https://codereview.chromium.org/2857303002/diff/40001/content/browser/renderer_host/media/service_video_capture_device_launcher.cc File content/browser/renderer_host/media/service_video_capture_device_launcher.cc (right): https://codereview.chromium.org/2857303002/diff/40001/content/browser/renderer_host/media/service_video_capture_device_launcher.cc#newcode113 content/browser/renderer_host/media/service_video_capture_device_launcher.cc:113: bool abort_requested = (state_ == State::DEVICE_START_ABORTING); micro-nit: ...
3 years, 7 months ago (2017-05-09 19:00:54 UTC) #16
chfremer
Thanks for the review!
3 years, 7 months ago (2017-05-09 19:04:58 UTC) #17
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/2857303002/60001
3 years, 7 months ago (2017-05-09 19:06:20 UTC) #20
commit-bot: I haz the power
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm64_dbg_recipe/builds/264092) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, ...
3 years, 7 months ago (2017-05-09 19:12:27 UTC) #22
miu
PS4 lgtm. https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_host/media/service_video_capture_device_launcher.h File content/browser/renderer_host/media/service_video_capture_device_launcher.h (right): https://codereview.chromium.org/2857303002/diff/1/content/browser/renderer_host/media/service_video_capture_device_launcher.h#newcode53 content/browser/renderer_host/media/service_video_capture_device_launcher.h:53: void OnDeviceCreationFailed(Callbacks* callbacks, base::OnceClosure done_cb); On 2017/05/08 ...
3 years, 7 months ago (2017-05-09 19:34:19 UTC) #23
chfremer
https://codereview.chromium.org/2857303002/diff/20001/content/browser/renderer_host/media/service_launched_video_capture_device.h File content/browser/renderer_host/media/service_launched_video_capture_device.h (right): https://codereview.chromium.org/2857303002/diff/20001/content/browser/renderer_host/media/service_launched_video_capture_device.h#newcode43 content/browser/renderer_host/media/service_launched_video_capture_device.h:43: base::ThreadChecker thread_checker_; On 2017/05/09 19:34:19, miu wrote: > On ...
3 years, 7 months ago (2017-05-09 20:49:02 UTC) #24
chfremer
Added base::SequenceChecker with PatchSet 6
3 years, 7 months ago (2017-05-09 21:20:18 UTC) #26
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/2857303002/120001
3 years, 7 months ago (2017-05-10 18:03:50 UTC) #36
commit-bot: I haz the power
3 years, 7 months ago (2017-05-10 18:14:06 UTC) #39
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/46083e3783e7a71d7eef4aeb5c5f...

Powered by Google App Engine
This is Rietveld 408576698