[Mojo Video Capture] Implement a VideoCaptureProvider using the Mojo service (part 1)

[Mojo Video Capture] Implement a VideoCaptureProvider using the Mojo service (part 1)

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

Goal of CL24:
The interface content::VideoCaptureProvider currently has one implementation
called InProcessVideoCaptureProvider, which is essentially a factory for the
legacy in-process video capture stack. This CL adds a second implementation
called MojoServiceVideoCaptureProvider which is essentially a wrapper for
connecting to and communicating with the new video capture service.

Changes in part1:
* Add class MojoServiceVideoCaptureProvider
* Add skeletons for classes MojoServiceVideoCaptureDeviceLauncher and
  MojoServiceLaunchedVideoCaptureDevice
* Introduce a build flag enable_mojo_video_capture to prevent the new and
  incomplete code from going into builds.

BUG=584797
TEST=
  service_unittests --gtest_filter="*Video*"
  content_unittests --gtest_filter="*Video*"
  content_browsertests --gtest_filter="VideoCaptureBrowserTest.*"

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

Review-Url: https://codereview.chromium.org/2848973002
Cr-Commit-Position: refs/heads/master@{#469380}
Committed: https://chromium.googlesource.com/chromium/src/+/954d0aca7b6fa688b7e963c57475a4ef747384bb

[chfremer, 2017-04-28 19:17:24]

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

[chfremer, 2017-04-28 19:17:34]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-04-28 19:18:38]

Dry run: CQ is trying da patch.

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

[chfremer, 2017-04-28 19:19:49]

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

[chfremer, 2017-04-28 19:19:50]

mcasas@: PTAL
rockot@: PTAL service connection parts in mojo_service_video_capture_provider.cc
miu@, emircan@: FYI

[commit-bot: I haz the power, 2017-04-28 21:35:34]

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

[commit-bot: I haz the power, 2017-04-28 21:35:35]

Dry run: This issue passed the CQ dry run.

[chfremer, 2017-05-02 00:17:11]

On 2017/04/28 19:19:50, chfremer wrote:
> mcasas@: PTAL
> rockot@: PTAL service connection parts in
mojo_service_video_capture_provider.cc
> miu@, emircan@: FYI

ping

[mcasas, 2017-05-02 01:06:23]

looking good, a few comments.

https://codereview.chromium.org/2848973002/diff/20001/content/browser/rendere...
File
content/browser/renderer_host/media/mojo_service_launched_video_capture_device.h
(right):

https://codereview.chromium.org/2848973002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/mojo_service_launched_video_capture_device.h:15:
class MojoServiceLaunchedVideoCaptureDevice
Suggestion: no need for "Service" in the name, "Mojo" is
sufficient to convey that we have no control as to where 
it is implemented, it could be in process or out of process
(as a matter of fact if you use the flag --single-process,
everything is bundled up into one giant process !)

https://codereview.chromium.org/2848973002/diff/20001/content/browser/rendere...
File
content/browser/renderer_host/media/mojo_service_video_capture_device_launcher.h
(right):

https://codereview.chromium.org/2848973002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/mojo_service_video_capture_device_launcher.h:9:
#include "content/browser/renderer_host/media/video_capture_provider.h"
Are these two headers needed? Try to forward declare.

https://codereview.chromium.org/2848973002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/mojo_service_video_capture_provider.cc
(right):

https://codereview.chromium.org/2848973002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/mojo_service_video_capture_provider.cc:83:
// clients try to reconnect.
nit: Reflow text?

[chfremer, 2017-05-02 16:36:42]

PTAL

https://codereview.chromium.org/2848973002/diff/20001/content/browser/rendere...
File
content/browser/renderer_host/media/mojo_service_launched_video_capture_device.h
(right):

https://codereview.chromium.org/2848973002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/mojo_service_launched_video_capture_device.h:15:
class MojoServiceLaunchedVideoCaptureDevice
On 2017/05/02 01:06:23, mcasas wrote:
> Suggestion: no need for "Service" in the name, "Mojo" is
> sufficient to convey that we have no control as to where 
> it is implemented, it could be in process or out of process
> (as a matter of fact if you use the flag --single-process,
> everything is bundled up into one giant process !)

I don't feel strongly about this, but would like to push back renaming files to
the end of my stashed changes.

To me, including the word "service" makes sense, because I believe things can be
use Mojo for communication/interfaces without being a service.

@Ken, what's your view on this? 

If we agree on a better name, I will add the renaming task to my backlog.

https://codereview.chromium.org/2848973002/diff/20001/content/browser/rendere...
File
content/browser/renderer_host/media/mojo_service_video_capture_device_launcher.h
(right):

https://codereview.chromium.org/2848973002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/mojo_service_video_capture_device_launcher.h:9:
#include "content/browser/renderer_host/media/video_capture_provider.h"
On 2017/05/02 01:06:23, mcasas wrote:
> Are these two headers needed? Try to forward declare.

Indeed not needed. Thanks.

Done.

https://codereview.chromium.org/2848973002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/mojo_service_video_capture_provider.cc
(right):

https://codereview.chromium.org/2848973002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/mojo_service_video_capture_provider.cc:83:
// clients try to reconnect.
On 2017/05/02 01:06:23, mcasas wrote:
> nit: Reflow text?

Done.

[Ken Rockot(use gerrit already), 2017-05-03 16:37:30]

On 2017/05/02 at 16:36:42, chfremer wrote:
> PTAL
> 
>
https://codereview.chromium.org/2848973002/diff/20001/content/browser/rendere...
> File
content/browser/renderer_host/media/mojo_service_launched_video_capture_device.h
(right):
> 
>
https://codereview.chromium.org/2848973002/diff/20001/content/browser/rendere...
>
content/browser/renderer_host/media/mojo_service_launched_video_capture_device.h:15:
class MojoServiceLaunchedVideoCaptureDevice
> On 2017/05/02 01:06:23, mcasas wrote:
> > Suggestion: no need for "Service" in the name, "Mojo" is
> > sufficient to convey that we have no control as to where 
> > it is implemented, it could be in process or out of process
> > (as a matter of fact if you use the flag --single-process,
> > everything is bundled up into one giant process !)
> 
> I don't feel strongly about this, but would like to push back renaming files
to the end of my stashed changes.
> 
> To me, including the word "service" makes sense, because I believe things can
be use Mojo for communication/interfaces without being a service.
> 
> @Ken, what's your view on this? 

Just to add fuel to the bikeshed fire, I actually have more of a problem with
"Mojo" being sprinkled into names. I don't think it's a good practice and I try
to discourage it as much as possible. I'd rather it just be
"ServiceLaunchedVideoCaptureDevice".

> 
> If we agree on a better name, I will add the renaming task to my backlog.
> 
>
https://codereview.chromium.org/2848973002/diff/20001/content/browser/rendere...
> File
content/browser/renderer_host/media/mojo_service_video_capture_device_launcher.h
(right):
> 
>
https://codereview.chromium.org/2848973002/diff/20001/content/browser/rendere...
>
content/browser/renderer_host/media/mojo_service_video_capture_device_launcher.h:9:
#include "content/browser/renderer_host/media/video_capture_provider.h"
> On 2017/05/02 01:06:23, mcasas wrote:
> > Are these two headers needed? Try to forward declare.
> 
> Indeed not needed. Thanks.
> 
> Done.
> 
>
https://codereview.chromium.org/2848973002/diff/20001/content/browser/rendere...
> File
content/browser/renderer_host/media/mojo_service_video_capture_provider.cc
(right):
> 
>
https://codereview.chromium.org/2848973002/diff/20001/content/browser/rendere...
> content/browser/renderer_host/media/mojo_service_video_capture_provider.cc:83:
// clients try to reconnect.
> On 2017/05/02 01:06:23, mcasas wrote:
> > nit: Reflow text?
> 
> Done.

[chfremer, 2017-05-03 17:10:05]

PTAL

I renamed MojoServiceVideoCapture* to ServiceVideoCapture*.

[Ken Rockot(use gerrit already), 2017-05-03 17:59:46]

https://codereview.chromium.org/2848973002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/service_video_capture_provider.cc
(right):

https://codereview.chromium.org/2848973002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/service_video_capture_provider.cc:66:
BindToBrowserConnector(std::move(connector_request));
Can ServiceVideoCaptureProvider actually be created on arbitrary threads or is
it just created on the main thread?

[chfremer, 2017-05-03 18:29:09]

https://codereview.chromium.org/2848973002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/service_video_capture_provider.cc
(right):

https://codereview.chromium.org/2848973002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/service_video_capture_provider.cc:66:
BindToBrowserConnector(std::move(connector_request));
On 2017/05/03 17:59:46, Ken Rockot wrote:
> Can ServiceVideoCaptureProvider actually be created on arbitrary threads or is
> it just created on the main thread?

This is not going to be visible and tested before CL25, but in there it is going
to be as follows:
ServiceVideoCaptureProvider will be created on the main thread, but will then be
operated on the IO thread. I had to add a thread_checker_.DetachFromThread(); to
CL25 for this to work, but to make it more clear I can also port it to this CL.

[Ken Rockot(use gerrit already), 2017-05-03 18:32:19]

https://codereview.chromium.org/2848973002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/service_video_capture_provider.cc
(right):

https://codereview.chromium.org/2848973002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/service_video_capture_provider.cc:66:
BindToBrowserConnector(std::move(connector_request));
On 2017/05/03 at 18:29:09, chfremer wrote:
> On 2017/05/03 17:59:46, Ken Rockot wrote:
> > Can ServiceVideoCaptureProvider actually be created on arbitrary threads or
is
> > it just created on the main thread?
> 
> This is not going to be visible and tested before CL25, but in there it is
going to be as follows:
> ServiceVideoCaptureProvider will be created on the main thread, but will then
be operated on the IO thread. I had to add a thread_checker_.DetachFromThread();
to CL25 for this to work, but to make it more clear I can also port it to this
CL.

I see. You can probably simply things a bit then, by having this object own its
own std::unique_ptr<Connector>. You could initialize it in the constructor
(which as you say runs on the main thread) by simply calling
ServiceManagerConnection::GetForProcess()->GetConnector()->Clone(). The
Connector you get back won't be bound to a specific thread until you make a call
on it.

[mcasas, 2017-05-03 19:10:23]

non-mojo parts lgtm % a few comments

https://codereview.chromium.org/2848973002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/service_launched_video_capture_device.h
(right):

https://codereview.chromium.org/2848973002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/service_launched_video_capture_device.h:39:
video_capture::mojom::DevicePtr device_;
This can probably be const (since is passed on ctor and
none of the methods seem to destroy it, right?)

https://codereview.chromium.org/2848973002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/service_video_capture_device_launcher.h
(right):

https://codereview.chromium.org/2848973002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/service_video_capture_device_launcher.h:18:
ServiceVideoCaptureDeviceLauncher(
nit: explicit

https://codereview.chromium.org/2848973002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/service_video_capture_provider.cc
(right):

https://codereview.chromium.org/2848973002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/service_video_capture_provider.cc:28: }  //
namespace
IIRC in content/ these anonymous namespaces
are supposed to be inside content namespace
(which is different from media/ where these
anonymous go out of media namespace).

https://codereview.chromium.org/2848973002/diff/60001/services/video_capture/...
File services/video_capture/public/cpp/receiver_media_to_mojo_adapter.h (right):

https://codereview.chromium.org/2848973002/diff/60001/services/video_capture/...
services/video_capture/public/cpp/receiver_media_to_mojo_adapter.h:32: void
OnStarted() override;
These changes seem unrelated.

[chfremer, 2017-05-03 21:10:39]

rockot@: PTAL

I tried the simplification with the tests coming up in CL25, and it seems to
work well. Thanks.

mcasas@: I will incorporate your suggestions in the next patch set.

[Ken Rockot(use gerrit already), 2017-05-03 21:12:24]

lgtm

[chfremer, 2017-05-03 21:29:36]

Thanks for the review!

https://codereview.chromium.org/2848973002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/service_launched_video_capture_device.h
(right):

https://codereview.chromium.org/2848973002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/service_launched_video_capture_device.h:39:
video_capture::mojom::DevicePtr device_;
On 2017/05/03 19:10:23, mcasas wrote:
> This can probably be const (since is passed on ctor and
> none of the methods seem to destroy it, right?)

You are correct that, in this CL, it can be const.
However, this is only because |device_| is not yet being used.
In CL24_part2, I will have to remove the const again.

Done.

https://codereview.chromium.org/2848973002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/service_video_capture_device_launcher.h
(right):

https://codereview.chromium.org/2848973002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/service_video_capture_device_launcher.h:18:
ServiceVideoCaptureDeviceLauncher(
On 2017/05/03 19:10:23, mcasas wrote:
> nit: explicit

Done.

https://codereview.chromium.org/2848973002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/service_video_capture_provider.cc
(right):

https://codereview.chromium.org/2848973002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/service_video_capture_provider.cc:28: }  //
namespace
On 2017/05/03 19:10:23, mcasas wrote:
> IIRC in content/ these anonymous namespaces
> are supposed to be inside content namespace
> (which is different from media/ where these
> anonymous go out of media namespace).

Oh. This is confusing. I'll try to remember it.
Luckily, here, the anonymous namespace was eliminated based on rockot@'s
simplification.

https://codereview.chromium.org/2848973002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/service_video_capture_provider.cc:66:
BindToBrowserConnector(std::move(connector_request));
On 2017/05/03 18:32:19, Ken Rockot wrote:
> On 2017/05/03 at 18:29:09, chfremer wrote:
> > On 2017/05/03 17:59:46, Ken Rockot wrote:
> > > Can ServiceVideoCaptureProvider actually be created on arbitrary threads
or
> is
> > > it just created on the main thread?
> > 
> > This is not going to be visible and tested before CL25, but in there it is
> going to be as follows:
> > ServiceVideoCaptureProvider will be created on the main thread, but will
then
> be operated on the IO thread. I had to add a
thread_checker_.DetachFromThread();
> to CL25 for this to work, but to make it more clear I can also port it to this
> CL.
> 
> I see. You can probably simply things a bit then, by having this object own
its
> own std::unique_ptr<Connector>. You could initialize it in the constructor
> (which as you say runs on the main thread) by simply calling
> ServiceManagerConnection::GetForProcess()->GetConnector()->Clone(). The
> Connector you get back won't be bound to a specific thread until you make a
call
> on it.

Done.

https://codereview.chromium.org/2848973002/diff/60001/services/video_capture/...
File services/video_capture/public/cpp/receiver_media_to_mojo_adapter.h (right):

https://codereview.chromium.org/2848973002/diff/60001/services/video_capture/...
services/video_capture/public/cpp/receiver_media_to_mojo_adapter.h:32: void
OnStarted() override;
On 2017/05/03 19:10:23, mcasas wrote:
> These changes seem unrelated.

You are right. I guess my CL split was not ideal here.
The interface VideoFrameReceiver has changed, and the only reason there was no
error is that this class is not currently used. It will be used starting in
CL25.

For simplicity, please allow me to just keep the changes here.

[chfremer, 2017-05-03 21:29:56]

The CQ bit was checked by chfremer@chromium.org

[chfremer, 2017-05-03 21:29:57]

The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org,
rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2848973002/#ps100001
(title: "Incorporated mcasas@'s suggestions from PatchSet 3")

[commit-bot: I haz the power, 2017-05-03 21:30:36]

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-05-03 21:41:36]

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

[commit-bot: I haz the power, 2017-05-03 21:41:38]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[chfremer, 2017-05-03 22:17:28]

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

[chfremer, 2017-05-03 22:17:29]

avi@: PTAL BUILD.gn

[Avi (use Gerrit), 2017-05-04 17:47:08]

lgtm

stamp

[chfremer, 2017-05-04 17:48:03]

The CQ bit was checked by chfremer@chromium.org

[commit-bot: I haz the power, 2017-05-04 17:48:26]

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-05-04 18:14:00]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493920083019800,
"parent_rev": "240c255d33ba778844e6e86309400c4932298f6d", "commit_rev":
"954d0aca7b6fa688b7e963c57475a4ef747384bb"}

[commit-bot: I haz the power, 2017-05-04 18:14:15]

Description was changed from

==========
[Mojo Video Capture] Implement a VideoCaptureProvider using the Mojo service
(part 1)

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

Goal of CL24:
The interface content::VideoCaptureProvider currently has one implementation
called InProcessVideoCaptureProvider, which is essentially a factory for the
legacy in-process video capture stack. This CL adds a second implementation
called MojoServiceVideoCaptureProvider which is essentially a wrapper for
connecting to and communicating with the new video capture service.

Changes in part1:
* Add class MojoServiceVideoCaptureProvider
* Add skeletons for classes MojoServiceVideoCaptureDeviceLauncher and
  MojoServiceLaunchedVideoCaptureDevice
* Introduce a build flag enable_mojo_video_capture to prevent the new and
  incomplete code from going into builds.

BUG=584797
TEST=
  service_unittests --gtest_filter="*Video*"
  content_unittests --gtest_filter="*Video*"
  content_browsertests --gtest_filter="VideoCaptureBrowserTest.*"

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

to

==========
[Mojo Video Capture] Implement a VideoCaptureProvider using the Mojo service
(part 1)

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

Goal of CL24:
The interface content::VideoCaptureProvider currently has one implementation
called InProcessVideoCaptureProvider, which is essentially a factory for the
legacy in-process video capture stack. This CL adds a second implementation
called MojoServiceVideoCaptureProvider which is essentially a wrapper for
connecting to and communicating with the new video capture service.

Changes in part1:
* Add class MojoServiceVideoCaptureProvider
* Add skeletons for classes MojoServiceVideoCaptureDeviceLauncher and
  MojoServiceLaunchedVideoCaptureDevice
* Introduce a build flag enable_mojo_video_capture to prevent the new and
  incomplete code from going into builds.

BUG=584797
TEST=
  service_unittests --gtest_filter="*Video*"
  content_unittests --gtest_filter="*Video*"
  content_browsertests --gtest_filter="VideoCaptureBrowserTest.*"

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

Review-Url: https://codereview.chromium.org/2848973002
Cr-Commit-Position: refs/heads/master@{#469380}
Committed:
https://chromium.googlesource.com/chromium/src/+/954d0aca7b6fa688b7e963c57475...
==========

[commit-bot: I haz the power, 2017-05-04 18:14:17]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/954d0aca7b6fa688b7e963c57475...