Make FakeVideoCaptureDeviceFactory configurable to arbitrary fake device configurations

Part of a series of CLs for FakeVideoCaptureDevice with the goal of allowing us
to expand our test coverage of the video capture pipeline.

Goal of this change:
* Make FakeVideoCaptureDeviceFactory configurable to arbitrary fake device 
  configurations. This includes setting up fake devices that support more than 
  one pixel format, e.g. I420 and MJPEG.

Approach taken:
* Remove class FakeVideoCaptureDeviceMaker and place the logic for creating 
  instances of FakeVideoCaptureDevice into 
  FakeVideoCaptureDeviceFactory::CreateDeviceWithSupportedFormats().
  This requires moving the relevant class declaration from 
  fake_video_capture_device.cc to fake_video_capture_device.h.
* To allow being switched between different output formats, it is no longer 
  sufficient to provide instances of FakeVideoCaptureDevice with a fixed 
  FrameDeliverer at construction time. Instead, we now provide a
  FrameDelivererFactory at construction time and have the FakeVideoCaptureDevice
  request an appropriate FrameDeliverer in AllocateAndStart(). With a new 
  FrameDeliverer being created every time the device is started, there is no 
  longer a need for it to have an Uninitialize() method.
* Instead of just snapping to a supported size, fake devices now have to 
  select which supported pixel format to use (since they may support more than 
  one).
* Make FakeVideoCaptureDeviceFactory configurable through a public API instead 
  of privately pulling configuration from a command-line string. Configuration 
  through the command-line string still works as before when creating the 
  factory via VideoCaptureDeviceFactory::CreateFactory().
* Added test cases for configuring a fake device that does not support any 
  format.

This CL is part of the Mojo Video Capture work. For the bigger picture,
see [1] CL12b.

BUG=584797
TEST=
  capture_unittests --gtest_filter="*Fake*"
  content_unittests --gtest_filter="*Video*"
  out/Default/chrome --use-fake-device-for-media-stream=device-count=3 https://webrtc.github.io/samples/src/content/getusermedia/gum/

[1] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxsxWslJ_X-zpOhijvI8/edit?usp=sharing

Review-Url: https://codereview.chromium.org/2715513008
Cr-Commit-Position: refs/heads/master@{#454365}
Committed: https://chromium.googlesource.com/chromium/src/+/b0ee07cd95863c50ca5bf553bab82bf54b82d500

[chfremer, 2017-02-24 00:46:54]

Description was changed from

==========
Cleanups for review


Merge branch '12' into 12b


Merge branch '12' into 12b


Fix after merge. Add support for device with no supported formats.


Merge branch '12' into 12b


Merge branch '12' into 12b


Pull-in changes to fake_video_capture_device and factory from branch 32.

BUG=
==========

to

==========
Make FakeVideoCaptureDeviceFactory configurable to arbitrary fake device
configurations

Part of a series of CLs for FakeVideoCaptureDevice with the goal of allowing us
to expand our test coverage of the video capture pipeline.

Goal of this change:
* Make FakeVideoCaptureDeviceFactory configurable to arbitrary fake device 
  configurations. This includes setting up fake devices that support more than 
  one pixel format, e.g. I420 and MJPEG.

Approach taken:
* Remove class FakeVideoCaptureDeviceMaker and place the logic for creating 
  instances of FakeVideoCaptureDevice into 
  FakeVideoCaptureDeviceFactory::CreateDeviceWithSupportedFormats().
  This requires moving the relevant class declaration from 
  fake_video_capture_device.cc to fake_video_capture_device.h.
* To allow being switched between different output formats, it is no longer 
  sufficient to provide instances of FakeVideoCaptureDevice with a fixed 
  FrameDeliverer at construction time. Instead, we now provide a
  FrameDelivererFactory at construction time and have the FakeVideoCaptureDevice
  request an appropriate FrameDeliverer in AllocateAndStart(). With a new 
  FrameDeliverer being created every time the device is started, there is no 
  longer a need for it to have an Uninitialize() method.
* Instead of just snapping to a supported size, fake devices now have to 
  select which supported pixel format to use (since they may support more than 
  one).
* Make FakeVideoCaptureDeviceFactory configurable through a public API instead 
  of privately pulling configuration from a command-line string. Configuration 
  through the command-line string still works as before when creating the 
  factory via VideoCaptureDeviceFactory::CreateFactory().
* Added test cases for configuring a fake device that does not support any 
  format.

This CL is part of the Mojo Video Capture work. For the bigger picture,
see [1] CL12b.

BUG=584797
TEST=
  capture_unittests --gtest_filter="*Fake*"

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

[chfremer, 2017-02-24 00:47:28]

Description was changed from

==========
Make FakeVideoCaptureDeviceFactory configurable to arbitrary fake device
configurations

Part of a series of CLs for FakeVideoCaptureDevice with the goal of allowing us
to expand our test coverage of the video capture pipeline.

Goal of this change:
* Make FakeVideoCaptureDeviceFactory configurable to arbitrary fake device 
  configurations. This includes setting up fake devices that support more than 
  one pixel format, e.g. I420 and MJPEG.

Approach taken:
* Remove class FakeVideoCaptureDeviceMaker and place the logic for creating 
  instances of FakeVideoCaptureDevice into 
  FakeVideoCaptureDeviceFactory::CreateDeviceWithSupportedFormats().
  This requires moving the relevant class declaration from 
  fake_video_capture_device.cc to fake_video_capture_device.h.
* To allow being switched between different output formats, it is no longer 
  sufficient to provide instances of FakeVideoCaptureDevice with a fixed 
  FrameDeliverer at construction time. Instead, we now provide a
  FrameDelivererFactory at construction time and have the FakeVideoCaptureDevice
  request an appropriate FrameDeliverer in AllocateAndStart(). With a new 
  FrameDeliverer being created every time the device is started, there is no 
  longer a need for it to have an Uninitialize() method.
* Instead of just snapping to a supported size, fake devices now have to 
  select which supported pixel format to use (since they may support more than 
  one).
* Make FakeVideoCaptureDeviceFactory configurable through a public API instead 
  of privately pulling configuration from a command-line string. Configuration 
  through the command-line string still works as before when creating the 
  factory via VideoCaptureDeviceFactory::CreateFactory().
* Added test cases for configuring a fake device that does not support any 
  format.

This CL is part of the Mojo Video Capture work. For the bigger picture,
see [1] CL12b.

BUG=584797
TEST=
  capture_unittests --gtest_filter="*Fake*"

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

to

==========
Part of a series of CLs for FakeVideoCaptureDevice with the goal of allowing us
to expand our test coverage of the video capture pipeline.

Goal of this change:
* Make FakeVideoCaptureDeviceFactory configurable to arbitrary fake device 
  configurations. This includes setting up fake devices that support more than 
  one pixel format, e.g. I420 and MJPEG.

Approach taken:
* Remove class FakeVideoCaptureDeviceMaker and place the logic for creating 
  instances of FakeVideoCaptureDevice into 
  FakeVideoCaptureDeviceFactory::CreateDeviceWithSupportedFormats().
  This requires moving the relevant class declaration from 
  fake_video_capture_device.cc to fake_video_capture_device.h.
* To allow being switched between different output formats, it is no longer 
  sufficient to provide instances of FakeVideoCaptureDevice with a fixed 
  FrameDeliverer at construction time. Instead, we now provide a
  FrameDelivererFactory at construction time and have the FakeVideoCaptureDevice
  request an appropriate FrameDeliverer in AllocateAndStart(). With a new 
  FrameDeliverer being created every time the device is started, there is no 
  longer a need for it to have an Uninitialize() method.
* Instead of just snapping to a supported size, fake devices now have to 
  select which supported pixel format to use (since they may support more than 
  one).
* Make FakeVideoCaptureDeviceFactory configurable through a public API instead 
  of privately pulling configuration from a command-line string. Configuration 
  through the command-line string still works as before when creating the 
  factory via VideoCaptureDeviceFactory::CreateFactory().
* Added test cases for configuring a fake device that does not support any 
  format.

This CL is part of the Mojo Video Capture work. For the bigger picture,
see [1] CL12b.

BUG=584797
TEST=
  capture_unittests --gtest_filter="*Fake*"

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

[chfremer, 2017-02-24 17:35:10]

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

[chfremer, 2017-02-24 17:35:21]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-02-24 17:35:34]

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

[chfremer, 2017-02-24 17:38:45]

Description was changed from

==========
Part of a series of CLs for FakeVideoCaptureDevice with the goal of allowing us
to expand our test coverage of the video capture pipeline.

Goal of this change:
* Make FakeVideoCaptureDeviceFactory configurable to arbitrary fake device 
  configurations. This includes setting up fake devices that support more than 
  one pixel format, e.g. I420 and MJPEG.

Approach taken:
* Remove class FakeVideoCaptureDeviceMaker and place the logic for creating 
  instances of FakeVideoCaptureDevice into 
  FakeVideoCaptureDeviceFactory::CreateDeviceWithSupportedFormats().
  This requires moving the relevant class declaration from 
  fake_video_capture_device.cc to fake_video_capture_device.h.
* To allow being switched between different output formats, it is no longer 
  sufficient to provide instances of FakeVideoCaptureDevice with a fixed 
  FrameDeliverer at construction time. Instead, we now provide a
  FrameDelivererFactory at construction time and have the FakeVideoCaptureDevice
  request an appropriate FrameDeliverer in AllocateAndStart(). With a new 
  FrameDeliverer being created every time the device is started, there is no 
  longer a need for it to have an Uninitialize() method.
* Instead of just snapping to a supported size, fake devices now have to 
  select which supported pixel format to use (since they may support more than 
  one).
* Make FakeVideoCaptureDeviceFactory configurable through a public API instead 
  of privately pulling configuration from a command-line string. Configuration 
  through the command-line string still works as before when creating the 
  factory via VideoCaptureDeviceFactory::CreateFactory().
* Added test cases for configuring a fake device that does not support any 
  format.

This CL is part of the Mojo Video Capture work. For the bigger picture,
see [1] CL12b.

BUG=584797
TEST=
  capture_unittests --gtest_filter="*Fake*"

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

to

==========
Part of a series of CLs for FakeVideoCaptureDevice with the goal of allowing us
to expand our test coverage of the video capture pipeline.

Goal of this change:
* Make FakeVideoCaptureDeviceFactory configurable to arbitrary fake device 
  configurations. This includes setting up fake devices that support more than 
  one pixel format, e.g. I420 and MJPEG.

Approach taken:
* Remove class FakeVideoCaptureDeviceMaker and place the logic for creating 
  instances of FakeVideoCaptureDevice into 
  FakeVideoCaptureDeviceFactory::CreateDeviceWithSupportedFormats().
  This requires moving the relevant class declaration from 
  fake_video_capture_device.cc to fake_video_capture_device.h.
* To allow being switched between different output formats, it is no longer 
  sufficient to provide instances of FakeVideoCaptureDevice with a fixed 
  FrameDeliverer at construction time. Instead, we now provide a
  FrameDelivererFactory at construction time and have the FakeVideoCaptureDevice
  request an appropriate FrameDeliverer in AllocateAndStart(). With a new 
  FrameDeliverer being created every time the device is started, there is no 
  longer a need for it to have an Uninitialize() method.
* Instead of just snapping to a supported size, fake devices now have to 
  select which supported pixel format to use (since they may support more than 
  one).
* Make FakeVideoCaptureDeviceFactory configurable through a public API instead 
  of privately pulling configuration from a command-line string. Configuration 
  through the command-line string still works as before when creating the 
  factory via VideoCaptureDeviceFactory::CreateFactory().
* Added test cases for configuring a fake device that does not support any 
  format.

This CL is part of the Mojo Video Capture work. For the bigger picture,
see [1] CL12b.

BUG=584797
TEST=
  capture_unittests --gtest_filter="*Fake*"
  content_unittests --gtest_filter="*Video*"
  out/Default/chrome --use-fake-device-for-media-stream=device-count=3
https://webrtc.github.io/samples/src/content/getusermedia/gum/

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

[chfremer, 2017-02-24 17:47:33]

chfremer@chromium.org changed reviewers:
+ emircan@chromium.org, guidou@chromium.org, mcasas@chromium.org

[chfremer, 2017-02-24 17:47:34]

guidou@: PTAL content/*
mcasas@: PTAL media/*
emircan@: FYI

[commit-bot: I haz the power, 2017-02-24 18:00:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 18:00:08]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[chfremer, 2017-02-24 18:36:44]

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

[commit-bot: I haz the power, 2017-02-24 18:37:18]

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

[commit-bot: I haz the power, 2017-02-24 20:06:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 20:06:17]

Dry run: This issue passed the CQ dry run.

[Guido Urdaneta, 2017-02-27 16:09:33]

content lgtm

[emircan, 2017-03-01 18:59:01]

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

https://codereview.chromium.org/2715513008/diff/40001/media/capture/video/fak...
media/capture/video/fake_video_capture_device.cc:53: if (requested_format ==
media::PIXEL_FORMAT_I420) {
Merge to a single if statement.

https://codereview.chromium.org/2715513008/diff/40001/media/capture/video/fak...
media/capture/video/fake_video_capture_device.cc:64: if (current_match ==
PixelFormatMatchType::INCOMPATIBLE) {
I don't think comparing like this is scalable. Can you just do |current_match >
best_match| since you actually ranked them in declaration?

https://codereview.chromium.org/2715513008/diff/40001/media/capture/video/fak...
media/capture/video/fake_video_capture_device.cc:217:
"USE_DEVICE_INTERNAL_BUFFERS.";
We should still return an error here to propagate proper use.

https://codereview.chromium.org/2715513008/diff/40001/media/capture/video/fak...
media/capture/video/fake_video_capture_device.cc:431:
supported_formats_[closest_supported_format_index];
Why don't we make the function FindClosestSupportedFormat() since we do not have
any other use for the index?

https://codereview.chromium.org/2715513008/diff/40001/media/capture/video/fak...
File media/capture/video/fake_video_capture_device.h (right):

https://codereview.chromium.org/2715513008/diff/40001/media/capture/video/fak...
media/capture/video/fake_video_capture_device.h:41: void
PaintFrame(base::TimeDelta elapsed_time, uint8_t* target_buffer);
const base::TimeDelta&

https://codereview.chromium.org/2715513008/diff/40001/media/capture/video/fak...
media/capture/video/fake_video_capture_device.h:44: void
DrawGradientSquares(base::TimeDelta elapsed_time,
const base::TimeDelta&

https://codereview.chromium.org/2715513008/diff/40001/media/capture/video/fak...
media/capture/video/fake_video_capture_device.h:74: class FakeVideoCaptureDevice
: public VideoCaptureDevice {
Can you reorder these classes in a way that would help reading top-to-bottom?

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

https://codereview.chromium.org/2715513008/diff/40001/media/capture/video/fak...
media/capture/video/fake_video_capture_device_factory.cc:243:
std::vector<gfx::Size> resolutions = ArrayToVector(kDefaultResolutions);
Is it necessary to copy them into vector? AFAICT You can pass both as
std::arrays.

[chfremer, 2017-03-01 21:55:31]

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

[chfremer, 2017-03-01 21:55:48]

PTAL.

Incorporated suggestions.

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

https://codereview.chromium.org/2715513008/diff/40001/media/capture/video/fak...
media/capture/video/fake_video_capture_device.cc:53: if (requested_format ==
media::PIXEL_FORMAT_I420) {
On 2017/03/01 18:58:58, emircan wrote:
> Merge to a single if statement.

Done.

https://codereview.chromium.org/2715513008/diff/40001/media/capture/video/fak...
media/capture/video/fake_video_capture_device.cc:64: if (current_match ==
PixelFormatMatchType::INCOMPATIBLE) {
On 2017/03/01 18:58:58, emircan wrote:
> I don't think comparing like this is scalable. Can you just do |current_match
>
> best_match| since you actually ranked them in declaration?

Done.

https://codereview.chromium.org/2715513008/diff/40001/media/capture/video/fak...
media/capture/video/fake_video_capture_device.cc:217:
"USE_DEVICE_INTERNAL_BUFFERS.";
On 2017/03/01 18:58:58, emircan wrote:
> We should still return an error here to propagate proper use.

I tried out your suggestions but then ended up reverting.
The reason is that properly supporting an error case for this requires adding
quite a bit of complexity. 

In addition to returning nullptr here, we would have to make sure the
FakeVideoCaptureDeviceFactory::GetSupportedFormats() strips out all MJPEG
formats when using USE_CLIENT_PROVIDED_BUFFERS. And then, we would probably want
to strip those cases out already when they are set in one of the configuration
calls, but it would be bad to do that silently. So we would have to add an error
case to the configuration API. And then we would need unit test cases for all
this.

Considering this, I feel the silent switching to USE_DEVICE_INTERNAL_BUFFERS is
probably the better choice.

https://codereview.chromium.org/2715513008/diff/40001/media/capture/video/fak...
media/capture/video/fake_video_capture_device.cc:431:
supported_formats_[closest_supported_format_index];
On 2017/03/01 18:58:58, emircan wrote:
> Why don't we make the function FindClosestSupportedFormat() since we do not
have
> any other use for the index?

Done.

https://codereview.chromium.org/2715513008/diff/40001/media/capture/video/fak...
File media/capture/video/fake_video_capture_device.h (right):

https://codereview.chromium.org/2715513008/diff/40001/media/capture/video/fak...
media/capture/video/fake_video_capture_device.h:41: void
PaintFrame(base::TimeDelta elapsed_time, uint8_t* target_buffer);
On 2017/03/01 18:58:58, emircan wrote:
> const base::TimeDelta&

Doing so raises a build warning that says this type needs to be passed by value.

https://codereview.chromium.org/2715513008/diff/40001/media/capture/video/fak...
media/capture/video/fake_video_capture_device.h:44: void
DrawGradientSquares(base::TimeDelta elapsed_time,
On 2017/03/01 18:58:58, emircan wrote:
> const base::TimeDelta&

same

https://codereview.chromium.org/2715513008/diff/40001/media/capture/video/fak...
media/capture/video/fake_video_capture_device.h:74: class FakeVideoCaptureDevice
: public VideoCaptureDevice {
On 2017/03/01 18:58:58, emircan wrote:
> Can you reorder these classes in a way that would help reading top-to-bottom?

Done.

I kept the current order in the .cc file, since it otherwise would make review
harder. I will match the order of the .h file (or extract to separate files) in
a cleanup step.

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

https://codereview.chromium.org/2715513008/diff/40001/media/capture/video/fak...
media/capture/video/fake_video_capture_device_factory.cc:243:
std::vector<gfx::Size> resolutions = ArrayToVector(kDefaultResolutions);
On 2017/03/01 18:58:59, emircan wrote:
> Is it necessary to copy them into vector? AFAICT You can pass both as
> std::arrays.

It is true that |resolutions| is currently never modified at runtime. It could,
in theory, be hard-coded into AppendAllCombinationsToFormatsContainer().
|frame_rates| however, can be modified using the options_string, so it needs to
be converted to a std::vector. I am doing |resolutions| as std::vector for
consistency with the other parameters.

[commit-bot: I haz the power, 2017-03-01 21:56:27]

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

[commit-bot: I haz the power, 2017-03-01 23:35:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 23:35:37]

Dry run: This issue passed the CQ dry run.

[emircan, 2017-03-02 20:28:18]

lgtm

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

https://codereview.chromium.org/2715513008/diff/40001/media/capture/video/fak...
media/capture/video/fake_video_capture_device.cc:217:
"USE_DEVICE_INTERNAL_BUFFERS.";
On 2017/03/01 21:55:48, chfremer wrote:
> On 2017/03/01 18:58:58, emircan wrote:
> > We should still return an error here to propagate proper use.
> 
> I tried out your suggestions but then ended up reverting.
> The reason is that properly supporting an error case for this requires adding
> quite a bit of complexity. 
> 
> In addition to returning nullptr here, we would have to make sure the
> FakeVideoCaptureDeviceFactory::GetSupportedFormats() strips out all MJPEG
> formats when using USE_CLIENT_PROVIDED_BUFFERS. And then, we would probably
want
> to strip those cases out already when they are set in one of the configuration
> calls, but it would be bad to do that silently. So we would have to add an
error
> case to the configuration API. And then we would need unit test cases for all
> this.
> 
> Considering this, I feel the silent switching to USE_DEVICE_INTERNAL_BUFFERS
is
> probably the better choice.

Sounds good.

[chfremer, 2017-03-02 20:30:57]

The CQ bit was checked by chfremer@chromium.org

[chfremer, 2017-03-02 20:30:57]

The patchset sent to the CQ was uploaded after l-g-t-m from guidou@chromium.org
Link to the patchset: https://codereview.chromium.org/2715513008/#ps60001
(title: "emircan@ suggestions")

[commit-bot: I haz the power, 2017-03-02 20:31:46]

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

[commit-bot: I haz the power, 2017-03-02 20:39:05]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488486657410800,
"parent_rev": "e506b1518fc4633aee4f2b32193de02f987a0096", "commit_rev":
"b0ee07cd95863c50ca5bf553bab82bf54b82d500"}

[commit-bot: I haz the power, 2017-03-02 20:39:46]

Description was changed from

==========
Part of a series of CLs for FakeVideoCaptureDevice with the goal of allowing us
to expand our test coverage of the video capture pipeline.

Goal of this change:
* Make FakeVideoCaptureDeviceFactory configurable to arbitrary fake device 
  configurations. This includes setting up fake devices that support more than 
  one pixel format, e.g. I420 and MJPEG.

Approach taken:
* Remove class FakeVideoCaptureDeviceMaker and place the logic for creating 
  instances of FakeVideoCaptureDevice into 
  FakeVideoCaptureDeviceFactory::CreateDeviceWithSupportedFormats().
  This requires moving the relevant class declaration from 
  fake_video_capture_device.cc to fake_video_capture_device.h.
* To allow being switched between different output formats, it is no longer 
  sufficient to provide instances of FakeVideoCaptureDevice with a fixed 
  FrameDeliverer at construction time. Instead, we now provide a
  FrameDelivererFactory at construction time and have the FakeVideoCaptureDevice
  request an appropriate FrameDeliverer in AllocateAndStart(). With a new 
  FrameDeliverer being created every time the device is started, there is no 
  longer a need for it to have an Uninitialize() method.
* Instead of just snapping to a supported size, fake devices now have to 
  select which supported pixel format to use (since they may support more than 
  one).
* Make FakeVideoCaptureDeviceFactory configurable through a public API instead 
  of privately pulling configuration from a command-line string. Configuration 
  through the command-line string still works as before when creating the 
  factory via VideoCaptureDeviceFactory::CreateFactory().
* Added test cases for configuring a fake device that does not support any 
  format.

This CL is part of the Mojo Video Capture work. For the bigger picture,
see [1] CL12b.

BUG=584797
TEST=
  capture_unittests --gtest_filter="*Fake*"
  content_unittests --gtest_filter="*Video*"
  out/Default/chrome --use-fake-device-for-media-stream=device-count=3
https://webrtc.github.io/samples/src/content/getusermedia/gum/

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

to

==========
Part of a series of CLs for FakeVideoCaptureDevice with the goal of allowing us
to expand our test coverage of the video capture pipeline.

Goal of this change:
* Make FakeVideoCaptureDeviceFactory configurable to arbitrary fake device 
  configurations. This includes setting up fake devices that support more than 
  one pixel format, e.g. I420 and MJPEG.

Approach taken:
* Remove class FakeVideoCaptureDeviceMaker and place the logic for creating 
  instances of FakeVideoCaptureDevice into 
  FakeVideoCaptureDeviceFactory::CreateDeviceWithSupportedFormats().
  This requires moving the relevant class declaration from 
  fake_video_capture_device.cc to fake_video_capture_device.h.
* To allow being switched between different output formats, it is no longer 
  sufficient to provide instances of FakeVideoCaptureDevice with a fixed 
  FrameDeliverer at construction time. Instead, we now provide a
  FrameDelivererFactory at construction time and have the FakeVideoCaptureDevice
  request an appropriate FrameDeliverer in AllocateAndStart(). With a new 
  FrameDeliverer being created every time the device is started, there is no 
  longer a need for it to have an Uninitialize() method.
* Instead of just snapping to a supported size, fake devices now have to 
  select which supported pixel format to use (since they may support more than 
  one).
* Make FakeVideoCaptureDeviceFactory configurable through a public API instead 
  of privately pulling configuration from a command-line string. Configuration 
  through the command-line string still works as before when creating the 
  factory via VideoCaptureDeviceFactory::CreateFactory().
* Added test cases for configuring a fake device that does not support any 
  format.

This CL is part of the Mojo Video Capture work. For the bigger picture,
see [1] CL12b.

BUG=584797
TEST=
  capture_unittests --gtest_filter="*Fake*"
  content_unittests --gtest_filter="*Video*"
  out/Default/chrome --use-fake-device-for-media-stream=device-count=3
https://webrtc.github.io/samples/src/content/getusermedia/gum/

[1]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...

Review-Url: https://codereview.chromium.org/2715513008
Cr-Commit-Position: refs/heads/master@{#454365}
Committed:
https://chromium.googlesource.com/chromium/src/+/b0ee07cd95863c50ca5bf553bab8...
==========

[commit-bot: I haz the power, 2017-03-02 20:39:47]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/b0ee07cd95863c50ca5bf553bab8...

[mcasas, 2017-03-02 21:07:09]

https://codereview.chromium.org/2715513008/diff/60001/media/capture/video/fak...
File media/capture/video/fake_video_capture_device.cc (right):

https://codereview.chromium.org/2715513008/diff/60001/media/capture/video/fak...
media/capture/video/fake_video_capture_device.cc:47:
SUPPORTED_THROUGH_CONVERSION = 1,
IIUC SUPPORTED_THROUGH_CONVERSION has only
two uses, so who cares about it? What you want to define here
is a scale of unsupportedness, which is not what an enum
is for, and its use inside FindClosestSupportedFormat()
is obscure because of that.  Ideally, you should be able
to enumerate the formats in preference order, e.g.

// VideoPixelFormats in order from most to least preferred.
static kPreferredVideoPixelFormat kPreferredVideoPixelFormats[] = {
  // We like these formats less because they are Skia native.
  PIXEL_FORMAT_RGBA
  PIXEL_FORMAT_RGB32,
  // Next-best formats are those that need internal conversion.
  PIXEL_FORMAT_I420,
  PIXEL_FORMAT_MJPEG,
  //...
};

If all formats are present then
  static_assert(sizeof(kPreferredVideoPixelFormats) == PIXEL_FORMAT_MAX, "bla");
would be needed.

Perhaps this list should be a correspondence instead,
between requested pixel format to produced pixel format,
again in priority order. Might, I'm not sure.

https://codereview.chromium.org/2715513008/diff/60001/media/capture/video/fak...
media/capture/video/fake_video_capture_device.cc:97: }
This code looks very familiar, is also in media_stream_video_source.cc, 
[1] it'd be cool to have it all in one single place, so that 
resolution-selecting policies are coherent across chrome. 

Can be a bug with a TODO().

[1] vicinity of
https://cs.chromium.org/chromium/src/content/renderer/media/media_stream_vide...

https://codereview.chromium.org/2715513008/diff/60001/media/capture/video/fak...
File media/capture/video/fake_video_capture_device.h (right):

https://codereview.chromium.org/2715513008/diff/60001/media/capture/video/fak...
media/capture/video/fake_video_capture_device.h:24: class PacmanFramePainter {
This class can and should be forward declared here.

Probably FakeDeviceState, FrameDelivererFactory
and FakePhotoDevice can also be forward declared
and defined in the .cc file. 

It's fine to do it in a follow-up CL

[chfremer, 2017-03-02 23:03:04]

Thanks for the review and comments.

mcasas@, since this one has already landed, I will post a separate CL13b
incorporating your suggestions.

https://codereview.chromium.org/2715513008/diff/60001/media/capture/video/fak...
File media/capture/video/fake_video_capture_device.cc (right):

https://codereview.chromium.org/2715513008/diff/60001/media/capture/video/fak...
media/capture/video/fake_video_capture_device.cc:47:
SUPPORTED_THROUGH_CONVERSION = 1,
On 2017/03/02 21:07:09, mcasas wrote:
> IIUC SUPPORTED_THROUGH_CONVERSION has only
> two uses, so who cares about it? What you want to define here
> is a scale of unsupportedness, which is not what an enum
> is for, and its use inside FindClosestSupportedFormat()
> is obscure because of that.  Ideally, you should be able
> to enumerate the formats in preference order, e.g.
> 
> // VideoPixelFormats in order from most to least preferred.
> static kPreferredVideoPixelFormat kPreferredVideoPixelFormats[] = {
>   // We like these formats less because they are Skia native.
>   PIXEL_FORMAT_RGBA
>   PIXEL_FORMAT_RGB32,
>   // Next-best formats are those that need internal conversion.
>   PIXEL_FORMAT_I420,
>   PIXEL_FORMAT_MJPEG,
>   //...
> };
> 
> If all formats are present then
>   static_assert(sizeof(kPreferredVideoPixelFormats) == PIXEL_FORMAT_MAX,
"bla");
> would be needed.
> 
> Perhaps this list should be a correspondence instead,
> between requested pixel format to produced pixel format,
> again in priority order. Might, I'm not sure.
> 
> 

The root cause for why this admittedly ugly construct was needed is that clients
send their format request to the VCDevice but receive their frames from the
VCDeviceClient, which can perform conversion. 

Since the client also provides the VCDeviceClient, it should probably be itself
responsible to know which conversions it performs. Let me go clean that up in a
follow-up CL while the next CL is under review.

Will do.

https://codereview.chromium.org/2715513008/diff/60001/media/capture/video/fak...
media/capture/video/fake_video_capture_device.cc:97: }
On 2017/03/02 21:07:09, mcasas wrote:
> This code looks very familiar, is also in media_stream_video_source.cc, 
> [1] it'd be cool to have it all in one single place, so that 
> resolution-selecting policies are coherent across chrome. 
> 
> Can be a bug with a TODO().
> 
> [1] vicinity of
>
https://cs.chromium.org/chromium/src/content/renderer/media/media_stream_vide...

I am going to look into it, and will track it as part of CL13b in the design
doc.

https://codereview.chromium.org/2715513008/diff/60001/media/capture/video/fak...
File media/capture/video/fake_video_capture_device.h (right):

https://codereview.chromium.org/2715513008/diff/60001/media/capture/video/fak...
media/capture/video/fake_video_capture_device.h:24: class PacmanFramePainter {
On 2017/03/02 21:07:09, mcasas wrote:
> This class can and should be forward declared here.
> 
> Probably FakeDeviceState, FrameDelivererFactory
> and FakePhotoDevice can also be forward declared
> and defined in the .cc file. 
> 
> It's fine to do it in a follow-up CL

They used to be forward-declared, but I had to move them back to the header,
because the logic for creating instances of FakeVideoCaptureDevice has moved
back to class FakeVideoCaptureDeviceFactory.