Reland [Mojo Video Capture] Introduce abstraction VideoCaptureSystem

Reland [Mojo Video Capture] Introduce abstraction VideoCaptureSystem

Link to original CL: https://codereview.chromium.org/2769543002/

PatchSet 1 is state as reviewed previously.
PatchSet 2 applies a fix for the issue described below.

This was reverted because it caused a test flakiness for test 
MediaDevicesDispatcherHostTest.GetVideoInputCapabilities on Windows.

Analysis of the issue:
The CL created a race condition that could cause the test method to exit 
before the exercise had finished executing. What made the difference is that 
VideoCaptureSystem::GetDeviceInfosAsync() invoked by VideoCaptureManager 
now actually runs asynchronous, i.e. it posts invocation of its 
|result_callback| to the end of a message queue. Before this CL, 
VideoCaptureManager directly called 
VideoCaptureDeviceFactory::EnumerateDeviceDescriptors which, despite its 
asynchronous-looking API invoked its callback synchronously. The test code was 
relying on this being the case. After the CL, the following bad sequence could
happen:
1.) Test calls |host_->GetVideoInputCapabilities()| which eventually kicks of 
asynchronous work A on the "device thread".
2.) Test method reaches statement 
|media_stream_manager_->FlushVideoCaptureThreadForTesting()| and blocks until 
the already posted work A on the device thread finishes.
3.) Work A posts new work B to the device thread.
4.) Work A finishes.
5.) FlushVideoCaptureThreadForTesting() returns and the test method exits before
work B has run.

Description of fix:
The way that the test was waiting for the asynchronous exercise to complete was 
based on dangerous (and now incorrect) assumptions of how and where the 
implementation under test internally posts task. The fix is to eliminate these
assumptions and, instead, block the test method until the waited-for callback 
arrives.


Original CL description:

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:
* Move nested struct VideoCaptureManager::DeviceInfo to struct
  media::VideoCaptureDeviceInfo.
* Create abstraction VideoCaptureSystem and move caching of
  VideoCaptureDeviceInfo to there. This allows us to use this functionality in
  both InProcess and MojoService code paths.
* Simplify BuildableDeviceCallbacks interface. With the mapping from device_id
  to descriptor being moved into VideoCaptureSystem, BuildableDeviceCallbacks no
  longer needs to offer a LookupDeviceDescriptor() method.

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

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

[chfremer, 2017-04-05 22:29:04]

Description was changed from

==========
Reland [Mojo Video Capture] Introduce abstraction VideoCaptureSystem

PatchSet 1 is state as reviewed previously.
PatchSet 2 applies a fix for the issue described below.

This was reverted because it caused a test flakiness for test
MediaDevicesDispatcherHostTest.GetVideoInputCapabilities on Windows.

Analysis of the issue:
The CL created a race condition that could cause the test method to exit
before the exercise had finished executing. What made the difference is that
VideoCaptureSystem::GetDeviceInfosAsync() invoked by VideoCaptureManager
now actually runs asynchronous, i.e. it posts invocation of its
|result_callback| to the end of a message queue. Before this CL,
VideoCaptureManager directly called
VideoCaptureDeviceFactory::EnumerateDeviceDescriptors which, despite its
asynchronous-looking API invoked its callback synchronously. The test code was
relying on this being the case. After the CL, the following bad sequence could
happen:
1.) Test calls |host_->GetVideoInputCapabilities()| which eventually kicks of
asynchronous work A on the "device thread".
2.) Test method reaches statement
|media_stream_manager_->FlushVideoCaptureThreadForTesting()| and blocks until
the already posted work A on the device thread finishes.
3.) Work A posts new work B to the device thread.
4.) Work A finishes.
5.) FlushVideoCaptureThreadForTesting() returns and the test method exits before
work B has run.

Description of fix:
The way that the test was waiting for the asynchronous exercise to complete was
based on dangerous (and now incorrect) assumptions of how and where the
implementation under test internally posts task. The fix is to eliminate these
assumptions and, instead, block the test method until the waited-for callback
arrives.


Original CL description:

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:
* Move nested struct VideoCaptureManager::DeviceInfo to struct
  media::VideoCaptureDeviceInfo.
* Create abstraction VideoCaptureSystem and move caching of
  VideoCaptureDeviceInfo to there. This allows us to use this functionality in
  both InProcess and MojoService code paths.
* Simplify BuildableDeviceCallbacks interface. With the mapping from device_id
  to descriptor being moved into VideoCaptureSystem, BuildableDeviceCallbacks no
  longer needs to offer a LookupDeviceDescriptor() method.

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

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

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

to

==========
Reland [Mojo Video Capture] Introduce abstraction VideoCaptureSystem

Link to original CL: https://codereview.chromium.org/2769543002/

PatchSet 1 is state as reviewed previously.
PatchSet 2 applies a fix for the issue described below.

This was reverted because it caused a test flakiness for test 
MediaDevicesDispatcherHostTest.GetVideoInputCapabilities on Windows.

Analysis of the issue:
The CL created a race condition that could cause the test method to exit 
before the exercise had finished executing. What made the difference is that 
VideoCaptureSystem::GetDeviceInfosAsync() invoked by VideoCaptureManager 
now actually runs asynchronous, i.e. it posts invocation of its 
|result_callback| to the end of a message queue. Before this CL, 
VideoCaptureManager directly called 
VideoCaptureDeviceFactory::EnumerateDeviceDescriptors which, despite its 
asynchronous-looking API invoked its callback synchronously. The test code was 
relying on this being the case. After the CL, the following bad sequence could
happen:
1.) Test calls |host_->GetVideoInputCapabilities()| which eventually kicks of 
asynchronous work A on the "device thread".
2.) Test method reaches statement 
|media_stream_manager_->FlushVideoCaptureThreadForTesting()| and blocks until 
the already posted work A on the device thread finishes.
3.) Work A posts new work B to the device thread.
4.) Work A finishes.
5.) FlushVideoCaptureThreadForTesting() returns and the test method exits before
work B has run.

Description of fix:
The way that the test was waiting for the asynchronous exercise to complete was 
based on dangerous (and now incorrect) assumptions of how and where the 
implementation under test internally posts task. The fix is to eliminate these
assumptions and, instead, block the test method until the waited-for callback 
arrives.


Original CL description:

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:
* Move nested struct VideoCaptureManager::DeviceInfo to struct
  media::VideoCaptureDeviceInfo.
* Create abstraction VideoCaptureSystem and move caching of
  VideoCaptureDeviceInfo to there. This allows us to use this functionality in
  both InProcess and MojoService code paths.
* Simplify BuildableDeviceCallbacks interface. With the mapping from device_id
  to descriptor being moved into VideoCaptureSystem, BuildableDeviceCallbacks no
  longer needs to offer a LookupDeviceDescriptor() method.

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

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

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

[chfremer, 2017-04-05 22:35:42]

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

[commit-bot: I haz the power, 2017-04-05 22:36:38]

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-05 22:46:42]

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

[chfremer, 2017-04-05 22:46:43]

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

[emircan, 2017-04-05 23:11:08]

lgtm

[commit-bot: I haz the power, 2017-04-06 00:22:52]

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

[commit-bot: I haz the power, 2017-04-06 00:22:53]

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

[gab, 2017-04-06 16:10:45]

On 2017/04/05 22:46:43, chfremer wrote:
> emircan@: PTAL
> mcasas@, miu@, gab@: FYI

Curious why I'd have to know :)?

[gab, 2017-04-06 16:10:59]

gab@chromium.org changed reviewers:
- gab@chromium.org

[gab, 2017-04-06 17:09:08]

gab@chromium.org changed reviewers:
+ gab@chromium.org

[gab, 2017-04-06 17:09:09]

On 2017/04/06 16:10:45, gab wrote:
> On 2017/04/05 22:46:43, chfremer wrote:
> > emircan@: PTAL
> > mcasas@, miu@, gab@: FYI
> 
> Curious why I'd have to know :)?

Ah nvm, I just stumbled on
MediaStreamManager::FlushVideoCaptureThreadForTesting() in code just now for
orthogonal reasons and realized I wrote it to patch an issue I was having when
migrating BrowserThreadImpls/BrowserMainLoop to modern threading constructs.

Waiting for the async dance to callback is definitely a better solution if
possible :)

diff lgtm w/ comment

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

https://codereview.chromium.org/2805533002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/media_devices_dispatcher_host_unittest.cc:360:
.WillOnce(InvokeWithoutArgs([&run_loop]() { run_loop.Quit(); }));
Bind run_loop.QuitClosure() instead of this lambda which is effectively the
same.

[chfremer, 2017-04-06 17:14:06]

On 2017/04/06 17:09:09, gab wrote:
> On 2017/04/06 16:10:45, gab wrote:
> > On 2017/04/05 22:46:43, chfremer wrote:
> > > emircan@: PTAL
> > > mcasas@, miu@, gab@: FYI
> > 
> > Curious why I'd have to know :)?
> 
> Ah nvm, I just stumbled on
> MediaStreamManager::FlushVideoCaptureThreadForTesting() in code just now for
> orthogonal reasons and realized I wrote it to patch an issue I was having when
> migrating BrowserThreadImpls/BrowserMainLoop to modern threading constructs.
> 
> Waiting for the async dance to callback is definitely a better solution if
> possible :)
> 
> diff lgtm w/ comment
> 
>
https://codereview.chromium.org/2805533002/diff/20001/content/browser/rendere...
> File
> content/browser/renderer_host/media/media_devices_dispatcher_host_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/2805533002/diff/20001/content/browser/rendere...
>
content/browser/renderer_host/media/media_devices_dispatcher_host_unittest.cc:360:
> .WillOnce(InvokeWithoutArgs([&run_loop]() { run_loop.Quit(); }));
> Bind run_loop.QuitClosure() instead of this lambda which is effectively the
> same.

Sorry about the lack of context :-).
Seems you already figured it out though. I included you as FYI, because of
https://codereview.chromium.org/2535043003.

[chfremer, 2017-04-06 17:19:11]

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

https://codereview.chromium.org/2805533002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/media_devices_dispatcher_host_unittest.cc:360:
.WillOnce(InvokeWithoutArgs([&run_loop]() { run_loop.Quit(); }));
On 2017/04/06 17:09:09, gab wrote:
> Bind run_loop.QuitClosure() instead of this lambda which is effectively the
> same.

I prefer the lambda, because it works with the gmock built-in Invoke and
InvokeWithoutArgs, whereas I believe running a base::Closure requires defining a
custom gmock Action (something we typically call RunClosure). Because of that, I
feel that the lambda approach is cleaner. WDYT?

[gab, 2017-04-06 17:20:24]

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

https://codereview.chromium.org/2805533002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/media_devices_dispatcher_host_unittest.cc:360:
.WillOnce(InvokeWithoutArgs([&run_loop]() { run_loop.Quit(); }));
On 2017/04/06 17:19:11, chfremer wrote:
> On 2017/04/06 17:09:09, gab wrote:
> > Bind run_loop.QuitClosure() instead of this lambda which is effectively the
> > same.
> 
> I prefer the lambda, because it works with the gmock built-in Invoke and
> InvokeWithoutArgs, whereas I believe running a base::Closure requires defining
a
> custom gmock Action (something we typically call RunClosure). Because of that,
I
> feel that the lambda approach is cleaner. WDYT?

Hmmm that's unfortunate, but yeah okay.

[chfremer, 2017-04-07 16:11:15]

The CQ bit was checked by chfremer@chromium.org

[commit-bot: I haz the power, 2017-04-07 16:11:50]

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:45:31]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491581475213440,
"parent_rev": "a11848984a8ca5cc9481f0db43d70007501fd6fe", "commit_rev":
"8927c43c9dfd0fa73d092eea903f9f3a1f3292f1"}

[commit-bot: I haz the power, 2017-04-07 17:46:20]

Description was changed from

==========
Reland [Mojo Video Capture] Introduce abstraction VideoCaptureSystem

Link to original CL: https://codereview.chromium.org/2769543002/

PatchSet 1 is state as reviewed previously.
PatchSet 2 applies a fix for the issue described below.

This was reverted because it caused a test flakiness for test 
MediaDevicesDispatcherHostTest.GetVideoInputCapabilities on Windows.

Analysis of the issue:
The CL created a race condition that could cause the test method to exit 
before the exercise had finished executing. What made the difference is that 
VideoCaptureSystem::GetDeviceInfosAsync() invoked by VideoCaptureManager 
now actually runs asynchronous, i.e. it posts invocation of its 
|result_callback| to the end of a message queue. Before this CL, 
VideoCaptureManager directly called 
VideoCaptureDeviceFactory::EnumerateDeviceDescriptors which, despite its 
asynchronous-looking API invoked its callback synchronously. The test code was 
relying on this being the case. After the CL, the following bad sequence could
happen:
1.) Test calls |host_->GetVideoInputCapabilities()| which eventually kicks of 
asynchronous work A on the "device thread".
2.) Test method reaches statement 
|media_stream_manager_->FlushVideoCaptureThreadForTesting()| and blocks until 
the already posted work A on the device thread finishes.
3.) Work A posts new work B to the device thread.
4.) Work A finishes.
5.) FlushVideoCaptureThreadForTesting() returns and the test method exits before
work B has run.

Description of fix:
The way that the test was waiting for the asynchronous exercise to complete was 
based on dangerous (and now incorrect) assumptions of how and where the 
implementation under test internally posts task. The fix is to eliminate these
assumptions and, instead, block the test method until the waited-for callback 
arrives.


Original CL description:

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:
* Move nested struct VideoCaptureManager::DeviceInfo to struct
  media::VideoCaptureDeviceInfo.
* Create abstraction VideoCaptureSystem and move caching of
  VideoCaptureDeviceInfo to there. This allows us to use this functionality in
  both InProcess and MojoService code paths.
* Simplify BuildableDeviceCallbacks interface. With the mapping from device_id
  to descriptor being moved into VideoCaptureSystem, BuildableDeviceCallbacks no
  longer needs to offer a LookupDeviceDescriptor() method.

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

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

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

to

==========
Reland [Mojo Video Capture] Introduce abstraction VideoCaptureSystem

Link to original CL: https://codereview.chromium.org/2769543002/

PatchSet 1 is state as reviewed previously.
PatchSet 2 applies a fix for the issue described below.

This was reverted because it caused a test flakiness for test 
MediaDevicesDispatcherHostTest.GetVideoInputCapabilities on Windows.

Analysis of the issue:
The CL created a race condition that could cause the test method to exit 
before the exercise had finished executing. What made the difference is that 
VideoCaptureSystem::GetDeviceInfosAsync() invoked by VideoCaptureManager 
now actually runs asynchronous, i.e. it posts invocation of its 
|result_callback| to the end of a message queue. Before this CL, 
VideoCaptureManager directly called 
VideoCaptureDeviceFactory::EnumerateDeviceDescriptors which, despite its 
asynchronous-looking API invoked its callback synchronously. The test code was 
relying on this being the case. After the CL, the following bad sequence could
happen:
1.) Test calls |host_->GetVideoInputCapabilities()| which eventually kicks of 
asynchronous work A on the "device thread".
2.) Test method reaches statement 
|media_stream_manager_->FlushVideoCaptureThreadForTesting()| and blocks until 
the already posted work A on the device thread finishes.
3.) Work A posts new work B to the device thread.
4.) Work A finishes.
5.) FlushVideoCaptureThreadForTesting() returns and the test method exits before
work B has run.

Description of fix:
The way that the test was waiting for the asynchronous exercise to complete was 
based on dangerous (and now incorrect) assumptions of how and where the 
implementation under test internally posts task. The fix is to eliminate these
assumptions and, instead, block the test method until the waited-for callback 
arrives.


Original CL description:

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:
* Move nested struct VideoCaptureManager::DeviceInfo to struct
  media::VideoCaptureDeviceInfo.
* Create abstraction VideoCaptureSystem and move caching of
  VideoCaptureDeviceInfo to there. This allows us to use this functionality in
  both InProcess and MojoService code paths.
* Simplify BuildableDeviceCallbacks interface. With the mapping from device_id
  to descriptor being moved into VideoCaptureSystem, BuildableDeviceCallbacks no
  longer needs to offer a LookupDeviceDescriptor() method.

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

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

[commit-bot: I haz the power, 2017-04-07 17:46:22]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/8927c43c9dfd0fa73d092eea903f...