Skia supports this through SkColorSpace::MakeICC and SkColorSpaceXform (though I believe you need to use the private version of MakeICC).  It is integrated with SkCodec.

This Skia functionality could potentially be made public and integrated with ImageDecoder.  However, I lean towards saying "this will be fixed when Chrome switches to SkCodec for image decoding", which is a long term goal anyway.

Yes, we're getting close to having SkCodec plumbed into Chrome and this should be fixed when that in progress work is done.

Submitted the following 2 patch to fix the color issue for CMYK images
https://skia-review.googlesource.com/#/c/20000/
https://codereview.chromium.org/2942873002/

I'm going to be picking up the Skia technical side of this from Matt.  Both those CLs lgtm in that sense.

What I'm not too familiar with is the strategic aspect of this.  Were we waiting on SkCodec to fix this because we thought it was a free way to fix the bug without spending any extra effort, or were we waiting on SkCodec for some other reason?  It looks like most of the effort we'd need to fix this bug has now already been spent.

> Were we waiting on SkCodec to fix this because we thought it was a free way to
> fix the bug without spending any extra effort, or were we waiting on SkCodec
> for some other reason?

My impression that it was partially saving the extra effort, and possibly to avoid exposing things on SkColorSpace we did not want to expose?

Yep, using SkCodec to decode end to end would have the nice side effect of encapsulating any non-RGB color spaces inside Skia.

What's the ETA on JPEG and WEBP SkCodec decodes?  We ok to keep waiting?

WebP might be pretty soon. It is similar to gif in the way we are handling it from the Chrome side. And gif is waiting on 1.) approval of our timing method to make sure we didn't regress, and 2.) adopting an update that landed in Skia which provides us the frame disposal method.

Jpeg on the other hand might be significantly more work. That might be a while.



If we adopt this change now, the down side would be that we are exposing Skia internals which we don't want to expose, right? So that is trade-off here? I'm also not sure how urgently we need this. The prior bug only had 7 stars since 2009.

Yeah, if we can keep non-RGB color spaces from leaking out of Skia, that's ideal.  There's really nothing we can do with them except during these image decodes, so it's nice to just not have them representable externally and keep them hidden away inside SkCodec.

SkColorSpace::MakeICC API is exposed from skia but currently it can be used only for RGB colorspace. Better to provide the  options to use for other color space also.

That's explicitly what we're trying to avoid.  It's easier to reason about colorspaces inside Skia if we can know that clients can only provide us RGB colorspaces.  Non-RGB color spaces are currently only useful in image decoding, and clients using SkCodec will never need to explicitly handle that intermediate color space object.

OK understood 

If we want to fix this bug sooner than we use SkCodec, what if we (temporarily) provide public Skia methods that read the ICC data from a jpg/webp and return the proper SkColorSpace? That way we hide ICCTypeFlag and still disallow manually creating non-RGB color spaces.

That sounds fine, but I thought the main ifrs was to avoid letting anyone but Skia see non-RGB SkColorSpaces.  If we're willing to do that, I don't see any big difference between that and https://skia-review.googlesource.com/c/20000/.

I know I just wrote that 6 minutes ago, but I'm not exactly sure what I was trying to write with "ifrs".  thrust?  gist?  idea?  Something like that...

Even with the switch to SkCodec, someone outside of Skia can see the non-RGB color space by calling

  codec->getInfo().colorSpace()

We could hide that by not storing it on fSrcInfo, which is already sort of a mix between "what the encoded data represents" and "what the caller should request". Then the fSrcInfo would be sRGB and we'd shift more towards the latter.

> I know I just wrote that 6 minutes ago, but I'm not exactly sure what I was
> trying to write with "ifrs".  thrust?  gist?  idea?  Something like that...

Haha, I couldn't find an acronym that looked right, but I think you moved your left hand to the right one key and meant to type "idea".

Oh, spot on about ifrs.

Yeah, I agree that exposing "what the caller should request" is nicer.

This may all just be paranoia, and I personally don't really mind trying this and maybe making a mistake.  We just want to make sure that by the time we get to drawing, we're not being asked to draw into or from anything weird.

Sounds good. What should the caller request? Is it something like:

  if (outColorType == F16) {
    return SRGBLinear;
  // Condition from SkImageInfoIsValidRenderingCS
  } else if (nativeColorSpace->gammaCloseToSRGB() || nativeColorSpace->gammaIsLinear()) {
    return nativeColorSpace;
  } else {
    return SRGB;
  }

I see that SkAndroidCodec::computeOutputColorSpace will allow the client to use the encoded color space if nativeColorSpace->isNumericalTransferFn() (or use the client's preference if prefColorSpace->isNumericalTransferFn()). Are such color spaces RGB?

I think the order I suggested is wrong - we probably want to return the native color space if it is valid for rendering. If it's not, perhaps we shouldn't suggest anything at all, and then the client can choose what makes sense for them/their color type?

Looking at both the Chromium [1] and Android [2] code for choosing an SkColorSpace for the decoded image, it looks like they need to know the color space even if it isn't supported for drawing. (On that note, they both query isNumericalTransferFn() - is that the right way to determine whether Skia can draw with it?) Chromium preserves the gamut, and Android checks the gamut to determine whether it is a wide gamut. Maybe we need to just expose
- the SkColorSpace, if isNumericalTransferFn
- else the matrix that describes the color gamut if there is one

?

[1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp?sq=package:chromium&l=540
[2] https://cs.chromium.org/chromium/src/third_party/skia/src/codec/SkAndroidCodec.cpp?q=skandroidcodec&sq=package:chromium&l=146

Talked this over with brianosman, and we agreed that my comments in #22 are the way to go. Filed skbug.com/6839

I just came across https://skia-review.googlesource.com/c/skia/+/9152, which added methods to SkImage to report the SkColorSpace, even if it's one we don't know how to draw. That seems to have been added in response to [1].

In that thread, the desire is to both (a) skip color space conversion (which can already be done in SkCodec with a null SkColorSpace on SkImageInfo) and (b) later apply the color space using a gfx::ColorSpace that was created from the SkImage's SkColorSpace.

To fully support that use case, it seems like we need to report the original SkColorSpace, even if it's one we cannot draw?

[1] https://groups.google.com/a/google.com/forum/?utm_medium=email&utm_source=footer#!msg/chrome-color/Xd358axEvtE/tcY51A2VDgAJ

Any interest in starting up the approach to fixing CMYK from Comment 5 again?  I think scroggo@, brianosman@, and I have all collectively mellowed out about exposing non-RGB color profiles outside Skia.

(We did even just try a simpler version of that approach, which is to drop the requirement to pass the desired profile type at all, but if we do that we lose the ability to drop mismatched profiles like a CMYK profile against RGB data.)

I should probably take myself off owner of this. I've got my plate full at the moment.

I'm reviewing the patches in #5. https://codereview.chromium.org/2942873002 Isn't taking my comments though, I'm guessing because that code review site is deprecated. It should probably be switched to PolyGerrit.

I would assign to nagarajan.n@samsung.com, who has uploaded CLs, but "Issue owner must be a project member", so I'll list it as me.

The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/6566b97b76d3cb76ee255a3d128d048376d300bd

commit 6566b97b76d3cb76ee255a3d128d048376d300bd
Author: Mike Reed <reed@google.com>
Date: Fri Oct 27 14:59:02 2017

add Type enum to SkColorSpace

Bug:  727128 
Change-Id: I116de4efd6e64504a4e1892f431f528533b1173a
Reviewed-on: https://skia-review.googlesource.com/64261
Commit-Queue: Mike Reed <reed@google.com>
Reviewed-by: Leon Scroggins <scroggo@google.com>

[modify] https://crrev.com/6566b97b76d3cb76ee255a3d128d048376d300bd/include/core/SkColorSpace.h
[modify] https://crrev.com/6566b97b76d3cb76ee255a3d128d048376d300bd/src/core/SkColorSpace.cpp

https://chromium-review.googlesource.com/c/chromium/src/+/741843 in Chromium rejects based on the Type, similar to how SkCodec already rejects some types.

https://skia-review.googlesource.com/c/skia/+/64841 in Skia makes SkColorSpace::MakeICC return all types we support (i.e. including CMYK), depending on the client to reject the wrong type (as in the Chromium change).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/673b9aae0375c7a290bc5688ad08868510f5c04e

commit 673b9aae0375c7a290bc5688ad08868510f5c04e
Author: Leon Scroggins III <scroggo@google.com>
Date: Mon Oct 30 15:12:16 2017

Read ICC profile's type and possibly reject it

Some images contain ICC profiles that do not match their color space.
After creating an SkColorSpace, if it does not have the right Type,
drop it.

This depends on https://skia-review.googlesource.com/c/skia/+/64261 to
build.

The only immediate behavior change should be if a CMYK image has an RGB
ICC profile, we will now drop the RGB profile. A follow-on change to
Skia (https://skia-review.googlesource.com/c/skia/+/64841) will allow
SkColorSpace::MakeICC to return CMYK SkColorSpaces. That will fix
Bug:  727128 , while this change will prevent that fix from "breaking"
images that are not CMYK but have CMYK profiles.

Bug:  727128 
Change-Id: I49b307b71d44e15f5491c7bd1b0d00eabada12bf
Reviewed-on: https://chromium-review.googlesource.com/741843
Reviewed-by: Mike Klein <mtklein@chromium.org>
Reviewed-by: Leon Scroggins <scroggo@chromium.org>
Commit-Queue: Leon Scroggins <scroggo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512496}
[modify] https://crrev.com/673b9aae0375c7a290bc5688ad08868510f5c04e/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp
[modify] https://crrev.com/673b9aae0375c7a290bc5688ad08868510f5c04e/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h
[modify] https://crrev.com/673b9aae0375c7a290bc5688ad08868510f5c04e/third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp
[modify] https://crrev.com/673b9aae0375c7a290bc5688ad08868510f5c04e/third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp

The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/f78b55cb94f4ac89b76a26d5a56d6380aa8fea6b

commit f78b55cb94f4ac89b76a26d5a56d6380aa8fea6b
Author: Leon Scroggins III <scroggo@google.com>
Date: Tue Oct 31 18:13:04 2017

Simplify SkColorSpace::MakeICC

Rather than restricting the supported ICC types in MakeICC, create any
ICC type that we support, and make the client reject them as necessary
by querying the SkColorSpace::Type.

Remove ICCTypeFlag and replace uses of it with SkColorSpace::Type.

This depends on a change in Chromium
(https://chromium-review.googlesource.com/c/chromium/src/+/741843).
Without that, this change will start allowing non-CMYK images to use
CMYK profiles.

Bug:  727128 
Change-Id: I085b4665e49bc80083264496d864cc4cd62ae914
Reviewed-on: https://skia-review.googlesource.com/64841
Commit-Queue: Leon Scroggins <scroggo@google.com>
Reviewed-by: Mike Klein <mtklein@chromium.org>

[modify] https://crrev.com/f78b55cb94f4ac89b76a26d5a56d6380aa8fea6b/tests/ColorSpaceXformTest.cpp
[modify] https://crrev.com/f78b55cb94f4ac89b76a26d5a56d6380aa8fea6b/src/core/SkColorSpace_A2B.cpp
[modify] https://crrev.com/f78b55cb94f4ac89b76a26d5a56d6380aa8fea6b/src/codec/SkHeifCodec.cpp
[modify] https://crrev.com/f78b55cb94f4ac89b76a26d5a56d6380aa8fea6b/src/core/SkColorSpace_A2B.h
[modify] https://crrev.com/f78b55cb94f4ac89b76a26d5a56d6380aa8fea6b/src/core/SkColorSpace_Base.h
[modify] https://crrev.com/f78b55cb94f4ac89b76a26d5a56d6380aa8fea6b/tools/colorspaceinfo.cpp
[modify] https://crrev.com/f78b55cb94f4ac89b76a26d5a56d6380aa8fea6b/src/core/SkColorSpace_ICC.cpp
[modify] https://crrev.com/f78b55cb94f4ac89b76a26d5a56d6380aa8fea6b/src/core/SkColorSpaceXform_A2B.cpp
[modify] https://crrev.com/f78b55cb94f4ac89b76a26d5a56d6380aa8fea6b/src/codec/SkPngCodec.cpp
[modify] https://crrev.com/f78b55cb94f4ac89b76a26d5a56d6380aa8fea6b/src/codec/SkWebpCodec.cpp
[modify] https://crrev.com/f78b55cb94f4ac89b76a26d5a56d6380aa8fea6b/src/codec/SkJpegCodec.cpp

Have we confirmed this works yet?  I don't think that last CL has even rolled into Chrome yet, right?

I confirmed it worked locally. I'm fine to leave it open until it rolls into Chrome.

Ah, great!  We should probably just wait until it rolls into Chrome head before closing the bug, so as not to confuse anyone else who goes to test it out.