Add getters for gfx::ColorSpace components

Add getters for gfx::ColorSpace components

[servolk, 2016-09-16 23:52:26]

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

[commit-bot: I haz the power, 2016-09-16 23:52:47]

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

[servolk, 2016-09-16 23:53:33]

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

[hubbe, 2016-09-16 23:57:53]

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

[hubbe, 2016-09-16 23:57:54]

We've intentionally avoided this so far.
I'm personally ok with adding them, but I'd like to know why you need them.

[hubbe, 2016-09-16 23:58:56]

On 2016/09/16 23:57:54, hubbe wrote:
> We've intentionally avoided this so far.
> I'm personally ok with adding them, but I'd like to know why you need them.

Also; if we're going to have getters(), we should probably use them. (In
gfx::ColorTransform)

[servolk, 2016-09-17 00:02:09]

On 2016/09/16 23:57:54, hubbe wrote:
> We've intentionally avoided this so far.
> I'm personally ok with adding them, but I'd like to know why you need them.

On Chromecast we are using VideoDecoderConfig to carry this information through
Chromium media pipeline, but in the end we need to get this out of the
VideoDecoderConfig to give it to our platform hardware decoders (which are using
vendor-specific data structures for this, and not Chromium's
VideoDecoderConfig).

[servolk, 2016-09-17 00:09:06]

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

[servolk, 2016-09-17 00:09:24]

On 2016/09/16 23:58:56, hubbe wrote:
> On 2016/09/16 23:57:54, hubbe wrote:
> > We've intentionally avoided this so far.
> > I'm personally ok with adding them, but I'd like to know why you need them.
> 
> Also; if we're going to have getters(), we should probably use them. (In
> gfx::ColorTransform)

Added tests for getters in patchset #2

[commit-bot: I haz the power, 2016-09-17 00:09:34]

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, 2016-09-17 01:20:27]

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

[commit-bot: I haz the power, 2016-09-17 01:20:29]

Dry run: This issue passed the CQ dry run.

[hubbe, 2016-09-19 17:34:06]

lgtm

[ccameron, 2016-09-19 17:58:02]

On 2016/09/17 00:02:09, servolk wrote:
> On 2016/09/16 23:57:54, hubbe wrote:
> > We've intentionally avoided this so far.
> > I'm personally ok with adding them, but I'd like to know why you need them.
> 
> On Chromecast we are using VideoDecoderConfig to carry this information
through
> Chromium media pipeline, but in the end we need to get this out of the
> VideoDecoderConfig to give it to our platform hardware decoders (which are
using
> vendor-specific data structures for this, and not Chromium's
> VideoDecoderConfig).

Sorry, I'm not okay with this.

I'd prefer:
1. If the HW decoder only supports a few formats, then just compare
gfx::ColorSpace objects (like, say "input_color_space ==
gfx::ColorSpace::GetSRGB()" to test if it's SRGB).

If that's not possible:
2. Just add a friend class to do the conversion.

[servolk, 2016-09-19 18:16:58]

On 2016/09/19 17:58:02, ccameron wrote:
> On 2016/09/17 00:02:09, servolk wrote:
> > On 2016/09/16 23:57:54, hubbe wrote:
> > > We've intentionally avoided this so far.
> > > I'm personally ok with adding them, but I'd like to know why you need
them.
> > 
> > On Chromecast we are using VideoDecoderConfig to carry this information
> through
> > Chromium media pipeline, but in the end we need to get this out of the
> > VideoDecoderConfig to give it to our platform hardware decoders (which are
> using
> > vendor-specific data structures for this, and not Chromium's
> > VideoDecoderConfig).
> 
> Sorry, I'm not okay with this.
> 
> I'd prefer:
> 1. If the HW decoder only supports a few formats, then just compare
> gfx::ColorSpace objects (like, say "input_color_space ==
> gfx::ColorSpace::GetSRGB()" to test if it's SRGB).
> 
> If that's not possible:
> 2. Just add a friend class to do the conversion.

Chris, could you please explan why you don't like having getters? I think that's
consistent with other structs that we have elsewhere in ui/gfx and media/ code,
so if anything I'd prefer to have getters also here for consistency.

I'm not sure that what you suggested are really viable solutions.
#1 is not scalable, there are many possible combinations of color space
parameters. It's probably not a good idea to introduce a separate ColorSpace
constructor for each possible combination. Besides, there is actually a TODO
saying we should remove at least some of those
(https://cs.chromium.org/chromium/src/ui/gfx/color_space.h?rcl=0&l=127).

#2 adding friends for every place where we need to access individual components
also doesn't seem like a good scalable solution to me. To quote Google C++ style
guide (https://google.github.io/styleguide/cppguide.html#Friends):
> Friends should usually be defined in the same file so that the reader does not
have to look in another file
> to find uses of the private members of a class.
> In some cases this is better than making a member public when you want to give
only one other class access to it.
> However, most classes should interact with other classes solely through their
public members.
So there's a few problems here:
1. We already have 3 friend declarations there, and now we are about to add one
more.
2. We won't be able to have Chromecast-specific conversion code added in the
same file as gfx::ColorSpace declaration.

All in all having getters in gfx::ColorSpace seems like a much cleaner solution
to me. Are there any downsides to having getters?

[ccameron, 2016-09-19 18:27:32]

On 2016/09/19 18:16:58, servolk wrote:
> On 2016/09/19 17:58:02, ccameron wrote:
> > On 2016/09/17 00:02:09, servolk wrote:
> > > On 2016/09/16 23:57:54, hubbe wrote:
> > > > We've intentionally avoided this so far.
> > > > I'm personally ok with adding them, but I'd like to know why you need
> them.
> > > 
> > > On Chromecast we are using VideoDecoderConfig to carry this information
> > through
> > > Chromium media pipeline, but in the end we need to get this out of the
> > > VideoDecoderConfig to give it to our platform hardware decoders (which are
> > using
> > > vendor-specific data structures for this, and not Chromium's
> > > VideoDecoderConfig).
> > 
> > Sorry, I'm not okay with this.
> > 
> > I'd prefer:
> > 1. If the HW decoder only supports a few formats, then just compare
> > gfx::ColorSpace objects (like, say "input_color_space ==
> > gfx::ColorSpace::GetSRGB()" to test if it's SRGB).
> > 
> > If that's not possible:
> > 2. Just add a friend class to do the conversion.
> 
> Chris, could you please explan why you don't like having getters? I think
that's
> consistent with other structs that we have elsewhere in ui/gfx and media/
code,
> so if anything I'd prefer to have getters also here for consistency.
> 
> I'm not sure that what you suggested are really viable solutions.
> #1 is not scalable, there are many possible combinations of color space
> parameters. It's probably not a good idea to introduce a separate ColorSpace
> constructor for each possible combination. Besides, there is actually a TODO
> saying we should remove at least some of those
> (https://cs.chromium.org/chromium/src/ui/gfx/color_space.h?rcl=0&l=127).
> 
> #2 adding friends for every place where we need to access individual
components
> also doesn't seem like a good scalable solution to me. To quote Google C++
style
> guide (https://google.github.io/styleguide/cppguide.html#Friends):
> > Friends should usually be defined in the same file so that the reader does
not
> have to look in another file
> > to find uses of the private members of a class.
> > In some cases this is better than making a member public when you want to
give
> only one other class access to it.
> > However, most classes should interact with other classes solely through
their
> public members.
> So there's a few problems here:
> 1. We already have 3 friend declarations there, and now we are about to add
one
> more.
> 2. We won't be able to have Chromecast-specific conversion code added in the
> same file as gfx::ColorSpace declaration.
> 
> All in all having getters in gfx::ColorSpace seems like a much cleaner
solution
> to me. Are there any downsides to having getters?

gfx::ColorSpace structure is planned to change substantially in the near future
(see crbug.com/634102)

I'd prefer to have a friend so that we don't give the idea that these things are
okay to access and can be relied on in the future.

The downside to adding getters is that it legitimizes accessing these members
(and could lead to access proliferating).

[hubbe, 2016-09-19 19:30:58]

On 2016/09/19 17:58:02, ccameron wrote:
> On 2016/09/17 00:02:09, servolk wrote:
> > On 2016/09/16 23:57:54, hubbe wrote:
> > > We've intentionally avoided this so far.
> > > I'm personally ok with adding them, but I'd like to know why you need
them.
> > 
> > On Chromecast we are using VideoDecoderConfig to carry this information
> through
> > Chromium media pipeline, but in the end we need to get this out of the
> > VideoDecoderConfig to give it to our platform hardware decoders (which are
> using
> > vendor-specific data structures for this, and not Chromium's
> > VideoDecoderConfig).
> 
> Sorry, I'm not okay with this.
> 
> I'd prefer:
> 1. If the HW decoder only supports a few formats, then just compare
> gfx::ColorSpace objects (like, say "input_color_space ==
> gfx::ColorSpace::GetSRGB()" to test if it's SRGB).

This is probably going to be difficult to do. Especially since there are lots of
different
combinations of enums that means nearly the same thing. You could for instance
have rec601
primaries with rec709 transfer functions. But since rec709 transfer functions
are the same
as rec601 functions, this would still be the same thing.

We could make this sort of approach much simpler if we implement an
"functionally equal", or
an "almost equal" function in gfx::ColorSpace().

[servolk, 2016-09-19 21:40:20]

On 2016/09/19 18:27:32, ccameron wrote:
> On 2016/09/19 18:16:58, servolk wrote:
> > On 2016/09/19 17:58:02, ccameron wrote:
> > > On 2016/09/17 00:02:09, servolk wrote:
> > > > On 2016/09/16 23:57:54, hubbe wrote:
> > > > > We've intentionally avoided this so far.
> > > > > I'm personally ok with adding them, but I'd like to know why you need
> > them.
> > > > 
> > > > On Chromecast we are using VideoDecoderConfig to carry this information
> > > through
> > > > Chromium media pipeline, but in the end we need to get this out of the
> > > > VideoDecoderConfig to give it to our platform hardware decoders (which
are
> > > using
> > > > vendor-specific data structures for this, and not Chromium's
> > > > VideoDecoderConfig).
> > > 
> > > Sorry, I'm not okay with this.
> > > 
> > > I'd prefer:
> > > 1. If the HW decoder only supports a few formats, then just compare
> > > gfx::ColorSpace objects (like, say "input_color_space ==
> > > gfx::ColorSpace::GetSRGB()" to test if it's SRGB).
> > > 
> > > If that's not possible:
> > > 2. Just add a friend class to do the conversion.
> > 
> > Chris, could you please explan why you don't like having getters? I think
> that's
> > consistent with other structs that we have elsewhere in ui/gfx and media/
> code,
> > so if anything I'd prefer to have getters also here for consistency.
> > 
> > I'm not sure that what you suggested are really viable solutions.
> > #1 is not scalable, there are many possible combinations of color space
> > parameters. It's probably not a good idea to introduce a separate ColorSpace
> > constructor for each possible combination. Besides, there is actually a TODO
> > saying we should remove at least some of those
> > (https://cs.chromium.org/chromium/src/ui/gfx/color_space.h?rcl=0&l=127).
> > 
> > #2 adding friends for every place where we need to access individual
> components
> > also doesn't seem like a good scalable solution to me. To quote Google C++
> style
> > guide (https://google.github.io/styleguide/cppguide.html#Friends):
> > > Friends should usually be defined in the same file so that the reader does
> not
> > have to look in another file
> > > to find uses of the private members of a class.
> > > In some cases this is better than making a member public when you want to
> give
> > only one other class access to it.
> > > However, most classes should interact with other classes solely through
> their
> > public members.
> > So there's a few problems here:
> > 1. We already have 3 friend declarations there, and now we are about to add
> one
> > more.
> > 2. We won't be able to have Chromecast-specific conversion code added in the
> > same file as gfx::ColorSpace declaration.
> > 
> > All in all having getters in gfx::ColorSpace seems like a much cleaner
> solution
> > to me. Are there any downsides to having getters?
> 
> gfx::ColorSpace structure is planned to change substantially in the near
future
> (see crbug.com/634102)
> 
> I'd prefer to have a friend so that we don't give the idea that these things
are
> okay to access and can be relied on in the future.
> 
> The downside to adding getters is that it legitimizes accessing these members
> (and could lead to access proliferating).

Ok, thanks for pointing this out Christopher. I now understand better the
motivation for not having getters so far. But I have a few further questions
now. Perhaps you and/or hubbe@ could help with answering those:
1. gfx::ColorSpace has things like transfer function, range, primaries which map
nicely to what's available in video streams (e.g. here is the color metadata
available in WebM container for video -
www.webmproject.org/docs/container/#colour, as you can see it essentially
directly to the current gfx::ColorSpace, which I was planning to use in this CL
https://codereview.chromium.org/2333663003/). SkColorSpace looks like a generic
4x4 matrix. Now I understand that it might possible to represent all those color
transforms in the form of generic matrix, but that might lead to loss of
efficiency. E.g. on Chromecast we might be able to do some typical conversions
using fixed-function hardware (e.g. BT709 -> BT2020 conversion). If we just deal
with generic matrices how are we going to be able to optimize cases like that?
hubbe@, you are more familiar with gfx::ColorSpace and media code - any ideas
how we would deal in media code with gfx::ColorSpace being a wrapper over
generic SkColorSpace 4x4 matrix?

2. Another problem with using a friend in this case is that the code that does
the conversion into Chromecast internal structures would ideally live under
chromecast/ directory (and in chromecast namespace). But Chromecast code is
generally considered a higher-level than the relatively low-level ui/gfx
primitives. To me it feels like a potential dependency/layering violation if we
declared some Chromecast class to be a friend of gfx::ColorSpace. Christopher,
do you think that would be acceptable from the dependencies point of view?

[hubbe, 2016-09-19 22:22:43]

On 2016/09/19 21:40:20, servolk wrote:
> On 2016/09/19 18:27:32, ccameron wrote:
> > On 2016/09/19 18:16:58, servolk wrote:
> > > On 2016/09/19 17:58:02, ccameron wrote:
> > > > On 2016/09/17 00:02:09, servolk wrote:
> > > > > On 2016/09/16 23:57:54, hubbe wrote:
> > > > > > We've intentionally avoided this so far.
> > > > > > I'm personally ok with adding them, but I'd like to know why you
need
> > > them.
> > > > > 
> > > > > On Chromecast we are using VideoDecoderConfig to carry this
information
> > > > through
> > > > > Chromium media pipeline, but in the end we need to get this out of the
> > > > > VideoDecoderConfig to give it to our platform hardware decoders (which
> are
> > > > using
> > > > > vendor-specific data structures for this, and not Chromium's
> > > > > VideoDecoderConfig).
> > > > 
> > > > Sorry, I'm not okay with this.
> > > > 
> > > > I'd prefer:
> > > > 1. If the HW decoder only supports a few formats, then just compare
> > > > gfx::ColorSpace objects (like, say "input_color_space ==
> > > > gfx::ColorSpace::GetSRGB()" to test if it's SRGB).
> > > > 
> > > > If that's not possible:
> > > > 2. Just add a friend class to do the conversion.
> > > 
> > > Chris, could you please explan why you don't like having getters? I think
> > that's
> > > consistent with other structs that we have elsewhere in ui/gfx and media/
> > code,
> > > so if anything I'd prefer to have getters also here for consistency.
> > > 
> > > I'm not sure that what you suggested are really viable solutions.
> > > #1 is not scalable, there are many possible combinations of color space
> > > parameters. It's probably not a good idea to introduce a separate
ColorSpace
> > > constructor for each possible combination. Besides, there is actually a
TODO
> > > saying we should remove at least some of those
> > > (https://cs.chromium.org/chromium/src/ui/gfx/color_space.h?rcl=0&l=127).
> > > 
> > > #2 adding friends for every place where we need to access individual
> > components
> > > also doesn't seem like a good scalable solution to me. To quote Google C++
> > style
> > > guide (https://google.github.io/styleguide/cppguide.html#Friends):
> > > > Friends should usually be defined in the same file so that the reader
does
> > not
> > > have to look in another file
> > > > to find uses of the private members of a class.
> > > > In some cases this is better than making a member public when you want
to
> > give
> > > only one other class access to it.
> > > > However, most classes should interact with other classes solely through
> > their
> > > public members.
> > > So there's a few problems here:
> > > 1. We already have 3 friend declarations there, and now we are about to
add
> > one
> > > more.
> > > 2. We won't be able to have Chromecast-specific conversion code added in
the
> > > same file as gfx::ColorSpace declaration.
> > > 
> > > All in all having getters in gfx::ColorSpace seems like a much cleaner
> > solution
> > > to me. Are there any downsides to having getters?
> > 
> > gfx::ColorSpace structure is planned to change substantially in the near
> future
> > (see crbug.com/634102)
> > 
> > I'd prefer to have a friend so that we don't give the idea that these things
> are
> > okay to access and can be relied on in the future.
> > 
> > The downside to adding getters is that it legitimizes accessing these
members
> > (and could lead to access proliferating).
> 
> Ok, thanks for pointing this out Christopher. I now understand better the
> motivation for not having getters so far. But I have a few further questions
> now. Perhaps you and/or hubbe@ could help with answering those:
> 1. gfx::ColorSpace has things like transfer function, range, primaries which
map
> nicely to what's available in video streams (e.g. here is the color metadata
> available in WebM container for video -
> http://www.webmproject.org/docs/container/#colour, as you can see it
essentially
> directly to the current gfx::ColorSpace, which I was planning to use in this
CL
> https://codereview.chromium.org/2333663003/). SkColorSpace looks like a
generic
> 4x4 matrix. Now I understand that it might possible to represent all those
color
> transforms in the form of generic matrix, but that might lead to loss of
> efficiency. E.g. on Chromecast we might be able to do some typical conversions
> using fixed-function hardware (e.g. BT709 -> BT2020 conversion). If we just
deal
> with generic matrices how are we going to be able to optimize cases like that?
> hubbe@, you are more familiar with gfx::ColorSpace and media code - any ideas
> how we would deal in media code with gfx::ColorSpace being a wrapper over
> generic SkColorSpace 4x4 matrix?
> 

Unfortunately, a 4x4 matrix cannot properly convey how video color spaces
are usually done, as the non-linear transforms are usually sandwiched between
two linear transforms. (matrices)

One approach to check if an input color space is bt2020, without using getters
would be to use gfx::ColorTransform(). Just map a few example colors from bt2020
to
the unknown color space. (or vice versa) If the transform is nearly 1:1, then
using
the bt2020->bt709 pipeline is clearly the right thing to do.

It's also possible to extract the primaries and approximate transfer functions
using this
method, which might be what is needed for semi-generic hardware.

I'm not sure if this is the *best* way to do things, but there are some
advantages
to this level of abstraction.


> 2. Another problem with using a friend in this case is that the code that does
> the conversion into Chromecast internal structures would ideally live under
> chromecast/ directory (and in chromecast namespace). But Chromecast code is
> generally considered a higher-level than the relatively low-level ui/gfx
> primitives. To me it feels like a potential dependency/layering violation if
we
> declared some Chromecast class to be a friend of gfx::ColorSpace. Christopher,
> do you think that would be acceptable from the dependencies point of view?

What data does the chromecast want exactly?

[servolk, 2016-09-20 21:50:23]

On 2016/09/19 22:22:43, hubbe wrote:
> On 2016/09/19 21:40:20, servolk wrote:
> > On 2016/09/19 18:27:32, ccameron wrote:
> > > On 2016/09/19 18:16:58, servolk wrote:
> > > > On 2016/09/19 17:58:02, ccameron wrote:
> > > > > On 2016/09/17 00:02:09, servolk wrote:
> > > > > > On 2016/09/16 23:57:54, hubbe wrote:
> > > > > > > We've intentionally avoided this so far.
> > > > > > > I'm personally ok with adding them, but I'd like to know why you
> need
> > > > them.
> > > > > > 
> > > > > > On Chromecast we are using VideoDecoderConfig to carry this
> information
> > > > > through
> > > > > > Chromium media pipeline, but in the end we need to get this out of
the
> > > > > > VideoDecoderConfig to give it to our platform hardware decoders
(which
> > are
> > > > > using
> > > > > > vendor-specific data structures for this, and not Chromium's
> > > > > > VideoDecoderConfig).
> > > > > 
> > > > > Sorry, I'm not okay with this.
> > > > > 
> > > > > I'd prefer:
> > > > > 1. If the HW decoder only supports a few formats, then just compare
> > > > > gfx::ColorSpace objects (like, say "input_color_space ==
> > > > > gfx::ColorSpace::GetSRGB()" to test if it's SRGB).
> > > > > 
> > > > > If that's not possible:
> > > > > 2. Just add a friend class to do the conversion.
> > > > 
> > > > Chris, could you please explan why you don't like having getters? I
think
> > > that's
> > > > consistent with other structs that we have elsewhere in ui/gfx and
media/
> > > code,
> > > > so if anything I'd prefer to have getters also here for consistency.
> > > > 
> > > > I'm not sure that what you suggested are really viable solutions.
> > > > #1 is not scalable, there are many possible combinations of color space
> > > > parameters. It's probably not a good idea to introduce a separate
> ColorSpace
> > > > constructor for each possible combination. Besides, there is actually a
> TODO
> > > > saying we should remove at least some of those
> > > > (https://cs.chromium.org/chromium/src/ui/gfx/color_space.h?rcl=0&l=127).
> > > > 
> > > > #2 adding friends for every place where we need to access individual
> > > components
> > > > also doesn't seem like a good scalable solution to me. To quote Google
C++
> > > style
> > > > guide (https://google.github.io/styleguide/cppguide.html#Friends):
> > > > > Friends should usually be defined in the same file so that the reader
> does
> > > not
> > > > have to look in another file
> > > > > to find uses of the private members of a class.
> > > > > In some cases this is better than making a member public when you want
> to
> > > give
> > > > only one other class access to it.
> > > > > However, most classes should interact with other classes solely
through
> > > their
> > > > public members.
> > > > So there's a few problems here:
> > > > 1. We already have 3 friend declarations there, and now we are about to
> add
> > > one
> > > > more.
> > > > 2. We won't be able to have Chromecast-specific conversion code added in
> the
> > > > same file as gfx::ColorSpace declaration.
> > > > 
> > > > All in all having getters in gfx::ColorSpace seems like a much cleaner
> > > solution
> > > > to me. Are there any downsides to having getters?
> > > 
> > > gfx::ColorSpace structure is planned to change substantially in the near
> > future
> > > (see crbug.com/634102)
> > > 
> > > I'd prefer to have a friend so that we don't give the idea that these
things
> > are
> > > okay to access and can be relied on in the future.
> > > 
> > > The downside to adding getters is that it legitimizes accessing these
> members
> > > (and could lead to access proliferating).
> > 
> > Ok, thanks for pointing this out Christopher. I now understand better the
> > motivation for not having getters so far. But I have a few further questions
> > now. Perhaps you and/or hubbe@ could help with answering those:
> > 1. gfx::ColorSpace has things like transfer function, range, primaries which
> map
> > nicely to what's available in video streams (e.g. here is the color metadata
> > available in WebM container for video -
> > http://www.webmproject.org/docs/container/#colour, as you can see it
> essentially
> > directly to the current gfx::ColorSpace, which I was planning to use in this
> CL
> > https://codereview.chromium.org/2333663003/). SkColorSpace looks like a
> generic
> > 4x4 matrix. Now I understand that it might possible to represent all those
> color
> > transforms in the form of generic matrix, but that might lead to loss of
> > efficiency. E.g. on Chromecast we might be able to do some typical
conversions
> > using fixed-function hardware (e.g. BT709 -> BT2020 conversion). If we just
> deal
> > with generic matrices how are we going to be able to optimize cases like
that?
> > hubbe@, you are more familiar with gfx::ColorSpace and media code - any
ideas
> > how we would deal in media code with gfx::ColorSpace being a wrapper over
> > generic SkColorSpace 4x4 matrix?
> > 
> 
> Unfortunately, a 4x4 matrix cannot properly convey how video color spaces
> are usually done, as the non-linear transforms are usually sandwiched between
> two linear transforms. (matrices)

Ok, so are we going to add video color spaces separate from the 4x4 matrix in
SkColorSpace then? If that's the case then perhaps we could still provide
getters for video color spaces?

> One approach to check if an input color space is bt2020, without using getters
> would be to use gfx::ColorTransform(). Just map a few example colors from
bt2020
> to
> the unknown color space. (or vice versa) If the transform is nearly 1:1, then
> using
> the bt2020->bt709 pipeline is clearly the right thing to do.
>
> It's also possible to extract the primaries and approximate transfer functions
> using this
> method, which might be what is needed for semi-generic hardware.
> 
> I'm not sure if this is the *best* way to do things, but there are some
> advantages
> to this level of abstraction.
> 
> 
> > 2. Another problem with using a friend in this case is that the code that
does
> > the conversion into Chromecast internal structures would ideally live under
> > chromecast/ directory (and in chromecast namespace). But Chromecast code is
> > generally considered a higher-level than the relatively low-level ui/gfx
> > primitives. To me it feels like a potential dependency/layering violation if
> we
> > declared some Chromecast class to be a friend of gfx::ColorSpace.
Christopher,
> > do you think that would be acceptable from the dependencies point of view?
> 
> What data does the chromecast want exactly?

For now I think we'll want to know at least the range (full/limited), the
transfer function and the primaries being used by the video stream (and
specified in the WebM Colour element). But ideally we'll probably want to have
access to all information available in the Colour element, even if for now some
of it going to be ignored.

[hubbe, 2016-09-20 22:21:36]

On 2016/09/20 21:50:23, servolk wrote:
> On 2016/09/19 22:22:43, hubbe wrote:
> > On 2016/09/19 21:40:20, servolk wrote:
> > > On 2016/09/19 18:27:32, ccameron wrote:
> > > > On 2016/09/19 18:16:58, servolk wrote:
> > > > > On 2016/09/19 17:58:02, ccameron wrote:
> > > > > > On 2016/09/17 00:02:09, servolk wrote:
> > > > > > > On 2016/09/16 23:57:54, hubbe wrote:
> > > > > > > > We've intentionally avoided this so far.
> > > > > > > > I'm personally ok with adding them, but I'd like to know why you
> > need
> > > > > them.
> > > > > > > 
> > > > > > > On Chromecast we are using VideoDecoderConfig to carry this
> > information
> > > > > > through
> > > > > > > Chromium media pipeline, but in the end we need to get this out of
> the
> > > > > > > VideoDecoderConfig to give it to our platform hardware decoders
> (which
> > > are
> > > > > > using
> > > > > > > vendor-specific data structures for this, and not Chromium's
> > > > > > > VideoDecoderConfig).
> > > > > > 
> > > > > > Sorry, I'm not okay with this.
> > > > > > 
> > > > > > I'd prefer:
> > > > > > 1. If the HW decoder only supports a few formats, then just compare
> > > > > > gfx::ColorSpace objects (like, say "input_color_space ==
> > > > > > gfx::ColorSpace::GetSRGB()" to test if it's SRGB).
> > > > > > 
> > > > > > If that's not possible:
> > > > > > 2. Just add a friend class to do the conversion.
> > > > > 
> > > > > Chris, could you please explan why you don't like having getters? I
> think
> > > > that's
> > > > > consistent with other structs that we have elsewhere in ui/gfx and
> media/
> > > > code,
> > > > > so if anything I'd prefer to have getters also here for consistency.
> > > > > 
> > > > > I'm not sure that what you suggested are really viable solutions.
> > > > > #1 is not scalable, there are many possible combinations of color
space
> > > > > parameters. It's probably not a good idea to introduce a separate
> > ColorSpace
> > > > > constructor for each possible combination. Besides, there is actually
a
> > TODO
> > > > > saying we should remove at least some of those
> > > > >
(https://cs.chromium.org/chromium/src/ui/gfx/color_space.h?rcl=0&l=127).
> > > > > 
> > > > > #2 adding friends for every place where we need to access individual
> > > > components
> > > > > also doesn't seem like a good scalable solution to me. To quote Google
> C++
> > > > style
> > > > > guide (https://google.github.io/styleguide/cppguide.html#Friends):
> > > > > > Friends should usually be defined in the same file so that the
reader
> > does
> > > > not
> > > > > have to look in another file
> > > > > > to find uses of the private members of a class.
> > > > > > In some cases this is better than making a member public when you
want
> > to
> > > > give
> > > > > only one other class access to it.
> > > > > > However, most classes should interact with other classes solely
> through
> > > > their
> > > > > public members.
> > > > > So there's a few problems here:
> > > > > 1. We already have 3 friend declarations there, and now we are about
to
> > add
> > > > one
> > > > > more.
> > > > > 2. We won't be able to have Chromecast-specific conversion code added
in
> > the
> > > > > same file as gfx::ColorSpace declaration.
> > > > > 
> > > > > All in all having getters in gfx::ColorSpace seems like a much cleaner
> > > > solution
> > > > > to me. Are there any downsides to having getters?
> > > > 
> > > > gfx::ColorSpace structure is planned to change substantially in the near
> > > future
> > > > (see crbug.com/634102)
> > > > 
> > > > I'd prefer to have a friend so that we don't give the idea that these
> things
> > > are
> > > > okay to access and can be relied on in the future.
> > > > 
> > > > The downside to adding getters is that it legitimizes accessing these
> > members
> > > > (and could lead to access proliferating).
> > > 
> > > Ok, thanks for pointing this out Christopher. I now understand better the
> > > motivation for not having getters so far. But I have a few further
questions
> > > now. Perhaps you and/or hubbe@ could help with answering those:
> > > 1. gfx::ColorSpace has things like transfer function, range, primaries
which
> > map
> > > nicely to what's available in video streams (e.g. here is the color
metadata
> > > available in WebM container for video -
> > > http://www.webmproject.org/docs/container/#colour, as you can see it
> > essentially
> > > directly to the current gfx::ColorSpace, which I was planning to use in
this
> > CL
> > > https://codereview.chromium.org/2333663003/). SkColorSpace looks like a
> > generic
> > > 4x4 matrix. Now I understand that it might possible to represent all those
> > color
> > > transforms in the form of generic matrix, but that might lead to loss of
> > > efficiency. E.g. on Chromecast we might be able to do some typical
> conversions
> > > using fixed-function hardware (e.g. BT709 -> BT2020 conversion). If we
just
> > deal
> > > with generic matrices how are we going to be able to optimize cases like
> that?
> > > hubbe@, you are more familiar with gfx::ColorSpace and media code - any
> ideas
> > > how we would deal in media code with gfx::ColorSpace being a wrapper over
> > > generic SkColorSpace 4x4 matrix?
> > > 
> > 
> > Unfortunately, a 4x4 matrix cannot properly convey how video color spaces
> > are usually done, as the non-linear transforms are usually sandwiched
between
> > two linear transforms. (matrices)
> 
> Ok, so are we going to add video color spaces separate from the 4x4 matrix in
> SkColorSpace then? If that's the case then perhaps we could still provide
> getters for video color spaces?
> 

Don't know. The people working on SkColorSpace keep telling me they want to
replace
gfx::ColorSpace with SkColorSpace, but so far they have been reluctant to make
SkColorSpace capable of implementing all the things that is needed from it, at
least
from a video use case perspective.  Last I heard the plan was to try to break it
up so that the YUV aspects are handled by gfx::ColorSpace/gfx::ColorTransform
and
SkColorSpace would only deal with RGB color spaces, but I'm not a big fan of
that
plan myself.

> > One approach to check if an input color space is bt2020, without using
getters
> > would be to use gfx::ColorTransform(). Just map a few example colors from
> bt2020
> > to
> > the unknown color space. (or vice versa) If the transform is nearly 1:1,
then
> > using
> > the bt2020->bt709 pipeline is clearly the right thing to do.
> >
> > It's also possible to extract the primaries and approximate transfer
functions
> > using this
> > method, which might be what is needed for semi-generic hardware.
> > 
> > I'm not sure if this is the *best* way to do things, but there are some
> > advantages
> > to this level of abstraction.
> > 
> > 
> > > 2. Another problem with using a friend in this case is that the code that
> does
> > > the conversion into Chromecast internal structures would ideally live
under
> > > chromecast/ directory (and in chromecast namespace). But Chromecast code
is
> > > generally considered a higher-level than the relatively low-level ui/gfx
> > > primitives. To me it feels like a potential dependency/layering violation
if
> > we
> > > declared some Chromecast class to be a friend of gfx::ColorSpace.
> Christopher,
> > > do you think that would be acceptable from the dependencies point of view?
> > 
> > What data does the chromecast want exactly?
> 
> For now I think we'll want to know at least the range (full/limited), the
> transfer function and the primaries being used by the video stream (and
> specified in the WebM Colour element). But ideally we'll probably want to have
> access to all information available in the Colour element, even if for now
some
> of it going to be ignored.

That's everything except for the YUV->RGB matrix. :)
Do you care what the format of this information is?
If you got the primaries as a list of floats, would it matter?

[servolk, 2016-09-20 23:01:31]

On 2016/09/20 22:21:36, hubbe wrote:
> On 2016/09/20 21:50:23, servolk wrote:
> > On 2016/09/19 22:22:43, hubbe wrote:
> > > On 2016/09/19 21:40:20, servolk wrote:
> > > > On 2016/09/19 18:27:32, ccameron wrote:
> > > > > On 2016/09/19 18:16:58, servolk wrote:
> > > > > > On 2016/09/19 17:58:02, ccameron wrote:
> > > > > > > On 2016/09/17 00:02:09, servolk wrote:
> > > > > > > > On 2016/09/16 23:57:54, hubbe wrote:
> > > > > > > > > We've intentionally avoided this so far.
> > > > > > > > > I'm personally ok with adding them, but I'd like to know why
you
> > > need
> > > > > > them.
> > > > > > > > 
> > > > > > > > On Chromecast we are using VideoDecoderConfig to carry this
> > > information
> > > > > > > through
> > > > > > > > Chromium media pipeline, but in the end we need to get this out
of
> > the
> > > > > > > > VideoDecoderConfig to give it to our platform hardware decoders
> > (which
> > > > are
> > > > > > > using
> > > > > > > > vendor-specific data structures for this, and not Chromium's
> > > > > > > > VideoDecoderConfig).
> > > > > > > 
> > > > > > > Sorry, I'm not okay with this.
> > > > > > > 
> > > > > > > I'd prefer:
> > > > > > > 1. If the HW decoder only supports a few formats, then just
compare
> > > > > > > gfx::ColorSpace objects (like, say "input_color_space ==
> > > > > > > gfx::ColorSpace::GetSRGB()" to test if it's SRGB).
> > > > > > > 
> > > > > > > If that's not possible:
> > > > > > > 2. Just add a friend class to do the conversion.
> > > > > > 
> > > > > > Chris, could you please explan why you don't like having getters? I
> > think
> > > > > that's
> > > > > > consistent with other structs that we have elsewhere in ui/gfx and
> > media/
> > > > > code,
> > > > > > so if anything I'd prefer to have getters also here for consistency.
> > > > > > 
> > > > > > I'm not sure that what you suggested are really viable solutions.
> > > > > > #1 is not scalable, there are many possible combinations of color
> space
> > > > > > parameters. It's probably not a good idea to introduce a separate
> > > ColorSpace
> > > > > > constructor for each possible combination. Besides, there is
actually
> a
> > > TODO
> > > > > > saying we should remove at least some of those
> > > > > >
> (https://cs.chromium.org/chromium/src/ui/gfx/color_space.h?rcl=0&l=127).
> > > > > > 
> > > > > > #2 adding friends for every place where we need to access individual
> > > > > components
> > > > > > also doesn't seem like a good scalable solution to me. To quote
Google
> > C++
> > > > > style
> > > > > > guide (https://google.github.io/styleguide/cppguide.html#Friends):
> > > > > > > Friends should usually be defined in the same file so that the
> reader
> > > does
> > > > > not
> > > > > > have to look in another file
> > > > > > > to find uses of the private members of a class.
> > > > > > > In some cases this is better than making a member public when you
> want
> > > to
> > > > > give
> > > > > > only one other class access to it.
> > > > > > > However, most classes should interact with other classes solely
> > through
> > > > > their
> > > > > > public members.
> > > > > > So there's a few problems here:
> > > > > > 1. We already have 3 friend declarations there, and now we are about
> to
> > > add
> > > > > one
> > > > > > more.
> > > > > > 2. We won't be able to have Chromecast-specific conversion code
added
> in
> > > the
> > > > > > same file as gfx::ColorSpace declaration.
> > > > > > 
> > > > > > All in all having getters in gfx::ColorSpace seems like a much
cleaner
> > > > > solution
> > > > > > to me. Are there any downsides to having getters?
> > > > > 
> > > > > gfx::ColorSpace structure is planned to change substantially in the
near
> > > > future
> > > > > (see crbug.com/634102)
> > > > > 
> > > > > I'd prefer to have a friend so that we don't give the idea that these
> > things
> > > > are
> > > > > okay to access and can be relied on in the future.
> > > > > 
> > > > > The downside to adding getters is that it legitimizes accessing these
> > > members
> > > > > (and could lead to access proliferating).
> > > > 
> > > > Ok, thanks for pointing this out Christopher. I now understand better
the
> > > > motivation for not having getters so far. But I have a few further
> questions
> > > > now. Perhaps you and/or hubbe@ could help with answering those:
> > > > 1. gfx::ColorSpace has things like transfer function, range, primaries
> which
> > > map
> > > > nicely to what's available in video streams (e.g. here is the color
> metadata
> > > > available in WebM container for video -
> > > > http://www.webmproject.org/docs/container/#colour, as you can see it
> > > essentially
> > > > directly to the current gfx::ColorSpace, which I was planning to use in
> this
> > > CL
> > > > https://codereview.chromium.org/2333663003/). SkColorSpace looks like a
> > > generic
> > > > 4x4 matrix. Now I understand that it might possible to represent all
those
> > > color
> > > > transforms in the form of generic matrix, but that might lead to loss of
> > > > efficiency. E.g. on Chromecast we might be able to do some typical
> > conversions
> > > > using fixed-function hardware (e.g. BT709 -> BT2020 conversion). If we
> just
> > > deal
> > > > with generic matrices how are we going to be able to optimize cases like
> > that?
> > > > hubbe@, you are more familiar with gfx::ColorSpace and media code - any
> > ideas
> > > > how we would deal in media code with gfx::ColorSpace being a wrapper
over
> > > > generic SkColorSpace 4x4 matrix?
> > > > 
> > > 
> > > Unfortunately, a 4x4 matrix cannot properly convey how video color spaces
> > > are usually done, as the non-linear transforms are usually sandwiched
> between
> > > two linear transforms. (matrices)
> > 
> > Ok, so are we going to add video color spaces separate from the 4x4 matrix
in
> > SkColorSpace then? If that's the case then perhaps we could still provide
> > getters for video color spaces?
> > 
> 
> Don't know. The people working on SkColorSpace keep telling me they want to
> replace
> gfx::ColorSpace with SkColorSpace, but so far they have been reluctant to make
> SkColorSpace capable of implementing all the things that is needed from it, at
> least
> from a video use case perspective.  Last I heard the plan was to try to break
it
> up so that the YUV aspects are handled by gfx::ColorSpace/gfx::ColorTransform
> and
> SkColorSpace would only deal with RGB color spaces, but I'm not a big fan of
> that
> plan myself.

Ok, so it sounds like there's no consensus here yet. Do we have an ETA for when
we are planning to merge gfx::ColorSpace / SkColorSpace? Is it likely to happen
after ~M55 branch point?

> > > One approach to check if an input color space is bt2020, without using
> getters
> > > would be to use gfx::ColorTransform(). Just map a few example colors from
> > bt2020
> > > to
> > > the unknown color space. (or vice versa) If the transform is nearly 1:1,
> then
> > > using
> > > the bt2020->bt709 pipeline is clearly the right thing to do.
> > >
> > > It's also possible to extract the primaries and approximate transfer
> functions
> > > using this
> > > method, which might be what is needed for semi-generic hardware.
> > > 
> > > I'm not sure if this is the *best* way to do things, but there are some
> > > advantages
> > > to this level of abstraction.
> > > 
> > > 
> > > > 2. Another problem with using a friend in this case is that the code
that
> > does
> > > > the conversion into Chromecast internal structures would ideally live
> under
> > > > chromecast/ directory (and in chromecast namespace). But Chromecast code
> is
> > > > generally considered a higher-level than the relatively low-level ui/gfx
> > > > primitives. To me it feels like a potential dependency/layering
violation
> if
> > > we
> > > > declared some Chromecast class to be a friend of gfx::ColorSpace.
> > Christopher,
> > > > do you think that would be acceptable from the dependencies point of
view?
> > > 
> > > What data does the chromecast want exactly?
> > 
> > For now I think we'll want to know at least the range (full/limited), the
> > transfer function and the primaries being used by the video stream (and
> > specified in the WebM Colour element). But ideally we'll probably want to
have
> > access to all information available in the Colour element, even if for now
> some
> > of it going to be ignored.
> 
> That's everything except for the YUV->RGB matrix. :)
> Do you care what the format of this information is?
> If you got the primaries as a list of floats, would it matter?

I'll reply to this separately shortly.