ICC profile color translations

ICC profile color translations

Uses QCMS to translate colors to/from ICC profile-based color spaces.

BUG=622133
Committed: https://crrev.com/c289ee43b1fc399a703cdda029b3601fa7bb0ad7
Cr-Commit-Position: refs/heads/master@{#409954}

[hubbe, 2016-08-04 17:41:30]

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

[hubbe, 2016-08-04 17:41:38]

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

[commit-bot: I haz the power, 2016-08-04 17:42:13]

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-08-04 17:59:31]

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

[commit-bot: I haz the power, 2016-08-04 17:59:31]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[hubbe, 2016-08-04 18:03:44]

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

[commit-bot: I haz the power, 2016-08-04 18:04:08]

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-08-04 18:17:17]

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

[commit-bot: I haz the power, 2016-08-04 18:17:18]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[hubbe, 2016-08-04 19:00:55]

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

[commit-bot: I haz the power, 2016-08-04 19:01:24]

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

[ccameron, 2016-08-04 19:07:35]

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

[ccameron, 2016-08-04 19:07:36]

+junov

I'm worried that we're creating too much of a proliferation of things here. In
particular, we now have
- gfx::ColorSpace/ColorTransform
- SkColorSpace transforms
- QCMS color transforms

We should, at least, consolidate these.

IIUC, long-term, we're all on the same page about wanting to lean on
SkColorSpace 100%.

In the short term, perhaps we could just have the named gfx::ColorSpace objects
create an ICC profile, and then lean 100% on QCMS to create the lookup table, in
all cases.

Sorry to hit the breaks like this, but I think we should probably write down
what we're planning to do in the short term, before turning it into code.

Also, I was talking with junov earlier, and he expressed some concern about LUT
performance/power characteristics (in particular, dependent texture fetches). A
doc would be a good place to document the results of that investigation (it may
be that these are only issues on mobile, where we're not about to deploy this
anytime soon).

[hubbe, 2016-08-04 19:25:27]

On 2016/08/04 19:07:36, ccameron wrote:
> +junov
> 
> I'm worried that we're creating too much of a proliferation of things here. In
> particular, we now have
> - gfx::ColorSpace/ColorTransform
> - SkColorSpace transforms
> - QCMS color transforms
> 
> We should, at least, consolidate these.
> 
> IIUC, long-term, we're all on the same page about wanting to lean on
> SkColorSpace 100%.
> 
> In the short term, perhaps we could just have the named gfx::ColorSpace
objects
> create an ICC profile, and then lean 100% on QCMS to create the lookup table,
in
> all cases.
> 
> Sorry to hit the breaks like this, but I think we should probably write down
> what we're planning to do in the short term, before turning it into code.
> 
> Also, I was talking with junov earlier, and he expressed some concern about
LUT
> performance/power characteristics (in particular, dependent texture fetches).
A
> doc would be a good place to document the results of that investigation (it
may
> be that these are only issues on mobile, where we're not about to deploy this
> anytime soon).

This implementation lets out swap out the internal implementation later though,
as it is
entirely hidden from the caller. AFAIK, SkColorSpace doesn't publicly expose the
methods needed
to do what ColorTransform does in this CL, which is why I'm using QCMS, but as
soon as
SkColorSpace can do it, we can swap it out. If you don't like QCMS, I can just
write my
own ICC parser. :)  (I almost did that anyways since I don't particularly like
QCMS)

I've done power measurements on LUTs, and on standard laptop computers the power
impact
was about 1%, very close to the limit what we can actually measure with the
tools we use
in the AV testing lab.

Clearly we need to do more power measurements before we launch, but at least on
intel
chipsets it doesn't seem to be a major hurdle. (I sent out the results of that
test
to chrome-color@ a while ago.)

As for documenting:
https://docs.google.com/document/d/1H7kaV-JHUAwCvuLeH1kiZUgydUEgLDuYrmO1Hqh-V...

I should probably update that doc though.

[commit-bot: I haz the power, 2016-08-04 20:00:13]

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

[commit-bot: I haz the power, 2016-08-04 20:00:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Justin Novosad, 2016-08-04 20:40:25]

On 2016/08/04 19:25:27, hubbe wrote:
> On 2016/08/04 19:07:36, ccameron wrote:
> > +junov
> > 
> > I'm worried that we're creating too much of a proliferation of things here.
In
> > particular, we now have
> > - gfx::ColorSpace/ColorTransform
> > - SkColorSpace transforms
> > - QCMS color transforms
> > 
> > We should, at least, consolidate these.
> > 
> > IIUC, long-term, we're all on the same page about wanting to lean on
> > SkColorSpace 100%.
> > 
> > In the short term, perhaps we could just have the named gfx::ColorSpace
> objects
> > create an ICC profile, and then lean 100% on QCMS to create the lookup
table,
> in
> > all cases.
> > 
> > Sorry to hit the breaks like this, but I think we should probably write down
> > what we're planning to do in the short term, before turning it into code.
> > 
> > Also, I was talking with junov earlier, and he expressed some concern about
> LUT
> > performance/power characteristics (in particular, dependent texture
fetches).
> A
> > doc would be a good place to document the results of that investigation (it
> may
> > be that these are only issues on mobile, where we're not about to deploy
this
> > anytime soon).
> 
> This implementation lets out swap out the internal implementation later
though,
> as it is
> entirely hidden from the caller. AFAIK, SkColorSpace doesn't publicly expose
the
> methods needed
> to do what ColorTransform does in this CL, which is why I'm using QCMS, but as
> soon as
> SkColorSpace can do it, we can swap it out. If you don't like QCMS, I can just
> write my
> own ICC parser. :)  (I almost did that anyways since I don't particularly like
> QCMS)
> 
> I've done power measurements on LUTs, and on standard laptop computers the
power
> impact
> was about 1%, very close to the limit what we can actually measure with the
> tools we use
> in the AV testing lab.
> 
> Clearly we need to do more power measurements before we launch, but at least
on
> intel
> chipsets it doesn't seem to be a major hurdle. (I sent out the results of that
> test
> to chrome-color@ a while ago.)
> 
> As for documenting:
>
https://docs.google.com/document/d/1H7kaV-JHUAwCvuLeH1kiZUgydUEgLDuYrmO1Hqh-V...
> 
> I should probably update that doc though.

I think this is succinct enough to serve well as temporary plumbing, to get
things working until skia is in all the right places, which will likely be a
much larger refactor.

No objections from me.

[ccameron, 2016-08-04 20:45:23]

cool, then this lgtm

I would vaguely prefer to do like-everything through QCMS if we're going to use
it at all, but whatever un-blocks progress for you the best.

[Justin Novosad, 2016-08-04 20:54:15]

FWIW, skia provides CPU+GPU implementations transforms and their use is
completely transparent to the caller. Just tag your images and rendering
surfaces with colorspace, and the right transforms get set-up automagically. 
Also, the GPU implementation leverages built-in hardware support for the sRGB
transfer curves, so we get them for free.  The skia team is investing a lot into
provide an efficient and simple to use implementation that is going to be a lot
of fun to work with IMHO.

[hubbe, 2016-08-04 22:26:38]

On 2016/08/04 20:54:15, junov (slow until August 8) wrote:
> FWIW, skia provides CPU+GPU implementations transforms and their use is
> completely transparent to the caller. Just tag your images and rendering
> surfaces with colorspace, and the right transforms get set-up automagically. 
> Also, the GPU implementation leverages built-in hardware support for the sRGB
> transfer curves, so we get them for free.  The skia team is investing a lot
into
> provide an efficient and simple to use implementation that is going to be a
lot
> of fun to work with IMHO.

Nice, but not enough.
In our case we need to be able to do the same thing to video textures and other
things, and
while it output is *usually* sRGB, inputs and/or outputs could be using the P/Q
transfer curve,
or some table from an ICC profile. Some transfer functions do not belong in a
shader.

Also, looking forward to HDR, we're probably going to need to do some things
where part of the
curve uses different transfer function than the rest to compress part of the
dynamic range.
3D-LUTs is the only thing we can ever do that is future-safe; meaning that no
matter how complex
the math gets, the rendering complexity stays the same.

[hubbe, 2016-08-04 22:35:25]

On 2016/08/04 20:45:23, ccameron wrote:
> cool, then this lgtm
> 
> I would vaguely prefer to do like-everything through QCMS if we're going to
use
> it at all, but whatever un-blocks progress for you the best.

It wouldn't work well.
QCMS doesn't support YUV color spaces. I might be able to trick it to produce
YUV anyways, but it would not handle the constant luminance case.

I'm hoping that as we go forward with moving this into skia, it will support
ICC profiles better than QCMS does.

[hubbe, 2016-08-04 22:39:18]

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

[commit-bot: I haz the power, 2016-08-04 22:40:09]

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-08-04 23:42:09]

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

[commit-bot: I haz the power, 2016-08-04 23:42:10]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hubbe, 2016-08-05 00:01:10]

The CQ bit was checked by hubbe@chromium.org

[hubbe, 2016-08-05 00:01:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
ccameron@chromium.org
Link to the patchset: https://codereview.chromium.org/2203213002/#ps80001
(title: "win compile fix")

[commit-bot: I haz the power, 2016-08-05 00:01:45]

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-08-05 00:50:14]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-05 00:52:23]

Description was changed from

==========
ICC profile color translations

Uses QCMS to translate colors to/from ICC profile-based color spaces.

BUG=622133
==========

to

==========
ICC profile color translations

Uses QCMS to translate colors to/from ICC profile-based color spaces.

BUG=622133

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

[commit-bot: I haz the power, 2016-08-05 00:52:24]

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