Allow FakeVideoCaptureDeviceFactory to specify number of supported formats per device.

Allow FakeVideoCaptureDeviceFactory to specify number of supported formats per device.

This is intended to allow testing that devices that cannot provide their
supported formats get a fallback list of supported formats in
MediaDevicesDispatcherHost.

BUG=691559

[Guido Urdaneta, 2017-02-14 15:22:22]

guidou@chromium.org changed reviewers:
+ hbos@chromium.org, magjed@chromium.org

[Guido Urdaneta, 2017-02-14 15:22:22]

Hi, PTAL

[Guido Urdaneta, 2017-02-14 15:23:47]

The CQ bit was checked by guidou@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 15:24:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Guido Urdaneta, 2017-02-14 15:32:34]

Patchset #2 (id:20001) has been deleted

[mcasas, 2017-02-14 17:01:01]

mcasas@chromium.org changed reviewers:
+ mcasas@chromium.org

[mcasas, 2017-02-14 17:01:01]

https://codereview.chromium.org/2692883004/diff/40001/media/capture/video/fak...
File media/capture/video/fake_video_capture_device_factory.cc (right):

https://codereview.chromium.org/2692883004/diff/40001/media/capture/video/fak...
media/capture/video/fake_video_capture_device_factory.cc:169: }
I think adding a new command line flag and member variable
is overkill just for a unittest (media_devices_dispatcher_host_unittest.cc).

Can we do it some other way? E.g. by adding a new
ReturnNoEnumeratedFormatsForTesting() to FakeVCDFactory ?

[chfremer, 2017-02-14 17:30:37]

On 2017/02/14 15:22:22, Guido Urdaneta wrote:
> Hi, PTAL

As an FYI, I have done some work recently on both FakeVideoCaptureDevice and 
FakeVideoCaptureDeviceFactory. One CL that is currently under review [1] is just
a refactoring in preparation of adding new functionality. A follow-up CL, which 
is not yet published for review, will be proposing to add MJPEG output support 
and full configurability of fake devices. This would enable us to specifically 
configure how many devices we have and which formats are supported by each 
device individually. This includes cases where a device supports more than one 
pixel format, e.g. I420 and MJPEG and ARGB. And it should also allow devices 
that support no formats.

Not sure if this would be sufficient to cover the case we are interested in for
this CL. Do I understand correctly that we want to simulate a device that does 
not report any supported formats, but still ends up working properly when we 
open it with a format we have guessed?

[1] https://codereview.chromium.org/2619503003/

[Guido Urdaneta, 2017-02-14 21:31:14]

On 2017/02/14 17:30:37, chfremer wrote:
> On 2017/02/14 15:22:22, Guido Urdaneta wrote:
> > Hi, PTAL
> 
> As an FYI, I have done some work recently on both FakeVideoCaptureDevice and 
> FakeVideoCaptureDeviceFactory. One CL that is currently under review [1] is
just
> a refactoring in preparation of adding new functionality. A follow-up CL,
which 
> is not yet published for review, will be proposing to add MJPEG output support

> and full configurability of fake devices. This would enable us to specifically

> configure how many devices we have and which formats are supported by each 
> device individually. This includes cases where a device supports more than one

> pixel format, e.g. I420 and MJPEG and ARGB. And it should also allow devices 
> that support no formats.
> 
> Not sure if this would be sufficient to cover the case we are interested in
for
> this CL. Do I understand correctly that we want to simulate a device that does

> not report any supported formats, but still ends up working properly when we 
> open it with a format we have guessed?
> 
> [1] https://codereview.chromium.org/2619503003/

It sounds like your refactoring would provide the necessary functionality for
the test in this CL. So, let's close this CL and write the test once you land
the refactoring.

Just to be clear, what this CL wanted to test was a recent patch to
MediaDevicesDispatcherHost where the GetVideoInputDeviceCapabilities() returns a
list of fallback formats for devices that don't provide a list of supported
formats (see http://crbug.com/691559 and
https://codereview.chromium.org/2694163004/). Since the bug was a P0 I landed
the fix and made the test a separate CL, so that we can discuss the best way to
test this with the FakeVideoCaptureDeviceFactory.

[chfremer, 2017-02-14 23:27:34]

On 2017/02/14 21:31:14, Guido Urdaneta wrote:
> On 2017/02/14 17:30:37, chfremer wrote:
> > On 2017/02/14 15:22:22, Guido Urdaneta wrote:
> > > Hi, PTAL
> > 
> > As an FYI, I have done some work recently on both FakeVideoCaptureDevice and

> > FakeVideoCaptureDeviceFactory. One CL that is currently under review [1] is
> just
> > a refactoring in preparation of adding new functionality. A follow-up CL,
> which 
> > is not yet published for review, will be proposing to add MJPEG output
support
> 
> > and full configurability of fake devices. This would enable us to
specifically
> 
> > configure how many devices we have and which formats are supported by each 
> > device individually. This includes cases where a device supports more than
one
> 
> > pixel format, e.g. I420 and MJPEG and ARGB. And it should also allow devices

> > that support no formats.
> > 
> > Not sure if this would be sufficient to cover the case we are interested in
> for
> > this CL. Do I understand correctly that we want to simulate a device that
does
> 
> > not report any supported formats, but still ends up working properly when we

> > open it with a format we have guessed?
> > 
> > [1] https://codereview.chromium.org/2619503003/
> 
> It sounds like your refactoring would provide the necessary functionality for
> the test in this CL. So, let's close this CL and write the test once you land
> the refactoring.
> 
> Just to be clear, what this CL wanted to test was a recent patch to
> MediaDevicesDispatcherHost where the GetVideoInputDeviceCapabilities() returns
a
> list of fallback formats for devices that don't provide a list of supported
> formats (see http://crbug.com/691559 and
> https://codereview.chromium.org/2694163004/). Since the bug was a P0 I landed
> the fix and made the test a separate CL, so that we can discuss the best way
to
> test this with the FakeVideoCaptureDeviceFactory.

Sounds reasonable to me. I'll expedite the work to land the refactoring and 
will include you on the CLs to keep you in the loop.

[Guido Urdaneta, 2017-02-15 10:39:02]

Description was changed from

==========
Allow FakeVideoCaptureDeviceFactory to specify number of supported formats per
device.

This is intended to allow testing that devices that cannot provide their
supported formats get a fallback list of supported formats in
MediaDevicesDispatcherHost.

BUG=691559
==========

to

==========
Allow FakeVideoCaptureDeviceFactory to specify number of supported formats per
device.

This is intended to allow testing that devices that cannot provide their
supported formats get a fallback list of supported formats in
MediaDevicesDispatcherHost.

BUG=691559
==========

[Guido Urdaneta, 2017-02-15 10:39:37]

Closed. Will revisit once chfremer's refactoring lands.