[Mojo Video Capture] Hook up video capture service behind a feature flag

[Mojo Video Capture] Hook up video capture service behind a feature flag

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

After this CL, the video capture service can be enabled by starting Chrome with
command-line flag --enable-features=MojoVideoCapture.

Changes in this CL:
* Add a base::Feature kMojoVideoCapture to allow switching the service on and
  off.
* Add a class VideoCaptureProviderSwitcher that can route device capture
  requests to the service's VideoCaptureProvider while routing screen capture
  requests to the InProcessVideoCaptureProvider.
* Hook up the VideoCaptureProviderSwitcher in the factory code in
  MediaStreamManager.
* Register video capture service in the service manager context for the Browser
  process.
* Remove build flag "enable_mojo_video_capture", since we want the service to
  be testable in all builds.
* Forward switches for using fake video capture devices to utility processes.
* Add test cases for exercising the service to VideoCaptureBrowserTest.

BUG=584797
TEST=
  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/2867213004
Cr-Commit-Position: refs/heads/master@{#473358}
Committed: https://chromium.googlesource.com/chromium/src/+/d99842d2bfbffba9b80aaa81aced1f1e7ebb39c8

[chfremer, 2017-05-09 19:25:08]

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

[chfremer, 2017-05-09 19:26:04]

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

[chfremer, 2017-05-09 19:26:05]

mcasas@, rockot@: PTAL
miu@, emircan@: FYI

[commit-bot: I haz the power, 2017-05-09 19:26:07]

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-05-09 19:32:21]

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

[commit-bot: I haz the power, 2017-05-09 19:32:22]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[chfremer, 2017-05-09 21:24:02]

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

[commit-bot: I haz the power, 2017-05-09 21:25:50]

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-05-09 22:47:17]

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

[commit-bot: I haz the power, 2017-05-09 22:47:17]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chfremer, 2017-05-09 23:49:02]

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

[chfremer, 2017-05-09 23:50:15]

Patchset #3 (id:40001) has been deleted

[chfremer, 2017-05-09 23:50:38]

The CQ bit was unchecked by chfremer@chromium.org

[chfremer, 2017-05-10 00:30:46]

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

[commit-bot: I haz the power, 2017-05-10 00:31:56]

Dry run: CQ is trying da patch.

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

[mcasas, 2017-05-10 02:33:33]

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

https://codereview.chromium.org/2867213004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_browsertest.cc:189: // Mojo
video capture is currently not supported on Android
This sounds pretty bad: at least a TODO() and a bug should
be available.  Same in l.216.

https://codereview.chromium.org/2867213004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_browsertest.cc:317:
gfx::Size(640, 480), 6.0f, true, false}));
Consider using ::testing::Combine to simplify all this cases

https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...
(search for "Combine" in the link, plenty of examples in
Cr codebase too).

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

https://codereview.chromium.org/2867213004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_provider_switcher.cc:56: }
else {
No need for else after a return.
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

https://codereview.chromium.org/2867213004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_provider_switcher.cc:71: }
Instead of |last_launched_stream_type_|, bind a member 
callback in LaunchDeviceAsync() to AbortLaunch() and, if 
such Callback is initialized (!callback.is_null()), you call it here.

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

https://codereview.chromium.org/2867213004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_provider_switcher.h:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
s/2016/2017/

https://codereview.chromium.org/2867213004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_provider_switcher.h:38: class
CONTENT_EXPORT VideoCaptureDeviceLauncherSwitcher
I'm not sure about having two classes bound together, have
you found more examples of such construction in //content ?

https://codereview.chromium.org/2867213004/diff/60001/content/browser/service...
File content/browser/service_manager/service_manager_context.cc (right):

https://codereview.chromium.org/2867213004/diff/60001/content/browser/service...
content/browser/service_manager/service_manager_context.cc:347:
unsandboxed_services.insert(
Shouldn't we have a command line check like in l.339?

https://codereview.chromium.org/2867213004/diff/60001/content/public/app/mojo...
File content/public/app/mojo/content_browser_manifest.json (right):

https://codereview.chromium.org/2867213004/diff/60001/content/public/app/mojo...
content/public/app/mojo/content_browser_manifest.json:82: "video_capture": [
"production" ]
"Production" ?

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

https://codereview.chromium.org/2867213004/diff/60001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.cc:46: }
    translated_format.pixel_format =
        format.pixel_format == media::PIXEL_FORMAT_Y16
            ? media::PIXEL_FORMAT_Y16
            : media::PIXEL_FORMAT_I420;

[commit-bot: I haz the power, 2017-05-10 02:37:31]

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

[commit-bot: I haz the power, 2017-05-10 02:37:32]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has
not started yet; builder either lacks capacity or does not exist (misspelled?))
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet;
builder either lacks capacity or does not exist (misspelled?))
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
build has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
build has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[chfremer, 2017-05-10 17:59:40]

PTAL

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

https://codereview.chromium.org/2867213004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_browsertest.cc:189: // Mojo
video capture is currently not supported on Android
On 2017/05/10 02:33:32, mcasas wrote:
> This sounds pretty bad: at least a TODO() and a bug should
> be available.  Same in l.216.

Done.

https://codereview.chromium.org/2867213004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_browsertest.cc:317:
gfx::Size(640, 480), 6.0f, true, false}));
On 2017/05/10 02:33:33, mcasas wrote:
> Consider using ::testing::Combine to simplify all this cases
> 
>
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...
> (search for "Combine" in the link, plenty of examples in
> Cr codebase too).

I prefer to not use ::testing::Combine() here, because we do not need to test
all combinations. 
I agree that this hand-select list is quite verbose, but I think it is worth the
build/bot time savings of not running test cases that are not needed, especially
since these tests are fairly slow (can be > 1s per test case).

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

https://codereview.chromium.org/2867213004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_provider_switcher.cc:56: }
else {
On 2017/05/10 02:33:33, mcasas wrote:
> No need for else after a return.
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Done.

https://codereview.chromium.org/2867213004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_provider_switcher.cc:71: }
On 2017/05/10 02:33:33, mcasas wrote:
> Instead of |last_launched_stream_type_|, bind a member 
> callback in LaunchDeviceAsync() to AbortLaunch() and, if 
> such Callback is initialized (!callback.is_null()), you call it here.

Done.

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

https://codereview.chromium.org/2867213004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_provider_switcher.h:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/05/10 02:33:33, mcasas wrote:
> s/2016/2017/

Done.

https://codereview.chromium.org/2867213004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_provider_switcher.h:38: class
CONTENT_EXPORT VideoCaptureDeviceLauncherSwitcher
On 2017/05/10 02:33:33, mcasas wrote:
> I'm not sure about having two classes bound together, have
> you found more examples of such construction in //content ?

What is your concern about this type of construction?
To me it appears to be a clean solution to a valid need.

https://codereview.chromium.org/2867213004/diff/60001/content/browser/service...
File content/browser/service_manager/service_manager_context.cc (right):

https://codereview.chromium.org/2867213004/diff/60001/content/browser/service...
content/browser/service_manager/service_manager_context.cc:347:
unsandboxed_services.insert(
On 2017/05/10 02:33:33, mcasas wrote:
> Shouldn't we have a command line check like in l.339?

You are right. Unless the feature is enabled, we don't have to register the
service here.

Done.

https://codereview.chromium.org/2867213004/diff/60001/content/public/app/mojo...
File content/public/app/mojo/content_browser_manifest.json (right):

https://codereview.chromium.org/2867213004/diff/60001/content/public/app/mojo...
content/public/app/mojo/content_browser_manifest.json:82: "video_capture": [
"production" ]
On 2017/05/10 02:33:33, mcasas wrote:
> "Production" ?

Is this about upper-case vs. lower-case? It seems the convention here is to use
lower-case.

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

https://codereview.chromium.org/2867213004/diff/60001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.cc:46: }
On 2017/05/10 02:33:33, mcasas wrote:
>     translated_format.pixel_format =
>         format.pixel_format == media::PIXEL_FORMAT_Y16
>             ? media::PIXEL_FORMAT_Y16
>             : media::PIXEL_FORMAT_I420;

Done.

[mcasas, 2017-05-10 18:15:44]

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

https://codereview.chromium.org/2867213004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_browsertest.cc:317:
gfx::Size(640, 480), 6.0f, true, false}));
On 2017/05/10 17:59:39, chfremer wrote:
> On 2017/05/10 02:33:33, mcasas wrote:
> > Consider using ::testing::Combine to simplify all this cases
> > 
> >
>
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...
> > (search for "Combine" in the link, plenty of examples in
> > Cr codebase too).
> 
> I prefer to not use ::testing::Combine() here, because we do not need to test
> all combinations. 
Why not?  It should be made explicit here why not,
otherwise Combine() seems to be easier to read, and
the time spent in bots should not be a concern: we want
more testing, not less; and also, needed infra would be 
provisioned only when there's load for it.

> I agree that this hand-select list is quite verbose, but I think it is worth
the
> build/bot time savings of not running test cases that are not needed,
especially
> since these tests are fairly slow (can be > 1s per test case).

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

https://codereview.chromium.org/2867213004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_provider_switcher.h:38: class
CONTENT_EXPORT VideoCaptureDeviceLauncherSwitcher
On 2017/05/10 17:59:39, chfremer wrote:
> On 2017/05/10 02:33:33, mcasas wrote:
> > I'm not sure about having two classes bound together, have
> > you found more examples of such construction in //content ?
> 
> What is your concern about this type of construction?
> To me it appears to be a clean solution to a valid need.

My concern is what I wrote: "having two classes bound 
together" in the same file.  Examples?

https://codereview.chromium.org/2867213004/diff/60001/content/public/app/mojo...
File content/public/app/mojo/content_browser_manifest.json (right):

https://codereview.chromium.org/2867213004/diff/60001/content/public/app/mojo...
content/public/app/mojo/content_browser_manifest.json:82: "video_capture": [
"production" ]
On 2017/05/10 17:59:40, chfremer wrote:
> On 2017/05/10 02:33:33, mcasas wrote:
> > "Production" ?
> 
> Is this about upper-case vs. lower-case? It seems the convention here is to
use
> lower-case.

My concern is that "production" does not seem to correspond
to any service identifier, which is the rule followed by the 
other services in the vicinity, i.e. something like
"service" : ["interface", "interface"].  
E.g.  "media" says "media": ["media:media"], so, in this case, 
I would expect
"video_capture" : [ "video_capture" ] 
or
"video_capture" : [ "capture" ] 
(the second kind-a points out to more interfaces being
available, e.g. "enumeration").

[chfremer, 2017-05-10 21:08:06]

PTAL

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

https://codereview.chromium.org/2867213004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_browsertest.cc:317:
gfx::Size(640, 480), 6.0f, true, false}));
On 2017/05/10 18:15:43, mcasas wrote:
> On 2017/05/10 17:59:39, chfremer wrote:
> > On 2017/05/10 02:33:33, mcasas wrote:
> > > Consider using ::testing::Combine to simplify all this cases
> > > 
> > >
> >
>
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...
> > > (search for "Combine" in the link, plenty of examples in
> > > Cr codebase too).
> > 
> > I prefer to not use ::testing::Combine() here, because we do not need to
test
> > all combinations. 
> Why not?  It should be made explicit here why not,
> otherwise Combine() seems to be easier to read, and
> the time spent in bots should not be a concern: we want
> more testing, not less; and also, needed infra would be 
> provisioned only when there's load for it.
> 
> > I agree that this hand-select list is quite verbose, but I think it is worth
> the
> > build/bot time savings of not running test cases that are not needed,
> especially
> > since these tests are fairly slow (can be > 1s per test case).
> 

Done.

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

https://codereview.chromium.org/2867213004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_provider_switcher.h:38: class
CONTENT_EXPORT VideoCaptureDeviceLauncherSwitcher
On 2017/05/10 18:15:44, mcasas wrote:
> On 2017/05/10 17:59:39, chfremer wrote:
> > On 2017/05/10 02:33:33, mcasas wrote:
> > > I'm not sure about having two classes bound together, have
> > > you found more examples of such construction in //content ?
> > 
> > What is your concern about this type of construction?
> > To me it appears to be a clean solution to a valid need.
> 
> My concern is what I wrote: "having two classes bound 
> together" in the same file.  Examples?

Ah, two classes in the same file. 
Yes that is indeed uncommon in //content.
Since VideoCaptureDeviceLauncherSwitcher is only ever created by
VideoCaptureProviderSwitcher, I now moved it into its cc-file-scope.

Done.

https://codereview.chromium.org/2867213004/diff/60001/content/public/app/mojo...
File content/public/app/mojo/content_browser_manifest.json (right):

https://codereview.chromium.org/2867213004/diff/60001/content/public/app/mojo...
content/public/app/mojo/content_browser_manifest.json:82: "video_capture": [
"production" ]
On 2017/05/10 18:15:44, mcasas wrote:
> On 2017/05/10 17:59:40, chfremer wrote:
> > On 2017/05/10 02:33:33, mcasas wrote:
> > > "Production" ?
> > 
> > Is this about upper-case vs. lower-case? It seems the convention here is to
> use
> > lower-case.
> 
> My concern is that "production" does not seem to correspond
> to any service identifier, which is the rule followed by the 
> other services in the vicinity, i.e. something like
> "service" : ["interface", "interface"].  
> E.g.  "media" says "media": ["media:media"], so, in this case, 
> I would expect
> "video_capture" : [ "video_capture" ] 
> or
> "video_capture" : [ "capture" ] 
> (the second kind-a points out to more interfaces being
> available, e.g. "enumeration").

Done.

[chfremer, 2017-05-12 17:16:20]

ping, rockot@: Could you please take a quick look at the service hookup parts?

[Ken Rockot(use gerrit already), 2017-05-12 19:29:22]

lgtm

https://codereview.chromium.org/2867213004/diff/100001/services/video_capture...
File services/video_capture/service_manifest.json (right):

https://codereview.chromium.org/2867213004/diff/100001/services/video_capture...
services/video_capture/service_manifest.json:7: "capture": [ "*" ],
I guess this is OK since you're just updating the name in this CL, but
preferably services would be specific about which top-level interfaces are
exposed by each capability (testing capabilities excluded).

[mcasas, 2017-05-12 22:07:03]

Almost there

https://codereview.chromium.org/2867213004/diff/100001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/2867213004/diff/100001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.cc:428:
std::move(device_task_runner));
|device_task_runner| is unused if 
base::FeatureList::IsEnabled(video_capture::kMojoVideoCapture),
and also a |video_capture_provider| is constructed in l.424 and
then discarded in l.431.  Please refactor the block in l.414-434.

https://codereview.chromium.org/2867213004/diff/100001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.cc:430: DVLOG(3) <<
"Mojo video capture enabled";
nit: remove this.

https://codereview.chromium.org/2867213004/diff/100001/content/browser/render...
File
content/browser/renderer_host/media/service_video_capture_device_launcher.cc
(right):

https://codereview.chromium.org/2867213004/diff/100001/content/browser/render...
content/browser/renderer_host/media/service_video_capture_device_launcher.cc:94:
device_id, std::move(device_request),
Why this change?

https://codereview.chromium.org/2867213004/diff/100001/content/browser/render...
File content/browser/renderer_host/media/video_capture_browsertest.cc (right):

https://codereview.chromium.org/2867213004/diff/100001/content/browser/render...
content/browser/renderer_host/media/video_capture_browsertest.cc:219: //
TODO(chfremer): Remove this as soon as crbug.com/720500 is resolved.
nit: prefix https:// for bug links, here and in l.224, l.247, 251.

https://codereview.chromium.org/2867213004/diff/100001/content/browser/render...
content/browser/renderer_host/media/video_capture_browsertest.cc:346: Bool())); 
// UseMojoService
Yay!!

https://codereview.chromium.org/2867213004/diff/100001/content/browser/render...
File content/browser/renderer_host/media/video_capture_provider_switcher.cc
(right):

https://codereview.chromium.org/2867213004/diff/100001/content/browser/render...
content/browser/renderer_host/media/video_capture_provider_switcher.cc:54:
std::unique_ptr<VideoCaptureDeviceLauncher> other_types_launcher_;
Can we make any of these two const?

https://codereview.chromium.org/2867213004/diff/100001/content/browser/render...
File content/browser/renderer_host/media/video_capture_provider_switcher.h
(right):

https://codereview.chromium.org/2867213004/diff/100001/content/browser/render...
content/browser/renderer_host/media/video_capture_provider_switcher.h:35:
std::unique_ptr<VideoCaptureProvider> other_types_capture_provider_;
Can we make any of these members const?

https://codereview.chromium.org/2867213004/diff/100001/services/video_capture...
File services/video_capture/public/cpp/constants.h (right):

https://codereview.chromium.org/2867213004/diff/100001/services/video_capture...
services/video_capture/public/cpp/constants.h:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
s/2016/2017/ here and in constants.cc

[chfremer, 2017-05-12 23:30:21]

mcasas@: PTAL

https://codereview.chromium.org/2867213004/diff/100001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/2867213004/diff/100001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.cc:428:
std::move(device_task_runner));
On 2017/05/12 22:07:02, mcasas wrote:
> |device_task_runner| is unused if 
> base::FeatureList::IsEnabled(video_capture::kMojoVideoCapture),
> and also a |video_capture_provider| is constructed in l.424 and
> then discarded in l.431.  Please refactor the block in l.414-434.

Excellent point. Thanks for raising this.
Unfortunately this will require changes outside this block, because right now,
we still need the InProcessVideoCaptureProvider instance for the non-device
capture cases. We will probably have to add a new class
InProcessNonDeviceVideoCaptureProvider, which will not require a
device_task_runner. Should we defer this to a follow-up CL or do it as a
PatchSet here?

https://codereview.chromium.org/2867213004/diff/100001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.cc:430: DVLOG(3) <<
"Mojo video capture enabled";
On 2017/05/12 22:07:02, mcasas wrote:
> nit: remove this.

Done.

https://codereview.chromium.org/2867213004/diff/100001/content/browser/render...
File
content/browser/renderer_host/media/service_video_capture_device_launcher.cc
(right):

https://codereview.chromium.org/2867213004/diff/100001/content/browser/render...
content/browser/renderer_host/media/service_video_capture_device_launcher.cc:94:
device_id, std::move(device_request),
On 2017/05/12 22:07:02, mcasas wrote:
> Why this change?

I'll add a comment to the code. TL;DR is this was a bug that surfaced on
Windows. The mojo::MakeRequest(&device) call happened _after_
base::Passed(&device) on line 99. This change is to enforce the correct order.

https://codereview.chromium.org/2867213004/diff/100001/content/browser/render...
File content/browser/renderer_host/media/video_capture_browsertest.cc (right):

https://codereview.chromium.org/2867213004/diff/100001/content/browser/render...
content/browser/renderer_host/media/video_capture_browsertest.cc:219: //
TODO(chfremer): Remove this as soon as crbug.com/720500 is resolved.
On 2017/05/12 22:07:02, mcasas wrote:
> nit: prefix https:// for bug links, here and in l.224, l.247, 251.

Done.

https://codereview.chromium.org/2867213004/diff/100001/content/browser/render...
content/browser/renderer_host/media/video_capture_browsertest.cc:346: Bool())); 
// UseMojoService
On 2017/05/12 22:07:02, mcasas wrote:
> Yay!!

Acknowledged.

https://codereview.chromium.org/2867213004/diff/100001/content/browser/render...
File content/browser/renderer_host/media/video_capture_provider_switcher.cc
(right):

https://codereview.chromium.org/2867213004/diff/100001/content/browser/render...
content/browser/renderer_host/media/video_capture_provider_switcher.cc:54:
std::unique_ptr<VideoCaptureDeviceLauncher> other_types_launcher_;
On 2017/05/12 22:07:02, mcasas wrote:
> Can we make any of these two const?

Done.

https://codereview.chromium.org/2867213004/diff/100001/content/browser/render...
File content/browser/renderer_host/media/video_capture_provider_switcher.h
(right):

https://codereview.chromium.org/2867213004/diff/100001/content/browser/render...
content/browser/renderer_host/media/video_capture_provider_switcher.h:35:
std::unique_ptr<VideoCaptureProvider> other_types_capture_provider_;
On 2017/05/12 22:07:02, mcasas wrote:
> Can we make any of these members const?

Done.

https://codereview.chromium.org/2867213004/diff/100001/services/video_capture...
File services/video_capture/public/cpp/constants.h (right):

https://codereview.chromium.org/2867213004/diff/100001/services/video_capture...
services/video_capture/public/cpp/constants.h:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
On 2017/05/12 22:07:03, mcasas wrote:
> s/2016/2017/ here and in constants.cc

Done.

https://codereview.chromium.org/2867213004/diff/100001/services/video_capture...
File services/video_capture/service_manifest.json (right):

https://codereview.chromium.org/2867213004/diff/100001/services/video_capture...
services/video_capture/service_manifest.json:7: "capture": [ "*" ],
On 2017/05/12 19:29:22, Ken Rockot wrote:
> I guess this is OK since you're just updating the name in this CL, but
> preferably services would be specific about which top-level interfaces are
> exposed by each capability (testing capabilities excluded).

Done.

[mcasas, 2017-05-12 23:33:01]

lgtm

https://codereview.chromium.org/2867213004/diff/100001/content/browser/render...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/2867213004/diff/100001/content/browser/render...
content/browser/renderer_host/media/media_stream_manager.cc:428:
std::move(device_task_runner));
On 2017/05/12 23:30:21, chfremer wrote:
> On 2017/05/12 22:07:02, mcasas wrote:
> > |device_task_runner| is unused if 
> > base::FeatureList::IsEnabled(video_capture::kMojoVideoCapture),
> > and also a |video_capture_provider| is constructed in l.424 and
> > then discarded in l.431.  Please refactor the block in l.414-434.
> 
> Excellent point. Thanks for raising this.
> Unfortunately this will require changes outside this block, because right now,
> we still need the InProcessVideoCaptureProvider instance for the non-device
> capture cases. We will probably have to add a new class
> InProcessNonDeviceVideoCaptureProvider, which will not require a
> device_task_runner. Should we defer this to a follow-up CL or do it as a
> PatchSet here?

Follow up CL is fine.

[chfremer, 2017-05-15 18:24:31]

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

[commit-bot: I haz the power, 2017-05-15 18:25:42]

Dry run: CQ is trying da patch.

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

[chfremer, 2017-05-15 18:31:45]

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

[chfremer, 2017-05-15 18:31:46]

piman@: PTAL
tsepez@: Please RS content_browser_manifest.json

[Tom Sepez, 2017-05-15 18:41:21]

lgtm

RS

[piman, 2017-05-15 19:34:32]

LGTM + nits

https://codereview.chromium.org/2867213004/diff/120001/content/browser/render...
File content/browser/renderer_host/media/video_capture_provider_switcher.cc
(right):

https://codereview.chromium.org/2867213004/diff/120001/content/browser/render...
content/browser/renderer_host/media/video_capture_provider_switcher.cc:33:
base::Unretained(media_device_launcher_.get()));
nit: keeping around a callback works, but feels convoluted (and brings questions
about the safety of the base::Unretained). You could just as well just keep a
pointer to the VideoCaptureDeviceLauncher you're launching, or even simply a
bool telling you which one

https://codereview.chromium.org/2867213004/diff/120001/content/browser/render...
File content/browser/renderer_host/media/video_capture_provider_switcher.h
(right):

https://codereview.chromium.org/2867213004/diff/120001/content/browser/render...
content/browser/renderer_host/media/video_capture_provider_switcher.h:28: const
std::vector<media::VideoCaptureDeviceInfo>&)>& result_callback)
nit: should this callback type be a typedef in VideoCaptureProvider?

[commit-bot: I haz the power, 2017-05-15 19:35:01]

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

[commit-bot: I haz the power, 2017-05-15 19:35:02]

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

[chfremer, 2017-05-15 22:07:43]

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

[chfremer, 2017-05-15 22:08:13]

Thanks for the review!

https://codereview.chromium.org/2867213004/diff/120001/content/browser/render...
File content/browser/renderer_host/media/video_capture_provider_switcher.cc
(right):

https://codereview.chromium.org/2867213004/diff/120001/content/browser/render...
content/browser/renderer_host/media/video_capture_provider_switcher.cc:33:
base::Unretained(media_device_launcher_.get()));
On 2017/05/15 19:34:32, piman wrote:
> nit: keeping around a callback works, but feels convoluted (and brings
questions
> about the safety of the base::Unretained). You could just as well just keep a
> pointer to the VideoCaptureDeviceLauncher you're launching, or even simply a
> bool telling you which one

I do not disagree. It appears opinions among reviewers are split on this, see
PatchSet #3. Please allow me to keep it as is in order to not move back and
forth. I am adding comments to explain why the usage of Unretained() is safe.

https://codereview.chromium.org/2867213004/diff/120001/content/browser/render...
File content/browser/renderer_host/media/video_capture_provider_switcher.h
(right):

https://codereview.chromium.org/2867213004/diff/120001/content/browser/render...
content/browser/renderer_host/media/video_capture_provider_switcher.h:28: const
std::vector<media::VideoCaptureDeviceInfo>&)>& result_callback)
On 2017/05/15 19:34:32, piman wrote:
> nit: should this callback type be a typedef in VideoCaptureProvider?

Done.

[commit-bot: I haz the power, 2017-05-15 22:08:37]

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-05-15 23:23:36]

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

[commit-bot: I haz the power, 2017-05-15 23:23:37]

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

[chfremer, 2017-05-16 21:21:45]

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

[commit-bot: I haz the power, 2017-05-16 21:23:13]

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-05-16 23:22:41]

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

[commit-bot: I haz the power, 2017-05-16 23:22:42]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[chfremer, 2017-05-19 16:57:54]

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

[commit-bot: I haz the power, 2017-05-19 16:58:28]

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-05-19 22:06:49]

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

[commit-bot: I haz the power, 2017-05-19 22:06:50]

Dry run: This issue passed the CQ dry run.

[chfremer, 2017-05-19 22:11:57]

The CQ bit was checked by chfremer@chromium.org

[chfremer, 2017-05-19 22:11:57]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
piman@chromium.org, mcasas@chromium.org, tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2867213004/#ps180001
(title: "Rebase to May 19th")

[commit-bot: I haz the power, 2017-05-19 22:12:47]

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-19 22:18:20]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1495231917472390,
"parent_rev": "98ac5521519151b6cbcd4f501bf640f92e3a74e4", "commit_rev":
"d99842d2bfbffba9b80aaa81aced1f1e7ebb39c8"}

[commit-bot: I haz the power, 2017-05-19 22:18:38]

Description was changed from

==========
[Mojo Video Capture] Hook up video capture service behind a feature flag

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

After this CL, the video capture service can be enabled by starting Chrome with
command-line flag --enable-features=MojoVideoCapture.

Changes in this CL:
* Add a base::Feature kMojoVideoCapture to allow switching the service on and
  off.
* Add a class VideoCaptureProviderSwitcher that can route device capture
  requests to the service's VideoCaptureProvider while routing screen capture
  requests to the InProcessVideoCaptureProvider.
* Hook up the VideoCaptureProviderSwitcher in the factory code in
  MediaStreamManager.
* Register video capture service in the service manager context for the Browser
  process.
* Remove build flag "enable_mojo_video_capture", since we want the service to
  be testable in all builds.
* Forward switches for using fake video capture devices to utility processes.
* Add test cases for exercising the service to VideoCaptureBrowserTest.

BUG=584797
TEST=
  content_browsertests --gtest_filter="VideoCaptureBrowserTest.*"

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

to

==========
[Mojo Video Capture] Hook up video capture service behind a feature flag

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

After this CL, the video capture service can be enabled by starting Chrome with
command-line flag --enable-features=MojoVideoCapture.

Changes in this CL:
* Add a base::Feature kMojoVideoCapture to allow switching the service on and
  off.
* Add a class VideoCaptureProviderSwitcher that can route device capture
  requests to the service's VideoCaptureProvider while routing screen capture
  requests to the InProcessVideoCaptureProvider.
* Hook up the VideoCaptureProviderSwitcher in the factory code in
  MediaStreamManager.
* Register video capture service in the service manager context for the Browser
  process.
* Remove build flag "enable_mojo_video_capture", since we want the service to
  be testable in all builds.
* Forward switches for using fake video capture devices to utility processes.
* Add test cases for exercising the service to VideoCaptureBrowserTest.

BUG=584797
TEST=
  content_browsertests --gtest_filter="VideoCaptureBrowserTest.*"

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

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

[commit-bot: I haz the power, 2017-05-19 22:18:40]

Committed patchset #9 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/d99842d2bfbffba9b80aaa81aced...

[tsergeant, 2017-05-22 00:08:13]

A revert of this CL (patchset #9 id:180001) has been created in
https://codereview.chromium.org/2900583002/ by tsergeant@chromium.org.

The reason for reverting is: This CL appears to cause test failures on the
Webkit Android bots for blink_heap_unittests and webkit_unit_tests.

Example failing build:
https://luci-milo.appspot.com/buildbot/chromium.webkit/WebKit%20Android%20%28...

OSError: [Errno 2] No such file or directory:
'/b/c/b/WebKit_Android__Nexus4_/src/out/Release/gen/services/video_capture/manifest.json'.