Strangely enough, when the image is embedded in the page here, the top line is red (which is wrong).

Let me know if there's anything I can do to help reduce this issue.

I'll take a look.

Alright, I know what's going on here.  So, step by step...

(1) We call drawImage(img) on the SkCSXformCanvas.
(2) We call img->makeColorSpace(targetColorSpace) to xform the image to a new color space.
(3) The img happens to still be encoded data - backed by DecodingImageGenerator.  The img->makeColorSpace(targetCS), triggers a call to DecodingImageGenerator::directGeneratePixels(targetCS) which calls the virtual DecodingImageGenerator::onGetPixels(targetCS).
(4) DecodingImageGenerator's implementation of onGetPixels(targetCS) completely ignores the targetCS - the decoded color space is controlled by other mechanisms.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp?dr=CSs&l=73
SkCodecImageGenerator (which Skia uses for testing) will decode into the targetCS as requested.

I think the fix here is to respect the targetCS in DecodingImageGenerator::onGetPixels().  I'll start writing that.

Ideally, we would do the color space transformation to targetCS at decode time.  Or we could do it in a separate pass after.  Chris, does this raise any red flags?  I'm hoping it won't interfere with the various other pathways that trigger image decodes.

I would suspect that doing it in a separate pass after decoder, rather than at decode time, won't make a huge difference (none of the ImageDecoder instances do color conversion during decode -- at best they do it row-by-row as the RGBA data comes out). Worst case is that we get slightly less cache use (I could be wrong, though).

The main concern is to ensure that, if two different canvases request two different color spaces, we do indeed give them what they asked for (we don't just convert once and forget about it).

Attaching another example file.

Deleted: red-at-12-oclock-with-color-profile.png 209 KB

	red-at-12-oclock-with-color-profile.png 209 KB View Download

As you might already know, the example in #6 shows a problem with my CL :).
https://codereview.chromium.org/2787053004/

SkPixmap::readPixels() (and other raster color space conversion APIs) require that the transfer functions be sRGB or Linear.

The idea is that weird transfer functions will be handled by the codec.

I could do a couple things:
(1) Try to rewrite the CL to make the ImageDecoder handle the color space transformation.
(2) Directly use SkColorSpaceXform in DecodingImageGenerator::onGetPixels().
(3) Expand the capabilities of SkPixmap::readPixels().

I'm leaning towards (2), but it's worth mentioning that I'm retyping code that lives in other places.

FWIW, I think I have the same bug here - which is an argument for (3):
https://cs.chromium.org/chromium/src/third_party/skia/src/image/SkImage_Raster.cpp?l=379

Hmm, it also appears that how I'm using SkCreateColorSpaceXformCanvas seems to be breaking the various cc:: Image caches.

In particular, with the way that I put in SkCreateColorSpaceXformCanvas, we call DecodingImageGenerator::onGetPixels every time we rasterize the same image.

If I don't use it, then we only hit it once, either through cc::SoftwareImageDecodeCache or through cc::GpuImageDecodeCache, depending on if we're doing GPU raster or not.

Is it because the SkColorSpaceXformCanvas is wrapping the ImageHijackCanvas?  So we're triggering the decode before the image can be hijacked?

I'll give that a try, maybe I'm doing this the hard way.

Okay, this is pretty straight-forward -- I need to wrap the ImageHijackCanvas in a SkColorSpaceXformCanvas, and add target-color-space-awareness to the image cache.

I can take this from here if you'd like (cause it's mostly cc:: work).

Sounds good to me.

Does that mean you'll entirely bypass the bugs in DecodingImageGenerator?
  Regardless, I think I'll go ahead and finish fixing that code path anyway.

Oh, I was planning to use DecodingImageGenerator, using an SkColorSpaceXform in there to do the conversion. If you're planning to grab the DecodingImageGenerator part, go for it.

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

commit bdb2737223ac58596bffeb66a079dc6a80c3e5cc
Author: ccameron <ccameron@chromium.org>
Date: Thu Apr 06 06:27:16 2017

cc: Add color space to image decode caches

Add a color space argument to the image decode caches, so that they can
- create SkImages in the correct color space
- not re-use SkImages decoded into different color spaces

Add a gfx::ColorSpace argument to DrawImage, which is used (roughly) as
a key in SoftwareImageDecodeCache and GpuImageDecodeCache. Ensure that
the caches respect this key and add tests. Do not ensure that the caches
always create SkImages using this color space, because that
functionality still needs some work.

Specify a gfx::ColorSpace to the ImageHijackCanvas, indicating the
target color space for images to be put in and pulled from the image
caches.

Update RasterSource::GetDiscardableImagesInRect to specify the color
space for images, and plumb this through to
DiscardableImageMap::GetDiscardableImagesInRect, which will populate
the color spaces for decoding tasks.

This patch will have no effect while color correct rendering is
disabled, because the ColorSpace objects being passed around will
all be invalid.

BUG= 706613 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2797583002
Cr-Commit-Position: refs/heads/master@{#462371}

[modify] https://crrev.com/bdb2737223ac58596bffeb66a079dc6a80c3e5cc/cc/layers/recording_source_unittest.cc
[modify] https://crrev.com/bdb2737223ac58596bffeb66a079dc6a80c3e5cc/cc/paint/discardable_image_map.cc
[modify] https://crrev.com/bdb2737223ac58596bffeb66a079dc6a80c3e5cc/cc/paint/discardable_image_map.h
[modify] https://crrev.com/bdb2737223ac58596bffeb66a079dc6a80c3e5cc/cc/paint/discardable_image_map_unittest.cc
[modify] https://crrev.com/bdb2737223ac58596bffeb66a079dc6a80c3e5cc/cc/paint/display_item_list.cc
[modify] https://crrev.com/bdb2737223ac58596bffeb66a079dc6a80c3e5cc/cc/paint/display_item_list.h
[modify] https://crrev.com/bdb2737223ac58596bffeb66a079dc6a80c3e5cc/cc/paint/draw_image.cc
[modify] https://crrev.com/bdb2737223ac58596bffeb66a079dc6a80c3e5cc/cc/paint/draw_image.h
[modify] https://crrev.com/bdb2737223ac58596bffeb66a079dc6a80c3e5cc/cc/raster/image_hijack_canvas.cc
[modify] https://crrev.com/bdb2737223ac58596bffeb66a079dc6a80c3e5cc/cc/raster/image_hijack_canvas.h
[modify] https://crrev.com/bdb2737223ac58596bffeb66a079dc6a80c3e5cc/cc/raster/image_hijack_canvas_unittest.cc
[modify] https://crrev.com/bdb2737223ac58596bffeb66a079dc6a80c3e5cc/cc/raster/raster_source.cc
[modify] https://crrev.com/bdb2737223ac58596bffeb66a079dc6a80c3e5cc/cc/raster/raster_source.h
[modify] https://crrev.com/bdb2737223ac58596bffeb66a079dc6a80c3e5cc/cc/raster/raster_source_unittest.cc
[modify] https://crrev.com/bdb2737223ac58596bffeb66a079dc6a80c3e5cc/cc/tiles/checker_image_tracker_unittest.cc
[modify] https://crrev.com/bdb2737223ac58596bffeb66a079dc6a80c3e5cc/cc/tiles/gpu_image_decode_cache.cc
[modify] https://crrev.com/bdb2737223ac58596bffeb66a079dc6a80c3e5cc/cc/tiles/gpu_image_decode_cache.h
[modify] https://crrev.com/bdb2737223ac58596bffeb66a079dc6a80c3e5cc/cc/tiles/gpu_image_decode_cache_unittest.cc
[modify] https://crrev.com/bdb2737223ac58596bffeb66a079dc6a80c3e5cc/cc/tiles/image_controller.cc
[modify] https://crrev.com/bdb2737223ac58596bffeb66a079dc6a80c3e5cc/cc/tiles/software_image_decode_cache.cc
[modify] https://crrev.com/bdb2737223ac58596bffeb66a079dc6a80c3e5cc/cc/tiles/software_image_decode_cache.h
[modify] https://crrev.com/bdb2737223ac58596bffeb66a079dc6a80c3e5cc/cc/tiles/software_image_decode_cache_perftest.cc
[modify] https://crrev.com/bdb2737223ac58596bffeb66a079dc6a80c3e5cc/cc/tiles/software_image_decode_cache_unittest.cc
[modify] https://crrev.com/bdb2737223ac58596bffeb66a079dc6a80c3e5cc/cc/tiles/tile_manager.cc
[modify] https://crrev.com/bdb2737223ac58596bffeb66a079dc6a80c3e5cc/cc/trees/layer_tree_host_impl.cc

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

commit 669daddab5f2f01c57add7ec00ab87fa8453b463
Author: msarett <msarett@google.com>
Date: Thu Apr 13 17:41:52 2017

Respect colorSpace in DecodingImageGenerator::onGetPixels()

ImageDecoder.cpp
In case the of a rare image with an embedded color space that
is not parametric, we will do a color space xform at decode
time.  Otherwise, leave it as is.

DecodingImageGenerator.cpp
Based on cc behavior, we should almost always be asking for
the SkImage "as is" (in the same color space that it is
already in).  In the rare case that we miss the cc cache and
request it in a different color space, will need to do the
color space xform here.

We always use an unpremul decoder when we will be doing a
color space xform in a second pass.  That way we don't
need to unpremultiply.

BUG= 706613 

Review-Url: https://codereview.chromium.org/2787053004
Cr-Commit-Position: refs/heads/master@{#464458}

[modify] https://crrev.com/669daddab5f2f01c57add7ec00ab87fa8453b463/third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp
[modify] https://crrev.com/669daddab5f2f01c57add7ec00ab87fa8453b463/third_party/WebKit/Source/platform/graphics/ImageDecodingStore.cpp
[modify] https://crrev.com/669daddab5f2f01c57add7ec00ab87fa8453b463/third_party/WebKit/Source/platform/graphics/ImageDecodingStore.h
[modify] https://crrev.com/669daddab5f2f01c57add7ec00ab87fa8453b463/third_party/WebKit/Source/platform/graphics/ImageDecodingStoreTest.cpp
[modify] https://crrev.com/669daddab5f2f01c57add7ec00ab87fa8453b463/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
[modify] https://crrev.com/669daddab5f2f01c57add7ec00ab87fa8453b463/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h
[modify] https://crrev.com/669daddab5f2f01c57add7ec00ab87fa8453b463/third_party/WebKit/Source/platform/graphics/ImageFrameGeneratorTest.cpp
[modify] https://crrev.com/669daddab5f2f01c57add7ec00ab87fa8453b463/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp
[modify] https://crrev.com/669daddab5f2f01c57add7ec00ab87fa8453b463/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h

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

commit 11694bf1f0daf864723ed81304720ba949db4517
Author: ccameron <ccameron@chromium.org>
Date: Thu Apr 13 22:43:11 2017

color: Enable color conversion in image decoder caches

In SoftwareImageDecodeCache, specify the color space for readPixels().
In the case of scalePixels, don't specify a color space, but interpret the
colors as being in this space at the end.

Make DecodingImageGenerator::onGetPixels() respect this color space
by performing conversion if needed.

In GpuImageDecodeCache, call makeColorSpace on the decoder-backed
image before uploading pixels. Add TODOs to make this call after upload.

BUG= 706613 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2801413002
Cr-Commit-Position: refs/heads/master@{#464581}

[modify] https://crrev.com/11694bf1f0daf864723ed81304720ba949db4517/cc/tiles/gpu_image_decode_cache.cc
[modify] https://crrev.com/11694bf1f0daf864723ed81304720ba949db4517/cc/tiles/software_image_decode_cache.cc
[modify] https://crrev.com/11694bf1f0daf864723ed81304720ba949db4517/cc/tiles/software_image_decode_cache.h