[Mojo Video Capture] Use actual factory instances implementing media::VideoCaptureDeviceFactory

[Mojo Video Capture] Use actual factory instances implementing media::VideoCaptureDeviceFactory.

* Replace video_capture::VideoCaptureDeviceFactoryImpl (which was capable of
  hosting instances of media::VideoCaptureDevice as well as mock devices) with
  video_capture::DeviceFactoryMediaToMojoAdapter, which wraps a
  media::VideoCaptureDeviceFactory. This can be used with the existing
  platform-specific implementations as well as with
  FakeVideoCaptureDeviceFactory and FileVideoCaptureDeviceFactory.
* Add class video_capture::MockDeviceFactory, which takes over the
  responsibility of hosting mock devices and making them accessible through the
  media::VideoCaptureDeviceFactory interface.
* Make media::VideoCaptureDeviceFactory::GetDeviceDescriptors() public so that
  it can be called from DeviceFactoryMediaToMojoAdapter (which is much simpler
  than having to call the asynchronous variant EnumerateDeviceDescriptors).

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

BUG=584797
TEST=video_capture_unittests, content_unittests, capture_unittests

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

Committed: https://crrev.com/e7408f4ef1f966edc09f907f2df421d32f72d2cb
Cr-Commit-Position: refs/heads/master@{#430409}

[chfremer, 2016-11-02 21:38:18]

Description was changed from

==========
format


Apply patch "Part_3_after_Add_Typemap.diff"

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

to

==========
[Mojo Video Capture] Use actual factory instances implementing
media::VideoCaptureDeviceFactory.

* Replace video_capture::VideoCaptureDeviceFactoryImpl (which was capable of
hosting instances of media::VideoCaptureDevice as well as mock devices) with
video_capture::DeviceFactoryMediaToMojoAdapter, which wraps a
media::VideoCaptureDeviceFactory. This can be used with the existing
platform-specific implementations as well as with FakeVideoCaptureDeviceFactory
and FileVideoCaptureDeviceFactory.
* Add class video_capture::MockDeviceFactory, which takes over the
responsibility of hosting mock devices and making them accessible through the
media::VideoCaptureDeviceFactory interface.
* Make media::VideoCaptureDeviceFactory::GetDeviceDescriptors() public so that
it can be called from DeviceFactoryMediaToMojoAdapter (which is much simpler
than having to call the asynchronous variant EnumerateDeviceDescriptors).

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

BUG=584797
TEST=video_capture_unittests, content_unittests, capture_unittests

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

[chfremer, 2016-11-02 21:41:42]

Patchset #1 (id:1) has been deleted

[chfremer, 2016-11-02 21:42:34]

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

[chfremer, 2016-11-02 21:42:35]

mcasas@: PTAL

[chfremer, 2016-11-04 20:39:14]

chfremer@chromium.org changed reviewers:
+ emircan@chromium.org

[chfremer, 2016-11-04 20:39:14]

emircan@: PTAL

[mcasas, 2016-11-04 23:24:29]

lgtm % comments and with bots happy.

Plz try to format the CL description to
80 columns :-)

https://codereview.chromium.org/2471613005/diff/20001/services/video_capture/...
File services/video_capture/device_factory_media_to_mojo_adapter.cc (right):

https://codereview.chromium.org/2471613005/diff/20001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.cc:61: // Revoke the
access and close the device, then bind to the new request.
Is this what we want to do? Or should 
subsequent requests for a device in use
just be rejected?

https://codereview.chromium.org/2471613005/diff/20001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.cc:73: // Create
device
Superfluous comment?

https://codereview.chromium.org/2471613005/diff/20001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.cc:81: // Add entry
to active_devices to keep track of it
nit: |active_devices|

https://codereview.chromium.org/2471613005/diff/20001/services/video_capture/...
File services/video_capture/device_factory_media_to_mojo_adapter.h (right):

https://codereview.chromium.org/2471613005/diff/20001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.h:59:
media::VideoCaptureDeviceDescriptor descriptor);
const & ?

https://codereview.chromium.org/2471613005/diff/20001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.h:64:
media::VideoCaptureJpegDecoderFactoryCB jpeg_decoder_factory_callback_;
can |jpeg_decoder_factory_callback_| and |device_factory_|
be made const?

https://codereview.chromium.org/2471613005/diff/20001/services/video_capture/...
File services/video_capture/video_capture_service.cc (right):

https://codereview.chromium.org/2471613005/diff/20001/services/video_capture/...
services/video_capture/video_capture_service.cc:77: // Task runner does not seem
to actually be used.
Nit: it's actually used for CrOs implementation [1]
to observe the rotation of the screen (and by that,
the whole camera) in here [2].

[1]
https://cs.chromium.org/chromium/src/media/capture/video/video_capture_device...
[2]
https://cs.chromium.org/chromium/src/media/capture/video/linux/video_capture_...

https://codereview.chromium.org/2471613005/diff/20001/services/video_capture/...
services/video_capture/video_capture_service.cc:94:
std::move(media_fake_device_factory), base::Bind(CreateJpegDecoder));
This might work for l.90-94:

  fake_device_factory_ = base::MakeUnique<DeviceFactoryMediaToMojoAdapter>(
      new media::FakeVideoCaptureDeviceFactory(),
      base::Bind(&CreateJpegDecoder));

Also, note the & before the method, here an in l.108.

https://codereview.chromium.org/2471613005/diff/20001/services/video_capture/...
File services/video_capture/video_capture_service.h (right):

https://codereview.chromium.org/2471613005/diff/20001/services/video_capture/...
services/video_capture/video_capture_service.h:51: void
LazyInitializeMockDeviceFactory();
Unrelated nit: shouldn't we add a ForTesting suffix to 
the Mock method ? (I understand that the Fake can
be used in Release builds, but the mock is only used
for tests, correct?)

[chfremer, 2016-11-07 17:21:04]

Description was changed from

==========
[Mojo Video Capture] Use actual factory instances implementing
media::VideoCaptureDeviceFactory.

* Replace video_capture::VideoCaptureDeviceFactoryImpl (which was capable of
hosting instances of media::VideoCaptureDevice as well as mock devices) with
video_capture::DeviceFactoryMediaToMojoAdapter, which wraps a
media::VideoCaptureDeviceFactory. This can be used with the existing
platform-specific implementations as well as with FakeVideoCaptureDeviceFactory
and FileVideoCaptureDeviceFactory.
* Add class video_capture::MockDeviceFactory, which takes over the
responsibility of hosting mock devices and making them accessible through the
media::VideoCaptureDeviceFactory interface.
* Make media::VideoCaptureDeviceFactory::GetDeviceDescriptors() public so that
it can be called from DeviceFactoryMediaToMojoAdapter (which is much simpler
than having to call the asynchronous variant EnumerateDeviceDescriptors).

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

BUG=584797
TEST=video_capture_unittests, content_unittests, capture_unittests

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

to

==========
[Mojo Video Capture] Use actual factory instances implementing
media::VideoCaptureDeviceFactory.

* Replace video_capture::VideoCaptureDeviceFactoryImpl (which was capable of 
  hosting instances of media::VideoCaptureDevice as well as mock devices) with
  video_capture::DeviceFactoryMediaToMojoAdapter, which wraps a 
  media::VideoCaptureDeviceFactory. This can be used with the existing 
  platform-specific implementations as well as with 
  FakeVideoCaptureDeviceFactory and FileVideoCaptureDeviceFactory.
* Add class video_capture::MockDeviceFactory, which takes over the 
  responsibility of hosting mock devices and making them accessible through the 
  media::VideoCaptureDeviceFactory interface.
* Make media::VideoCaptureDeviceFactory::GetDeviceDescriptors() public so that 
  it can be called from DeviceFactoryMediaToMojoAdapter (which is much simpler 
  than having to call the asynchronous variant EnumerateDeviceDescriptors).

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

BUG=584797
TEST=video_capture_unittests, content_unittests, capture_unittests

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

[chfremer, 2016-11-07 18:42:18]

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

[chfremer, 2016-11-07 18:42:37]

Addressed mcasas@ comments

https://codereview.chromium.org/2471613005/diff/20001/services/video_capture/...
File services/video_capture/device_factory_media_to_mojo_adapter.cc (right):

https://codereview.chromium.org/2471613005/diff/20001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.cc:61: // Revoke the
access and close the device, then bind to the new request.
On 2016/11/04 23:24:28, mcasas wrote:
> Is this what we want to do? Or should 
> subsequent requests for a device in use
> just be rejected?

This design was a suggestion by yzshen@ in response to a concern I had.
The concern was that if client A closes its |proxy_request|, this information 
arrives at the service asynchronously, and it may not be clear at what time 
a client B would be able to successfully call CreateDeviceProxy() again.

By always honoring the latest request and revoking older ones, we avoid this
potential issue.

I added a comment in the *.mojom to explain this design choice.

https://codereview.chromium.org/2471613005/diff/20001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.cc:73: // Create
device
On 2016/11/04 23:24:28, mcasas wrote:
> Superfluous comment?

Done.

https://codereview.chromium.org/2471613005/diff/20001/services/video_capture/...
File services/video_capture/device_factory_media_to_mojo_adapter.h (right):

https://codereview.chromium.org/2471613005/diff/20001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.h:59:
media::VideoCaptureDeviceDescriptor descriptor);
On 2016/11/04 23:24:29, mcasas wrote:
> const & ?

Done.

https://codereview.chromium.org/2471613005/diff/20001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.h:64:
media::VideoCaptureJpegDecoderFactoryCB jpeg_decoder_factory_callback_;
On 2016/11/04 23:24:29, mcasas wrote:
> can |jpeg_decoder_factory_callback_| and |device_factory_|
> be made const?

Done.

https://codereview.chromium.org/2471613005/diff/20001/services/video_capture/...
File services/video_capture/video_capture_service.cc (right):

https://codereview.chromium.org/2471613005/diff/20001/services/video_capture/...
services/video_capture/video_capture_service.cc:77: // Task runner does not seem
to actually be used.
On 2016/11/04 23:24:29, mcasas wrote:
> Nit: it's actually used for CrOs implementation [1]
> to observe the rotation of the screen (and by that,
> the whole camera) in here [2].
> 
> [1]
>
https://cs.chromium.org/chromium/src/media/capture/video/video_capture_device...
> [2]
>
https://cs.chromium.org/chromium/src/media/capture/video/linux/video_capture_...

Thanks. I should have seen that.
Makes me wonder if this connection to a global |g_screen| will still work
running in the Mojo service instead of the Browser process. I will add a task to
investigate that to the design doc.

https://codereview.chromium.org/2471613005/diff/20001/services/video_capture/...
services/video_capture/video_capture_service.cc:94:
std::move(media_fake_device_factory), base::Bind(CreateJpegDecoder));
On 2016/11/04 23:24:29, mcasas wrote:
> This might work for l.90-94:
> 
>   fake_device_factory_ = base::MakeUnique<DeviceFactoryMediaToMojoAdapter>(
>       new media::FakeVideoCaptureDeviceFactory(),
>       base::Bind(&CreateJpegDecoder));
> 
> Also, note the & before the method, here an in l.108.

Done.

https://codereview.chromium.org/2471613005/diff/20001/services/video_capture/...
File services/video_capture/video_capture_service.h (right):

https://codereview.chromium.org/2471613005/diff/20001/services/video_capture/...
services/video_capture/video_capture_service.h:51: void
LazyInitializeMockDeviceFactory();
On 2016/11/04 23:24:29, mcasas wrote:
> Unrelated nit: shouldn't we add a ForTesting suffix to 
> the Mock method ? (I understand that the Fake can
> be used in Release builds, but the mock is only used
> for tests, correct?)

You are correct. The mock factory is indeed testing-only.
Please allow me to keep things as is for now, since I already have a CL ready in
my local branch where I separate testing from production stuff. Better than just
indicating test-only stuff using a suffix, this CL separates things such that
testing-only stuff does not even appear in the production code or binaries.

[commit-bot: I haz the power, 2016-11-07 18:42:49]

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, 2016-11-07 18:45:16]

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

[commit-bot: I haz the power, 2016-11-07 18:45:16]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chfremer, 2016-11-07 19:19:28]

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

[commit-bot: I haz the power, 2016-11-07 19:20:02]

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

[chfremer, 2016-11-07 19:49:43]

chfremer@chromium.org changed reviewers:
+ tsepez@chromium.org

[chfremer, 2016-11-07 19:49:44]

tsepez@: Please RS added comment in *.mojom

[Tom Sepez, 2016-11-07 19:51:36]

RS LGTM

[commit-bot: I haz the power, 2016-11-07 22:20:25]

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

[commit-bot: I haz the power, 2016-11-07 22:20:26]

Dry run: This issue passed the CQ dry run.

[chfremer, 2016-11-07 22:24:23]

The CQ bit was checked by chfremer@chromium.org

[chfremer, 2016-11-07 22:24:24]

The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org
Link to the patchset: https://codereview.chromium.org/2471613005/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-07 22:25:02]

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, 2016-11-07 23:22:19]

Description was changed from

==========
[Mojo Video Capture] Use actual factory instances implementing
media::VideoCaptureDeviceFactory.

* Replace video_capture::VideoCaptureDeviceFactoryImpl (which was capable of 
  hosting instances of media::VideoCaptureDevice as well as mock devices) with
  video_capture::DeviceFactoryMediaToMojoAdapter, which wraps a 
  media::VideoCaptureDeviceFactory. This can be used with the existing 
  platform-specific implementations as well as with 
  FakeVideoCaptureDeviceFactory and FileVideoCaptureDeviceFactory.
* Add class video_capture::MockDeviceFactory, which takes over the 
  responsibility of hosting mock devices and making them accessible through the 
  media::VideoCaptureDeviceFactory interface.
* Make media::VideoCaptureDeviceFactory::GetDeviceDescriptors() public so that 
  it can be called from DeviceFactoryMediaToMojoAdapter (which is much simpler 
  than having to call the asynchronous variant EnumerateDeviceDescriptors).

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

BUG=584797
TEST=video_capture_unittests, content_unittests, capture_unittests

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

to

==========
[Mojo Video Capture] Use actual factory instances implementing
media::VideoCaptureDeviceFactory.

* Replace video_capture::VideoCaptureDeviceFactoryImpl (which was capable of 
  hosting instances of media::VideoCaptureDevice as well as mock devices) with
  video_capture::DeviceFactoryMediaToMojoAdapter, which wraps a 
  media::VideoCaptureDeviceFactory. This can be used with the existing 
  platform-specific implementations as well as with 
  FakeVideoCaptureDeviceFactory and FileVideoCaptureDeviceFactory.
* Add class video_capture::MockDeviceFactory, which takes over the 
  responsibility of hosting mock devices and making them accessible through the 
  media::VideoCaptureDeviceFactory interface.
* Make media::VideoCaptureDeviceFactory::GetDeviceDescriptors() public so that 
  it can be called from DeviceFactoryMediaToMojoAdapter (which is much simpler 
  than having to call the asynchronous variant EnumerateDeviceDescriptors).

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

BUG=584797
TEST=video_capture_unittests, content_unittests, capture_unittests

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

[commit-bot: I haz the power, 2016-11-07 23:22:21]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-11-07 23:25:58]

Description was changed from

==========
[Mojo Video Capture] Use actual factory instances implementing
media::VideoCaptureDeviceFactory.

* Replace video_capture::VideoCaptureDeviceFactoryImpl (which was capable of 
  hosting instances of media::VideoCaptureDevice as well as mock devices) with
  video_capture::DeviceFactoryMediaToMojoAdapter, which wraps a 
  media::VideoCaptureDeviceFactory. This can be used with the existing 
  platform-specific implementations as well as with 
  FakeVideoCaptureDeviceFactory and FileVideoCaptureDeviceFactory.
* Add class video_capture::MockDeviceFactory, which takes over the 
  responsibility of hosting mock devices and making them accessible through the 
  media::VideoCaptureDeviceFactory interface.
* Make media::VideoCaptureDeviceFactory::GetDeviceDescriptors() public so that 
  it can be called from DeviceFactoryMediaToMojoAdapter (which is much simpler 
  than having to call the asynchronous variant EnumerateDeviceDescriptors).

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

BUG=584797
TEST=video_capture_unittests, content_unittests, capture_unittests

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

to

==========
[Mojo Video Capture] Use actual factory instances implementing
media::VideoCaptureDeviceFactory.

* Replace video_capture::VideoCaptureDeviceFactoryImpl (which was capable of
  hosting instances of media::VideoCaptureDevice as well as mock devices) with
  video_capture::DeviceFactoryMediaToMojoAdapter, which wraps a
  media::VideoCaptureDeviceFactory. This can be used with the existing
  platform-specific implementations as well as with
  FakeVideoCaptureDeviceFactory and FileVideoCaptureDeviceFactory.
* Add class video_capture::MockDeviceFactory, which takes over the
  responsibility of hosting mock devices and making them accessible through the
  media::VideoCaptureDeviceFactory interface.
* Make media::VideoCaptureDeviceFactory::GetDeviceDescriptors() public so that
  it can be called from DeviceFactoryMediaToMojoAdapter (which is much simpler
  than having to call the asynchronous variant EnumerateDeviceDescriptors).

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

BUG=584797
TEST=video_capture_unittests, content_unittests, capture_unittests

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

Committed: https://crrev.com/e7408f4ef1f966edc09f907f2df421d32f72d2cb
Cr-Commit-Position: refs/heads/master@{#430409}
==========

[commit-bot: I haz the power, 2016-11-07 23:25:59]

Patchset 4 (id:??) landed as
https://crrev.com/e7408f4ef1f966edc09f907f2df421d32f72d2cb
Cr-Commit-Position: refs/heads/master@{#430409}