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

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

This CL introduces type mapping (via StructTraits) between
video_capture::mojom::VideoCaptureFormatDataView and
media::VideoCaptureFormat and adapts the code in
services/video_capture accordingly. This is needed to
progress in the migration of Video Capture messages between
renderer and browser being tracked in the mentioned bug.

BUG=651897, 642387
TEST= video_capture_unittests working.

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

[mcasas, 2016-10-01 00:29:51]

Description was changed from

==========
Video Capture Service: Add StructTraits between video_capture's
VideoCaptureFormat and media::VideoCaptureFormat

wip

BUG=651897
==========

to

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

wip

BUG=651897
==========

[mcasas, 2016-10-01 00:29:59]

Patchset #1 (id:1) has been deleted

[mcasas, 2016-10-02 22:47:49]

Patchset #1 (id:20001) has been deleted

[mcasas, 2016-10-02 22:50:11]

Description was changed from

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

wip

BUG=651897
==========

to

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

This CL introduces type mapping (via StructTraits) between
video_capture::mojom::VideoCaptureFormatDataView and 
media::VideoCaptureFormat and adapts the code in 
services/video_capture accordingly. This is needed to 
progress in the migration of Video Capture messages between
renderer and browser being tracked in the mentioned bug.

BUG=651897
TEST= video_capture_unittests working.
==========

[mcasas, 2016-10-02 22:52:15]

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

[mcasas, 2016-10-02 22:52:16]

dcheng@ my first stab at StructTraits, ptal

chfremer@ fyi/ptal (but I'd wait for dcheng@ in
case I need to move the code around).

[dcheng, 2016-10-03 01:03:09]

https://codereview.chromium.org/2379253003/diff/40001/services/video_capture/...
File services/video_capture/public/interfaces/video_capture_format_traits.h
(right):

https://codereview.chromium.org/2379253003/diff/40001/services/video_capture/...
services/video_capture/public/interfaces/video_capture_format_traits.h:30:
return static_cast<media::mojom::VideoFormat>(r.pixel_format);
Mojo has EnumTraits for mapping enums. Example:
https://cs.chromium.org/chromium/src/gpu/ipc/common/gpu_info_struct_traits.cc...

https://codereview.chromium.org/2379253003/diff/40001/services/video_capture/...
services/video_capture/public/interfaces/video_capture_format_traits.h:39:
static bool Read(video_capture::mojom::VideoCaptureFormatDataView data,
Nit: Out-of-line this.

https://codereview.chromium.org/2379253003/diff/40001/services/video_capture/...
services/video_capture/public/interfaces/video_capture_format_traits.h:42:
data.ReadFrameSize(&frame_size);
Nit: read directly into out->frame_size, and check the return value (I'm
surprised this doesn't warn, I filed
https://bugs.chromium.org/p/chromium/issues/detail?id=652129 to fix this)

https://codereview.chromium.org/2379253003/diff/40001/services/video_capture/...
File services/video_capture/video_capture_device_proxy_impl.cc (left):

https://codereview.chromium.org/2379253003/diff/40001/services/video_capture/...
services/video_capture/video_capture_device_proxy_impl.cc:27:
params.requested_format = ConvertFromMojoToMedia(std::move(requested_format));
Can we get rid of this helper?

[mcasas, 2016-10-03 07:09:24]

Description was changed from

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

This CL introduces type mapping (via StructTraits) between
video_capture::mojom::VideoCaptureFormatDataView and 
media::VideoCaptureFormat and adapts the code in 
services/video_capture accordingly. This is needed to 
progress in the migration of Video Capture messages between
renderer and browser being tracked in the mentioned bug.

BUG=651897
TEST= video_capture_unittests working.
==========

to

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

This CL introduces type mapping (via StructTraits) between
video_capture::mojom::VideoCaptureFormatDataView and 
media::VideoCaptureFormat and adapts the code in 
services/video_capture accordingly. This is needed to 
progress in the migration of Video Capture messages between
renderer and browser being tracked in the mentioned bug.

BUG=651897, 642387
TEST= video_capture_unittests working.
==========

[mcasas, 2016-10-03 07:29:18]

Patchset #2 (id:60001) has been deleted

[mcasas, 2016-10-03 07:31:51]

dcheng@ PTAL,

please advice what to do about VideoPixelFormat.

https://codereview.chromium.org/2379253003/diff/40001/services/video_capture/...
File services/video_capture/public/interfaces/video_capture_format_traits.h
(right):

https://codereview.chromium.org/2379253003/diff/40001/services/video_capture/...
services/video_capture/public/interfaces/video_capture_format_traits.h:30:
return static_cast<media::mojom::VideoFormat>(r.pixel_format);
On 2016/10/03 01:03:08, dcheng wrote:
> Mojo has EnumTraits for mapping enums. Example:
>
https://cs.chromium.org/chromium/src/gpu/ipc/common/gpu_info_struct_traits.cc...

Using it for VideoPixelStorage in l.33, but re. the mapping 
of media::VideoPixelFormat/media::mojom::VideoFormat, 
it is not  done via EnumTraits, but by hand via 
static_cast<>s  and a bunch of ASSERTs [1], what should
we do?

[1]
https://cs.chromium.org/chromium/src/media/mojo/common/media_type_converters....

https://codereview.chromium.org/2379253003/diff/40001/services/video_capture/...
services/video_capture/public/interfaces/video_capture_format_traits.h:39:
static bool Read(video_capture::mojom::VideoCaptureFormatDataView data,
On 2016/10/03 01:03:08, dcheng wrote:
> Nit: Out-of-line this.

Done.

https://codereview.chromium.org/2379253003/diff/40001/services/video_capture/...
services/video_capture/public/interfaces/video_capture_format_traits.h:42:
data.ReadFrameSize(&frame_size);
On 2016/10/03 01:03:08, dcheng wrote:
> Nit: read directly into out->frame_size, and check the return value (I'm
> surprised this doesn't warn, I filed
> https://bugs.chromium.org/p/chromium/issues/detail?id=652129 to fix this)

Done.

https://codereview.chromium.org/2379253003/diff/40001/services/video_capture/...
File services/video_capture/video_capture_device_proxy_impl.cc (left):

https://codereview.chromium.org/2379253003/diff/40001/services/video_capture/...
services/video_capture/video_capture_device_proxy_impl.cc:27:
params.requested_format = ConvertFromMojoToMedia(std::move(requested_format));
On 2016/10/03 01:03:08, dcheng wrote:
> Can we get rid of this helper?

Done.

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

A few comments and questions.

https://codereview.chromium.org/2379253003/diff/40001/services/video_capture/...
File services/video_capture/public/interfaces/video_capture_format_traits.h
(right):

https://codereview.chromium.org/2379253003/diff/40001/services/video_capture/...
services/video_capture/public/interfaces/video_capture_format_traits.h:30:
return static_cast<media::mojom::VideoFormat>(r.pixel_format);
On 2016/10/03 07:31:51, mcasas wrote:
> On 2016/10/03 01:03:08, dcheng wrote:
> > Mojo has EnumTraits for mapping enums. Example:
> >
>
https://cs.chromium.org/chromium/src/gpu/ipc/common/gpu_info_struct_traits.cc...
> 
> Using it for VideoPixelStorage in l.33, but re. the mapping 
> of media::VideoPixelFormat/media::mojom::VideoFormat, 
> it is not  done via EnumTraits, but by hand via 
> static_cast<>s  and a bunch of ASSERTs [1], what should
> we do?
> 
> [1]
>
https://cs.chromium.org/chromium/src/media/mojo/common/media_type_converters....

In my take on using the typmapping, I ended up removing the static_cast<> in
favor of the cleaner EnumTraits.
If we decide to go with that, please feel free to copy the corresponding code
from the not-yet-to-be-reviewed CL [1].

[1] https://codereview.chromium.org/2386183002/

https://codereview.chromium.org/2379253003/diff/80001/services/video_capture/...
File services/video_capture/fake_device_descriptor_unittest.cc (right):

https://codereview.chromium.org/2379253003/diff/80001/services/video_capture/...
services/video_capture/fake_device_descriptor_unittest.cc:74:
media::VideoCaptureFormat arbitrary_requested_format;
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...

https://codereview.chromium.org/2379253003/diff/80001/services/video_capture/...
services/video_capture/fake_device_descriptor_unittest.cc:87:
device_proxy_2->Start(arbitrary_requested_format,
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.

https://codereview.chromium.org/2379253003/diff/80001/services/video_capture/...
File services/video_capture/public/interfaces/video_capture_format_traits.cc
(right):

https://codereview.chromium.org/2379253003/diff/80001/services/video_capture/...
services/video_capture/public/interfaces/video_capture_format_traits.cc:15:
media::VideoCaptureFormat>::frame_size(const media::VideoCaptureFormat& r) {
Why "r"?
Even though this may come from one of the examples, I don't think it is a good
name.

[dcheng, 2016-10-03 20:45:53]

https://codereview.chromium.org/2379253003/diff/40001/services/video_capture/...
File services/video_capture/public/interfaces/video_capture_format_traits.h
(right):

https://codereview.chromium.org/2379253003/diff/40001/services/video_capture/...
services/video_capture/public/interfaces/video_capture_format_traits.h:30:
return static_cast<media::mojom::VideoFormat>(r.pixel_format);
On 2016/10/03 16:26:59, chfremer wrote:
> On 2016/10/03 07:31:51, mcasas wrote:
> > On 2016/10/03 01:03:08, dcheng wrote:
> > > Mojo has EnumTraits for mapping enums. Example:
> > >
> >
>
https://cs.chromium.org/chromium/src/gpu/ipc/common/gpu_info_struct_traits.cc...
> > 
> > Using it for VideoPixelStorage in l.33, but re. the mapping 
> > of media::VideoPixelFormat/media::mojom::VideoFormat, 
> > it is not  done via EnumTraits, but by hand via 
> > static_cast<>s  and a bunch of ASSERTs [1], what should
> > we do?
> > 
> > [1]
> >
>
https://cs.chromium.org/chromium/src/media/mojo/common/media_type_converters....
> 
> In my take on using the typmapping, I ended up removing the static_cast<> in
> favor of the cleaner EnumTraits.
> If we decide to go with that, please feel free to copy the corresponding code
> from the not-yet-to-be-reviewed CL [1].
> 
> [1] https://codereview.chromium.org/2386183002/

My recollection is the //media code predated the addition of EnumTraits. If we
can make EnumTraits work here, that would be nice.

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

ptal

https://codereview.chromium.org/2379253003/diff/80001/services/video_capture/...
File services/video_capture/fake_device_descriptor_unittest.cc (right):

https://codereview.chromium.org/2379253003/diff/80001/services/video_capture/...
services/video_capture/fake_device_descriptor_unittest.cc:74:
media::VideoCaptureFormat arbitrary_requested_format;
On 2016/10/03 16:26:59, chfremer wrote:
> 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...

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

https://codereview.chromium.org/2379253003/diff/80001/services/video_capture/...
services/video_capture/fake_device_descriptor_unittest.cc:87:
device_proxy_2->Start(arbitrary_requested_format,
On 2016/10/03 16:26:59, chfremer wrote:
> 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.

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

https://codereview.chromium.org/2379253003/diff/80001/services/video_capture/...
File services/video_capture/public/interfaces/video_capture_format_traits.cc
(right):

https://codereview.chromium.org/2379253003/diff/80001/services/video_capture/...
services/video_capture/public/interfaces/video_capture_format_traits.cc:15:
media::VideoCaptureFormat>::frame_size(const media::VideoCaptureFormat& r) {
On 2016/10/03 16:26:59, chfremer wrote:
> Why "r"?
> Even though this may come from one of the examples, I don't think it is a good
> name.

Done.

[chfremer, 2016-10-03 22:04:56]

lgtm

[mcasas, 2016-10-03 22:07:25]

mcasas@chromium.org changed reviewers:
+ rockot@chromium.org

[mcasas, 2016-10-03 22:07:26]

rockot@ plz RS mojo/public/tools/bindings/chromium_bindings_configuration.gni

[dcheng, 2016-10-04 01:33:18]

https://codereview.chromium.org/2379253003/diff/100001/services/video_capture...
File services/video_capture/public/interfaces/video_capture_format_traits.cc
(right):

https://codereview.chromium.org/2379253003/diff/100001/services/video_capture...
services/video_capture/public/interfaces/video_capture_format_traits.cc:16:
return gfx::Size(format.frame_size.width(), format.frame_size.height());
Return a const gfx::Size& and return format.frame_size directly here.

https://codereview.chromium.org/2379253003/diff/100001/services/video_capture...
services/video_capture/public/interfaces/video_capture_format_traits.cc:23:
return format.frame_rate;
Re: inline vs not-inline

If it's just calling a simple getter, it's OK to inline.

If it's more complicated than that, we should not inline it. So these things are
all OK to inline (including the frame_size getter above, once it returns a const
ref to the size member directly) as well as the ones below that just cast enum
types.

https://codereview.chromium.org/2379253003/diff/100001/services/video_capture...
services/video_capture/public/interfaces/video_capture_format_traits.cc:31:
return media::mojom::VideoFormat(format.pixel_format);
Nit: use static_cast to convert between types

[dcheng, 2016-10-04 01:34:37]

https://codereview.chromium.org/2379253003/diff/100001/services/video_capture...
File services/video_capture/public/interfaces/video_capture_format_traits.cc
(right):

https://codereview.chromium.org/2379253003/diff/100001/services/video_capture...
services/video_capture/public/interfaces/video_capture_format_traits.cc:16:
return gfx::Size(format.frame_size.width(), format.frame_size.height());
On 2016/10/04 01:33:18, dcheng wrote:
> Return a const gfx::Size& and return format.frame_size directly here.

Btw, I should have elaborated in my original message: the reason it's important
to do this is because struct traits "getters" like this are actually called
twice on serialization: so generally, they should be cheap and do minimal work
and just return a "view" of the data.

[mcasas, 2016-10-04 02:46:20]

dcheng@ PTAL

(rockot@ RS needed)

https://codereview.chromium.org/2379253003/diff/100001/services/video_capture...
File services/video_capture/public/interfaces/video_capture_format_traits.cc
(right):

https://codereview.chromium.org/2379253003/diff/100001/services/video_capture...
services/video_capture/public/interfaces/video_capture_format_traits.cc:16:
return gfx::Size(format.frame_size.width(), format.frame_size.height());
On 2016/10/04 01:33:18, dcheng wrote:
> Return a const gfx::Size& and return format.frame_size directly here.

Done.

https://codereview.chromium.org/2379253003/diff/100001/services/video_capture...
services/video_capture/public/interfaces/video_capture_format_traits.cc:23:
return format.frame_rate;
On 2016/10/04 01:33:18, dcheng wrote:
> Re: inline vs not-inline
> 
> If it's just calling a simple getter, it's OK to inline.
> 
> If it's more complicated than that, we should not inline it. So these things
are
> all OK to inline (including the frame_size getter above, once it returns a
const
> ref to the size member directly) as well as the ones below that just cast enum
> types.

Done.

https://codereview.chromium.org/2379253003/diff/100001/services/video_capture...
services/video_capture/public/interfaces/video_capture_format_traits.cc:31:
return media::mojom::VideoFormat(format.pixel_format);
On 2016/10/04 01:33:18, dcheng wrote:
> Nit: use static_cast to convert between types

Done.

[dcheng, 2016-10-04 22:09:26]

LGTM with comments addressed

sorry for the delay, I missed your original followup =(

https://codereview.chromium.org/2379253003/diff/120001/services/video_capture...
File services/video_capture/public/interfaces/video_capture_format_traits.cc
(right):

https://codereview.chromium.org/2379253003/diff/120001/services/video_capture...
services/video_capture/public/interfaces/video_capture_format_traits.cc:16:
return format.frame_size;
Inline, since we're just returning a member reference now =)

https://codereview.chromium.org/2379253003/diff/120001/services/video_capture...
services/video_capture/public/interfaces/video_capture_format_traits.cc:24:
return static_cast<media::mojom::VideoFormat>(format.pixel_format);
Ditto, inline these as well.

[mcasas, 2016-10-04 22:16:56]

mcasas@chromium.org changed reviewers:
+ danakj@chromium.org, sandersd@chromium.org

[mcasas, 2016-10-04 22:16:56]

for the addition to DEPS, plz RS:
 ui/gfx:  danakj@chromium.org
 media/base: sandersd@chromium.org

rockot@ plz RS chromium_bindings_configuration.gni

https://codereview.chromium.org/2379253003/diff/120001/services/video_capture...
File services/video_capture/public/interfaces/video_capture_format_traits.cc
(right):

https://codereview.chromium.org/2379253003/diff/120001/services/video_capture...
services/video_capture/public/interfaces/video_capture_format_traits.cc:16:
return format.frame_size;
On 2016/10/04 22:09:26, dcheng wrote:
> Inline, since we're just returning a member reference now =)

Done.

https://codereview.chromium.org/2379253003/diff/120001/services/video_capture...
services/video_capture/public/interfaces/video_capture_format_traits.cc:24:
return static_cast<media::mojom::VideoFormat>(format.pixel_format);
On 2016/10/04 22:09:26, dcheng wrote:
> Ditto, inline these as well.

Done.

[sandersd (OOO until July 31), 2016-10-04 22:23:30]

Dependency addition of //media/base LGTM.

[Ken Rockot(use gerrit already), 2016-10-04 22:26:22]

RS LGTM

[danakj, 2016-10-04 23:44:10]

geometry deps LGTM

[mcasas, 2016-10-04 23:44:48]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2016-10-04 23:44:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
chfremer@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2379253003/#ps140001
(title: "more traits methods inlining")

[commit-bot: I haz the power, 2016-10-04 23:45:22]

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-10-05 01:46:01]

Description was changed from

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

This CL introduces type mapping (via StructTraits) between
video_capture::mojom::VideoCaptureFormatDataView and 
media::VideoCaptureFormat and adapts the code in 
services/video_capture accordingly. This is needed to 
progress in the migration of Video Capture messages between
renderer and browser being tracked in the mentioned bug.

BUG=651897, 642387
TEST= video_capture_unittests working.
==========

to

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

This CL introduces type mapping (via StructTraits) between
video_capture::mojom::VideoCaptureFormatDataView and 
media::VideoCaptureFormat and adapts the code in 
services/video_capture accordingly. This is needed to 
progress in the migration of Video Capture messages between
renderer and browser being tracked in the mentioned bug.

BUG=651897, 642387
TEST= video_capture_unittests working.
==========

[commit-bot: I haz the power, 2016-10-05 01:46:02]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2016-10-05 01:50:28]

Description was changed from

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

This CL introduces type mapping (via StructTraits) between
video_capture::mojom::VideoCaptureFormatDataView and 
media::VideoCaptureFormat and adapts the code in 
services/video_capture accordingly. This is needed to 
progress in the migration of Video Capture messages between
renderer and browser being tracked in the mentioned bug.

BUG=651897, 642387
TEST= video_capture_unittests working.
==========

to

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

This CL introduces type mapping (via StructTraits) between
video_capture::mojom::VideoCaptureFormatDataView and
media::VideoCaptureFormat and adapts the code in
services/video_capture accordingly. This is needed to
progress in the migration of Video Capture messages between
renderer and browser being tracked in the mentioned bug.

BUG=651897, 642387
TEST= video_capture_unittests working.

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

[commit-bot: I haz the power, 2016-10-05 01:50:30]

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