[Mojo Video Capture] Add content_browsertest for exercising video capture

[Mojo Video Capture] Add content_browsertest for exercising video capture

Add content_browsertest for exercising video capture with a fake video capture 
device and various formats. The test exercises the VideoCaptureManager API. It 
provides test coverage for the upcoming refactorings and changes to 
VideoCaptureManager that will enable switching between Mojo service and 
in-process version of the video capture functionality.

Two additional changes to the API and behavior of VideoCaptureManager were 
useful/needed while adding this test:
1.) In interface MediaStreamProvider, in method Open(), pass in a 
MediaStreamDevice instead of a StreamDeviceInfo. No implementation was using the
extra information that comes with StreamDeviceInfo.
2.) Allow more than one MediaStreamProviderListener to be registered. This 
allows the test to listen to the events while the "regular" observer is present.

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

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

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

Review-Url: https://codereview.chromium.org/2729783003
Cr-Commit-Position: refs/heads/master@{#455190}
Committed: https://chromium.googlesource.com/chromium/src/+/fc2a14167187133bab1e82c6b47ad051d5768201

[chfremer, 2017-03-02 23:38:43]

Description was changed from

==========
format


Merge branch '12b' into 13


Fix after merge


Merge branch '12b' into 13


Remove debug log


Merge branch '12b' into 13


Merge branch '12b' into 13


Fix now illegal call to RegisterListener() with nullptr


Add video_capture_browsertest

BUG=
==========

to

==========
[Mojo Video Capture] Add content_browsertest for exercising video capture

Add content_browsertest for exercising video capture with a fake video capture 
device and various formats. The test exercises the VideoCaptureManager API. It 
provides test coverage for the upcoming refactorings and changes to 
VideoCaptureManager that will enable switching between Mojo service and 
in-process version of the video capture functionality.

Two additional changes to the API and behavior of VideoCaptureManager were 
useful/needed while adding this test:
1.) In interface MediaStreamProvider, in method Open(), pass in a 
MediaStreamDevice instead of a StreamDeviceInfo. No implementation was using the
extra information that comes with StreamDeviceInfo.
2.) Allow more than one MediaStreamProviderListener to be registered. This 
allows the test to listen to the events while the "regular" observer is present.

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

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

[chfremer, 2017-03-02 23:40:46]

Patchset #1 (id:1) has been deleted

[chfremer, 2017-03-02 23:42:49]

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

[chfremer, 2017-03-02 23:42:49]

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

[chfremer, 2017-03-02 23:42:54]

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

[commit-bot: I haz the power, 2017-03-02 23:43: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-03 00:07:49]

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

[commit-bot: I haz the power, 2017-03-03 00:07:50]

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-03-03 00:15:05]

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

[commit-bot: I haz the power, 2017-03-03 00:15:24]

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

[mcasas, 2017-03-03 00:21:13]

tests FTW! 

 lgtm % comments addressed

https://codereview.chromium.org/2729783003/diff/20001/content/browser/media/c...
File content/browser/media/capture/video_capture_browsertest.cc (right):

https://codereview.chromium.org/2729783003/diff/20001/content/browser/media/c...
content/browser/media/capture/video_capture_browsertest.cc:178: static const
size_t num_frames_to_receive = 3;
nit: s/num_frames_to_receive/kNumFramesToReceive/ 
because it's static.

https://codereview.chromium.org/2729783003/diff/20001/content/browser/media/c...
content/browser/media/capture/video_capture_browsertest.cc:203: return;
No need to return here since the callback does
nothing else, right?

https://codereview.chromium.org/2729783003/diff/20001/content/browser/media/c...
content/browser/media/capture/video_capture_browsertest.cc:241:
RunStartAndImmediatelyStopTest();
Instead of adding this function to VideoCaptureBrowserTest
that is only called once, move its contents here, making
clear what the test does (and also otherwise you mix test
infra and test cases logic in VideoCaptureBrowserTest, which
is kind of confusing).
Same for l.246.

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

https://codereview.chromium.org/2729783003/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:393:
DCHECK(listeners_.might_have_observers());
Not your fault, but this function name is inaccurate, since
the implementation does is
https://cs.chromium.org/chromium/src/base/observer_list.h?type=cs&sq=package:...

  bool might_have_observers() const {
    return ObserverListBase<ObserverType>::size() != 0;
  }

so it should be called 
  bool HasAtLeastOneObserver() const
or
  bool HasObserver() const

could we make a bug for it?  Label it GoodFirstBug.

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

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

[commit-bot: I haz the power, 2017-03-03 02:26:25]

Dry run: This issue passed the CQ dry run.

[chfremer, 2017-03-03 18:20:54]

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

[chfremer, 2017-03-03 18:21:14]

Incorporated suggestions from PS1.

https://codereview.chromium.org/2729783003/diff/20001/content/browser/media/c...
File content/browser/media/capture/video_capture_browsertest.cc (right):

https://codereview.chromium.org/2729783003/diff/20001/content/browser/media/c...
content/browser/media/capture/video_capture_browsertest.cc:178: static const
size_t num_frames_to_receive = 3;
On 2017/03/03 00:21:12, mcasas wrote:
> nit: s/num_frames_to_receive/kNumFramesToReceive/ 
> because it's static.

Done.

https://codereview.chromium.org/2729783003/diff/20001/content/browser/media/c...
content/browser/media/capture/video_capture_browsertest.cc:203: return;
On 2017/03/03 00:21:13, mcasas wrote:
> No need to return here since the callback does
> nothing else, right?

Done.

https://codereview.chromium.org/2729783003/diff/20001/content/browser/media/c...
content/browser/media/capture/video_capture_browsertest.cc:241:
RunStartAndImmediatelyStopTest();
On 2017/03/03 00:21:12, mcasas wrote:
> Instead of adding this function to VideoCaptureBrowserTest
> that is only called once, move its contents here, making
> clear what the test does (and also otherwise you mix test
> infra and test cases logic in VideoCaptureBrowserTest, which
> is kind of confusing).
> Same for l.246.

Done.

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

https://codereview.chromium.org/2729783003/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:393:
DCHECK(listeners_.might_have_observers());
On 2017/03/03 00:21:13, mcasas wrote:
> Not your fault, but this function name is inaccurate, since
> the implementation does is
>
https://cs.chromium.org/chromium/src/base/observer_list.h?type=cs&sq=package:...
> 
>   bool might_have_observers() const {
>     return ObserverListBase<ObserverType>::size() != 0;
>   }
> 
> so it should be called 
>   bool HasAtLeastOneObserver() const
> or
>   bool HasObserver() const
> 
> could we make a bug for it?  Label it GoodFirstBug.

An issue related to this already exists, see
https://bugs.chromium.org/p/chromium/issues/detail?id=656465

[commit-bot: I haz the power, 2017-03-03 18:21: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-03 19:17:59]

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

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

Dry run: This issue passed the CQ dry run.

[chfremer, 2017-03-06 16:12:28]

ping

miu@: Could you have a look, please?

[chfremer, 2017-03-06 18:03:27]

Description was changed from

==========
[Mojo Video Capture] Add content_browsertest for exercising video capture

Add content_browsertest for exercising video capture with a fake video capture 
device and various formats. The test exercises the VideoCaptureManager API. It 
provides test coverage for the upcoming refactorings and changes to 
VideoCaptureManager that will enable switching between Mojo service and 
in-process version of the video capture functionality.

Two additional changes to the API and behavior of VideoCaptureManager were 
useful/needed while adding this test:
1.) In interface MediaStreamProvider, in method Open(), pass in a 
MediaStreamDevice instead of a StreamDeviceInfo. No implementation was using the
extra information that comes with StreamDeviceInfo.
2.) Allow more than one MediaStreamProviderListener to be registered. This 
allows the test to listen to the events while the "regular" observer is present.

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

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

to

==========
[Mojo Video Capture] Add content_browsertest for exercising video capture

Add content_browsertest for exercising video capture with a fake video capture 
device and various formats. The test exercises the VideoCaptureManager API. It 
provides test coverage for the upcoming refactorings and changes to 
VideoCaptureManager that will enable switching between Mojo service and 
in-process version of the video capture functionality.

Two additional changes to the API and behavior of VideoCaptureManager were 
useful/needed while adding this test:
1.) In interface MediaStreamProvider, in method Open(), pass in a 
MediaStreamDevice instead of a StreamDeviceInfo. No implementation was using the
extra information that comes with StreamDeviceInfo.
2.) Allow more than one MediaStreamProviderListener to be registered. This 
allows the test to listen to the events while the "regular" observer is present.

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

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

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

[miu, 2017-03-06 21:54:10]

https://codereview.chromium.org/2729783003/diff/60001/content/browser/media/c...
File content/browser/media/capture/video_capture_browsertest.cc (right):

https://codereview.chromium.org/2729783003/diff/60001/content/browser/media/c...
content/browser/media/capture/video_capture_browsertest.cc:1: // Copyright 2017
The Chromium Authors. All rights reserved.
Consideration about file location: content/browser/media/capture currently only
contains screen-capture-related stuff. Here "capture" was originally intended to
be short-form of "screen capture." Perhaps this new file should go in
content/browser/media (or content/browser/renderer_host/media) instead?

https://codereview.chromium.org/2729783003/diff/60001/content/browser/media/c...
content/browser/media/capture/video_capture_browsertest.cc:80: void
SetUpAndStartCaptureDeviceOnIOThread(base::Closure continuation) {
Why are two methods public, and the rest protected?

https://codereview.chromium.org/2729783003/diff/60001/content/browser/media/c...
content/browser/media/capture/video_capture_browsertest.cc:121: void
SetUpRequiringBrowserMainLoopOnMainThread() {
Seems like this should be:

  void SetUp() override {
    ContentBrowserTest::SetUp();
    ...the rest as you have it...
  }

Then, you don't need to explicitly call SetUpRequiringBlahBlahBlah() at the top
of each fixture.

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

https://codereview.chromium.org/2729783003/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_device_manager.cc:86:
DCHECK(listeners_.might_have_observers());
This is weird. Why would a Close() operation require observers be present? Is
the DCHECK() just stating "what would normally be" instead of actually checking
an assumption that would break the impl? If so, it's probably too pedantic and
can be removed. If not, can you add a comment to make it clear what the
assumption is here?

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

https://codereview.chromium.org/2729783003/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:370:
DCHECK(listeners_.might_have_observers());
ditto: too-pedantic DCHECK?

https://codereview.chromium.org/2729783003/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:393:
DCHECK(listeners_.might_have_observers());
ditto: too-pedantic DCHECK?

[chfremer, 2017-03-06 23:38:53]

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

[commit-bot: I haz the power, 2017-03-06 23:39:18]

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

miu@: PTAL

https://codereview.chromium.org/2729783003/diff/60001/content/browser/media/c...
File content/browser/media/capture/video_capture_browsertest.cc (right):

https://codereview.chromium.org/2729783003/diff/60001/content/browser/media/c...
content/browser/media/capture/video_capture_browsertest.cc:1: // Copyright 2017
The Chromium Authors. All rights reserved.
On 2017/03/06 21:54:10, miu wrote:
> Consideration about file location: content/browser/media/capture currently
only
> contains screen-capture-related stuff. Here "capture" was originally intended
to
> be short-form of "screen capture." Perhaps this new file should go in
> content/browser/media (or content/browser/renderer_host/media) instead?

Good point. I believe that fact that there was no browsertest in
content/browser/renderer_host/media led me to believe that it would not be the
right place. But thinking about it now, it may actually be exactly the right
place, because it exercises the API of VideoCaptureManager.

Moved.

https://codereview.chromium.org/2729783003/diff/60001/content/browser/media/c...
content/browser/media/capture/video_capture_browsertest.cc:80: void
SetUpAndStartCaptureDeviceOnIOThread(base::Closure continuation) {
On 2017/03/06 21:54:10, miu wrote:
> Why are two methods public, and the rest protected?

These two methods are used in base::Bind() expressions in the test methods,
which via IN_PROC_BROWSER_TEST_P are methods of a subclass of
VideoCaptureBrowserTest. I got compiler errors when making these methods
protected and couldn't figure out if there is a way around it. Do you happen to
know a solution for this?

https://codereview.chromium.org/2729783003/diff/60001/content/browser/media/c...
content/browser/media/capture/video_capture_browsertest.cc:121: void
SetUpRequiringBrowserMainLoopOnMainThread() {
On 2017/03/06 21:54:10, miu wrote:
> Seems like this should be:
> 
>   void SetUp() override {
>     ContentBrowserTest::SetUp();
>     ...the rest as you have it...
>   }
> 
> Then, you don't need to explicitly call SetUpRequiringBlahBlahBlah() at the
top
> of each fixture.

Added code commment for clarification:
  // This cannot be part of an override of SetUp(), because at the time when 
  // SetUp() is invoked, the BrowserMainLoop does not exist yet.

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

https://codereview.chromium.org/2729783003/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_device_manager.cc:86:
DCHECK(listeners_.might_have_observers());
On 2017/03/06 21:54:10, miu wrote:
> This is weird. Why would a Close() operation require observers be present? Is
> the DCHECK() just stating "what would normally be" instead of actually
checking
> an assumption that would break the impl? If so, it's probably too pedantic and
> can be removed. If not, can you add a comment to make it clear what the
> assumption is here?

removed

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

https://codereview.chromium.org/2729783003/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:370:
DCHECK(listeners_.might_have_observers());
On 2017/03/06 21:54:10, miu wrote:
> ditto: too-pedantic DCHECK?

Done.

https://codereview.chromium.org/2729783003/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:393:
DCHECK(listeners_.might_have_observers());
On 2017/03/06 21:54:10, miu wrote:
> ditto: too-pedantic DCHECK?

Done.

[miu, 2017-03-07 00:31:36]

lgtm

[commit-bot: I haz the power, 2017-03-07 02:12:33]

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

[commit-bot: I haz the power, 2017-03-07 02:12:35]

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

[chfremer, 2017-03-07 18:51:38]

The CQ bit was checked by chfremer@chromium.org

[chfremer, 2017-03-07 18:51:38]

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

[commit-bot: I haz the power, 2017-03-07 18:53:13]

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-07 19:05:47]

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

[commit-bot: I haz the power, 2017-03-07 19:05:49]

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

[emircan, 2017-03-07 19:09:34]

RS lgtm

[chfremer, 2017-03-07 19:21:30]

The CQ bit was checked by chfremer@chromium.org

[commit-bot: I haz the power, 2017-03-07 19:22:47]

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-07 20:12:58]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488914490873080,
"parent_rev": "5274680b95f85a87cc068c911d525594f2656c47", "commit_rev":
"fc2a14167187133bab1e82c6b47ad051d5768201"}

[commit-bot: I haz the power, 2017-03-07 20:13:45]

Description was changed from

==========
[Mojo Video Capture] Add content_browsertest for exercising video capture

Add content_browsertest for exercising video capture with a fake video capture 
device and various formats. The test exercises the VideoCaptureManager API. It 
provides test coverage for the upcoming refactorings and changes to 
VideoCaptureManager that will enable switching between Mojo service and 
in-process version of the video capture functionality.

Two additional changes to the API and behavior of VideoCaptureManager were 
useful/needed while adding this test:
1.) In interface MediaStreamProvider, in method Open(), pass in a 
MediaStreamDevice instead of a StreamDeviceInfo. No implementation was using the
extra information that comes with StreamDeviceInfo.
2.) Allow more than one MediaStreamProviderListener to be registered. This 
allows the test to listen to the events while the "regular" observer is present.

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

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

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

to

==========
[Mojo Video Capture] Add content_browsertest for exercising video capture

Add content_browsertest for exercising video capture with a fake video capture 
device and various formats. The test exercises the VideoCaptureManager API. It 
provides test coverage for the upcoming refactorings and changes to 
VideoCaptureManager that will enable switching between Mojo service and 
in-process version of the video capture functionality.

Two additional changes to the API and behavior of VideoCaptureManager were 
useful/needed while adding this test:
1.) In interface MediaStreamProvider, in method Open(), pass in a 
MediaStreamDevice instead of a StreamDeviceInfo. No implementation was using the
extra information that comes with StreamDeviceInfo.
2.) Allow more than one MediaStreamProviderListener to be registered. This 
allows the test to listen to the events while the "regular" observer is present.

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

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

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

Review-Url: https://codereview.chromium.org/2729783003
Cr-Commit-Position: refs/heads/master@{#455190}
Committed:
https://chromium.googlesource.com/chromium/src/+/fc2a14167187133bab1e82c6b47a...
==========

[commit-bot: I haz the power, 2017-03-07 20:13:47]

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