Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2244763002:
Video Capture Mojo (1.4a.a): Add service configurator interface (Closed)
|
Created:
4 years, 4 months ago by chfremer Modified:
4 years, 4 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, posciak+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@MakeService Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionVideo Capture Mojo: Add service configurator interface
This CL is part of the Mojo Video Capture work. For the bigger picture, see [1]. This is a follow up to a CL [2] currently under review.
* Add top-level interface VideoCaptureService which exposes access to two different instances of VideoCaptureDevicFactory, one for actual devices and one for fake devices.
* Expose a new interface FakeVideoCaptureDeviceFactoryConfigurator for configuring which fake devices are available.
* Let method CreateDevice() return a result code to be able to report non-success outcome back to clients.
* Added service test:
+ FakeCaptureDeviceGetsEnumerated
BUG=584797
TEST=Build of video_capture_unittests succeeds. All video_capture_unittests pass.
[1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxsxWslJ_X-zpOhijvI8/edit?usp=sharing
[2] https://codereview.chromium.org/2224103002/
Committed: https://crrev.com/262b6e71844a2fe0948c36c78ef1c91f9f02042d
Cr-Commit-Position: refs/heads/master@{#413205}
Patch Set 1 #Patch Set 2 : Merge-in changes from upstream #
Total comments: 23
Patch Set 3 : mcasas' comments #
Total comments: 6
Patch Set 4 : Split out rename to separate CL #
Total comments: 23
Patch Set 5 : yzshen's comments #Patch Set 6 : Removed interface and functionality for configuring multiple devices in fake factory. #
Total comments: 10
Patch Set 7 : mcasas' comments #Dependent Patchsets: Messages
Total messages: 38 (19 generated)
Description was changed from ========== Remove unused headers Format Fill Service Part 1 BUG= ========== to ========== Video Capture Mojo: Add service configurator interface This CL is part of the Mojo Video Capture work. For the bigger picture, see [1]. This is a follow up to a CL [2] currently under review. * Add ServiceConfigurator interface for setting up fake/file-based capture devices. * In service_manifest.json declare different sets of interfaces for "testing" vs. "production". The idea is to expose ServiceConfigurator only where it is needed (currently in testing scenarios, but not in production). * Rename interface VideoCaptureDevice to VideoCaptureDeviceAccess. * Rename method VideoCaptureDeviceFactory::CreateDevice() to GetDeviceAccess(). * Let method GetDeviceAccess() return a result code to be able to report non-success outcome back to clients. * Added service test: + FakeCaptureDeviceGetsEnumerated BUG=584797 TEST=Build of video_capture_unittests succeeds. All video_capture_unittests pass. [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... [2] https://codereview.chromium.org/2224103002/ ==========
chfremer@chromium.org changed reviewers: + emircan@chromium.org, mcasas@chromium.org, rockot@chromium.org, yzshen@chromium.org
mcasas@: Please review general approach and style yzshen@: Please review Mojo aspects emircan@ & rockot@: FYI
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Comments, potentially simplifying this CL, so I'll stop here. https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... File services/video_capture/public/interfaces/service_configurator.mojom (right): https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... services/video_capture/public/interfaces/service_configurator.mojom:11: interface ServiceConfigurator { If this is just for testing (and it is: currently users need to add command-line flags for Fake and File VCDs), I think the mojom convention is to add Test to the name, e.g. ipc/ipc_test.mojom, url/mojo/url_test.mojom adn others. https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... services/video_capture/public/interfaces/service_configurator.mojom:14: AddFakeVideoCaptureDevice() A comment: Fake and File VCDs are used a lot for our own testing in bots that sometimes have working, but inadequate, capture devices; for example, the CQ linux_android_rel_ng uses an Android emulator that has two cameras, but that don't work well for content_browsertests. So: Fake and File do not coexist with the eventual 'real' webcams, but eclipse them altogether. I think we should try to repro that behaviour here as well. https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... File services/video_capture/public/interfaces/video_capture_device_access.mojom (right): https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... services/video_capture/public/interfaces/video_capture_device_access.mojom:25: interface VideoCaptureDeviceAccess { Why the name change? If the concern is that there is already a media::VideoCaptureDevice that would make it confusing vis-a-vis the local video_capture::VideoCaptureDevice, I'd propose VideoCaptureDeviceProxy and CreateDeviceProxy() ? (in this file and the other .mojom, resp). https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... File services/video_capture/public/interfaces/video_capture_device_factory.mojom (right): https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... services/video_capture/public/interfaces/video_capture_device_factory.mojom:58: GetDeviceAccess(VideoCaptureDeviceDescriptor device_descriptor, Yeah, see my comment in the previous .mojom, it might be a good idea to have a synchronous DeviceAccessResultCode on creation, but for all intents and purposes, the result of CreateDevice is a creation of the VCD itself, or a Proxy to it. https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... File services/video_capture/service_manifest.json (right): https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... services/video_capture/service_manifest.json:8: "production": ["video_capture::mojom::VideoCaptureDeviceFactory"] Note that Fake and File VCDevices, despite being test devices in nature, are shipped in production and are used out in the wild by dapper developers; because of this, I think the distinction testing/production is unnecessary so, what about just extending the video_capture_device_factory.mojom's VideoCaptureDeviceFactory to have a method SelectFakeVideoCaptureDevice(FakeDeviceType type) where the FakeDeviceType would be FILE or FAKE ? (For more fun, the FileVCD also needs a file path to operate, but can be read from the command line). https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... File services/video_capture/video_capture_device_factory_impl.cc (right): https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... services/video_capture/video_capture_device_factory_impl.cc:25: VideoCaptureDeviceFactoryImpl::DeviceEntry::~DeviceEntry() = default; empty lines after methods, here and in l.39 https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... services/video_capture/video_capture_device_factory_impl.cc:51: fake_device_descriptor->device_id = device_id_oss.str(); Use base::StringPrintf (which, also works magically for wstrings etc) instead of the ostringstream construction. https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... services/video_capture/video_capture_device_factory_impl.cc:57: base::WrapUnique(new VideoCaptureDeviceAccessImpl())); Per recent discussion in chromium-dev, prefer base::MakeUnique<>(): // MakeUnique<T>(args) should be preferred over WrapUnique(new T(args)) https://cs.chromium.org/chromium/src/base/memory/ptr_util.h?q=WrapUnique&sq=p... https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... services/video_capture/video_capture_device_factory_impl.cc:59: fake_device_count_ += 1; fake_device_count_++; https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... services/video_capture/video_capture_device_factory_impl.cc:68: } nit: no {} for one-line bodies. https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... services/video_capture/video_capture_device_factory_impl.cc:82: NOTIMPLEMENTED(); Don't we need to do some callback.Run(mojom::DeviceAccessResultCode::ERROR); or similar? Otherwise the callback would be... destroyed without having been resolved.
Description was changed from ========== Video Capture Mojo: Add service configurator interface This CL is part of the Mojo Video Capture work. For the bigger picture, see [1]. This is a follow up to a CL [2] currently under review. * Add ServiceConfigurator interface for setting up fake/file-based capture devices. * In service_manifest.json declare different sets of interfaces for "testing" vs. "production". The idea is to expose ServiceConfigurator only where it is needed (currently in testing scenarios, but not in production). * Rename interface VideoCaptureDevice to VideoCaptureDeviceAccess. * Rename method VideoCaptureDeviceFactory::CreateDevice() to GetDeviceAccess(). * Let method GetDeviceAccess() return a result code to be able to report non-success outcome back to clients. * Added service test: + FakeCaptureDeviceGetsEnumerated BUG=584797 TEST=Build of video_capture_unittests succeeds. All video_capture_unittests pass. [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... [2] https://codereview.chromium.org/2224103002/ ========== to ========== Video Capture Mojo: Add service configurator interface This CL is part of the Mojo Video Capture work. For the bigger picture, see [1]. This is a follow up to a CL [2] currently under review. * Add top-level interface VideoCaptureService which exposes access to two different instances of VideoCaptureDevicFactory, one for actual devices and one for fake devices. * Expose a new interface FakeVideoCaptureDeviceFactoryConfigurator for configuring which fake devices are available. * Rename interface VideoCaptureDevice to VideoCaptureDeviceProxy. * Rename method VideoCaptureDeviceFactory::CreateDevice() to CreateDeviceProxy(). * Let method CreateDeviceProxy() return a result code to be able to report non-success outcome back to clients. * Added service test: + FakeCaptureDeviceGetsEnumerated BUG=584797 TEST=Build of video_capture_unittests succeeds. All video_capture_unittests pass. [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... [2] https://codereview.chromium.org/2224103002/ ==========
mcasas@: PTAL Changes in PatchSet: * Renamed VideoCaptureDeviceAccess -> VideoCaptureDeviceProxy. * Renamed GetDeviceAccess() -> CreateDeviceProxy(). * Renamed ServiceConfigurator to FakeVideoCaptureDeviceFactoryConfigurator. * Added top-level interface VideoCaptureService. * Let VideoCaptureService expose two different instances of VideoCaptureDevicFactory, one for prod, one for testing. * Updated issue description. https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... File services/video_capture/public/interfaces/service_configurator.mojom (right): https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... services/video_capture/public/interfaces/service_configurator.mojom:11: interface ServiceConfigurator { On 2016/08/12 23:38:37, mcasas wrote: > If this is just for testing (and it is: currently users need > to add command-line flags for Fake and File VCDs), I > think the mojom convention is to add Test to the name, > e.g. ipc/ipc_test.mojom, url/mojo/url_test.mojom adn others. Good point. I am not sure, though, if this is really considered test-only "enough". As you pointed out in the other comment, the fake devices are actually shipped in production builds, so managing those devices can be considered legit production functionality. I'll keep this in mind, but since I am proposing to change the structure of the interface a little bit in the next PatchSet, this may not apply anymore. Please see next PatchSet. https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... services/video_capture/public/interfaces/service_configurator.mojom:14: AddFakeVideoCaptureDevice() On 2016/08/12 23:38:37, mcasas wrote: > A comment: Fake and File VCDs are used a lot for > our own testing in bots that sometimes have working, > but inadequate, capture devices; for example, the CQ > linux_android_rel_ng uses an Android emulator that has > two cameras, but that don't work well for > content_browsertests. So: Fake and File do not coexist > with the eventual 'real' webcams, but eclipse them > altogether. I think we should try to repro that behaviour > here as well. Agreed that we probably do not need to have a single factory that mixes both real and fake devices. After thinking about it for a bit, instead of exposing switch to switch the factory between real and fake, I propose to have the service expose two factory instances, one for the actual devices, and one for fake devices. A separate interface is exposed for configuration of the fake factory. WDYT? https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... File services/video_capture/public/interfaces/video_capture_device_access.mojom (right): https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... services/video_capture/public/interfaces/video_capture_device_access.mojom:25: interface VideoCaptureDeviceAccess { On 2016/08/12 23:38:37, mcasas wrote: > Why the name change? If the concern is that there is > already a media::VideoCaptureDevice that would make > it confusing vis-a-vis the local > video_capture::VideoCaptureDevice, I'd propose > VideoCaptureDeviceProxy and CreateDeviceProxy() ? > (in this file and the other .mojom, resp). The concern is that instances of this interface represent access to a device and not the device itself. The purpose of the rename was to reveal that more clearly in the interface name. The name "VideoCaptureDeviceProxy" will work for that as well, so let me follow your suggestion. https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... File services/video_capture/public/interfaces/video_capture_device_factory.mojom (right): https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... services/video_capture/public/interfaces/video_capture_device_factory.mojom:58: GetDeviceAccess(VideoCaptureDeviceDescriptor device_descriptor, On 2016/08/12 23:38:37, mcasas wrote: > Yeah, see my comment in the previous .mojom, it > might be a good idea to have a synchronous > DeviceAccessResultCode on creation, but for all > intents and purposes, the result of CreateDevice is > a creation of the VCD itself, or a Proxy to it. Done. https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... File services/video_capture/service_manifest.json (right): https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... services/video_capture/service_manifest.json:8: "production": ["video_capture::mojom::VideoCaptureDeviceFactory"] On 2016/08/12 23:38:37, mcasas wrote: > Note that Fake and File VCDevices, despite being > test devices in nature, are shipped in production and > are used out in the wild by dapper developers; > because of this, I think the distinction testing/production > is unnecessary so, what about just extending the > video_capture_device_factory.mojom's > VideoCaptureDeviceFactory to have a method > SelectFakeVideoCaptureDevice(FakeDeviceType type) > where the FakeDeviceType would be FILE or FAKE ? > (For more fun, the FileVCD also needs a file path to > operate, but can be read from the command line). Done. https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... File services/video_capture/video_capture_device_factory_impl.cc (right): https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... services/video_capture/video_capture_device_factory_impl.cc:25: VideoCaptureDeviceFactoryImpl::DeviceEntry::~DeviceEntry() = default; On 2016/08/12 23:38:37, mcasas wrote: > empty lines after methods, here and in l.39 Done. https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... services/video_capture/video_capture_device_factory_impl.cc:51: fake_device_descriptor->device_id = device_id_oss.str(); On 2016/08/12 23:38:37, mcasas wrote: > Use base::StringPrintf (which, also works magically > for wstrings etc) instead of the ostringstream construction. Done. https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... services/video_capture/video_capture_device_factory_impl.cc:57: base::WrapUnique(new VideoCaptureDeviceAccessImpl())); On 2016/08/12 23:38:37, mcasas wrote: > Per recent discussion in chromium-dev, prefer base::MakeUnique<>(): > // MakeUnique<T>(args) should be preferred over WrapUnique(new T(args)) > > https://cs.chromium.org/chromium/src/base/memory/ptr_util.h?q=WrapUnique&sq=p... Done. https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... services/video_capture/video_capture_device_factory_impl.cc:59: fake_device_count_ += 1; On 2016/08/12 23:38:37, mcasas wrote: > fake_device_count_++; Done. https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... services/video_capture/video_capture_device_factory_impl.cc:68: } On 2016/08/12 23:38:37, mcasas wrote: > nit: no {} for one-line bodies. Done. https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... services/video_capture/video_capture_device_factory_impl.cc:82: NOTIMPLEMENTED(); On 2016/08/12 23:38:37, mcasas wrote: > Don't we need to do some > callback.Run(mojom::DeviceAccessResultCode::ERROR); > or similar? Otherwise the callback would be... destroyed > without having been resolved. Done.
https://codereview.chromium.org/2244763002/diff/40001/services/video_capture/... File services/video_capture/public/interfaces/fake_video_capture_device_factory_configurator.mojom (right): https://codereview.chromium.org/2244763002/diff/40001/services/video_capture/... services/video_capture/public/interfaces/fake_video_capture_device_factory_configurator.mojom:15: => (VideoCaptureDeviceDescriptor descriptor); I still feel that this should be a) in video_capture_device_factory.mojom, and not in a .mojom file of its own b) should not just say "Add", since the first time we create a FakeVCD, it should not remove all "normal" system devices. ("Create" or "Append" might be better, but read on). c) should not refer to FakeVCD, because there is another FileVCD. (I sugggest "Test"). New proposal: In the browser, we know with certainty on startup if the VCDF is supposed to be full of test devices or not, and how many. So: add a new method called e.g. SwitchOnTestMode(number_of_devices). to the interface VideoCaptureDeviceFactory. We can either enforce this method to be called before EnumerateDevices() (adding DCHECKs for this in the code), or we can invalidate the VCDDescriptors generated so far and kill the potentiall associated capture sessions, but the latter seems overkill to me. https://codereview.chromium.org/2244763002/diff/40001/services/video_capture/... File services/video_capture/video_capture_device_proxy_impl.cc (right): https://codereview.chromium.org/2244763002/diff/40001/services/video_capture/... services/video_capture/video_capture_device_proxy_impl.cc:11: mojom::VideoCaptureFormatPtr requested_format, It might be easier to extract the s/VideoCaptureDevice/VideoCaptureDeviceProxy/ to another CL: reviewers tend to be more responsive if they read that a particular CL introduces no code changes.
Patchset #4 (id:60001) has been deleted
Description was changed from ========== Video Capture Mojo: Add service configurator interface This CL is part of the Mojo Video Capture work. For the bigger picture, see [1]. This is a follow up to a CL [2] currently under review. * Add top-level interface VideoCaptureService which exposes access to two different instances of VideoCaptureDevicFactory, one for actual devices and one for fake devices. * Expose a new interface FakeVideoCaptureDeviceFactoryConfigurator for configuring which fake devices are available. * Rename interface VideoCaptureDevice to VideoCaptureDeviceProxy. * Rename method VideoCaptureDeviceFactory::CreateDevice() to CreateDeviceProxy(). * Let method CreateDeviceProxy() return a result code to be able to report non-success outcome back to clients. * Added service test: + FakeCaptureDeviceGetsEnumerated BUG=584797 TEST=Build of video_capture_unittests succeeds. All video_capture_unittests pass. [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... [2] https://codereview.chromium.org/2224103002/ ========== to ========== Video Capture Mojo: Add service configurator interface This CL is part of the Mojo Video Capture work. For the bigger picture, see [1]. This is a follow up to a CL [2] currently under review. * Add top-level interface VideoCaptureService which exposes access to two different instances of VideoCaptureDevicFactory, one for actual devices and one for fake devices. * Expose a new interface FakeVideoCaptureDeviceFactoryConfigurator for configuring which fake devices are available. * Let method CreateDeviceProxy() return a result code to be able to report non-success outcome back to clients. * Added service test: + FakeCaptureDeviceGetsEnumerated BUG=584797 TEST=Build of video_capture_unittests succeeds. All video_capture_unittests pass. [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... [2] https://codereview.chromium.org/2224103002/ ==========
mcasas@: PTAL I split out the rename to a separate CL. https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... File services/video_capture/service_manifest.json (right): https://codereview.chromium.org/2244763002/diff/20001/services/video_capture/... services/video_capture/service_manifest.json:8: "production": ["video_capture::mojom::VideoCaptureDeviceFactory"] On 2016/08/12 23:38:37, mcasas wrote: > Note that Fake and File VCDevices, despite being > test devices in nature, are shipped in production and > are used out in the wild by dapper developers; > because of this, I think the distinction testing/production > is unnecessary so, what about just extending the > video_capture_device_factory.mojom's > VideoCaptureDeviceFactory to have a method > SelectFakeVideoCaptureDevice(FakeDeviceType type) > where the FakeDeviceType would be FILE or FAKE ? > (For more fun, the FileVCD also needs a file path to > operate, but can be read from the command line). I removed the differentiation between testing and production for now. https://codereview.chromium.org/2244763002/diff/40001/services/video_capture/... File services/video_capture/public/interfaces/fake_video_capture_device_factory_configurator.mojom (right): https://codereview.chromium.org/2244763002/diff/40001/services/video_capture/... services/video_capture/public/interfaces/fake_video_capture_device_factory_configurator.mojom:15: => (VideoCaptureDeviceDescriptor descriptor); On 2016/08/16 02:18:13, mcasas wrote: > New proposal: In the browser, we know with certainty on startup > if the VCDF is supposed to be full of test devices or not, and how > many. So: add a new method called e.g. > SwitchOnTestMode(number_of_devices). > to the interface VideoCaptureDeviceFactory. > We can either > enforce this method to be called before EnumerateDevices() > (adding DCHECKs for this in the code), or we can invalidate > the VCDDescriptors generated so far and kill the potentiall > associated capture sessions, but the latter seems overkill to > me. This ambiguity of what should happen when switching the factory to a test mode is why I want to avoid the switchable factory and am proposing to use two separate factories instead. The two-factory design does not suffer from this ambiguity and we don't need to put any extra documentation to explain correct usage or runtime checks to guard against wrong usage. > I still feel that this should be > a) in video_capture_device_factory.mojom, and not in a .mojom file of its > own Acknowledged, but I would love to see the argument for what makes you feel this way. An argument in favor of having the configurator interface separate is that it does not force clients that are only interested in using the factory interface to also know about fake devices. Setting up fake devices is a separate responsibility, so we should not allow clients to handle them separately. I feel the cost of having an extra *.mojom is lower than the cost of mixing responsibilities. > b) should not just say "Add", since the first time we create a FakeVCD, > it should not remove all "normal" system devices. ("Create" or > "Append" might be better, but read on). > c) should not refer to FakeVCD, because there is another FileVCD. > (I sugggest "Test"). I am not sure I understand your concern correctly. My idea is that a few CLs down the road, this interface will not only support fake devices, but also file-based devices and possibly mock devices. In this CL, we only have added support for fake devices. That is why there is currently only a single method specifically for adding fake devices. It is called "add", because it adds a new fake device to the fake factory. Did you mean that the second factory should be called "TestVideoCaptureDeviceFactory" instead of "FakeVideoCaptureDeviceFactory"? https://codereview.chromium.org/2244763002/diff/40001/services/video_capture/... File services/video_capture/video_capture_device_proxy_impl.cc (right): https://codereview.chromium.org/2244763002/diff/40001/services/video_capture/... services/video_capture/video_capture_device_proxy_impl.cc:11: mojom::VideoCaptureFormatPtr requested_format, On 2016/08/16 02:18:13, mcasas wrote: > It might be easier to extract the > s/VideoCaptureDevice/VideoCaptureDeviceProxy/ > to another CL: reviewers tend to be more responsive > if they read that a particular CL introduces no code > changes. Good point. I will split it out to a new CL.
https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... File services/video_capture/fake_video_capture_device_factory_configurator_impl.cc (right): https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... services/video_capture/fake_video_capture_device_factory_configurator_impl.cc:6: #include "services/video_capture/fake_video_capture_device_factory_configurator_impl.h" style nit: This should be the first include, with a blank line between it and the rest. https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... File services/video_capture/fake_video_capture_device_factory_configurator_impl.h (right): https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... services/video_capture/fake_video_capture_device_factory_configurator_impl.h:5: #ifndef SERVICES_VIDEO_CAPTURE_SERVICE_CONFIGURATOR_IMPL_H_ Please update this header guard to reflect the class name. https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... services/video_capture/fake_video_capture_device_factory_configurator_impl.h:16: FakeVideoCaptureDeviceFactoryConfiguratorImpl( explicit, please. https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... File services/video_capture/public/interfaces/BUILD.gn (right): https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... services/video_capture/public/interfaces/BUILD.gn:9: "fake_video_capture_device_factory_configurator.mojom", is this used only for test? if yes, please consider moving it to a separate target with testonly set to true. https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... File services/video_capture/public/interfaces/video_capture_service.mojom (right): https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... services/video_capture/public/interfaces/video_capture_service.mojom:13: // factory uses the same interface but provides access to a set of fake devices Is the "fake" one only used for testing? If yes, I would suggest separating production interfaces with testing ones. https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... services/video_capture/public/interfaces/video_capture_service.mojom:20: FakeVideoCaptureDeviceFactoryConfigurator & request); nit: no need to have a space before "&" https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... File services/video_capture/video_capture_device_factory_impl.cc (right): https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... services/video_capture/video_capture_device_factory_impl.cc:23: device_ = std::move(bindable_target); do it in the initialization list? https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... File services/video_capture/video_capture_device_factory_impl.h (right): https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... services/video_capture/video_capture_device_factory_impl.h:36: // inside std::map. The performance penalty of not using a map should If you want, you could easily define a comparison operator to use it with a map. Eventually mojo would like to provide an auto-generated one, but it is not available yet. https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... File services/video_capture/video_capture_device_impl.h (right): https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... services/video_capture/video_capture_device_impl.h:5: #ifndef SERVICES_VIDEO_CAPTURE_VIDEO_CAPTURE_DEVICE_PROXY_IMPL_H_ It seems weird: this file is flagged as "deleted", but the diff doesn't seem so. Is it actually deleted? https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... File services/video_capture/video_capture_service.cc (right): https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... services/video_capture/video_capture_service.cc:13: device_factory_ = base::MakeUnique<VideoCaptureDeviceFactoryImpl>(); Please do these in the initailizer list. https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... File services/video_capture/video_capture_service.h (right): https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... services/video_capture/video_capture_service.h:6: #define SERVICES_VIDEO_CAPTURE_SERVICE_H_ Please match file path exactly.
yzshen@: PTAL https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... File services/video_capture/fake_video_capture_device_factory_configurator_impl.cc (right): https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... services/video_capture/fake_video_capture_device_factory_configurator_impl.cc:6: #include "services/video_capture/fake_video_capture_device_factory_configurator_impl.h" On 2016/08/16 17:55:08, yzshen1 wrote: > style nit: This should be the first include, with a blank line between it and > the rest. Done. https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... File services/video_capture/fake_video_capture_device_factory_configurator_impl.h (right): https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... services/video_capture/fake_video_capture_device_factory_configurator_impl.h:5: #ifndef SERVICES_VIDEO_CAPTURE_SERVICE_CONFIGURATOR_IMPL_H_ On 2016/08/16 17:55:08, yzshen1 wrote: > Please update this header guard to reflect the class name. Done. https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... services/video_capture/fake_video_capture_device_factory_configurator_impl.h:16: FakeVideoCaptureDeviceFactoryConfiguratorImpl( On 2016/08/16 17:55:08, yzshen1 wrote: > explicit, please. Done. https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... File services/video_capture/public/interfaces/BUILD.gn (right): https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... services/video_capture/public/interfaces/BUILD.gn:9: "fake_video_capture_device_factory_configurator.mojom", On 2016/08/16 17:55:08, yzshen1 wrote: > is this used only for test? if yes, please consider moving it to a separate > target with testonly set to true. Contrary to what the name suggests, the fake device functionality is actually shipped as part of production builds. I'll keep an eye out for test-only stuff, though, and consider keeping it in separate interfaces. https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... File services/video_capture/public/interfaces/video_capture_service.mojom (right): https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... services/video_capture/public/interfaces/video_capture_service.mojom:13: // factory uses the same interface but provides access to a set of fake devices On 2016/08/16 17:55:08, yzshen1 wrote: > Is the "fake" one only used for testing? If yes, I would suggest separating > production interfaces with testing ones. Ack. The Fake factory is to be shipped as part of production builds. https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... services/video_capture/public/interfaces/video_capture_service.mojom:20: FakeVideoCaptureDeviceFactoryConfigurator & request); On 2016/08/16 17:55:08, yzshen1 wrote: > nit: no need to have a space before "&" Done. https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... File services/video_capture/video_capture_device_factory_impl.cc (right): https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... services/video_capture/video_capture_device_factory_impl.cc:23: device_ = std::move(bindable_target); On 2016/08/16 17:55:08, yzshen1 wrote: > do it in the initialization list? Done. https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... File services/video_capture/video_capture_device_factory_impl.h (right): https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... services/video_capture/video_capture_device_factory_impl.h:36: // inside std::map. The performance penalty of not using a map should On 2016/08/16 17:55:08, yzshen1 wrote: > If you want, you could easily define a comparison operator to use it with a map. > Eventually mojo would like to provide an auto-generated one, but it is not > available yet. Good point. Let me rephrase the comment to clarify that it is not impossible to use in std::map, just not (yet) very convenient. I'd like to keep the vector for now, though, instead of providing the comparison operator by hand. https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... File services/video_capture/video_capture_device_impl.h (right): https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... services/video_capture/video_capture_device_impl.h:5: #ifndef SERVICES_VIDEO_CAPTURE_VIDEO_CAPTURE_DEVICE_PROXY_IMPL_H_ On 2016/08/16 17:55:08, yzshen1 wrote: > It seems weird: this file is flagged as "deleted", but the diff doesn't seem so. > > Is it actually deleted? Sorry about this. This is because I first renamed it and then renamed it back to split out the renaming into a separate CL. Then I forgot to update the guard. https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... File services/video_capture/video_capture_service.cc (right): https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... services/video_capture/video_capture_service.cc:13: device_factory_ = base::MakeUnique<VideoCaptureDeviceFactoryImpl>(); On 2016/08/16 17:55:08, yzshen1 wrote: > Please do these in the initailizer list. In this case, I intentionally chose to do the initialization in the constructor body, because |configurator_| must be initialized after |fake_device_factory_| and relying on the order they are declared in in the class declaration seemed too fragile. Let me add a comment to explain this in the code. https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... File services/video_capture/video_capture_service.h (right): https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... services/video_capture/video_capture_service.h:6: #define SERVICES_VIDEO_CAPTURE_SERVICE_H_ On 2016/08/16 17:55:08, yzshen1 wrote: > Please match file path exactly. Done.
Description was changed from ========== Video Capture Mojo: Add service configurator interface This CL is part of the Mojo Video Capture work. For the bigger picture, see [1]. This is a follow up to a CL [2] currently under review. * Add top-level interface VideoCaptureService which exposes access to two different instances of VideoCaptureDevicFactory, one for actual devices and one for fake devices. * Expose a new interface FakeVideoCaptureDeviceFactoryConfigurator for configuring which fake devices are available. * Let method CreateDeviceProxy() return a result code to be able to report non-success outcome back to clients. * Added service test: + FakeCaptureDeviceGetsEnumerated BUG=584797 TEST=Build of video_capture_unittests succeeds. All video_capture_unittests pass. [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... [2] https://codereview.chromium.org/2224103002/ ========== to ========== Video Capture Mojo: Add service configurator interface This CL is part of the Mojo Video Capture work. For the bigger picture, see [1]. This is a follow up to a CL [2] currently under review. * Add top-level interface VideoCaptureService which exposes access to two different instances of VideoCaptureDevicFactory, one for actual devices and one for fake devices. * Expose a new interface FakeVideoCaptureDeviceFactoryConfigurator for configuring which fake devices are available. * Let method CreateDevice() return a result code to be able to report non-success outcome back to clients. * Added service test: + FakeCaptureDeviceGetsEnumerated BUG=584797 TEST=Build of video_capture_unittests succeeds. All video_capture_unittests pass. [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... [2] https://codereview.chromium.org/2224103002/ ==========
https://codereview.chromium.org/2244763002/diff/40001/services/video_capture/... File services/video_capture/public/interfaces/fake_video_capture_device_factory_configurator.mojom (right): https://codereview.chromium.org/2244763002/diff/40001/services/video_capture/... services/video_capture/public/interfaces/fake_video_capture_device_factory_configurator.mojom:15: => (VideoCaptureDeviceDescriptor descriptor); On 2016/08/16 16:48:02, chfremer wrote: > On 2016/08/16 02:18:13, mcasas wrote: > > New proposal: In the browser, we know with certainty on startup > > if the VCDF is supposed to be full of test devices or not, and how > > many. So: add a new method called e.g. > > SwitchOnTestMode(number_of_devices). > > to the interface VideoCaptureDeviceFactory. > > We can either > > enforce this method to be called before EnumerateDevices() > > (adding DCHECKs for this in the code), or we can invalidate > > the VCDDescriptors generated so far and kill the potentiall > > associated capture sessions, but the latter seems overkill to > > me. > > This ambiguity of what should happen when switching the factory to a test mode > is why I want to avoid the switchable factory and am proposing to use two > separate factories instead. The two-factory design does not suffer from this > ambiguity and we don't need to put any extra documentation to explain correct > usage or runtime checks to guard against wrong usage. Hmm I see no ambiguity :) - VCDF works in normal mode or in test mode ("test" mode would override "normal" mode). I think your pointis: 2 interfaces, one for normal operation, one for "test", versus one single interface with a specific method. I think the second one is simpler to a newcomer, hence my opinion. > > > I still feel that this should be > > a) in video_capture_device_factory.mojom, and not in a .mojom file of its > > own > > Acknowledged, but I would love to see the argument for what makes you feel this > way. > An argument in favor of having the configurator interface separate is that it > does not force clients that are only interested in using the factory interface > to also know about fake devices. Setting up fake devices is a separate > responsibility, so we should not allow clients to handle them separately. > I feel the cost of having an extra *.mojom is lower than the cost of mixing > responsibilities. VCDF service will need to be instantiated somewhere, (that'd be the VCManager nowadays, right?) which is where the parsing of the command line flags could happen, so something like (pseudocode, sorry): factory = connect_to_vcdf_service(); if (flag-for-fake-device-is-on) factory->useFakeVCDs(2 /* num_devices */); else if (flag-for-file-device-is-on) factory->useFileVCD(file-name); the client of the factory interface must know how to connect and use it, so why shouldn't it know about its special modes? And, let me insist, there is no coexistence of either mode (using Fakes, using Files, or "normal"); Also I'm not sure why a client would need the |descriptor|. Isn't it the purpose of the fake-mode to mimic the normal-mode? Any client, test or not, should do a EnumerateDeviceDescriptors() and then CreateDeviceProxy(), but if this code here returns a VideoCaptureDeviceDescriptor, callers might feel tempted to skip the enumeration, and just call directly CreateDeviceProxy(). > > > b) should not just say "Add", since the first time we create a FakeVCD, > > it should not remove all "normal" system devices. ("Create" or > > "Append" might be better, but read on). > > c) should not refer to FakeVCD, because there is another FileVCD. > > (I sugggest "Test"). > > I am not sure I understand your concern correctly. > My idea is that a few CLs down the road, this interface will not only support > fake devices, but also file-based devices and possibly mock devices. In this CL, > we only have added support for fake devices. That is why there is currently only > a single method specifically for adding fake devices. It is called "add", > because it adds a new fake device to the fake factory. > Did you mean that the second factory should be called > "TestVideoCaptureDeviceFactory" instead of "FakeVideoCaptureDeviceFactory"? > Why not adding FileVCDs as well? The VCD interface is the same, and for the time being you can access the CommandLine from services/ (examples abound).
mcasas@: PTAL (and sorry about the lengthy discussion thread, happy to chat offline if you prefer) https://codereview.chromium.org/2244763002/diff/40001/services/video_capture/... File services/video_capture/public/interfaces/fake_video_capture_device_factory_configurator.mojom (right): https://codereview.chromium.org/2244763002/diff/40001/services/video_capture/... services/video_capture/public/interfaces/fake_video_capture_device_factory_configurator.mojom:15: => (VideoCaptureDeviceDescriptor descriptor); On 2016/08/16 21:43:02, mcasas wrote: > On 2016/08/16 16:48:02, chfremer wrote: > > On 2016/08/16 02:18:13, mcasas wrote: > > > New proposal: In the browser, we know with certainty on startup > > > if the VCDF is supposed to be full of test devices or not, and how > > > many. So: add a new method called e.g. > > > SwitchOnTestMode(number_of_devices). > > > to the interface VideoCaptureDeviceFactory. > > > We can either > > > enforce this method to be called before EnumerateDevices() > > > (adding DCHECKs for this in the code), or we can invalidate > > > the VCDDescriptors generated so far and kill the potentiall > > > associated capture sessions, but the latter seems overkill to > > > me. > > > > This ambiguity of what should happen when switching the factory to a test mode > > is why I want to avoid the switchable factory and am proposing to use two > > separate factories instead. The two-factory design does not suffer from this > > ambiguity and we don't need to put any extra documentation to explain correct > > usage or runtime checks to guard against wrong usage. > > Hmm I see no ambiguity :) - VCDF works in normal mode > or in test mode ("test" mode would override "normal" mode). > I think your pointis: 2 interfaces, one for normal operation, > one for "test", versus one single interface with a specific > method. I think the second one is simpler to a newcomer, > hence my opinion. The ambiguity I see is that the interface allows clients to switch the mode at an "unconvenient" time, i.e. any time after it has already been in use. My design proposal is to have 1 interface for operation (production or test, both use the same interface) and 1 separate interface for configuration. To me this feels cleaner and simpler than 1 interface for both operation and configuration, because it separates responsibilities. > > > I still feel that this should be > > > a) in video_capture_device_factory.mojom, and not in a .mojom file of its > > > own > > > > Acknowledged, but I would love to see the argument for what makes you feel > this > > way. > > An argument in favor of having the configurator interface separate is that it > > does not force clients that are only interested in using the factory interface > > to also know about fake devices. Setting up fake devices is a separate > > responsibility, so we should not allow clients to handle them separately. > > I feel the cost of having an extra *.mojom is lower than the cost of mixing > > responsibilities. > > VCDF service will need to be instantiated somewhere, > (that'd be the VCManager nowadays, right?) which is where > the parsing of the command line flags could happen, so > something like (pseudocode, sorry): > > factory = connect_to_vcdf_service(); > if (flag-for-fake-device-is-on) > factory->useFakeVCDs(2 /* num_devices */); > else if (flag-for-file-device-is-on) > factory->useFileVCD(file-name); > > the client of the factory interface must know how > to connect and use it, so why shouldn't it know > about its special modes? And, let me insist, there > is no coexistence of either mode (using Fakes, > using Files, or "normal"); My point here is that the client who connects to the service and configures the special modes does not have to be the same as the client who eventually uses the factory. Hence the two separate interfaces. I understand that there is currently no coexistence of different modes. The proposed design makes it easy for the client who chooses the factory and configures the modes to replicate exactly that. But it does seem useful for tests to be able to configure more than one fake/file/mock device and it does not add significant complexity, so I don't see a need to have the interface design enforce single fake/file device modes. > Also I'm not sure why a client would need the |descriptor|. > Isn't it the purpose of the fake-mode to mimic the > normal-mode? Any client, test or not, should do a > EnumerateDeviceDescriptors() and then > CreateDeviceProxy(), but if this code here returns > a VideoCaptureDeviceDescriptor, callers might > feel tempted to skip the enumeration, and just call > directly CreateDeviceProxy(). My idea here was to give clients (tests) that set up more than one fake device the ability to identify which one is which without having to go through EnumerateDeviceDescriptors and "guessing" what identifier the service has generated. I think it is reasonable for a test that both sets up fake devices and exercises the factory interface to skip the EnumerateDeviceDescriptors() step. There are actually test in the follow-up CLs that do exactly that. > > > b) should not just say "Add", since the first time we create a FakeVCD, > > > it should not remove all "normal" system devices. ("Create" or > > > "Append" might be better, but read on). > > > c) should not refer to FakeVCD, because there is another FileVCD. > > > (I sugggest "Test"). > > > > I am not sure I understand your concern correctly. > > My idea is that a few CLs down the road, this interface will not only support > > fake devices, but also file-based devices and possibly mock devices. In this > CL, > > we only have added support for fake devices. That is why there is currently > only > > a single method specifically for adding fake devices. It is called "add", > > because it adds a new fake device to the fake factory. > > Did you mean that the second factory should be called > > "TestVideoCaptureDeviceFactory" instead of "FakeVideoCaptureDeviceFactory"? > > > > Why not adding FileVCDs as well? The VCD interface is the > same, and for the time being you can access the CommandLine > from services/ (examples abound). Sorry if my explanations were a bit unclear. I absolutely do want to add FileVCDs as well. But since they are not yet needed in the current tests, I have not done it just yet. We can have the VideoCaptureManager check the CommandLine and then pick and configure the corresponding VideoCaptureDeviceFactory accordingly. For the unit (service) tests, we do no need to go through the CommandLine. Please let me know if this makes sense. If not, I am always happy to chat about this offline in more detail.
https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... File services/video_capture/public/interfaces/BUILD.gn (right): https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... services/video_capture/public/interfaces/BUILD.gn:9: "fake_video_capture_device_factory_configurator.mojom", On 2016/08/16 18:34:40, chfremer wrote: > On 2016/08/16 17:55:08, yzshen1 wrote: > > is this used only for test? if yes, please consider moving it to a separate > > target with testonly set to true. > > Contrary to what the name suggests, the fake device functionality is actually > shipped as part of production builds. I'll keep an eye out for test-only stuff, > though, and consider keeping it in separate interfaces. Do you mean the fake device functionality is actually used by some production features? Or it is just built into the production for convenience? If it is not used by any production features, we should split it out. It helps to reduce binary size and minimizes the chance of exposing testing functionality unintentionally. WDYT?
On 2016/08/17 16:22:54, yzshen1 wrote: > https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... > File services/video_capture/public/interfaces/BUILD.gn (right): > > https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... > services/video_capture/public/interfaces/BUILD.gn:9: > "fake_video_capture_device_factory_configurator.mojom", > On 2016/08/16 18:34:40, chfremer wrote: > > On 2016/08/16 17:55:08, yzshen1 wrote: > > > is this used only for test? if yes, please consider moving it to a separate > > > target with testonly set to true. > > > > Contrary to what the name suggests, the fake device functionality is actually > > shipped as part of production builds. I'll keep an eye out for test-only > stuff, > > though, and consider keeping it in separate interfaces. > > Do you mean the fake device functionality is actually used by some production > features? Or it is just built into the production for convenience? > > If it is not used by any production features, we should split it out. It helps > to reduce binary size and minimizes the chance of exposing testing functionality > unintentionally. > > WDYT? Yes, I meant to say it is a production feature. The fake device can be activated via a command-line switch. This switch is available in production builds and the feature is probably used by many developers working on web apps that involve video capture. Therefore, we unfortunately cannot split it out to reduce the binary size.
On 2016/08/17 16:30:14, chfremer wrote: > On 2016/08/17 16:22:54, yzshen1 wrote: > > > https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... > > File services/video_capture/public/interfaces/BUILD.gn (right): > > > > > https://codereview.chromium.org/2244763002/diff/80001/services/video_capture/... > > services/video_capture/public/interfaces/BUILD.gn:9: > > "fake_video_capture_device_factory_configurator.mojom", > > On 2016/08/16 18:34:40, chfremer wrote: > > > On 2016/08/16 17:55:08, yzshen1 wrote: > > > > is this used only for test? if yes, please consider moving it to a > separate > > > > target with testonly set to true. > > > > > > Contrary to what the name suggests, the fake device functionality is > actually > > > shipped as part of production builds. I'll keep an eye out for test-only > > stuff, > > > though, and consider keeping it in separate interfaces. > > > > Do you mean the fake device functionality is actually used by some production > > features? Or it is just built into the production for convenience? > > > > If it is not used by any production features, we should split it out. It helps > > to reduce binary size and minimizes the chance of exposing testing > functionality > > unintentionally. > > > > WDYT? > > Yes, I meant to say it is a production feature. > The fake device can be activated via a command-line switch. This switch is > available > in production builds and the feature is probably used by many developers working > on > web apps that involve video capture. Therefore, we unfortunately cannot split it > out > to reduce the binary size. Thanks for the explanation! LGTM Please wait for the other owners's LG.
mcasas@: PTAL Removed interface and functionality for configuring multiple devices in fake factory. The fake factory now provides access to a single fake device with fixed configuration.
LGTM % comments https://codereview.chromium.org/2244763002/diff/120001/services/video_capture... File services/video_capture/service_unittest.cc (right): https://codereview.chromium.org/2244763002/diff/120001/services/video_capture... services/video_capture/service_unittest.cc:32: void HandleAddFakeVideoCaptureDeviceCallback( Unused? Remove if so. https://codereview.chromium.org/2244763002/diff/120001/services/video_capture... services/video_capture/service_unittest.cc:65: ; Remove extra ; https://codereview.chromium.org/2244763002/diff/120001/services/video_capture... services/video_capture/service_unittest.cc:73: TEST_F(VideoCaptureServiceTest, FakeDeviceFactroyEnumeratesOneDevice) { s/Factroy/Factory/ https://codereview.chromium.org/2244763002/diff/120001/services/video_capture... File services/video_capture/video_capture_device_factory_impl.h (right): https://codereview.chromium.org/2244763002/diff/120001/services/video_capture... services/video_capture/video_capture_device_factory_impl.h:53: }; DeviceEntry should not be copyable, so add here DISALLOW_COPY_AND_ASSIGN(DeviceEntry); https://cs.chromium.org/chromium/src/base/macros.h?q=disallow_copy_and&sq=pac... https://codereview.chromium.org/2244763002/diff/120001/services/video_capture... File services/video_capture/video_capture_service.cc (right): https://codereview.chromium.org/2244763002/diff/120001/services/video_capture... services/video_capture/video_capture_service.cc:28: base::MakeUnique<VideoCaptureDeviceImpl>()); Consider moving l.20-28 to ConnectToFakeDeviceFactory(), so this instantiation only happens if needed. Also, shouldn't the construction of |device_factory_| and |fake_device_factory_| be done lazily in, resp., ConnectToDeviceFactory() and ConnectToFakeDeviceFactory() ?
chfremer@chromium.org changed reviewers: + tsepez@chromium.org
tsepez@: PTAL *.mojom https://codereview.chromium.org/2244763002/diff/120001/services/video_capture... File services/video_capture/service_unittest.cc (right): https://codereview.chromium.org/2244763002/diff/120001/services/video_capture... services/video_capture/service_unittest.cc:32: void HandleAddFakeVideoCaptureDeviceCallback( On 2016/08/18 21:11:21, mcasas wrote: > Unused? Remove if so. Done. https://codereview.chromium.org/2244763002/diff/120001/services/video_capture... services/video_capture/service_unittest.cc:65: ; On 2016/08/18 21:11:21, mcasas wrote: > Remove extra ; Done. https://codereview.chromium.org/2244763002/diff/120001/services/video_capture... services/video_capture/service_unittest.cc:73: TEST_F(VideoCaptureServiceTest, FakeDeviceFactroyEnumeratesOneDevice) { On 2016/08/18 21:11:21, mcasas wrote: > s/Factroy/Factory/ Done. https://codereview.chromium.org/2244763002/diff/120001/services/video_capture... File services/video_capture/video_capture_device_factory_impl.h (right): https://codereview.chromium.org/2244763002/diff/120001/services/video_capture... services/video_capture/video_capture_device_factory_impl.h:53: }; On 2016/08/18 21:11:21, mcasas wrote: > DeviceEntry should not be copyable, so add here > DISALLOW_COPY_AND_ASSIGN(DeviceEntry); > > https://cs.chromium.org/chromium/src/base/macros.h?q=disallow_copy_and&sq=pac... Done. https://codereview.chromium.org/2244763002/diff/120001/services/video_capture... File services/video_capture/video_capture_service.cc (right): https://codereview.chromium.org/2244763002/diff/120001/services/video_capture... services/video_capture/video_capture_service.cc:28: base::MakeUnique<VideoCaptureDeviceImpl>()); On 2016/08/18 21:11:22, mcasas wrote: > Consider moving l.20-28 to ConnectToFakeDeviceFactory(), > so this instantiation only happens if needed. > > Also, shouldn't the construction of |device_factory_| and > |fake_device_factory_| be done lazily in, resp., > ConnectToDeviceFactory() and ConnectToFakeDeviceFactory() ? Excellent point. Done.
lgtm
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...
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 yzshen@chromium.org, mcasas@chromium.org Link to the patchset: https://codereview.chromium.org/2244763002/#ps140001 (title: "mcasas' comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Video Capture Mojo: Add service configurator interface This CL is part of the Mojo Video Capture work. For the bigger picture, see [1]. This is a follow up to a CL [2] currently under review. * Add top-level interface VideoCaptureService which exposes access to two different instances of VideoCaptureDevicFactory, one for actual devices and one for fake devices. * Expose a new interface FakeVideoCaptureDeviceFactoryConfigurator for configuring which fake devices are available. * Let method CreateDevice() return a result code to be able to report non-success outcome back to clients. * Added service test: + FakeCaptureDeviceGetsEnumerated BUG=584797 TEST=Build of video_capture_unittests succeeds. All video_capture_unittests pass. [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... [2] https://codereview.chromium.org/2224103002/ ========== to ========== Video Capture Mojo: Add service configurator interface This CL is part of the Mojo Video Capture work. For the bigger picture, see [1]. This is a follow up to a CL [2] currently under review. * Add top-level interface VideoCaptureService which exposes access to two different instances of VideoCaptureDevicFactory, one for actual devices and one for fake devices. * Expose a new interface FakeVideoCaptureDeviceFactoryConfigurator for configuring which fake devices are available. * Let method CreateDevice() return a result code to be able to report non-success outcome back to clients. * Added service test: + FakeCaptureDeviceGetsEnumerated BUG=584797 TEST=Build of video_capture_unittests succeeds. All video_capture_unittests pass. [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... [2] https://codereview.chromium.org/2224103002/ ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Video Capture Mojo: Add service configurator interface This CL is part of the Mojo Video Capture work. For the bigger picture, see [1]. This is a follow up to a CL [2] currently under review. * Add top-level interface VideoCaptureService which exposes access to two different instances of VideoCaptureDevicFactory, one for actual devices and one for fake devices. * Expose a new interface FakeVideoCaptureDeviceFactoryConfigurator for configuring which fake devices are available. * Let method CreateDevice() return a result code to be able to report non-success outcome back to clients. * Added service test: + FakeCaptureDeviceGetsEnumerated BUG=584797 TEST=Build of video_capture_unittests succeeds. All video_capture_unittests pass. [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... [2] https://codereview.chromium.org/2224103002/ ========== to ========== Video Capture Mojo: Add service configurator interface This CL is part of the Mojo Video Capture work. For the bigger picture, see [1]. This is a follow up to a CL [2] currently under review. * Add top-level interface VideoCaptureService which exposes access to two different instances of VideoCaptureDevicFactory, one for actual devices and one for fake devices. * Expose a new interface FakeVideoCaptureDeviceFactoryConfigurator for configuring which fake devices are available. * Let method CreateDevice() return a result code to be able to report non-success outcome back to clients. * Added service test: + FakeCaptureDeviceGetsEnumerated BUG=584797 TEST=Build of video_capture_unittests succeeds. All video_capture_unittests pass. [1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs... [2] https://codereview.chromium.org/2224103002/ Committed: https://crrev.com/262b6e71844a2fe0948c36c78ef1c91f9f02042d Cr-Commit-Position: refs/heads/master@{#413205} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/262b6e71844a2fe0948c36c78ef1c91f9f02042d Cr-Commit-Position: refs/heads/master@{#413205} |
