Refactoring of media::HDRMetadata struct

Refactoring of media::HDRMetadata struct

1. Rename max_cll and max_fall to max_content_light_level and
max_frame_average_light level.
2. Use gfx::PointF instead of a pair of floats for specifying primaries
and white point chromaticity.

Review-Url: https://codereview.chromium.org/2803563007
Cr-Commit-Position: refs/heads/master@{#463009}
Committed: https://chromium.googlesource.com/chromium/src/+/b37705712cc4c6282b07e701306d785636958a8c

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

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

[servolk, 2017-04-07 18:16:49]

Description was changed from

==========
Refactoring of media::HDRMetadata struct

1. Rename max_cll and max_fall to max_content_light_level and
max_frame_average_light level.
2. Use gfx::PointF instead of a pair of floats for specifying primaries
and white point chromaticity.
==========

to

==========
Refactoring of media::HDRMetadata struct

1. Rename max_cll and max_fall to max_content_light_level and
max_frame_average_light level.
2. Use gfx::PointF instead of a pair of floats for specifying primaries
and white point chromaticity.
==========

[servolk, 2017-04-07 18:16:49]

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

[commit-bot: I haz the power, 2017-04-07 18:16:58]

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 18:24:59]

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

https://codereview.chromium.org/2803563007/diff/1/media/base/hdr_metadata.h#n...
media/base/hdr_metadata.h:18: gfx::PointF white_point_chromaticity;
I don't feel we should depend on gfx for chromaticity. It's essentially the same
type (a pair of floats), but it's a very different concept. It's a way to define
a color, not a point in a 2D UI.

Also, if you have a Chromaticity type, you don't need the "chromaticity" suffix
for each member variable.

[hubbe, 2017-04-07 18:26:51]

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

https://codereview.chromium.org/2803563007/diff/1/media/base/hdr_metadata.h#n...
media/base/hdr_metadata.h:18: gfx::PointF white_point_chromaticity;
On 2017/04/07 18:24:59, xhwang_slow wrote:
> I don't feel we should depend on gfx for chromaticity. It's essentially the
same
> type (a pair of floats), but it's a very different concept. It's a way to
define
> a color, not a point in a 2D UI.
> 
> Also, if you have a Chromaticity type, you don't need the "chromaticity"
suffix
> for each member variable.

Do you think it would be enough to typedef gfx::PointF ?

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

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

https://codereview.chromium.org/2803563007/diff/1/media/base/hdr_metadata.h#n...
media/base/hdr_metadata.h:18: gfx::PointF white_point_chromaticity;
On 2017/04/07 18:24:59, xhwang_slow wrote:
> I don't feel we should depend on gfx for chromaticity. It's essentially the
same
> type (a pair of floats), but it's a very different concept. It's a way to
define
> a color, not a point in a 2D UI.
> 
> Also, if you have a Chromaticity type, you don't need the "chromaticity"
suffix
> for each member variable.

Well, chromaticities ARE actually 2D points in color space, describing
coordinates of primaries and white points. Is it worth introducing a separate
struct in this case? If we create a new struct, we'll have to add separate
operator== and Mojo definitions (StructTraits, etc) for the new struct. Seems
like too much boilerplate, don't you think?

https://codereview.chromium.org/2803563007/diff/1/media/base/hdr_metadata.h#n...
media/base/hdr_metadata.h:18: gfx::PointF white_point_chromaticity;
On 2017/04/07 18:26:51, hubbe wrote:
> On 2017/04/07 18:24:59, xhwang_slow wrote:
> > I don't feel we should depend on gfx for chromaticity. It's essentially the
> same
> > type (a pair of floats), but it's a very different concept. It's a way to
> define
> > a color, not a point in a 2D UI.
> > 
> > Also, if you have a Chromaticity type, you don't need the "chromaticity"
> suffix
> > for each member variable.
> 
> Do you think it would be enough to typedef gfx::PointF ?

Yes, I guess a typedef gfx::PointF might work. Xiaohan, WDYT?

[xhwang, 2017-04-07 18:46:15]

On 2017/04/07 18:30:53, servolk wrote:
> https://codereview.chromium.org/2803563007/diff/1/media/base/hdr_metadata.h
> File media/base/hdr_metadata.h (right):
> 
>
https://codereview.chromium.org/2803563007/diff/1/media/base/hdr_metadata.h#n...
> media/base/hdr_metadata.h:18: gfx::PointF white_point_chromaticity;
> On 2017/04/07 18:24:59, xhwang_slow wrote:
> > I don't feel we should depend on gfx for chromaticity. It's essentially the
> same
> > type (a pair of floats), but it's a very different concept. It's a way to
> define
> > a color, not a point in a 2D UI.
> > 
> > Also, if you have a Chromaticity type, you don't need the "chromaticity"
> suffix
> > for each member variable.
> 
> Well, chromaticities ARE actually 2D points in color space, describing
> coordinates of primaries and white points. Is it worth introducing a separate
> struct in this case? If we create a new struct, we'll have to add separate
> operator== and Mojo definitions (StructTraits, etc) for the new struct. Seems
> like too much boilerplate, don't you think?
> 
>
https://codereview.chromium.org/2803563007/diff/1/media/base/hdr_metadata.h#n...
> media/base/hdr_metadata.h:18: gfx::PointF white_point_chromaticity;
> On 2017/04/07 18:26:51, hubbe wrote:
> > On 2017/04/07 18:24:59, xhwang_slow wrote:
> > > I don't feel we should depend on gfx for chromaticity. It's essentially
the
> > same
> > > type (a pair of floats), but it's a very different concept. It's a way to
> > define
> > > a color, not a point in a 2D UI.
> > > 
> > > Also, if you have a Chromaticity type, you don't need the "chromaticity"
> > suffix
> > > for each member variable.
> > 
> > Do you think it would be enough to typedef gfx::PointF ?
> 
> Yes, I guess a typedef gfx::PointF might work. Xiaohan, WDYT?

hubbe made a good point that some existing transformation functions work on
PointF. Yes, a typedef (well "using") should work!

[servolk, 2017-04-07 18:56:03]

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

[commit-bot: I haz the power, 2017-04-07 18:56:15]

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-07 18:56:29]

On 2017/04/07 18:46:15, xhwang_slow wrote:
> On 2017/04/07 18:30:53, servolk wrote:
> > https://codereview.chromium.org/2803563007/diff/1/media/base/hdr_metadata.h
> > File media/base/hdr_metadata.h (right):
> > 
> >
>
https://codereview.chromium.org/2803563007/diff/1/media/base/hdr_metadata.h#n...
> > media/base/hdr_metadata.h:18: gfx::PointF white_point_chromaticity;
> > On 2017/04/07 18:24:59, xhwang_slow wrote:
> > > I don't feel we should depend on gfx for chromaticity. It's essentially
the
> > same
> > > type (a pair of floats), but it's a very different concept. It's a way to
> > define
> > > a color, not a point in a 2D UI.
> > > 
> > > Also, if you have a Chromaticity type, you don't need the "chromaticity"
> > suffix
> > > for each member variable.
> > 
> > Well, chromaticities ARE actually 2D points in color space, describing
> > coordinates of primaries and white points. Is it worth introducing a
separate
> > struct in this case? If we create a new struct, we'll have to add separate
> > operator== and Mojo definitions (StructTraits, etc) for the new struct.
Seems
> > like too much boilerplate, don't you think?
> > 
> >
>
https://codereview.chromium.org/2803563007/diff/1/media/base/hdr_metadata.h#n...
> > media/base/hdr_metadata.h:18: gfx::PointF white_point_chromaticity;
> > On 2017/04/07 18:26:51, hubbe wrote:
> > > On 2017/04/07 18:24:59, xhwang_slow wrote:
> > > > I don't feel we should depend on gfx for chromaticity. It's essentially
> the
> > > same
> > > > type (a pair of floats), but it's a very different concept. It's a way
to
> > > define
> > > > a color, not a point in a 2D UI.
> > > > 
> > > > Also, if you have a Chromaticity type, you don't need the "chromaticity"
> > > suffix
> > > > for each member variable.
> > > 
> > > Do you think it would be enough to typedef gfx::PointF ?
> > 
> > Yes, I guess a typedef gfx::PointF might work. Xiaohan, WDYT?
> 
> hubbe made a good point that some existing transformation functions work on
> PointF. Yes, a typedef (well "using") should work!

Done

[hubbe, 2017-04-07 19:33:09]

lgtm

[xhwang, 2017-04-07 19:39:01]

lgtm

[halliwell, 2017-04-07 19:55:43]

On 2017/04/07 19:39:01, xhwang_slow wrote:
> lgtm

chromecast/ lgtm

[Tom Sepez, 2017-04-07 20:37:29]

mojom lgtm.

[servolk, 2017-04-07 20:39:00]

The CQ bit was unchecked by servolk@chromium.org

[servolk, 2017-04-07 20:39:02]

The CQ bit was checked by servolk@chromium.org

[commit-bot: I haz the power, 2017-04-07 20:39:34]

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 21:37:56]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491597542013050,
"parent_rev": "67e8bdf6ec24a9c345a6978a65338c7497cb4f59", "commit_rev":
"b37705712cc4c6282b07e701306d785636958a8c"}

[commit-bot: I haz the power, 2017-04-07 21:38:53]

Description was changed from

==========
Refactoring of media::HDRMetadata struct

1. Rename max_cll and max_fall to max_content_light_level and
max_frame_average_light level.
2. Use gfx::PointF instead of a pair of floats for specifying primaries
and white point chromaticity.
==========

to

==========
Refactoring of media::HDRMetadata struct

1. Rename max_cll and max_fall to max_content_light_level and
max_frame_average_light level.
2. Use gfx::PointF instead of a pair of floats for specifying primaries
and white point chromaticity.

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

[commit-bot: I haz the power, 2017-04-07 21:38:54]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/b37705712cc4c6282b07e701306d...