[Mojo Video Capture] Add support to BuildableVideoCaptureDevice for aborting the device start.

[Mojo Video Capture] Add support to BuildableVideoCaptureDevice for aborting the device start.

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:
* When a request to stop a device comes in while the device is still starting up
  asynchronously, the device needs to be stopped as soon as it has finished
  starting up. This CL moves the logic for this from VideoCaptureManager to
  implementations of BuildableVideoCaptureDevice.
* Some naming improvements

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

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

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

Review-Url: https://codereview.chromium.org/2753073006
Cr-Commit-Position: refs/heads/master@{#461486}
Committed: https://chromium.googlesource.com/chromium/src/+/04f617af016ca074f249a17183a0eb79d7a2ad60

[chfremer, 2017-03-17 20:30:01]

Patchset #1 (id:1) has been deleted

[chfremer, 2017-03-17 20:30:25]

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

[commit-bot: I haz the power, 2017-03-17 20:30:48]

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

[chfremer, 2017-03-17 20:31:45]

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

[chfremer, 2017-03-17 20:31:46]

miu@: PTAL
emircan@: PTAL
mcasas@: FYI

[miu, 2017-03-17 21:48:02]

lgtm % a few things to consider to simplify things:

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

https://codereview.chromium.org/2753073006/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:302:
std::move(buffer_pool),
Note: Mistakes like this can be avoided by biasing towards a more functional
style (i.e., less imperative style):

  return base::MakeUnique<media::VideoCaptureDeviceClient>(
      base::MakeUnique<VideoFrameReceiverOnIOThread>(receiver),
      new media::VideoCaptureBufferPoolImpl(
          base::MakeUnique<media::VideoCaptureBufferTrackerFactoryImpl>(),
          buffer_pool_max_buffer_count),
      base::Bind(&CreateGpuJpegDecoder,
                 base::Bind(&media::VideoFrameReceiver::OnFrameReadyInBuffer,
                            receiver)));

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

https://codereview.chromium.org/2753073006/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:329:
device_start_request_queue_.begin(), device_start_request_queue_.end(),
nit (simplification): Make the 1st arg here begin() + 1, then you don't have to
check request_iter != queue_.begin() below.

https://codereview.chromium.org/2753073006/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:399:
DCHECK_EQ(controller->serial_id(),
OOC, why put a |serial_id| instead of the VCC pointer (i.e., raw pointer) in the
CaptureDeviceStartRequest? Can the VCC be destroyed and a new one possibly
created at the same location in memory before an entry is removed from the
|device_start_request_queue_|?

Also, seems like we could get rid of extra run time overhead if we could avoid
(or eliminate?) calls to LookupControllerBySerialId().

[commit-bot: I haz the power, 2017-03-17 22:19:39]

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

[commit-bot: I haz the power, 2017-03-17 22:19:40]

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

[chfremer, 2017-03-21 22:37:23]

PTAL

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

https://codereview.chromium.org/2753073006/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:302:
std::move(buffer_pool),
On 2017/03/17 21:48:02, miu wrote:
> Note: Mistakes like this can be avoided by biasing towards a more functional
> style (i.e., less imperative style):
> 
>   return base::MakeUnique<media::VideoCaptureDeviceClient>(
>       base::MakeUnique<VideoFrameReceiverOnIOThread>(receiver),
>       new media::VideoCaptureBufferPoolImpl(
>           base::MakeUnique<media::VideoCaptureBufferTrackerFactoryImpl>(),
>           buffer_pool_max_buffer_count),
>       base::Bind(&CreateGpuJpegDecoder,
>                  base::Bind(&media::VideoFrameReceiver::OnFrameReadyInBuffer,
>                             receiver)));

Done.

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

https://codereview.chromium.org/2753073006/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:329:
device_start_request_queue_.begin(), device_start_request_queue_.end(),
On 2017/03/17 21:48:02, miu wrote:
> nit (simplification): Make the 1st arg here begin() + 1, then you don't have
to
> check request_iter != queue_.begin() below.

Done.

Had to do it differently though, because |device_start_request_queue_| can be
empty, and because it is a std::list, so we need to use ++ instead of +1.

https://codereview.chromium.org/2753073006/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:399:
DCHECK_EQ(controller->serial_id(),
On 2017/03/17 21:48:02, miu wrote:
> OOC, why put a |serial_id| instead of the VCC pointer (i.e., raw pointer) in
the
> CaptureDeviceStartRequest? Can the VCC be destroyed and a new one possibly
> created at the same location in memory before an entry is removed from the
> |device_start_request_queue_|?
> 
> Also, seems like we could get rid of extra run time overhead if we could avoid
> (or eliminate?) calls to LookupControllerBySerialId().

I have no idea whether or not the |serial_id| was ever necessary. And I agree
that it appears to not be needed in the current version of the code.

Replacing it with VideoCaptureController* for next patch set.

Done.

[chfremer, 2017-03-21 23:05:28]

mcasas@: PTAL

[miu, 2017-03-22 21:33:28]

still lgtm

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

https://codereview.chromium.org/2753073006/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:329:
device_start_request_queue_.begin(), device_start_request_queue_.end(),
On 2017/03/21 22:37:23, chfremer wrote:
> On 2017/03/17 21:48:02, miu wrote:
> > nit (simplification): Make the 1st arg here begin() + 1, then you don't have
> to
> > check request_iter != queue_.begin() below.
> 
> Done.
> 
> Had to do it differently though, because |device_start_request_queue_| can be
> empty, and because it is a std::list, so we need to use ++ instead of +1.

Acknowledged.

https://codereview.chromium.org/2753073006/diff/40001/content/browser/rendere...
File
content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc
(right):

https://codereview.chromium.org/2753073006/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:68:
std::move(done_cb).Run();
nit: Is the std::move() necessary (and everywhere else you do this at the end of
a method)?

[chfremer, 2017-03-22 22:37:52]

https://codereview.chromium.org/2753073006/diff/40001/content/browser/rendere...
File
content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc
(right):

https://codereview.chromium.org/2753073006/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:68:
std::move(done_cb).Run();
On 2017/03/22 21:33:28, miu wrote:
> nit: Is the std::move() necessary (and everywhere else you do this at the end
of
> a method)?

Yes. I believe this is the only way to invoke Run() on a OnceCallback. It is
requiring a non-const R-value and otherwise hits a static_assert.

[mcasas, 2017-03-24 21:54:36]

lgtm

https://codereview.chromium.org/2753073006/diff/40001/content/browser/rendere...
File
content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc
(right):

https://codereview.chromium.org/2753073006/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:102:
DCHECK(state_ == State::NO_DEVICE);
DCHECK_EQ(State::NO_DEVICE, state_) ?
Not sure if enum classes have operator<<,
if not, then this construction won't work and
you should disregard this comment :-)

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

https://codereview.chromium.org/2753073006/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.h:120: void
ResumeCaptureForClient(
nit: Should we rename also {Pause,Resume}CaptureForClient
to {Pause,Resume}Client (resp.) ?

https://codereview.chromium.org/2753073006/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.h:312:
DeviceStartQueue device_start_request_queue_;
DeviceStartQueue is just a list of CaptureDeviceStartRequest,
which is a way of reinventing the wheel, in this case a 
CancellableTaskTracker [1].  I filed a bug a while ago and
it's still there : https://crbug.com/598465.  Would you mind
adding a TODO() here...? Thanks!


[1]
https://cs.chromium.org/chromium/src/base/task/cancelable_task_tracker.h?sq=p...

[chfremer, 2017-03-31 17:25:38]

Thanks for the review!

https://codereview.chromium.org/2753073006/diff/40001/content/browser/rendere...
File
content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc
(right):

https://codereview.chromium.org/2753073006/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/in_process_buildable_video_capture_device.cc:102:
DCHECK(state_ == State::NO_DEVICE);
On 2017/03/24 21:54:36, mcasas wrote:
> DCHECK_EQ(State::NO_DEVICE, state_) ?
> Not sure if enum classes have operator<<,
> if not, then this construction won't work and
> you should disregard this comment :-)

Done.

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

https://codereview.chromium.org/2753073006/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.h:120: void
ResumeCaptureForClient(
On 2017/03/24 21:54:36, mcasas wrote:
> nit: Should we rename also {Pause,Resume}CaptureForClient
> to {Pause,Resume}Client (resp.) ?

I actually prefer the naming as it is. To me, {Pause,Resume}Client somehow
sounds like we would be changing something inside the Client whereas
{Pause,Resume}CaptureForClient correctly implies that we simply pause/resume
forwarding frames to the Client.

https://codereview.chromium.org/2753073006/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.h:312:
DeviceStartQueue device_start_request_queue_;
On 2017/03/24 21:54:36, mcasas wrote:
> DeviceStartQueue is just a list of CaptureDeviceStartRequest,
> which is a way of reinventing the wheel, in this case a 
> CancellableTaskTracker [1].  I filed a bug a while ago and
> it's still there : https://crbug.com/598465.  Would you mind
> adding a TODO() here...? Thanks!
> 
> 
> [1]
>
https://cs.chromium.org/chromium/src/base/task/cancelable_task_tracker.h?sq=p...

Done.

[chfremer, 2017-03-31 17:25:43]

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

[commit-bot: I haz the power, 2017-03-31 17:26:17]

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 18:04:11]

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

[commit-bot: I haz the power, 2017-03-31 18:04:12]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[chfremer, 2017-04-03 16:41:53]

The CQ bit was checked by chfremer@chromium.org

[chfremer, 2017-04-03 16:41:54]

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

[commit-bot: I haz the power, 2017-04-03 16:42:15]

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-03 18:41:58]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491237713387660,
"parent_rev": "c1d887e1b41c37348a412bcb956b641ef5b5caf1", "commit_rev":
"04f617af016ca074f249a17183a0eb79d7a2ad60"}

[commit-bot: I haz the power, 2017-04-03 18:42:52]

Description was changed from

==========
[Mojo Video Capture] Add support to BuildableVideoCaptureDevice for aborting the
device start.

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:
* When a request to stop a device comes in while the device is still starting up
  asynchronously, the device needs to be stopped as soon as it has finished
  starting up. This CL moves the logic for this from VideoCaptureManager to
  implementations of BuildableVideoCaptureDevice.
* Some naming improvements

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

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

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

to

==========
[Mojo Video Capture] Add support to BuildableVideoCaptureDevice for aborting the
device start.

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:
* When a request to stop a device comes in while the device is still starting up
  asynchronously, the device needs to be stopped as soon as it has finished
  starting up. This CL moves the logic for this from VideoCaptureManager to
  implementations of BuildableVideoCaptureDevice.
* Some naming improvements

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

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

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

Review-Url: https://codereview.chromium.org/2753073006
Cr-Commit-Position: refs/heads/master@{#461486}
Committed:
https://chromium.googlesource.com/chromium/src/+/04f617af016ca074f249a17183a0...
==========

[commit-bot: I haz the power, 2017-04-03 18:42:53]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/04f617af016ca074f249a17183a0...