[Mojo Video Capture] Add test ReceiveFramesFromFakeCaptureDevice

[Mojo Video Capture] Add test ReceiveFramesFromFakeCaptureDevice

* Add test FakeDeviceTest.ReceiveFramesFromFakeCaptureDevice, which tests that a
  couple of frames from the fake device are received and match the expected
  format and have increasing timestamps.
* Implement mojom::VideoCaptureDeviceFactory::GetSupportedFormats(). This
  functionality is needed by the FakeDeviceTests in order to obtain a format
  supported by the fake device.

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

BUG=584797
TEST=video_capture_unittests, content_unittests, capture_unittests

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

Committed: https://crrev.com/fa001ca45e50cb6d7f80b05e6333126641db93ee
Cr-Commit-Position: refs/heads/master@{#431278}

[chfremer, 2016-11-07 22:34:16]

Description was changed from

==========
presubmit


Fix from merge


Merge branch 'Part_3_after' into Part_4


Merge branch 'Part_3' into Part_4


Merge branch 'Part_3_after' into Part_3


Merge branch 'Part_3' into Part_4


Merge branch 'Part_3_after' into Part_3


presubmit


Merge branch 'Part_2_2' into Part_3


Fix build


Merge branch 'Part_3' into Part_4


Merge branch 'Part_2_2' into Part_3


Merge branch 'Part_3' into Part_4


Merge branch 'Part_2_2' into Part_3


Merge branch 'Part_3' into Part_4


Merge branch 'Part_2_2' into Part_3


Merge branch 'Part_3' into Part_4


Merge branch 'Part_2' into Part_3


Move mojo types and mappings to media


Merge branch 'Part_3' into Part_4


Fixes to satisfy changes from upstream.


Merge branch 'Part_2' into Part_3


Merge branch 'Part_1' into Part_2


Merge branch 'Part_3' into Part_4


Merge branch 'Part_2' into Part_3


Merge branch 'Part_1' into Part_2


Merge branch 'Part_3' into Part_4


Merge branch 'Part_2' into Part_3


Merge branch 'Part_1' into Part_2


Merge branch 'Part_3' into Part_4


Merge branch 'Part_2' into Part_3


Merge branch 'Part_1' into Part_2


Apply patch part4.diff


Apply patch part3.diff


Apply patch part2.diff

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

to

==========
[Mojo Video Capture] Add test ReceiveFramesFromFakeCaptureDevice

* Add test FakeDeviceTest.ReceiveFramesFromFakeCaptureDevice, which tests that a
  couple of frames from the fake device are received and match the expected 
  format and have increasing timestamps.
* Implement mojom::VideoCaptureDeviceFactory::GetSupportedFormats(). This 
  functionality is needed by the FakeDeviceTests in order to obtain a format 
  supported by the fake device.

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

BUG=584797
TEST=video_capture_unittests, content_unittests, capture_unittests

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

[chfremer, 2016-11-07 22:37:21]

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

[chfremer, 2016-11-07 22:37:21]

mcasas@: PTAL
emircan@: Optional

[mcasas, 2016-11-09 23:50:29]

https://codereview.chromium.org/2486543002/diff/1/services/video_capture/devi...
File services/video_capture/device_factory_media_to_mojo_adapter.cc (right):

https://codereview.chromium.org/2486543002/diff/1/services/video_capture/devi...
services/video_capture/device_factory_media_to_mojo_adapter.cc:57:
media_format.pixel_format != media::PIXEL_FORMAT_MJPEG)
Y16 was added very recently to FakeVCD and Win [1]
to support depth-cameras, perhaps you should add 
it here as well ?

[1]
https://cs.chromium.org/search/?q=PIXEL_FORMAT_Y16+media/capture/video&sq=pac...

https://codereview.chromium.org/2486543002/diff/1/services/video_capture/devi...
services/video_capture/device_factory_media_to_mojo_adapter.cc:64: continue;  //
Result already contains this format
{} because the if() condition expands > 1 line.

https://codereview.chromium.org/2486543002/diff/1/services/video_capture/fake...
File services/video_capture/fake_device_unittest.cc (right):

https://codereview.chromium.org/2486543002/diff/1/services/video_capture/fake...
services/video_capture/fake_device_unittest.cc:55: };
Move this struct to an anonymous namespace
inside video_capture at the beginning of the file.

https://codereview.chromium.org/2486543002/diff/1/services/video_capture/fake...
services/video_capture/fake_device_unittest.cc:74: }
No {} for one-line bodies. Same for l.84, l.106

https://codereview.chromium.org/2486543002/diff/1/services/video_capture/fake...
services/video_capture/fake_device_unittest.cc:106:
ASSERT_GT(frame_info.timestamp, previous_timestamp);
nit: ASSERT_* will stop the test in its tracks, whereas
here you might want to EXPECT_* and let the test
run on to e.g. get more data about what expectations
were ok/nok.

Essentially, leave ASSERT_* for those assertions that
would make the test crash subsequently if not verified.

https://codereview.chromium.org/2486543002/diff/1/services/video_capture/mock...
File services/video_capture/mock_supported_formats_receiver.h (right):

https://codereview.chromium.org/2486543002/diff/1/services/video_capture/mock...
services/video_capture/mock_supported_formats_receiver.h:15: class
MockSupportedFormatsReceiver {
If this class is only used inside FakeDeviceTest,
define as a local class of it. Also, it's so tiny that
declaring it as internal class would allow you to
collapse its definition it into:

class MockSupportedFormatsReceiver {
 public:
  MockSupportedFormatsReceiver() = default;
  ~MockSupportedFormatsReceiver() = default;

  MOCK_METHOD1(OnGetSupportedFormatsCallback,
               void(const std::vector<VideoCaptureFormat>&));
 private:
  DISALLOW_COPY_AND_ASSIGN(MockSupportedFormatsReceiver);
};

[chfremer, 2016-11-10 00:28:40]

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

[chfremer, 2016-11-10 00:28:48]

mcasas@: PTAL

https://codereview.chromium.org/2486543002/diff/1/services/video_capture/devi...
File services/video_capture/device_factory_media_to_mojo_adapter.cc (right):

https://codereview.chromium.org/2486543002/diff/1/services/video_capture/devi...
services/video_capture/device_factory_media_to_mojo_adapter.cc:57:
media_format.pixel_format != media::PIXEL_FORMAT_MJPEG)
On 2016/11/09 23:50:29, mcasas wrote:
> Y16 was added very recently to FakeVCD and Win [1]
> to support depth-cameras, perhaps you should add 
> it here as well ?
> 
> [1]
>
https://cs.chromium.org/search/?q=PIXEL_FORMAT_Y16+media/capture/video&sq=pac...

Thanks for the heads up. 
I have already seen and merged in some of the Y16 code changes.
For the Mojo service, please allow me to defer adding the Y16 until we get
closer to the point where we are actually going to connect it to
VideoCaptureController. I will add a work item in the design doc to ensure we
add the support for Y16 before we connect it.

https://codereview.chromium.org/2486543002/diff/1/services/video_capture/devi...
services/video_capture/device_factory_media_to_mojo_adapter.cc:64: continue;  //
Result already contains this format
On 2016/11/09 23:50:29, mcasas wrote:
> {} because the if() condition expands > 1 line.

Done.

https://codereview.chromium.org/2486543002/diff/1/services/video_capture/fake...
File services/video_capture/fake_device_unittest.cc (right):

https://codereview.chromium.org/2486543002/diff/1/services/video_capture/fake...
services/video_capture/fake_device_unittest.cc:55: };
On 2016/11/09 23:50:29, mcasas wrote:
> Move this struct to an anonymous namespace
> inside video_capture at the beginning of the file.

Done.

https://codereview.chromium.org/2486543002/diff/1/services/video_capture/fake...
services/video_capture/fake_device_unittest.cc:74: }
On 2016/11/09 23:50:29, mcasas wrote:
> No {} for one-line bodies. Same for l.84, l.106

Done.

https://codereview.chromium.org/2486543002/diff/1/services/video_capture/fake...
services/video_capture/fake_device_unittest.cc:106:
ASSERT_GT(frame_info.timestamp, previous_timestamp);
On 2016/11/09 23:50:29, mcasas wrote:
> nit: ASSERT_* will stop the test in its tracks, whereas
> here you might want to EXPECT_* and let the test
> run on to e.g. get more data about what expectations
> were ok/nok.
> 
> Essentially, leave ASSERT_* for those assertions that
> would make the test crash subsequently if not verified.

Very nice. I didn't know about EXPECT_*. Thanks.
Done.

https://codereview.chromium.org/2486543002/diff/1/services/video_capture/mock...
File services/video_capture/mock_supported_formats_receiver.h (right):

https://codereview.chromium.org/2486543002/diff/1/services/video_capture/mock...
services/video_capture/mock_supported_formats_receiver.h:15: class
MockSupportedFormatsReceiver {
On 2016/11/09 23:50:29, mcasas wrote:
> If this class is only used inside FakeDeviceTest,
> define as a local class of it. Also, it's so tiny that
> declaring it as internal class would allow you to
> collapse its definition it into:
> 
> class MockSupportedFormatsReceiver {
>  public:
>   MockSupportedFormatsReceiver() = default;
>   ~MockSupportedFormatsReceiver() = default;
> 
>   MOCK_METHOD1(OnGetSupportedFormatsCallback,
>                void(const std::vector<VideoCaptureFormat>&));
>  private:
>   DISALLOW_COPY_AND_ASSIGN(MockSupportedFormatsReceiver);
> };

Done.

[commit-bot: I haz the power, 2016-11-10 00:29:22]

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

[chfremer, 2016-11-10 00:33:09]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-11-10 00:33:11]

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 on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  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, 2016-11-10 00:39:33]

Patchset #2 (id:40001) has been deleted

[chfremer, 2016-11-10 00:40:22]

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

[commit-bot: I haz the power, 2016-11-10 00:40:55]

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

[mcasas, 2016-11-10 01:21:25]

lgtm with some suggestions/comments.

https://codereview.chromium.org/2486543002/diff/1/services/video_capture/devi...
File services/video_capture/device_factory_media_to_mojo_adapter.cc (right):

https://codereview.chromium.org/2486543002/diff/1/services/video_capture/devi...
services/video_capture/device_factory_media_to_mojo_adapter.cc:57:
media_format.pixel_format != media::PIXEL_FORMAT_MJPEG)
On 2016/11/10 00:28:48, chfremer wrote:
> On 2016/11/09 23:50:29, mcasas wrote:
> > Y16 was added very recently to FakeVCD and Win [1]
> > to support depth-cameras, perhaps you should add 
> > it here as well ?
> > 
> > [1]
> >
>
https://cs.chromium.org/search/?q=PIXEL_FORMAT_Y16+media/capture/video&sq=pac...
> 
> Thanks for the heads up. 
> I have already seen and merged in some of the Y16 code changes.
> For the Mojo service, please allow me to defer adding the Y16 until we get
> closer to the point where we are actually going to connect it to
> VideoCaptureController. I will add a work item in the design doc to ensure we
> add the support for Y16 before we connect it.

Alright. If it's in a few places (ideally, one), just add a 
TODO to link to the bug 624436.

https://codereview.chromium.org/2486543002/diff/60001/services/video_capture/...
File services/video_capture/device_factory_media_to_mojo_adapter.cc (right):

https://codereview.chromium.org/2486543002/diff/60001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.cc:57:
media_format.pixel_format != media::PIXEL_FORMAT_MJPEG)
nit: {} here.

https://codereview.chromium.org/2486543002/diff/60001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.cc:63:
std::end(result)) {
Oops, I missed that here you could use 
base::ContainsValue [1] to make this shorter:

if (base::ContainsValue(result, format)) 
  continue; // ...

[1]
https://cs.chromium.org/chromium/src/base/stl_util.h?q=base::contains&sq=pack...

https://codereview.chromium.org/2486543002/diff/60001/services/video_capture/...
File services/video_capture/fake_device_unittest.cc (right):

https://codereview.chromium.org/2486543002/diff/60001/services/video_capture/...
services/video_capture/fake_device_unittest.cc:64: // Set up a mock
VideoFrameReceiver
Superfluous?

https://codereview.chromium.org/2486543002/diff/60001/services/video_capture/...
services/video_capture/fake_device_unittest.cc:69: int received_frame_count = 0;
nit: You could use a |num_frames_remaining| instead
of |num_frames_to_receive| and |received_frame_count|;
|num_frames_remaining| would be initialized to 2 and 
would be decreased it in l.85, simplifying l.86,
and it might make this test case a (little) bit cleaner by
having less variables of long names.

[commit-bot: I haz the power, 2016-11-10 01:37:38]

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

[commit-bot: I haz the power, 2016-11-10 01:37:39]

Dry run: This issue passed the CQ dry run.

[chfremer, 2016-11-10 04:41:12]

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

[commit-bot: I haz the power, 2016-11-10 04:41:25]

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

[chfremer, 2016-11-10 04:41:35]

Addressed comments

https://codereview.chromium.org/2486543002/diff/1/services/video_capture/devi...
File services/video_capture/device_factory_media_to_mojo_adapter.cc (right):

https://codereview.chromium.org/2486543002/diff/1/services/video_capture/devi...
services/video_capture/device_factory_media_to_mojo_adapter.cc:57:
media_format.pixel_format != media::PIXEL_FORMAT_MJPEG)
On 2016/11/10 01:21:25, mcasas wrote:
> On 2016/11/10 00:28:48, chfremer wrote:
> > On 2016/11/09 23:50:29, mcasas wrote:
> > > Y16 was added very recently to FakeVCD and Win [1]
> > > to support depth-cameras, perhaps you should add 
> > > it here as well ?
> > > 
> > > [1]
> > >
> >
>
https://cs.chromium.org/search/?q=PIXEL_FORMAT_Y16+media/capture/video&sq=pac...
> > 
> > Thanks for the heads up. 
> > I have already seen and merged in some of the Y16 code changes.
> > For the Mojo service, please allow me to defer adding the Y16 until we get
> > closer to the point where we are actually going to connect it to
> > VideoCaptureController. I will add a work item in the design doc to ensure
we
> > add the support for Y16 before we connect it.
> 
> Alright. If it's in a few places (ideally, one), just add a 
> TODO to link to the bug 624436.

Done.

https://codereview.chromium.org/2486543002/diff/60001/services/video_capture/...
File services/video_capture/device_factory_media_to_mojo_adapter.cc (right):

https://codereview.chromium.org/2486543002/diff/60001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.cc:57:
media_format.pixel_format != media::PIXEL_FORMAT_MJPEG)
On 2016/11/10 01:21:25, mcasas wrote:
> nit: {} here.

Done.

https://codereview.chromium.org/2486543002/diff/60001/services/video_capture/...
services/video_capture/device_factory_media_to_mojo_adapter.cc:63:
std::end(result)) {
On 2016/11/10 01:21:25, mcasas wrote:
> Oops, I missed that here you could use 
> base::ContainsValue [1] to make this shorter:
> 
> if (base::ContainsValue(result, format)) 
>   continue; // ...
> 
> [1]
>
https://cs.chromium.org/chromium/src/base/stl_util.h?q=base::contains&sq=pack...

Done.

https://codereview.chromium.org/2486543002/diff/60001/services/video_capture/...
File services/video_capture/fake_device_unittest.cc (right):

https://codereview.chromium.org/2486543002/diff/60001/services/video_capture/...
services/video_capture/fake_device_unittest.cc:64: // Set up a mock
VideoFrameReceiver
On 2016/11/10 01:21:25, mcasas wrote:
> Superfluous?

Done.

https://codereview.chromium.org/2486543002/diff/60001/services/video_capture/...
services/video_capture/fake_device_unittest.cc:69: int received_frame_count = 0;
On 2016/11/10 01:21:25, mcasas wrote:
> nit: You could use a |num_frames_remaining| instead
> of |num_frames_to_receive| and |received_frame_count|;
> |num_frames_remaining| would be initialized to 2 and 
> would be decreased it in l.85, simplifying l.86,
> and it might make this test case a (little) bit cleaner by
> having less variables of long names.

Tried your suggestion, but turns out we need the constexpr
|num_frame_to_receive| in order to initialize the stack array
|received_frame_infos|.

[commit-bot: I haz the power, 2016-11-10 05:56:45]

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

[commit-bot: I haz the power, 2016-11-10 05:56:46]

Dry run: This issue passed the CQ dry run.

[chfremer, 2016-11-10 16:59:56]

The CQ bit was checked by chfremer@chromium.org

[chfremer, 2016-11-10 16:59:56]

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/2486543002/#ps80001
(title: "mcasas comments mk2")

[commit-bot: I haz the power, 2016-11-10 17:00:19]

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, 2016-11-10 17:07:29]

Description was changed from

==========
[Mojo Video Capture] Add test ReceiveFramesFromFakeCaptureDevice

* Add test FakeDeviceTest.ReceiveFramesFromFakeCaptureDevice, which tests that a
  couple of frames from the fake device are received and match the expected 
  format and have increasing timestamps.
* Implement mojom::VideoCaptureDeviceFactory::GetSupportedFormats(). This 
  functionality is needed by the FakeDeviceTests in order to obtain a format 
  supported by the fake device.

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

BUG=584797
TEST=video_capture_unittests, content_unittests, capture_unittests

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

to

==========
[Mojo Video Capture] Add test ReceiveFramesFromFakeCaptureDevice

* Add test FakeDeviceTest.ReceiveFramesFromFakeCaptureDevice, which tests that a
  couple of frames from the fake device are received and match the expected 
  format and have increasing timestamps.
* Implement mojom::VideoCaptureDeviceFactory::GetSupportedFormats(). This 
  functionality is needed by the FakeDeviceTests in order to obtain a format 
  supported by the fake device.

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

BUG=584797
TEST=video_capture_unittests, content_unittests, capture_unittests

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

[commit-bot: I haz the power, 2016-11-10 17:07:30]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-11-10 17:37:31]

Description was changed from

==========
[Mojo Video Capture] Add test ReceiveFramesFromFakeCaptureDevice

* Add test FakeDeviceTest.ReceiveFramesFromFakeCaptureDevice, which tests that a
  couple of frames from the fake device are received and match the expected 
  format and have increasing timestamps.
* Implement mojom::VideoCaptureDeviceFactory::GetSupportedFormats(). This 
  functionality is needed by the FakeDeviceTests in order to obtain a format 
  supported by the fake device.

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

BUG=584797
TEST=video_capture_unittests, content_unittests, capture_unittests

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

to

==========
[Mojo Video Capture] Add test ReceiveFramesFromFakeCaptureDevice

* Add test FakeDeviceTest.ReceiveFramesFromFakeCaptureDevice, which tests that a
  couple of frames from the fake device are received and match the expected
  format and have increasing timestamps.
* Implement mojom::VideoCaptureDeviceFactory::GetSupportedFormats(). This
  functionality is needed by the FakeDeviceTests in order to obtain a format
  supported by the fake device.

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

BUG=584797
TEST=video_capture_unittests, content_unittests, capture_unittests

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

Committed: https://crrev.com/fa001ca45e50cb6d7f80b05e6333126641db93ee
Cr-Commit-Position: refs/heads/master@{#431278}
==========

[commit-bot: I haz the power, 2016-11-10 17:37:32]

Patchset 3 (id:??) landed as
https://crrev.com/fa001ca45e50cb6d7f80b05e6333126641db93ee
Cr-Commit-Position: refs/heads/master@{#431278}