[Mojo Video Capture] Create abstraction VideoCaptureProvider

[Mojo Video Capture] Create abstraction VideoCaptureProvider

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:
* Create abstraction VideoCaptureProvider, whose purpose is to hide the details 
  of how device enumeration is performed and how a BuildableVideoCaptureDevice 
  is created from class VideoCaptureManager.
* Create an implementation InProcessVideoCaptureProvider which encapsulates the 
  existing functionality for hosting devices in the browser process.

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

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

[chfremer, 2017-04-04 22:39:28]

Description was changed from

==========
Create abstraction VideoCaptureProvider
==========

to

==========
[Mojo Video Capture] Create abstraction VideoCaptureProvider

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:
* Create abstraction VideoCaptureProvider, whose purpose is to hide the details 
  of how device enumeration is performed and how a BuildableVideoCaptureDevice 
  is created from class VideoCaptureManager.
* Create an implementation InProcessVideoCaptureProvider which encapsulates the 
  existing functionality for hosting devices in the browser process.

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

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

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

[chfremer, 2017-04-04 22:41:28]

Patchset #1 (id:1) has been deleted

[chfremer, 2017-04-04 22:41:41]

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

[commit-bot: I haz the power, 2017-04-04 22:42:20]

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-04 23:50:52]

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

[commit-bot: I haz the power, 2017-04-04 23:50:52]

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-04-05 21:30:16]

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

[chfremer, 2017-04-05 21:30:17]

miu@: PTAL

[chfremer, 2017-04-05 23:22:03]

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:22:48]

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-05 23:26:02]

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

[commit-bot: I haz the power, 2017-04-05 23:26:03]

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-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-04-05 23:38:01]

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:38: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-04-05 23:43:02]

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

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

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-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...)
  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-04-07 17:51:02]

Patchset #1 (id:20001) has been deleted

[chfremer, 2017-04-07 17:51:09]

Patchset #1 (id:40001) has been deleted

[chfremer, 2017-04-07 17:51:20]

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

[commit-bot: I haz the power, 2017-04-07 17:51: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-07 17:51:58]

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

[chfremer, 2017-04-07 17:51:58]

emircan@: PTAL
mcasas@: FYI

[commit-bot: I haz the power, 2017-04-07 20:13:21]

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

[commit-bot: I haz the power, 2017-04-07 20:13:22]

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-04-10 16:33:35]

On 2017/04/07 17:51:58, chfremer wrote:
> emircan@: PTAL
> mcasas@: FYI

ping

[emircan, 2017-04-10 20:48:13]

https://codereview.chromium.org/2800493002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/in_process_video_capture_provider.cc
(right):

https://codereview.chromium.org/2800493002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/in_process_video_capture_provider.cc:17:
InProcessVideoCaptureProvider::~InProcessVideoCaptureProvider() = default;
This class is owned by VideoCaptureManager. Since it is a scoped_refptr, I
couldn't exactly pinpoint where it is destrcuted and this class is destructed.
Can you write more info on this threading model?

https://codereview.chromium.org/2800493002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/in_process_video_capture_provider.cc:24:
base::Unretained(video_capture_system_.get()),
Add a comment why it is safe to use Unretained. AFAICT, VideoCaptureManager adds
a reference to itself in |result_callback| and makes sure itself and this class
is alive until this call is completed. However, if there is any other client of
this class, that wouldn't be intuitive. 

My suggestion would be calling like below to guarantee safety.
device_task_runner_->DeleteSoon(FROM_HERE, device_task_runner_.release());

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

https://codereview.chromium.org/2800493002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.h:69: // using
MediaStreamRequester.
Can you add information about threading? This class is constructed and
destructed on browser main thread.

https://codereview.chromium.org/2800493002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://codereview.chromium.org/2800493002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.h:47: //
VideoCaptureManager opens/closes and start/stops video capture devices.
Can you add information about threading?

[chfremer, 2017-04-10 22:24:44]

PTAL

https://codereview.chromium.org/2800493002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/in_process_video_capture_provider.cc
(right):

https://codereview.chromium.org/2800493002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/in_process_video_capture_provider.cc:17:
InProcessVideoCaptureProvider::~InProcessVideoCaptureProvider() = default;
On 2017/04/10 20:48:12, emircan wrote:
> This class is owned by VideoCaptureManager. Since it is a scoped_refptr, I
> couldn't exactly pinpoint where it is destrcuted and this class is destructed.
> Can you write more info on this threading model? 

The threading model of this class has 3 aspects:
1. It is irrelevant which thread it is constructed or destroyed on.
2. |result_callback| must guarantee that the called instance stays alive during
the asynchronous execution of GetDeviceInfosAsync(). 
3. The instance is not destroyed while any of its methods are executing.

I added a comment to the interface VideoCaptureProvider to express aspect 2.
Aspect 3 is a standard requirement for every class, so it should not require
being called out explicitly. I feel that aspect 1 is already expressed by not
adding any thread checks that would restrict things otherwise, so I am not sure
adding a class-level comment for this would add value. WDYT?

I think the reason you wanted to know which thread calls the destructor is to
check safety of the Unretained() use. With aspect 2. clarified, knowing this
should no longer be needed.

https://codereview.chromium.org/2800493002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/in_process_video_capture_provider.cc:24:
base::Unretained(video_capture_system_.get()),
On 2017/04/10 20:48:12, emircan wrote:
> Add a comment why it is safe to use Unretained. AFAICT, VideoCaptureManager
adds
> a reference to itself in |result_callback| and makes sure itself and this
class
> is alive until this call is completed. However, if there is any other client
of
> this class, that wouldn't be intuitive. 

Done.

> My suggestion would be calling like below to guarantee safety.
> device_task_runner_->DeleteSoon(FROM_HERE, device_task_runner_.release());

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

https://codereview.chromium.org/2800493002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.h:69: // using
MediaStreamRequester.
On 2017/04/10 20:48:12, emircan wrote:
> Can you add information about threading? This class is constructed and
> destructed on browser main thread.

I don't feel I understand the class enough to add a class-level comment.
Also, the browser main thread is not the only thread that calls the constructor
if we count the 7 test environments where it gets constructed as well. I'll try
to write something, but not sure if it is really helpful.

https://codereview.chromium.org/2800493002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://codereview.chromium.org/2800493002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.h:47: //
VideoCaptureManager opens/closes and start/stops video capture devices.
On 2017/04/10 20:48:12, emircan wrote:
> Can you add information about threading?

Done. Moved the file-level comment down to the class-level comment and tried to
improve it a little bit.

[emircan, 2017-04-10 22:28:29]

lgtm

[chfremer, 2017-04-10 22:39:01]

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

[chfremer, 2017-04-10 22:39:02]

piman@: Please RS content/browser/BUILD.gn

[piman, 2017-04-10 23:51:39]

lgtm

[chfremer, 2017-04-11 16:19:13]

The CQ bit was checked by chfremer@chromium.org

[commit-bot: I haz the power, 2017-04-11 16:19:38]

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-11 17:30:55]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491927553688940,
"parent_rev": "d74396ebee830913c09f7d0d143f0d817613b382", "commit_rev":
"3b099810e0616186a6a27798170db53b72c153ef"}

[commit-bot: I haz the power, 2017-04-11 17:31:55]

Description was changed from

==========
[Mojo Video Capture] Create abstraction VideoCaptureProvider

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:
* Create abstraction VideoCaptureProvider, whose purpose is to hide the details 
  of how device enumeration is performed and how a BuildableVideoCaptureDevice 
  is created from class VideoCaptureManager.
* Create an implementation InProcessVideoCaptureProvider which encapsulates the 
  existing functionality for hosting devices in the browser process.

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

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

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

to

==========
[Mojo Video Capture] Create abstraction VideoCaptureProvider

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:
* Create abstraction VideoCaptureProvider, whose purpose is to hide the details 
  of how device enumeration is performed and how a BuildableVideoCaptureDevice 
  is created from class VideoCaptureManager.
* Create an implementation InProcessVideoCaptureProvider which encapsulates the 
  existing functionality for hosting devices in the browser process.

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

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

[commit-bot: I haz the power, 2017-04-11 17:31:56]

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