Add switch to use only virtual video capture devices [chromium/src : main]
Minju Kim (Gerrit)
Minju Kim voted and added 1 comment![]( "Open in Gerrit")
Votes added by Minju Kim
| Commit-Queue | +1 |
1 comment
Hi @il...@chromium.org,
I created a new CL following your suggestion to use a command-line switch.
Could you please take a look? Thanks again for the guidance.
Related details
- Ilya Nikolaevskiy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ilya Nikolaevskiy (Gerrit)
Ilya Nikolaevskiy voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ilya Nikolaevskiy
| Code-Review | +1 |
1 comment
- Minju Kim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minju Kim (Gerrit)
Minju Kim added 1 comment![]( "Open in Gerrit")
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
OnGetDeviceInfos(std::move(callback), {});I guess this is fine but I find the design a bit confusing. I don't get why we don't just check the command-line once and pass either a wrapper DeviceFactory to the constructor or a device factory that really only returns virtual devices otherwise. Then we don't have to check in ever single method?
Related details
- Minju Kim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ilya Nikolaevskiy (Gerrit)
Ilya Nikolaevskiy added 2 comments![]( "Open in Gerrit")
std::make_unique<media::FakeVideoCaptureDeviceFactory>();FakeVideoCaptureDeviceFactory is a different thing. It always has some fake devices, which will be visible in your tests. I think this isn't what you want. Don't use it.
You should make `media_factory` nullptr here. Then make sure that VirtualEnabledDeviceFactory can handle null device_factory_. You will have to restore most of the checks in PS#2 to do so, but now they won't check for cmd line switch, only for nullptr.
OnGetDeviceInfos(std::move(callback), {});I guess this is fine but I find the design a bit confusing. I don't get why we don't just check the command-line once and pass either a wrapper DeviceFactory to the constructor or a device factory that really only returns virtual devices otherwise. Then we don't have to check in ever single method?
The suggested approch is a little unnatural, because the virtual enabled factory is a wrapper around the real factory. But the command line switch is to disable the inner real factory, not the wrapper on top of it.
It's natural to choose between core and wrapper around it. It's easy to disable the wrapper with OOP. But the other way around is trickier. We will have to make a wrapper around null.
But this will introduce the checks for not-null here too.
Related details
- Minju Kim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Minju Kim (Gerrit)
Minju Kim added 3 comments![]( "Open in Gerrit")
Thanks for the feedback.
Please take another look.
std::make_unique<media::FakeVideoCaptureDeviceFactory>();FakeVideoCaptureDeviceFactory is a different thing. It always has some fake devices, which will be visible in your tests. I think this isn't what you want. Don't use it.
You should make `media_factory` nullptr here. Then make sure that VirtualEnabledDeviceFactory can handle null device_factory_. You will have to restore most of the checks in PS#2 to do so, but now they won't check for cmd line switch, only for nullptr.
Thanks for the feedback. I updated the CL to check the command-line switch once
in VideoCaptureServiceImpl::LazyInitializeDeviceFactory().
When the switch is enabled, VirtualDeviceEnabledDeviceFactory is constructed
without a wrapped DeviceFactory. I updated it to handle the missing wrapped
factory only at the points where it would normally delegate to it.
PTAL.
OnGetDeviceInfos(std::move(callback), {});Ilya NikolaevskiyI guess this is fine but I find the design a bit confusing. I don't get why we don't just check the command-line once and pass either a wrapper DeviceFactory to the constructor or a device factory that really only returns virtual devices otherwise. Then we don't have to check in ever single method?
The suggested approch is a little unnatural, because the virtual enabled factory is a wrapper around the real factory. But the command line switch is to disable the inner real factory, not the wrapper on top of it.
It's natural to choose between core and wrapper around it. It's easy to disable the wrapper with OOP. But the other way around is trickier. We will have to make a wrapper around null.
But this will introduce the checks for not-null here too.
Thanks for the feedback. I updated the CL to check the command-line switch once
in VideoCaptureServiceImpl::LazyInitializeDeviceFactory().
PTAL.
Related details
- Daniel Cheng
- Ilya Nikolaevskiy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ilya Nikolaevskiy (Gerrit)
Ilya Nikolaevskiy voted Code-Review+1![]( "Open in Gerrit")
- Daniel Cheng
- Minju Kim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |