[Mojo Video Capture] Fix VideoCaptureManager exposing implementation details to clients

[Mojo Video Capture] Fix VideoCaptureManager exposing implementation details to clients

This CL is supposed to be a pure refactoring. There should be no changes to
existing behavior.

This is part of a series of CLs with the goal of separating the class
VideoCaptureManager from knowledge of things that are going to be private
implementation details of the video capture service. The end goal is to have
VideoCaptureManager talk to an abstraction that can be implemented by either the
video capture Mojo service or the existing (legacy) in-process implementation.

Changes in this CL:
* In VideoCaptureManager, remove getters/setters that expose implementation
  details.
* In MediaStreamManager, expose a set of static factory methods that allow
  creating of custom-configured instances as needed by clients (as was
  previously done post-construction using getters and setters).
* Update usage sites
* Split class VideoCaptureSystem into interface VideoCaptureSystem and
  implementation VideoCaptureSystemImpl. This is to enable testing
  (MediaStreamDispatcherHostTest) to use a mock implementation instead of
  requiring the existance of a VideoCaptureDeviceFactory.

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

BUG=584797
TEST=
  capture_unittests --gtest_filter="*Video*"
  content_unittests --gtest_filter="*Video*"
  content_unittests --gtest_filter="*Media*"
  content_unittests --gtest_filter="*Audio*"
  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/2787703004
Cr-Commit-Position: refs/heads/master@{#463080}
Committed: https://chromium.googlesource.com/chromium/src/+/76a5bd00357df1a3a0274658a43dabfe277e0382

[chfremer, 2017-03-30 19:51:11]

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

[chfremer, 2017-03-30 19:51:59]

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

[chfremer, 2017-03-30 19:52:00]

miu@: PTAL

[commit-bot: I haz the power, 2017-03-30 19:52:04]

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-30 19:56:41]

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

[commit-bot: I haz the power, 2017-03-30 19:56:50]

Dry run: 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...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[chfremer, 2017-03-30 23:23:12]

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

[commit-bot: I haz the power, 2017-03-30 23:24:39]

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-31 00:11:47]

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

[commit-bot: I haz the power, 2017-03-31 00:11:48]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[chfremer, 2017-03-31 22:51:50]

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

[commit-bot: I haz the power, 2017-03-31 22:52:31]

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-04-01 00:34:34]

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

[commit-bot: I haz the power, 2017-04-01 00:34:35]

Dry run: This issue passed the CQ dry run.

[chfremer, 2017-04-03 18:49:03]

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

[chfremer, 2017-04-03 18:49:04]

emircan@: PTAL
mcasas@: Optional

[miu, 2017-04-03 21:31:24]

Comments on PS4:

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

https://codereview.chromium.org/2787703004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:63:
static const char* kRegularVideoDeviceId = "stub_device_0";
Prefer constexpr to static const here.

Also, all these constants (old and new) should be moved into the anonymous
namespace (below).

https://codereview.chromium.org/2787703004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:297:
const media::VideoCaptureApi stub_capture_api =
Seems that this constant defined here, then captured by the lambda, and only
used within the lambda. Could it just be used directly in the lambda? Meaning,
L309 would just be:

  info.descriptor.capture_api = media::VCApi::LINUX_V4L2_SINGLE_PLANE;

https://codereview.chromium.org/2787703004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:471:
media::AudioDeviceDescriptions physical_audio_devices_;
s/physical_audio/stub_audio/ too, or are they really physical during the unit
tests?

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

https://codereview.chromium.org/2787703004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1260: if
(BrowserThread::CurrentlyOn(BrowserThread::IO) == false) {
Usually, the style is:

  if (!BrowserThread::CurrentlyOn(...)) {

(I'm assuming CurrentlyOn() returns bool, and nothing fancy, right?)

https://codereview.chromium.org/2787703004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1307: void
MediaStreamManager::PerformCommonInitializationRoutines() {
After reviewing all these CreateXXX() and InitializeXXX() methods, I'd like to
suggest something simpler: Just do it all in the ctor, like it was being done
before. However, use a 3-argument ctor where the VideoCaptureSystem and
TaskRunner arguments are optional. Something like:

  // |audio_system| is required, but defaults will be used if either
  // |video_capture_system| or |device_task_runner| are null.
  MediaStreamManager(media::AudioSystem* audio_system,
                     std::unique_ptr<media::VideoCaptureSystem>
video_capture_system,
                     scoped_refptr<base::SingleThreadTaskRunner>
device_task_runner);

Then, in the .cc file, there's just one, linear, easy-to-follow workflow where,
somewhere we have:

  if (!video_capture_system) {
    video_capture_system = base::MakeUnique<media::VideoCaptureSystemImpl>(
        media::VideoCaptureDeviceFactory::CreateFactory(
            BrowserThread::GetTaskRunnerForThread(BrowserThread::UI)));
  }

  if (!device_task_runner) {
#if defined(OS_WIN)
    // Use an STA Video Capture Thread to try to avoid crashes on enumeration
of	
    // buggy third party Direct Show modules, http://crbug.com/428958.	
    video_capture_thread_.init_com_with_mta(false);	
    CHECK(video_capture_thread_.Start());
    device_task_runner = video_capture_thread_.task_runner();
#else
    device_task_runner = audio_system_->GetTaskRunner();
#endif
  }

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

https://codereview.chromium.org/2787703004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.h:89: static
std::unique_ptr<MediaStreamManager> CreateWithDefaults(
Perhaps include a comment about threading/task-runners to be assumed here?

https://codereview.chromium.org/2787703004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.h:93:
CreateWithCustomVideoCaptureDeviceTaskRunner(
Looks like nothing calls this. So, can we delete it?

https://codereview.chromium.org/2787703004/diff/60001/media/capture/video/vid...
File media/capture/video/video_capture_system.cc (right):

https://codereview.chromium.org/2787703004/diff/60001/media/capture/video/vid...
media/capture/video/video_capture_system.cc:1: // Copyright 2017 The Chromium
Authors. All rights reserved.
ditto: video_capture_system_impl.cc?

https://codereview.chromium.org/2787703004/diff/60001/media/capture/video/vid...
File media/capture/video/video_capture_system.h (right):

https://codereview.chromium.org/2787703004/diff/60001/media/capture/video/vid...
media/capture/video/video_capture_system.h:31: class CAPTURE_EXPORT
VideoCaptureSystemImpl : public VideoCaptureSystem {
Should this be in a separate video_capture_system_impl.h file?

[chfremer, 2017-04-04 21:55:42]

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

[commit-bot: I haz the power, 2017-04-04 21:56:53]

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-04 21:59:32]

miu@: PTAL

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

https://codereview.chromium.org/2787703004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:63:
static const char* kRegularVideoDeviceId = "stub_device_0";
On 2017/04/03 21:31:23, miu wrote:
> Prefer constexpr to static const here.
> 
> Also, all these constants (old and new) should be moved into the anonymous
> namespace (below).

Done.

https://codereview.chromium.org/2787703004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:297:
const media::VideoCaptureApi stub_capture_api =
On 2017/04/03 21:31:23, miu wrote:
> Seems that this constant defined here, then captured by the lambda, and only
> used within the lambda. Could it just be used directly in the lambda? Meaning,
> L309 would just be:
> 
>   info.descriptor.capture_api = media::VCApi::LINUX_V4L2_SINGLE_PLANE;

Done.

https://codereview.chromium.org/2787703004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc:471:
media::AudioDeviceDescriptions physical_audio_devices_;
On 2017/04/03 21:31:23, miu wrote:
> s/physical_audio/stub_audio/ too, or are they really physical during the unit
> tests?

I had the same question, but since it is fairly non-obvious to figure out, I
decided to not touch this. I believe what is being used is "fake" audio devices.

Renaming to "audio_device_descriptions_".

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

https://codereview.chromium.org/2787703004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1260: if
(BrowserThread::CurrentlyOn(BrowserThread::IO) == false) {
On 2017/04/03 21:31:23, miu wrote:
> Usually, the style is:
> 
>   if (!BrowserThread::CurrentlyOn(...)) {
> 
> (I'm assuming CurrentlyOn() returns bool, and nothing fancy, right?)

Done.

https://codereview.chromium.org/2787703004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:1307: void
MediaStreamManager::PerformCommonInitializationRoutines() {
On 2017/04/03 21:31:23, miu wrote:
> After reviewing all these CreateXXX() and InitializeXXX() methods, I'd like to
> suggest something simpler: Just do it all in the ctor, like it was being done
> before. However, use a 3-argument ctor where the VideoCaptureSystem and
> TaskRunner arguments are optional. Something like:
> 
>   // |audio_system| is required, but defaults will be used if either
>   // |video_capture_system| or |device_task_runner| are null.
>   MediaStreamManager(media::AudioSystem* audio_system,
>                      std::unique_ptr<media::VideoCaptureSystem>
> video_capture_system,
>                      scoped_refptr<base::SingleThreadTaskRunner>
> device_task_runner);
> 
> Then, in the .cc file, there's just one, linear, easy-to-follow workflow
where,
> somewhere we have:
> 
>   if (!video_capture_system) {
>     video_capture_system = base::MakeUnique<media::VideoCaptureSystemImpl>(
>         media::VideoCaptureDeviceFactory::CreateFactory(
>             BrowserThread::GetTaskRunnerForThread(BrowserThread::UI)));
>   }
> 
>   if (!device_task_runner) {
> #if defined(OS_WIN)
>     // Use an STA Video Capture Thread to try to avoid crashes on enumeration
> of	
>     // buggy third party Direct Show modules, http://crbug.com/428958.	
>     video_capture_thread_.init_com_with_mta(false);	
>     CHECK(video_capture_thread_.Start());
>     device_task_runner = video_capture_thread_.task_runner();
> #else
>     device_task_runner = audio_system_->GetTaskRunner();
> #endif
>   }

Thanks. That allows eliminating a lot of ugliness and duplication.
I want to note, though, that it changes some threading. Before, the constructor
of VideoCaptureSystemImpl and the static method
media::VideoCaptureDeviceFactory::CreateFactory() were executed on the
Browser::IO thread whereas after the simplification they are run on whichever
thread calls the constructor of MediaStreamManager (which is Browser Main thread
in production). But unless it causes any test failures, I assume that this is
not a problem.

Done.

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

https://codereview.chromium.org/2787703004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.h:89: static
std::unique_ptr<MediaStreamManager> CreateWithDefaults(
On 2017/04/03 21:31:23, miu wrote:
> Perhaps include a comment about threading/task-runners to be assumed here?

I wouldn't know what to say here. As a reader, what kind of information would
you be looking for here that would not have been given in the class-level
description?

I don't feel I understand the class MediaStreamManager enough to attempt
explaining it in a class-level comment.

https://codereview.chromium.org/2787703004/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.h:93:
CreateWithCustomVideoCaptureDeviceTaskRunner(
On 2017/04/03 21:31:23, miu wrote:
> Looks like nothing calls this. So, can we delete it?

Done.

https://codereview.chromium.org/2787703004/diff/60001/media/capture/video/vid...
File media/capture/video/video_capture_system.cc (right):

https://codereview.chromium.org/2787703004/diff/60001/media/capture/video/vid...
media/capture/video/video_capture_system.cc:1: // Copyright 2017 The Chromium
Authors. All rights reserved.
On 2017/04/03 21:31:24, miu wrote:
> ditto: video_capture_system_impl.cc?

Done.

https://codereview.chromium.org/2787703004/diff/60001/media/capture/video/vid...
File media/capture/video/video_capture_system.h (right):

https://codereview.chromium.org/2787703004/diff/60001/media/capture/video/vid...
media/capture/video/video_capture_system.h:31: class CAPTURE_EXPORT
VideoCaptureSystemImpl : public VideoCaptureSystem {
On 2017/04/03 21:31:24, miu wrote:
> Should this be in a separate video_capture_system_impl.h file?

Done.

[commit-bot: I haz the power, 2017-04-05 01:59:38]

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

[commit-bot: I haz the power, 2017-04-05 01:59:39]

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...)

[miu, 2017-04-05 21:10:00]

PS6 lgtm.

I see your point about constructing objects on the UI thread instead of the IO
thread. If you haven't already, you might take a quick look at the ctors to make
sure there isn't anything expensive, or required to be done on the IO thread
(such as ThreadCheckers?). IIRC, MediaStreamManager is part of the browser
start-up process; so if there is anything expensive in its ctor, it will
effectively make Chrome take longer to start for users. If you find it needs to
happen on the IO thread, you could std::move() the pointers to the base::Bind()
for the initialization task on the IO thread, and do the null-or-default code
there.

[chfremer, 2017-04-05 23:33:34]

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

[commit-bot: I haz the power, 2017-04-05 23:34:22]

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-04-06 01:16:42]

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

[commit-bot: I haz the power, 2017-04-06 01:16:43]

Dry run: This issue passed the CQ dry run.

[emircan, 2017-04-06 20:56:54]

lgtm

[chfremer, 2017-04-07 17:18:28]

On 2017/04/05 21:10:00, miu wrote:
> PS6 lgtm.
> 
> I see your point about constructing objects on the UI thread instead of the IO
> thread. If you haven't already, you might take a quick look at the ctors to
make
> sure there isn't anything expensive, or required to be done on the IO thread
> (such as ThreadCheckers?). IIRC, MediaStreamManager is part of the browser
> start-up process; so if there is anything expensive in its ctor, it will
> effectively make Chrome take longer to start for users. If you find it needs
to
> happen on the IO thread, you could std::move() the pointers to the
base::Bind()
> for the initialization task on the IO thread, and do the null-or-default code
> there.

I checked the constructors and found they essentially do nothing (which is
good).
In that case, running them on the Main thread should not be much of a concern.

[chfremer, 2017-04-07 21:30:57]

The CQ bit was checked by chfremer@chromium.org

[chfremer, 2017-04-07 21:30:57]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org
Link to the patchset: https://codereview.chromium.org/2787703004/#ps120001
(title: "Rebase to April 5")

[commit-bot: I haz the power, 2017-04-07 21:31:40]

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-04-08 00:48:48]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491600657262530,
"parent_rev": "feb3facf41216b12b606d85ba64c892168b101b1", "commit_rev":
"76a5bd00357df1a3a0274658a43dabfe277e0382"}

[commit-bot: I haz the power, 2017-04-08 00:49:46]

Description was changed from

==========
[Mojo Video Capture] Fix VideoCaptureManager exposing implementation details to
clients

This CL is supposed to be a pure refactoring. There should be no changes to
existing behavior.

This is part of a series of CLs with the goal of separating the class
VideoCaptureManager from knowledge of things that are going to be private
implementation details of the video capture service. The end goal is to have
VideoCaptureManager talk to an abstraction that can be implemented by either the
video capture Mojo service or the existing (legacy) in-process implementation.

Changes in this CL:
* In VideoCaptureManager, remove getters/setters that expose implementation
  details.
* In MediaStreamManager, expose a set of static factory methods that allow
  creating of custom-configured instances as needed by clients (as was
  previously done post-construction using getters and setters).
* Update usage sites
* Split class VideoCaptureSystem into interface VideoCaptureSystem and
  implementation VideoCaptureSystemImpl. This is to enable testing
  (MediaStreamDispatcherHostTest) to use a mock implementation instead of
  requiring the existance of a VideoCaptureDeviceFactory.

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

BUG=584797
TEST=
  capture_unittests --gtest_filter="*Video*"
  content_unittests --gtest_filter="*Video*"
  content_unittests --gtest_filter="*Media*"
  content_unittests --gtest_filter="*Audio*"
  content_browsertests --gtest_filter="VideoCaptureBrowserTest.*"

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

to

==========
[Mojo Video Capture] Fix VideoCaptureManager exposing implementation details to
clients

This CL is supposed to be a pure refactoring. There should be no changes to
existing behavior.

This is part of a series of CLs with the goal of separating the class
VideoCaptureManager from knowledge of things that are going to be private
implementation details of the video capture service. The end goal is to have
VideoCaptureManager talk to an abstraction that can be implemented by either the
video capture Mojo service or the existing (legacy) in-process implementation.

Changes in this CL:
* In VideoCaptureManager, remove getters/setters that expose implementation
  details.
* In MediaStreamManager, expose a set of static factory methods that allow
  creating of custom-configured instances as needed by clients (as was
  previously done post-construction using getters and setters).
* Update usage sites
* Split class VideoCaptureSystem into interface VideoCaptureSystem and
  implementation VideoCaptureSystemImpl. This is to enable testing
  (MediaStreamDispatcherHostTest) to use a mock implementation instead of
  requiring the existance of a VideoCaptureDeviceFactory.

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

BUG=584797
TEST=
  capture_unittests --gtest_filter="*Video*"
  content_unittests --gtest_filter="*Video*"
  content_unittests --gtest_filter="*Media*"
  content_unittests --gtest_filter="*Audio*"
  content_browsertests --gtest_filter="VideoCaptureBrowserTest.*"

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

Review-Url: https://codereview.chromium.org/2787703004
Cr-Commit-Position: refs/heads/master@{#463080}
Committed:
https://chromium.googlesource.com/chromium/src/+/76a5bd00357df1a3a0274658a43d...
==========

[commit-bot: I haz the power, 2017-04-08 00:49:47]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/76a5bd00357df1a3a0274658a43d...