Implement Mojo IPC for media::VideoColorSpace

Implement Mojo IPC for media::VideoColorSpace

BUG=708808
Review-Url: https://codereview.chromium.org/2799843002
Cr-Commit-Position: refs/heads/master@{#462695}
Committed: https://chromium.googlesource.com/chromium/src/+/64141289c192a1670547532da9360a312fd17f06

[servolk, 2017-04-05 22:45:54]

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

[servolk, 2017-04-05 22:46:19]

Description was changed from

==========
Implement Mojo IPC for media::VideoColorSpace

BUG=708808
==========

to

==========
Implement Mojo IPC for media::VideoColorSpace

BUG=708808
==========

[servolk, 2017-04-05 22:46:19]

servolk@chromium.org changed reviewers:
+ hubbe@chromium.org, xhwang@chromium.org

[commit-bot: I haz the power, 2017-04-05 22:46:34]

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

[servolk, 2017-04-05 22:47:14]

servolk@chromium.org changed reviewers:
+ tsepez@chromium.org

[servolk, 2017-04-05 22:47:30]

On 2017/04/05 22:46:24, servolk wrote:

+tsepez@ for media/mojo/interfaces/media_types.typemap and
media/mojo/interfaces/media_types.mojom

[hubbe, 2017-04-05 22:49:55]

I don't grok some of the mojo stuff, but it LGTM.

[Tom Sepez, 2017-04-05 23:02:20]

lgtm

[commit-bot: I haz the power, 2017-04-06 00:15:49]

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

[commit-bot: I haz the power, 2017-04-06 00:15:50]

Dry run: This issue passed the CQ dry run.

[xhwang, 2017-04-06 05:01:58]

Thanks for working on this!

However, TypeConverter has been deprecated and for any new code we should use
StructTraits:

https://chromium.googlesource.com/chromium/src/+/master/mojo/public/cpp/bindi...

We planned to move all media TypeConverters and never had time to do it :( 

Sorry I didn't make this clearer. For more context, please see:
https://bugs.chromium.org/p/chromium/issues/detail?id=611224
https://bugs.chromium.org/p/chromium/issues/detail?id=621383

Could you please give StructTraits a try?

[servolk, 2017-04-06 17:27:52]

On 2017/04/06 05:01:58, xhwang_slow wrote:
> Thanks for working on this!
> 
> However, TypeConverter has been deprecated and for any new code we should use
> StructTraits:
> 
>
https://chromium.googlesource.com/chromium/src/+/master/mojo/public/cpp/bindi...
> 
> We planned to move all media TypeConverters and never had time to do it :( 
> 
> Sorry I didn't make this clearer. For more context, please see:
> https://bugs.chromium.org/p/chromium/issues/detail?id=611224
> https://bugs.chromium.org/p/chromium/issues/detail?id=621383
> 
> Could you please give StructTraits a try?

Sure, I can try StructTraits, just a quick question, which is not obvious to me:
Is it possible to combine the StructTraits IPC with the old-stype TypeConverter?
I.e. if I define the StructTraits for the VideoColorSpace struct, but that
struct is actually within VideoDecoderConfig which doesn't have StructTraits
yet, how would that work?

[xhwang, 2017-04-06 17:34:30]

On 2017/04/06 17:27:52, servolk wrote:
> On 2017/04/06 05:01:58, xhwang_slow wrote:
> > Thanks for working on this!
> > 
> > However, TypeConverter has been deprecated and for any new code we should
use
> > StructTraits:
> > 
> >
>
https://chromium.googlesource.com/chromium/src/+/master/mojo/public/cpp/bindi...
> > 
> > We planned to move all media TypeConverters and never had time to do it :( 
> > 
> > Sorry I didn't make this clearer. For more context, please see:
> > https://bugs.chromium.org/p/chromium/issues/detail?id=611224
> > https://bugs.chromium.org/p/chromium/issues/detail?id=621383
> > 
> > Could you please give StructTraits a try?
> 
> Sure, I can try StructTraits, just a quick question, which is not obvious to
me:
> Is it possible to combine the StructTraits IPC with the old-stype
TypeConverter?
> I.e. if I define the StructTraits for the VideoColorSpace struct, but that
> struct is actually within VideoDecoderConfig which doesn't have StructTraits
> yet, how would that work?

Good question. I thought about it yesterday felt it ~should~ work. But to be
honest I am not sure. If it doesn't, I feel we should just replace the
VideoDecoderConfig type converter with StructTraits as well.

Thank you again for working on this!

[servolk, 2017-04-06 21:37:30]

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

[servolk, 2017-04-06 21:38:48]

On 2017/04/06 17:34:30, xhwang_slow wrote:
> On 2017/04/06 17:27:52, servolk wrote:
> > On 2017/04/06 05:01:58, xhwang_slow wrote:
> > > Thanks for working on this!
> > > 
> > > However, TypeConverter has been deprecated and for any new code we should
> use
> > > StructTraits:
> > > 
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/master/mojo/public/cpp/bindi...
> > > 
> > > We planned to move all media TypeConverters and never had time to do it :(

> > > 
> > > Sorry I didn't make this clearer. For more context, please see:
> > > https://bugs.chromium.org/p/chromium/issues/detail?id=611224
> > > https://bugs.chromium.org/p/chromium/issues/detail?id=621383
> > > 
> > > Could you please give StructTraits a try?
> > 
> > Sure, I can try StructTraits, just a quick question, which is not obvious to
> me:
> > Is it possible to combine the StructTraits IPC with the old-stype
> TypeConverter?
> > I.e. if I define the StructTraits for the VideoColorSpace struct, but that
> > struct is actually within VideoDecoderConfig which doesn't have StructTraits
> > yet, how would that work?
> 
> Good question. I thought about it yesterday felt it ~should~ work. But to be
> honest I am not sure. If it doesn't, I feel we should just replace the
> VideoDecoderConfig type converter with StructTraits as well.
> 
> Thank you again for working on this!

Ok, I think I'm getting close to making it work with StructTraits, PTAL at
patchset #2.

[commit-bot: I haz the power, 2017-04-06 21:38:52]

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

[xhwang, 2017-04-06 21:43:41]

cool! LGTM

https://codereview.chromium.org/2799843002/diff/20001/media/mojo/interfaces/v...
File media/mojo/interfaces/video_color_space.typemap (right):

https://codereview.chromium.org/2799843002/diff/20001/media/mojo/interfaces/v...
media/mojo/interfaces/video_color_space.typemap:1: # Copyright 2016 The Chromium
Authors. All rights reserved.
nit: 2017

[servolk, 2017-04-06 21:48:02]

The CQ bit was checked by servolk@chromium.org

[servolk, 2017-04-06 21:48:02]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
hubbe@chromium.org, xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/2799843002/#ps40001
(title: "Fixed the year in comment")

[servolk, 2017-04-06 21:48:19]

https://codereview.chromium.org/2799843002/diff/20001/media/mojo/interfaces/v...
File media/mojo/interfaces/video_color_space.typemap (right):

https://codereview.chromium.org/2799843002/diff/20001/media/mojo/interfaces/v...
media/mojo/interfaces/video_color_space.typemap:1: # Copyright 2016 The Chromium
Authors. All rights reserved.
On 2017/04/06 21:43:40, xhwang_slow wrote:
> nit: 2017

Done.

[commit-bot: I haz the power, 2017-04-06 21:49:08]

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 00:01:50]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491515282253610,
"parent_rev": "1af4615f37b6cd225efc2500c58ef4a8058cc945", "commit_rev":
"64141289c192a1670547532da9360a312fd17f06"}

[commit-bot: I haz the power, 2017-04-07 00:44:12]

Description was changed from

==========
Implement Mojo IPC for media::VideoColorSpace

BUG=708808
==========

to

==========
Implement Mojo IPC for media::VideoColorSpace

BUG=708808

Review-Url: https://codereview.chromium.org/2799843002
Cr-Commit-Position: refs/heads/master@{#462695}
Committed:
https://chromium.googlesource.com/chromium/src/+/64141289c192a1670547532da936...
==========

[commit-bot: I haz the power, 2017-04-07 00:44:13]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/64141289c192a1670547532da936...