Implement Mojo IPC for media::HDRMetadata

Implement Mojo IPC for media::HDRMetadata

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

[servolk, 2017-04-07 00:27:08]

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

[servolk, 2017-04-07 00:27:28]

Description was changed from

==========
Implement Mojo IPC for media::HDRMetadata

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

to

==========
Implement Mojo IPC for media::HDRMetadata

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

[servolk, 2017-04-07 00:27:28]

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

[commit-bot: I haz the power, 2017-04-07 00:27:32]

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-04-07 01:07:41]

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

[commit-bot: I haz the power, 2017-04-07 01:07:42]

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

[servolk, 2017-04-07 01:11:04]

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

[commit-bot: I haz the power, 2017-04-07 01:11:44]

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-04-07 03:13:58]

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

[commit-bot: I haz the power, 2017-04-07 03:14:01]

Dry run: This issue passed the CQ dry run.

[servolk, 2017-04-07 16:23:14]

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

[commit-bot: I haz the power, 2017-04-07 16:23: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-07 16:24:46]

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

[xhwang, 2017-04-07 16:24:47]

lgtm

hubbe: please see my comments in this CL about HDRMetadata :)

https://codereview.chromium.org/2805533005/diff/20001/media/mojo/interfaces/h...
File media/mojo/interfaces/hdr_metadata_struct_traits.h (right):

https://codereview.chromium.org/2805533005/diff/20001/media/mojo/interfaces/h...
media/mojo/interfaces/hdr_metadata_struct_traits.h:21: }
+hubbe:

Does it make sense to put x and y in a data structure, e.g.

struct Chromaticity {
  float x;
  float y;
}

Chromaticity primary_r;
Chromaticity primary_g;
...

https://codereview.chromium.org/2805533005/diff/20001/media/mojo/interfaces/m...
File media/mojo/interfaces/media_types.mojom (right):

https://codereview.chromium.org/2805533005/diff/20001/media/mojo/interfaces/m...
media/mojo/interfaces/media_types.mojom:106: uint32 max_cll;
+hubbe:

I have no idea what "cll" is and what "fall" means. Could you please add some
reference in media/base/hdr_metadata.h?

[servolk, 2017-04-07 16:30:37]

https://codereview.chromium.org/2805533005/diff/20001/media/mojo/interfaces/m...
File media/mojo/interfaces/media_types.mojom (right):

https://codereview.chromium.org/2805533005/diff/20001/media/mojo/interfaces/m...
media/mojo/interfaces/media_types.mojom:106: uint32 max_cll;
On 2017/04/07 16:24:47, xhwang_slow wrote:
> +hubbe:
> 
> I have no idea what "cll" is and what "fall" means. Could you please add some
> reference in media/base/hdr_metadata.h?

Yeah, sorry about that, it's HDR jargon. I've added a couple of comments to
explain those values in hdr_metadata.h.

[xhwang, 2017-04-07 16:46:05]

lgtm

https://codereview.chromium.org/2805533005/diff/60001/media/base/hdr_metadata.h
File media/base/hdr_metadata.h (right):

https://codereview.chromium.org/2805533005/diff/60001/media/base/hdr_metadata...
media/base/hdr_metadata.h:50: unsigned max_fall = 0;
Thank you for adding the comments!

Per the style guide:
https://google.github.io/styleguide/cppguide.html#General_Naming_Rules

"Names should be descriptive; avoid abbreviation... Do not use abbreviations
that are ambiguous or unfamiliar to readers outside your project."

By doing a Google search [1], I don't feel "content light level" is a well known
concept (with 8530 search results), let alone the abbreviated form.

[1]
https://www.google.com/search?q=content+light+level&oq=content+light+level&aq...

But feel free to land the current CL since these are existing code. And we can
fix this later.

[hubbe, 2017-04-07 17:24:22]

https://codereview.chromium.org/2805533005/diff/20001/media/mojo/interfaces/h...
File media/mojo/interfaces/hdr_metadata_struct_traits.h (right):

https://codereview.chromium.org/2805533005/diff/20001/media/mojo/interfaces/h...
media/mojo/interfaces/hdr_metadata_struct_traits.h:21: }
On 2017/04/07 16:24:47, xhwang_slow wrote:
> +hubbe:
> 
> Does it make sense to put x and y in a data structure, e.g.
> 
> struct Chromaticity {
>   float x;
>   float y;
> }
> 
> Chromaticity primary_r;
> Chromaticity primary_g;
> ...

Yes, yes it does.

https://codereview.chromium.org/2805533005/diff/20001/media/mojo/interfaces/m...
File media/mojo/interfaces/media_types.mojom (right):

https://codereview.chromium.org/2805533005/diff/20001/media/mojo/interfaces/m...
media/mojo/interfaces/media_types.mojom:106: uint32 max_cll;
On 2017/04/07 16:30:37, servolk wrote:
> On 2017/04/07 16:24:47, xhwang_slow wrote:
> > +hubbe:
> > 
> > I have no idea what "cll" is and what "fall" means. Could you please add
some
> > reference in media/base/hdr_metadata.h?
> 
> Yeah, sorry about that, it's HDR jargon. I've added a couple of comments to
> explain those values in hdr_metadata.h.

Renaming the variables would be better.

[Tom Sepez, 2017-04-07 17:29:01]

lgtm

[servolk, 2017-04-07 17:29:42]

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

[commit-bot: I haz the power, 2017-04-07 17:30:13]

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-07 17:42:09]

On 2017/04/07 17:24:22, hubbe wrote:
>
https://codereview.chromium.org/2805533005/diff/20001/media/mojo/interfaces/h...
> File media/mojo/interfaces/hdr_metadata_struct_traits.h (right):
> 
>
https://codereview.chromium.org/2805533005/diff/20001/media/mojo/interfaces/h...
> media/mojo/interfaces/hdr_metadata_struct_traits.h:21: }
> On 2017/04/07 16:24:47, xhwang_slow wrote:
> > +hubbe:
> > 
> > Does it make sense to put x and y in a data structure, e.g.
> > 
> > struct Chromaticity {
> >   float x;
> >   float y;
> > }
> > 
> > Chromaticity primary_r;
> > Chromaticity primary_g;
> > ...
> 
> Yes, yes it does.
> 
>
https://codereview.chromium.org/2805533005/diff/20001/media/mojo/interfaces/m...
> File media/mojo/interfaces/media_types.mojom (right):
> 
>
https://codereview.chromium.org/2805533005/diff/20001/media/mojo/interfaces/m...
> media/mojo/interfaces/media_types.mojom:106: uint32 max_cll;
> On 2017/04/07 16:30:37, servolk wrote:
> > On 2017/04/07 16:24:47, xhwang_slow wrote:
> > > +hubbe:
> > > 
> > > I have no idea what "cll" is and what "fall" means. Could you please add
> some
> > > reference in media/base/hdr_metadata.h?
> > 
> > Yeah, sorry about that, it's HDR jargon. I've added a couple of comments to
> > explain those values in hdr_metadata.h.
> 
> Renaming the variables would be better.

Thanks for the feedback, hubbe!

Sergey: would you have time to do this minor refactoring (say, as a followup
CL)? It'd be nice to do this earlier before they spread further.

[servolk, 2017-04-07 17:46:50]

On 2017/04/07 17:42:09, xhwang_slow wrote:
> On 2017/04/07 17:24:22, hubbe wrote:
> >
>
https://codereview.chromium.org/2805533005/diff/20001/media/mojo/interfaces/h...
> > File media/mojo/interfaces/hdr_metadata_struct_traits.h (right):
> > 
> >
>
https://codereview.chromium.org/2805533005/diff/20001/media/mojo/interfaces/h...
> > media/mojo/interfaces/hdr_metadata_struct_traits.h:21: }
> > On 2017/04/07 16:24:47, xhwang_slow wrote:
> > > +hubbe:
> > > 
> > > Does it make sense to put x and y in a data structure, e.g.
> > > 
> > > struct Chromaticity {
> > >   float x;
> > >   float y;
> > > }
> > > 
> > > Chromaticity primary_r;
> > > Chromaticity primary_g;
> > > ...
> > 
> > Yes, yes it does.
> > 
> >
>
https://codereview.chromium.org/2805533005/diff/20001/media/mojo/interfaces/m...
> > File media/mojo/interfaces/media_types.mojom (right):
> > 
> >
>
https://codereview.chromium.org/2805533005/diff/20001/media/mojo/interfaces/m...
> > media/mojo/interfaces/media_types.mojom:106: uint32 max_cll;
> > On 2017/04/07 16:30:37, servolk wrote:
> > > On 2017/04/07 16:24:47, xhwang_slow wrote:
> > > > +hubbe:
> > > > 
> > > > I have no idea what "cll" is and what "fall" means. Could you please add
> > some
> > > > reference in media/base/hdr_metadata.h?
> > > 
> > > Yeah, sorry about that, it's HDR jargon. I've added a couple of comments
to
> > > explain those values in hdr_metadata.h.
> > 
> > Renaming the variables would be better.
> 
> Thanks for the feedback, hubbe!
> 
> Sergey: would you have time to do this minor refactoring (say, as a followup
> CL)? It'd be nice to do this earlier before they spread further.

Sure, I will do the renaming/refactoring, but I'd prefer to leave this CL
focused on the Mojo IPC and do the refactoring in a follow up CL (since the
refactoring would also need to touch files completely unrelated to this CL, e.g.
webm parser, which also uses max_cll,max_fall ids).

[xhwang, 2017-04-07 17:57:42]

On 2017/04/07 17:46:50, servolk wrote:
> On 2017/04/07 17:42:09, xhwang_slow wrote:
> > On 2017/04/07 17:24:22, hubbe wrote:
> > >
> >
>
https://codereview.chromium.org/2805533005/diff/20001/media/mojo/interfaces/h...
> > > File media/mojo/interfaces/hdr_metadata_struct_traits.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2805533005/diff/20001/media/mojo/interfaces/h...
> > > media/mojo/interfaces/hdr_metadata_struct_traits.h:21: }
> > > On 2017/04/07 16:24:47, xhwang_slow wrote:
> > > > +hubbe:
> > > > 
> > > > Does it make sense to put x and y in a data structure, e.g.
> > > > 
> > > > struct Chromaticity {
> > > >   float x;
> > > >   float y;
> > > > }
> > > > 
> > > > Chromaticity primary_r;
> > > > Chromaticity primary_g;
> > > > ...
> > > 
> > > Yes, yes it does.
> > > 
> > >
> >
>
https://codereview.chromium.org/2805533005/diff/20001/media/mojo/interfaces/m...
> > > File media/mojo/interfaces/media_types.mojom (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2805533005/diff/20001/media/mojo/interfaces/m...
> > > media/mojo/interfaces/media_types.mojom:106: uint32 max_cll;
> > > On 2017/04/07 16:30:37, servolk wrote:
> > > > On 2017/04/07 16:24:47, xhwang_slow wrote:
> > > > > +hubbe:
> > > > > 
> > > > > I have no idea what "cll" is and what "fall" means. Could you please
add
> > > some
> > > > > reference in media/base/hdr_metadata.h?
> > > > 
> > > > Yeah, sorry about that, it's HDR jargon. I've added a couple of comments
> to
> > > > explain those values in hdr_metadata.h.
> > > 
> > > Renaming the variables would be better.
> > 
> > Thanks for the feedback, hubbe!
> > 
> > Sergey: would you have time to do this minor refactoring (say, as a followup
> > CL)? It'd be nice to do this earlier before they spread further.
> 
> Sure, I will do the renaming/refactoring, but I'd prefer to leave this CL
> focused on the Mojo IPC and do the refactoring in a follow up CL (since the
> refactoring would also need to touch files completely unrelated to this CL,
e.g.
> webm parser, which also uses max_cll,max_fall ids).

That totally makes sense! Thank you for doing this!

[servolk, 2017-04-07 18:18:30]

On 2017/04/07 17:57:42, xhwang_slow wrote:
> On 2017/04/07 17:46:50, servolk wrote:
> > On 2017/04/07 17:42:09, xhwang_slow wrote:
> > > On 2017/04/07 17:24:22, hubbe wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2805533005/diff/20001/media/mojo/interfaces/h...
> > > > File media/mojo/interfaces/hdr_metadata_struct_traits.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2805533005/diff/20001/media/mojo/interfaces/h...
> > > > media/mojo/interfaces/hdr_metadata_struct_traits.h:21: }
> > > > On 2017/04/07 16:24:47, xhwang_slow wrote:
> > > > > +hubbe:
> > > > > 
> > > > > Does it make sense to put x and y in a data structure, e.g.
> > > > > 
> > > > > struct Chromaticity {
> > > > >   float x;
> > > > >   float y;
> > > > > }
> > > > > 
> > > > > Chromaticity primary_r;
> > > > > Chromaticity primary_g;
> > > > > ...
> > > > 
> > > > Yes, yes it does.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2805533005/diff/20001/media/mojo/interfaces/m...
> > > > File media/mojo/interfaces/media_types.mojom (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2805533005/diff/20001/media/mojo/interfaces/m...
> > > > media/mojo/interfaces/media_types.mojom:106: uint32 max_cll;
> > > > On 2017/04/07 16:30:37, servolk wrote:
> > > > > On 2017/04/07 16:24:47, xhwang_slow wrote:
> > > > > > +hubbe:
> > > > > > 
> > > > > > I have no idea what "cll" is and what "fall" means. Could you please
> add
> > > > some
> > > > > > reference in media/base/hdr_metadata.h?
> > > > > 
> > > > > Yeah, sorry about that, it's HDR jargon. I've added a couple of
comments
> > to
> > > > > explain those values in hdr_metadata.h.
> > > > 
> > > > Renaming the variables would be better.
> > > 
> > > Thanks for the feedback, hubbe!
> > > 
> > > Sergey: would you have time to do this minor refactoring (say, as a
followup
> > > CL)? It'd be nice to do this earlier before they spread further.
> > 
> > Sure, I will do the renaming/refactoring, but I'd prefer to leave this CL
> > focused on the Mojo IPC and do the refactoring in a follow up CL (since the
> > refactoring would also need to touch files completely unrelated to this CL,
> e.g.
> > webm parser, which also uses max_cll,max_fall ids).
> 
> That totally makes sense! Thank you for doing this!

Here is the refactoring CL: https://codereview.chromium.org/2803563007
I had to make changes in chromecast code as well as webm parser, where
media::HDRMetadata struct is used.

[servolk, 2017-04-07 18:19:07]

The CQ bit was unchecked by servolk@chromium.org

[servolk, 2017-04-07 18:19:08]

The CQ bit was checked by servolk@chromium.org

[commit-bot: I haz the power, 2017-04-07 18:19:30]

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 19:15:40]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491589148440270,
"parent_rev": "94ea653247ced7ebb1c30df9118ba43a9d2b462f", "commit_rev":
"aec2cb67d84ba9aae398953fd8c2e0e707645e3a"}

[commit-bot: I haz the power, 2017-04-07 19:16:28]

Description was changed from

==========
Implement Mojo IPC for media::HDRMetadata

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

to

==========
Implement Mojo IPC for media::HDRMetadata

BUG=708808

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

[commit-bot: I haz the power, 2017-04-07 19:16:29]

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