Video Capture Service: typemap video_capture's and media's VideoCaptureFormat

services/video_capture/fake_device_descriptor_unittest.cc

Index: services/video_capture/fake_device_descriptor_unittest.cc
diff --git a/services/video_capture/fake_device_descriptor_unittest.cc b/services/video_capture/fake_device_descriptor_unittest.cc
index c0764270b85bbaac80af5b03565c7cb939b01419..c20c020e8b08ceb477ff8b293dba025a95a077ac 100644
--- a/services/video_capture/fake_device_descriptor_unittest.cc
+++ b/services/video_capture/fake_device_descriptor_unittest.cc
@@ -71,11 +71,11 @@ TEST_F(FakeDeviceDescriptorTest, CanUseSecondRequestedProxy) {
           &wait_loop));
   wait_loop.Run();
 
-  auto arbitrary_requested_format = mojom::VideoCaptureFormat::New();
-  arbitrary_requested_format->frame_size.SetSize(640, 480);
-  arbitrary_requested_format->frame_rate = 15;
-  arbitrary_requested_format->pixel_format = media::mojom::VideoFormat::I420;
-  arbitrary_requested_format->pixel_storage = mojom::VideoPixelStorage::CPU;
+  media::VideoCaptureFormat arbitrary_requested_format;

[chfremer, 2016/10/03 16:26:59]

We ended up doing duplicate work here. 
I already ported all Mojo-Media type conversions to Mojo typemapping last week,
but it hasn't been visible, because it is a few CLs down the line. In the design
doc [1] it is CL1.9.3.

But should be no big issue to merge.

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

[mcasas, 2016/10/03 21:19:07]

Ah! Apologies, my bad, I started working on migrating
the Video Capture messages to mojo and before I 
noticed I was already uploading this CL :P
In upcoming CLs I'll have to migrate the 
VideoCaptureParams struct [1]  and I'll take your
code in.


[1]
https://cs.chromium.org/chromium/src/media/base/video_capture_types.h?q=Video...

+  arbitrary_requested_format.frame_size.SetSize(640, 480);
+  arbitrary_requested_format.frame_rate = 15;
+  arbitrary_requested_format.pixel_format = media::PIXEL_FORMAT_I420;
+  arbitrary_requested_format.pixel_storage = media::PIXEL_STORAGE_CPU;
 
   base::RunLoop wait_loop_2;
   mojom::VideoCaptureDeviceClientPtr client_proxy;
@@ -84,7 +84,7 @@ TEST_F(FakeDeviceDescriptorTest, CanUseSecondRequestedProxy) {
       .WillRepeatedly(
           InvokeWithoutArgs([&wait_loop_2]() { wait_loop_2.Quit(); }));
 
-  device_proxy_2->Start(std::move(arbitrary_requested_format),
+  device_proxy_2->Start(arbitrary_requested_format,

[chfremer, 2016/10/03 16:26:59]

One thing I have asked myself here is whether or not we should keep the
std::move(). This code should not make any assumption as to whether or not the
type media::VideoCaptureFormat supports moving for better efficiency. It would
probably be best to, by default, always use std::move() at the last usage of a
variable within a method.

[mcasas, 2016/10/03 21:19:07]

That' a legit concern. I believe in Chromium we force 
users of a data type to understand the move semantics
and use them explicitly, i.e. a struct is moved as const&
and a class is moved according to its constructors. 
dcheng@ please keep me honest :)

                         mojom::ResolutionChangePolicy::FIXED_RESOLUTION,
                         mojom::PowerLineFrequency::DEFAULT,
                         std::move(client_proxy));