[Mojo Video Capture] Split interface BuildableVideoCaptureDevice

[Mojo Video Capture] Split interface BuildableVideoCaptureDevice into “device launcher” and “launched device”

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

Changes in this CL:
* Split interface BuildableVideoCaptureDevice into interfaces 
  VideoCaptureDeviceLauncher and LaunchedVideoCaptureDevice.
* Split implementations accordingly.
* VideoCaptureController is freed from knowledge of 
  VideoFrameConsumerFeedbackObserver and VideoCaptureDeviceLauncher::Callbacks.
* Moved mock implementations to a separate file, since they are used by more 
  than one test.

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

BUG=584797, 711754
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/2803483003
Cr-Commit-Position: refs/heads/master@{#464935}
Committed: https://chromium.googlesource.com/chromium/src/+/c914815faeda1e930178e47c3458d387e30b8876

[chfremer, 2017-04-04 23:01:41]

Patchset #1 (id:1) has been deleted

[chfremer, 2017-04-04 23:01:46]

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

[commit-bot: I haz the power, 2017-04-04 23:03: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-04-04 23:06:46]

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

[commit-bot: I haz the power, 2017-04-04 23:06:46]

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_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chfremer, 2017-04-04 23:27:34]

Description was changed from

==========
[Mojo Video Capture] Split interface BuildableVideoCaptureDevice into “device
launcher” and “launched device”
==========

to

==========
[Mojo Video Capture] Split interface BuildableVideoCaptureDevice into “device
launcher” and “launched device”

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

Changes in this CL:
* Split interface BuildableVideoCaptureDevice into interfaces 
  VideoCaptureDeviceLauncher and LaunchedVideoCaptureDevice.
* Split implementations accordingly.
* VideoCaptureController is freed from knowledge of 
  VideoFrameConsumerFeedbackObserver and VideoCaptureDeviceLauncher::Callbacks.
* Moved mock implementations to a separate file, since they are used by more 
  than one test.

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

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 23:51:39]

Patchset #1 (id:20001) has been deleted

[chfremer, 2017-04-04 23:51:46]

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

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

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:59:58]

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

[commit-bot: I haz the power, 2017-04-04 23:59:58]

Dry run: Try jobs failed on following builders:
  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:54:31]

Patchset #2 (id:60001) has been deleted

[chfremer, 2017-04-07 17:54:56]

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:55:34]

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-07 17:59:23]

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

[commit-bot: I haz the power, 2017-04-07 17:59:24]

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...)
  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 18:31:22]

Patchset #1 (id:40001) has been deleted

[chfremer, 2017-04-07 18:31:30]

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

[commit-bot: I haz the power, 2017-04-07 18:32:30]

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-07 18:49:17]

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

[commit-bot: I haz the power, 2017-04-07 18:49:17]

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

[chfremer, 2017-04-07 19:36:11]

Patchset #1 (id:80001) has been deleted

[chfremer, 2017-04-07 19:36:21]

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

[commit-bot: I haz the power, 2017-04-07 19:37: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-07 20:39:12]

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

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

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-07 20:54:14]

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

[chfremer, 2017-04-07 20:54:20]

Patchset #1 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-04-07 20:54:43]

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-07 22:38:26]

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

[commit-bot: I haz the power, 2017-04-07 22:38:27]

Dry run: This issue passed the CQ dry run.

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

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

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

Patchset #1 (id:120001) has been deleted

[commit-bot: I haz the power, 2017-04-10 22:41:33]

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-10 22:41:55]

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

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

emircan@: PTAL
miu@: optional
mcasas@: optional

[commit-bot: I haz the power, 2017-04-10 23:56:04]

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

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

Dry run: This issue passed the CQ dry run.

[chfremer, 2017-04-14 16:01:56]

On 2017/04/10 22:41:56, chfremer wrote:
> emircan@: PTAL
> miu@: optional
> mcasas@: optional

ping

[chfremer, 2017-04-14 16:50:42]

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

[commit-bot: I haz the power, 2017-04-14 16:50:57]

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

[emircan, 2017-04-14 17:19:38]

https://codereview.chromium.org/2803483003/diff/140001/content/browser/render...
File
content/browser/renderer_host/media/in_process_launched_video_capture_device.cc
(right):

https://codereview.chromium.org/2803483003/diff/140001/content/browser/render...
content/browser/renderer_host/media/in_process_launched_video_capture_device.cc:54:
StopAndReleaseDeviceOnDeviceThread(device_ptr,
I don't think we can have this fallback. Consider the case where the task on
l.147 is run on device thread after this is runs on IO thread. |device| would be
deleted by then. Either we should attempt posting a task on
|device_task_runner_| again or expect not to have any fallback case.

https://codereview.chromium.org/2803483003/diff/140001/content/browser/render...
File
content/browser/renderer_host/media/in_process_video_capture_device_launcher.cc
(right):

https://codereview.chromium.org/2803483003/diff/140001/content/browser/render...
content/browser/renderer_host/media/in_process_video_capture_device_launcher.cc:85:
// Use of |this| is safe, because |context_reference| guarantees that |this|
Where is |context_reference|?

https://codereview.chromium.org/2803483003/diff/140001/content/browser/render...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://codereview.chromium.org/2803483003/diff/140001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.h:117: //
Implementation of VideoCaptureDeviceLauncherCallbacks interface:
VideoCaptureDeviceLauncher::Callbacks

[commit-bot: I haz the power, 2017-04-14 17:47:19]

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

[commit-bot: I haz the power, 2017-04-14 17:47:20]

Dry run: This issue passed the CQ dry run.

[chfremer, 2017-04-14 20:00:23]

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

[chfremer, 2017-04-14 20:00:30]

PTAL

https://codereview.chromium.org/2803483003/diff/140001/content/browser/render...
File
content/browser/renderer_host/media/in_process_launched_video_capture_device.cc
(right):

https://codereview.chromium.org/2803483003/diff/140001/content/browser/render...
content/browser/renderer_host/media/in_process_launched_video_capture_device.cc:54:
StopAndReleaseDeviceOnDeviceThread(device_ptr,
On 2017/04/14 17:19:37, emircan wrote:
> I don't think we can have this fallback. Consider the case where the task on
> l.147 is run on device thread after this is runs on IO thread. |device| would
be
> deleted by then. Either we should attempt posting a task on
> |device_task_runner_| again or expect not to have any fallback case.

I am also skeptical as to whether or not this fallback works. I just kept it
around in order to not change existing behavior. I propose we add a crbug entry
to either add test coverage for this fallback or remove it. Added a comment for
this. See crbug.com/711754.

Regarding the concrete case that you mention where a task is posted on l.147
after this destructor is run: Wouldn't this be a use-after-free, which would be
an issue in the code using this class (as opposed to being related to this
fallback mechanism)?

https://codereview.chromium.org/2803483003/diff/140001/content/browser/render...
File
content/browser/renderer_host/media/in_process_video_capture_device_launcher.cc
(right):

https://codereview.chromium.org/2803483003/diff/140001/content/browser/render...
content/browser/renderer_host/media/in_process_video_capture_device_launcher.cc:85:
// Use of |this| is safe, because |context_reference| guarantees that |this|
On 2017/04/14 17:19:38, emircan wrote:
> Where is |context_reference|?

Sorry about the outdated comment. Changed to |done_cb|. Also found two more of
those and fixed them.

https://codereview.chromium.org/2803483003/diff/140001/content/browser/render...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://codereview.chromium.org/2803483003/diff/140001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.h:117: //
Implementation of VideoCaptureDeviceLauncherCallbacks interface:
On 2017/04/14 17:19:38, emircan wrote:
> VideoCaptureDeviceLauncher::Callbacks

Done.

[commit-bot: I haz the power, 2017-04-14 20:00:41]

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

[emircan, 2017-04-14 20:15:21]

https://codereview.chromium.org/2803483003/diff/140001/content/browser/render...
File
content/browser/renderer_host/media/in_process_launched_video_capture_device.cc
(right):

https://codereview.chromium.org/2803483003/diff/140001/content/browser/render...
content/browser/renderer_host/media/in_process_launched_video_capture_device.cc:54:
StopAndReleaseDeviceOnDeviceThread(device_ptr,
On 2017/04/14 20:00:30, chfremer wrote:
> On 2017/04/14 17:19:37, emircan wrote:
> > I don't think we can have this fallback. Consider the case where the task on
> > l.147 is run on device thread after this is runs on IO thread. |device|
would
> be
> > deleted by then. Either we should attempt posting a task on
> > |device_task_runner_| again or expect not to have any fallback case.
> 
> I am also skeptical as to whether or not this fallback works. I just kept it
> around in order to not change existing behavior. I propose we add a crbug
entry
> to either add test coverage for this fallback or remove it. Added a comment
for
> this. See crbug.com/711754.
> 
> Regarding the concrete case that you mention where a task is posted on l.147
> after this destructor is run: Wouldn't this be a use-after-free, which would
be
> an issue in the code using this class (as opposed to being related to this
> fallback mechanism)?

I think we should just remove it. As you described it will be a use after free
and crash as |device| might be released. Then, it is no longer valid to use
Unretained on l.147 which is introduced in this CL.

[chfremer, 2017-04-14 21:02:54]

Description was changed from

==========
[Mojo Video Capture] Split interface BuildableVideoCaptureDevice into “device
launcher” and “launched device”

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

Changes in this CL:
* Split interface BuildableVideoCaptureDevice into interfaces 
  VideoCaptureDeviceLauncher and LaunchedVideoCaptureDevice.
* Split implementations accordingly.
* VideoCaptureController is freed from knowledge of 
  VideoFrameConsumerFeedbackObserver and VideoCaptureDeviceLauncher::Callbacks.
* Moved mock implementations to a separate file, since they are used by more 
  than one test.

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

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] Split interface BuildableVideoCaptureDevice into “device
launcher” and “launched device”

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

Changes in this CL:
* Split interface BuildableVideoCaptureDevice into interfaces 
  VideoCaptureDeviceLauncher and LaunchedVideoCaptureDevice.
* Split implementations accordingly.
* VideoCaptureController is freed from knowledge of 
  VideoFrameConsumerFeedbackObserver and VideoCaptureDeviceLauncher::Callbacks.
* Moved mock implementations to a separate file, since they are used by more 
  than one test.

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

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

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

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

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

[commit-bot: I haz the power, 2017-04-14 21:04:16]

Dry run: This issue passed the CQ dry run.

[chfremer, 2017-04-14 21:07:12]

PTAL

https://codereview.chromium.org/2803483003/diff/140001/content/browser/render...
File
content/browser/renderer_host/media/in_process_launched_video_capture_device.cc
(right):

https://codereview.chromium.org/2803483003/diff/140001/content/browser/render...
content/browser/renderer_host/media/in_process_launched_video_capture_device.cc:54:
StopAndReleaseDeviceOnDeviceThread(device_ptr,
On 2017/04/14 20:15:20, emircan wrote:
> On 2017/04/14 20:00:30, chfremer wrote:
> > On 2017/04/14 17:19:37, emircan wrote:
> > > I don't think we can have this fallback. Consider the case where the task
on
> > > l.147 is run on device thread after this is runs on IO thread. |device|
> would
> > be
> > > deleted by then. Either we should attempt posting a task on
> > > |device_task_runner_| again or expect not to have any fallback case.
> > 
> > I am also skeptical as to whether or not this fallback works. I just kept it
> > around in order to not change existing behavior. I propose we add a crbug
> entry
> > to either add test coverage for this fallback or remove it. Added a comment
> for
> > this. See crbug.com/711754.
> > 
> > Regarding the concrete case that you mention where a task is posted on l.147
> > after this destructor is run: Wouldn't this be a use-after-free, which would
> be
> > an issue in the code using this class (as opposed to being related to this
> > fallback mechanism)?
> 
> I think we should just remove it. As you described it will be a use after free
> and crash as |device| might be released. Then, it is no longer valid to use
> Unretained on l.147 which is introduced in this CL. 

Done.

[chfremer, 2017-04-14 21:07:19]

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

[commit-bot: I haz the power, 2017-04-14 21:07:48]

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

[emircan, 2017-04-14 21:21:17]

lgtm

[chfremer, 2017-04-14 21:26:55]

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

[chfremer, 2017-04-14 21:26:56]

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

[commit-bot: I haz the power, 2017-04-14 22:19:41]

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

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

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2017-04-15 00:00:58]

lgtm

stampity stamp

[chfremer, 2017-04-17 16:11:05]

The CQ bit was checked by chfremer@chromium.org

[commit-bot: I haz the power, 2017-04-17 16:11:23]

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-17 17:10:31]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1492445465027510,
"parent_rev": "0e7bbf149b075810960c71f8eb873d08f3984b89", "commit_rev":
"c914815faeda1e930178e47c3458d387e30b8876"}

[commit-bot: I haz the power, 2017-04-17 17:10:42]

Description was changed from

==========
[Mojo Video Capture] Split interface BuildableVideoCaptureDevice into “device
launcher” and “launched device”

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

Changes in this CL:
* Split interface BuildableVideoCaptureDevice into interfaces 
  VideoCaptureDeviceLauncher and LaunchedVideoCaptureDevice.
* Split implementations accordingly.
* VideoCaptureController is freed from knowledge of 
  VideoFrameConsumerFeedbackObserver and VideoCaptureDeviceLauncher::Callbacks.
* Moved mock implementations to a separate file, since they are used by more 
  than one test.

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

BUG=584797, 711754
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] Split interface BuildableVideoCaptureDevice into “device
launcher” and “launched device”

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

Changes in this CL:
* Split interface BuildableVideoCaptureDevice into interfaces 
  VideoCaptureDeviceLauncher and LaunchedVideoCaptureDevice.
* Split implementations accordingly.
* VideoCaptureController is freed from knowledge of 
  VideoFrameConsumerFeedbackObserver and VideoCaptureDeviceLauncher::Callbacks.
* Moved mock implementations to a separate file, since they are used by more 
  than one test.

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

BUG=584797, 711754
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/2803483003
Cr-Commit-Position: refs/heads/master@{#464935}
Committed:
https://chromium.googlesource.com/chromium/src/+/c914815faeda1e930178e47c3458...
==========

[commit-bot: I haz the power, 2017-04-17 17:10:45]

Committed patchset #4 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/c914815faeda1e930178e47c3458...