cc: Add color space to image decode caches

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}
Committed: https://chromium.googlesource.com/chromium/src/+/bdb2737223ac58596bffeb66a079dc6a80c3e5cc

[ccameron, 2017-04-03 23:21:22]

Description was changed from

==========
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=701942
==========

to

==========
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=701942
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[ccameron, 2017-04-03 23:31:59]

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

[commit-bot: I haz the power, 2017-04-03 23:32:35]

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, 2017-04-03 23:53:01]

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

[commit-bot: I haz the power, 2017-04-03 23:53:02]

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

[ccameron, 2017-04-04 00:28:42]

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

[commit-bot: I haz the power, 2017-04-04 00:29:19]

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

[ccameron, 2017-04-04 00:43:06]

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

[ccameron, 2017-04-04 00:43:06]

ptal -- this is the stuff that we were discussing earlier, all grown up and gone
evil.

This doesn't actually do conversion correctly in any but a small handful of
cases, but it plumbs the necessary arguments to the doorstep. I'm still
discussing with skia folks the best way to go about the remaining conversion
parts.

[ccameron, 2017-04-04 00:57:06]

Description was changed from

==========
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=701942
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
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
==========

[vmpstr, 2017-04-04 01:16:42]

vmpstr@chromium.org changed reviewers:
+ ericrk@chromium.org

[vmpstr, 2017-04-04 01:16:43]

Also +ericrk for gpu images

https://codereview.chromium.org/2797583002/diff/60001/cc/paint/discardable_im...
File cc/paint/discardable_image_map.cc (right):

https://codereview.chromium.org/2797583002/diff/60001/cc/paint/discardable_im...
cc/paint/discardable_image_map.cc:284:
images->push_back(all_images_[index].first.ApplyScaleAndColorSpace(
nit: Can you make this ApplyScale(...).ApplyColorSpace(...) instead?

https://codereview.chromium.org/2797583002/diff/60001/cc/paint/discardable_im...
File cc/paint/discardable_image_map_unittest.cc (right):

https://codereview.chromium.org/2797583002/diff/60001/cc/paint/discardable_im...
cc/paint/discardable_image_map_unittest.cc:46: gfx::ColorSpace
target_color_space = gfx::ColorSpace::CreateXYZD50();
Ah yes, the elusive XYZD50 color space. We meet again.

https://codereview.chromium.org/2797583002/diff/60001/cc/raster/raster_source.cc
File cc/raster/raster_source.cc (right):

https://codereview.chromium.org/2797583002/diff/60001/cc/raster/raster_source...
cc/raster/raster_source.cc:104: &settings.images_to_skip, target_color_space);
ImageHijackCanvas intercepts some calls to things like drawImage, grabs the
needed image out of the cache, and then passes that to the underlying canvas'
drawImage. I assume that works correctly here? That is, we would be passing a
color converted image to the canvas created on line 90. Are SkImages already
tagged with the right color space at that point?

(I'm trying to avoid double conversion)

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/gpu_image_deco...
File cc/tiles/gpu_image_decode_cache.cc (left):

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/gpu_image_deco...
cc/tiles/gpu_image_decode_cache.cc:115: //   ║       image_id       │ mip_level
│  quality  ║
Oh no, the fancy comment is gone!

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/gpu_image_deco...
File cc/tiles/gpu_image_decode_cache.cc (right):

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/gpu_image_deco...
cc/tiles/gpu_image_decode_cache.cc:140: return
static_cast<size_t>(cache_key.image_id) ^ (cache_key.mip_level << 16) ^
There has base hash functions that I think you should use here instead (see
base::HashInts). I'm not 100% sure what's better, but it seems a bit awkward to
have your own hashing.

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/gpu_image_deco...
File cc/tiles/gpu_image_decode_cache.h (right):

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/gpu_image_deco...
cc/tiles/gpu_image_decode_cache.h:271: explicit InUseCacheKey(const DrawImage&
draw_image);
Can you use a similar pattern as in the software case (static
InUseCacheKey::FromDrawImage(...))

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/software_image...
File cc/tiles/software_image_decode_cache.cc (right):

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/software_image...
cc/tiles/software_image_decode_cache.cc:969: if
(image.target_color_space().IsValid()) {
Hmm, right now the check whether we can use the original decode is only used for
sizing (ie, the scale adjustment is 1.f, 1.f if we can use it, which means that
the image is passed from here to skia without any additional scales. 

I _think_ this is OK, since the calculation of the scale adjustment should
result in 0 anyway if we have a low/none filter quality, but I was wondering why
this check?

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/software_image...
cc/tiles/software_image_decode_cache.cc:1039: // TODO(vmpstr): This is a mess.
Maybe it's faster to just search the vector
You probably need to add to this mess, since otherwise every pair of images that
only differ by color space will result in a hash collision.

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/software_image...
File cc/tiles/software_image_decode_cache_unittest.cc (right):

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/software_image...
cc/tiles/software_image_decode_cache_unittest.cc:723: gfx::ColorSpace
color_space_b(gfx::ColorSpace::PrimaryID::SMPTE170M,
:D

[ericrk, 2017-04-04 02:38:04]

A few comments on the GPU part. My biggest concerns:
-  ColorProfile objects are large (~100 bytes), I worry about including them in
cache keys - maybe ScopedRefptr (see my comment)?
- As written, the GPU cache won't ever store the same image with two different
profiles long-term. It will always orphan the previous color-profile if a user
requests a new one. This may be fine, just want to make sure this is what we
want.

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/gpu_image_deco...
File cc/tiles/gpu_image_decode_cache.cc (left):

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/gpu_image_deco...
cc/tiles/gpu_image_decode_cache.cc:115: //   ║       image_id       │ mip_level
│  quality  ║
On 2017/04/04 at 01:16:43, vmpstr wrote:
> Oh no, the fancy comment is gone!

;_;

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/gpu_image_deco...
File cc/tiles/gpu_image_decode_cache.cc (right):

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/gpu_image_deco...
cc/tiles/gpu_image_decode_cache.cc:121: static_assert(
this static_assert isn't needed now that we aren't squishing SkFilterQuality
into a uint16_t.

actually, the rest of the function is sort of not needed any more (comments and
DCHECKs mattered when we were packing things into a uin64_t)... collapse to {}?

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/gpu_image_deco...
cc/tiles/gpu_image_decode_cache.cc:817:
DCHECK(found_image->second->upload_params.fPreScaleMipLevel <=
Can you replace this DCHECK with one that covers color-space (and quality) as
well?

DCHECK(IsCompatible(found_image->second.get(), draw_image));

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/gpu_image_deco...
File cc/tiles/gpu_image_decode_cache.h (right):

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/gpu_image_deco...
cc/tiles/gpu_image_decode_cache.h:270: struct InUseCacheKey {
I don't think we ever access the internals of this class - can you make this a
class and make the data members private?

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/gpu_image_deco...
cc/tiles/gpu_image_decode_cache.h:276: gfx::ColorSpace target_color_space;
This looks really big (close to 100 bytes) - I sort of wonder about copying this
whole thing to the cache key.

Do you think we should do something like store a scoped_refptr to this (so it's
still copyable) and maybe cache the hash for performance?

[ccameron, 2017-04-04 06:41:09]

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

[ccameron, 2017-04-04 06:41:10]

Updated.

I could pass around an sk_sp<SkColorSpace> instead of a gfx::ColorSpace. I'm
sort-of-agnostic on this one ... if both of you prefer that (that the
cache-coherent ref+unref will be less expensive than the copy of the
gfx::ColorSpace), then I can make that change.

The only thing that we'd lose there would be having a hash for the color space,
but, as you mentioned, we'll only transiently have two color space versions of
the same image in the cache, so, having it not be part of the hash (and having
to make a SkColorSpace::equals call) probably isn't the worst thing ever.

https://codereview.chromium.org/2797583002/diff/60001/cc/paint/discardable_im...
File cc/paint/discardable_image_map.cc (right):

https://codereview.chromium.org/2797583002/diff/60001/cc/paint/discardable_im...
cc/paint/discardable_image_map.cc:284:
images->push_back(all_images_[index].first.ApplyScaleAndColorSpace(
On 2017/04/04 01:16:43, vmpstr wrote:
> nit: Can you make this ApplyScale(...).ApplyColorSpace(...) instead?

Done.

https://codereview.chromium.org/2797583002/diff/60001/cc/paint/discardable_im...
File cc/paint/discardable_image_map_unittest.cc (right):

https://codereview.chromium.org/2797583002/diff/60001/cc/paint/discardable_im...
cc/paint/discardable_image_map_unittest.cc:46: gfx::ColorSpace
target_color_space = gfx::ColorSpace::CreateXYZD50();
On 2017/04/04 01:16:43, vmpstr wrote:
> Ah yes, the elusive XYZD50 color space. We meet again.

Added a comment about the choice ... it's a "not nothing and not sRGB value...".

https://codereview.chromium.org/2797583002/diff/60001/cc/raster/raster_source.cc
File cc/raster/raster_source.cc (right):

https://codereview.chromium.org/2797583002/diff/60001/cc/raster/raster_source...
cc/raster/raster_source.cc:104: &settings.images_to_skip, target_color_space);
On 2017/04/04 01:16:43, vmpstr wrote:
> ImageHijackCanvas intercepts some calls to things like drawImage, grabs the
> needed image out of the cache, and then passes that to the underlying canvas'
> drawImage. I assume that works correctly here? That is, we would be passing a
> color converted image to the canvas created on line 90. Are SkImages already
> tagged with the right color space at that point?
> 
> (I'm trying to avoid double conversion)

Yeah, the input SkImage will be in "image color space" (usually sRGB, but could
be anything, in principle), and then the ImageHijackCanvas will replace it with
an SkImage that is in target_color_space. The SkColorSpaceXformCanvas will see
"oh, this image is already in my target_color_space, I don't need to do anything
here".

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/gpu_image_deco...
File cc/tiles/gpu_image_decode_cache.cc (right):

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/gpu_image_deco...
cc/tiles/gpu_image_decode_cache.cc:121: static_assert(
On 2017/04/04 02:38:04, ericrk wrote:
> this static_assert isn't needed now that we aren't squishing SkFilterQuality
> into a uint16_t.
> 
> actually, the rest of the function is sort of not needed any more (comments
and
> DCHECKs mattered when we were packing things into a uin64_t)... collapse to
{}?

Done.

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/gpu_image_deco...
cc/tiles/gpu_image_decode_cache.cc:140: return
static_cast<size_t>(cache_key.image_id) ^ (cache_key.mip_level << 16) ^
On 2017/04/04 01:16:43, vmpstr wrote:
> There has base hash functions that I think you should use here instead (see
> base::HashInts). I'm not 100% sure what's better, but it seems a bit awkward
to
> have your own hashing. 

Sounds reasonable -- added a nested HashInts(w, HashInts(x, HashInts(y, z)))

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/gpu_image_deco...
cc/tiles/gpu_image_decode_cache.cc:817:
DCHECK(found_image->second->upload_params.fPreScaleMipLevel <=
On 2017/04/04 02:38:04, ericrk wrote:
> Can you replace this DCHECK with one that covers color-space (and quality) as
> well?
> 
> DCHECK(IsCompatible(found_image->second.get(), draw_image));

Done (I just put this line here).

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/gpu_image_deco...
File cc/tiles/gpu_image_decode_cache.h (right):

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/gpu_image_deco...
cc/tiles/gpu_image_decode_cache.h:270: struct InUseCacheKey {
On 2017/04/04 02:38:04, ericrk wrote:
> I don't think we ever access the internals of this class - can you make this a
> class and make the data members private?

Done.

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/gpu_image_deco...
cc/tiles/gpu_image_decode_cache.h:271: explicit InUseCacheKey(const DrawImage&
draw_image);
On 2017/04/04 01:16:43, vmpstr wrote:
> Can you use a similar pattern as in the software case (static
> InUseCacheKey::FromDrawImage(...))

Done.

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/gpu_image_deco...
cc/tiles/gpu_image_decode_cache.h:276: gfx::ColorSpace target_color_space;
On 2017/04/04 02:38:04, ericrk wrote:
> This looks really big (close to 100 bytes) - I sort of wonder about copying
this
> whole thing to the cache key.
> 
> Do you think we should do something like store a scoped_refptr to this (so
it's
> still copyable) and maybe cache the hash for performance?

I'm surprised that this is a performance-critical path.

That said, gfx::ColorSpace needs to go on a diet. I have a patch to shrink it a
little, and I may change its floats to half-floats to shrink it some more (to
~40 bytes or so). And maybe even make it internally be a refptr of sorts.

That and that said, there's really no reason not to be using a
sk_sp<SkColorSpace> here. So, I could do that.

That and that and that said, WRT performance, I worry about the cost of
refcounting, especially if it needs to be thread-safe, and especially on ARM. My
(like, five-years-out-of-date) recollection is that doing a thread-safe refcount
increment on ARM was non-trivial (especially compared to just copying a ~100
bytes).

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/software_image...
File cc/tiles/software_image_decode_cache.cc (right):

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/software_image...
cc/tiles/software_image_decode_cache.cc:969: if
(image.target_color_space().IsValid()) {
On 2017/04/04 01:16:43, vmpstr wrote:
> Hmm, right now the check whether we can use the original decode is only used
for
> sizing (ie, the scale adjustment is 1.f, 1.f if we can use it, which means
that
> the image is passed from here to skia without any additional scales. 
> 
> I _think_ this is OK, since the calculation of the scale adjustment should
> result in 0 anyway if we have a low/none filter quality, but I was wondering
why
> this check?

I renamed can_use_original_decode to can_use_original_size_decode, since this
really just encodes information about size, and I removed the color space bit
here (but added it to the hash).

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/software_image...
cc/tiles/software_image_decode_cache.cc:1039: // TODO(vmpstr): This is a mess.
Maybe it's faster to just search the vector
On 2017/04/04 01:16:43, vmpstr wrote:
> You probably need to add to this mess, since otherwise every pair of images
that
> only differ by color space will result in a hash collision.

Added it to both sides of the if(), cause can_use_original_decode (now) is just
about size.

[ericrk, 2017-04-04 16:28:40]

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/gpu_image_deco...
File cc/tiles/gpu_image_decode_cache.h (right):

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/gpu_image_deco...
cc/tiles/gpu_image_decode_cache.h:276: gfx::ColorSpace target_color_space;
On 2017/04/04 06:41:10, ccameron wrote:
> On 2017/04/04 02:38:04, ericrk wrote:
> > This looks really big (close to 100 bytes) - I sort of wonder about copying
> this
> > whole thing to the cache key.
> > 
> > Do you think we should do something like store a scoped_refptr to this (so
> it's
> > still copyable) and maybe cache the hash for performance?
> 
> I'm surprised that this is a performance-critical path.
> 
> That said, gfx::ColorSpace needs to go on a diet. I have a patch to shrink it
a
> little, and I may change its floats to half-floats to shrink it some more (to
> ~40 bytes or so). And maybe even make it internally be a refptr of sorts.
> 
> That and that said, there's really no reason not to be using a
> sk_sp<SkColorSpace> here. So, I could do that.
> 
> That and that and that said, WRT performance, I worry about the cost of
> refcounting, especially if it needs to be thread-safe, and especially on ARM.
My
> (like, five-years-out-of-date) recollection is that doing a thread-safe
refcount
> increment on ARM was non-trivial (especially compared to just copying a ~100
> bytes).

I was actually more concerned about the memory cost - it feels like the
performance is probably not too big a deal in either case.

Also, given that the object holds an sk_sp<SkColorSpace>, isn't there a refcount
either way?

We can end up with a few hundred of these objects live at a time - 10 or 20k may
be an OK memory cost to pay, it just feels heavier than needed. vmpstr@, WDYT?

I don't feel super strongly - especially if we're going to slim this guy down.

[vmpstr, 2017-04-04 17:30:09]

Around now would actually be a good time to try and run this through cluster
telemetry to see if there are any perf impacts from the changes you're making
before we actually convert colors.

Specifically, I'd like to know what the impact of the color profile in the cache
keys is and what is the impact of the extra layer of canvas indirection at
raster.

Do you think that's reasonable to test?

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/gpu_image_deco...
File cc/tiles/gpu_image_decode_cache.h (right):

https://codereview.chromium.org/2797583002/diff/60001/cc/tiles/gpu_image_deco...
cc/tiles/gpu_image_decode_cache.h:276: gfx::ColorSpace target_color_space;
On 2017/04/04 16:28:40, ericrk wrote:
> On 2017/04/04 06:41:10, ccameron wrote:
> > On 2017/04/04 02:38:04, ericrk wrote:
> > > This looks really big (close to 100 bytes) - I sort of wonder about
copying
> > this
> > > whole thing to the cache key.
> > > 
> > > Do you think we should do something like store a scoped_refptr to this (so
> > it's
> > > still copyable) and maybe cache the hash for performance?
> > 
> > I'm surprised that this is a performance-critical path.
> > 
> > That said, gfx::ColorSpace needs to go on a diet. I have a patch to shrink
it
> a
> > little, and I may change its floats to half-floats to shrink it some more
(to
> > ~40 bytes or so). And maybe even make it internally be a refptr of sorts.
> > 
> > That and that said, there's really no reason not to be using a
> > sk_sp<SkColorSpace> here. So, I could do that.
> > 
> > That and that and that said, WRT performance, I worry about the cost of
> > refcounting, especially if it needs to be thread-safe, and especially on
ARM.
> My
> > (like, five-years-out-of-date) recollection is that doing a thread-safe
> refcount
> > increment on ARM was non-trivial (especially compared to just copying a ~100
> > bytes).
> 
> I was actually more concerned about the memory cost - it feels like the
> performance is probably not too big a deal in either case.
> 
> Also, given that the object holds an sk_sp<SkColorSpace>, isn't there a
refcount
> either way?
> 
> We can end up with a few hundred of these objects live at a time - 10 or 20k
may
> be an OK memory cost to pay, it just feels heavier than needed. vmpstr@, WDYT?
> 
> I don't feel super strongly - especially if we're going to slim this guy down.

I don't feel strongly about this. I think we do end up copying this objects,
although we've been careful about this in the past (in both gpu and software).
If we do move to sk_sp, does that mean we'll share the color space object itself
across different keys? If we always allocate a new one, the memory impact would
be the same...

I vote for making this an sk_sp to indicate that it's not a trivial thing to
copy, but am willing to be convinced otherwise.

https://codereview.chromium.org/2797583002/diff/80001/cc/paint/discardable_im...
File cc/paint/discardable_image_map.cc (right):

https://codereview.chromium.org/2797583002/diff/80001/cc/paint/discardable_im...
cc/paint/discardable_image_map.cc:220: gfx::ColorSpace target_color_space;
Is there something to initialize this to like ColorSpace::Whatever, or is the
default ctor smart enough to make this a reasonable value?

https://codereview.chromium.org/2797583002/diff/80001/cc/paint/draw_image.h
File cc/paint/draw_image.h (right):

https://codereview.chromium.org/2797583002/diff/80001/cc/paint/draw_image.h#n...
cc/paint/draw_image.h:48: DrawImage ApplyTargetColorSpace(const gfx::ColorSpace&
target_color_space) {
For various trivia buffs, this would actually be a nice case for overriding with
rvalue qualifiers..

for example,

DrawImage ApplyTargetColorSpace(color_space) && {
 target_color_space_ = color_space;
 return *this;
}

but i don't think that's allowed <_<

https://codereview.chromium.org/2797583002/diff/80001/cc/tiles/software_image...
File cc/tiles/software_image_decode_cache.cc (right):

https://codereview.chromium.org/2797583002/diff/80001/cc/tiles/software_image...
cc/tiles/software_image_decode_cache.cc:1043: hash_ = base::HashInts(hash_,
target_color_space.GetHash());
leave a comment here please

[ccameron, 2017-04-04 17:42:03]

Sg -- I'll update this to use an sk_sp<SkColorSpace> instead. I'll update here
when I'm done with that.

You're right -- there is an sk_sp<SkColorSpace> member ... it happens to be
nullptr ~99% of the time, but I was thinking to change that.

https://codereview.chromium.org/2797583002/diff/80001/cc/paint/discardable_im...
File cc/paint/discardable_image_map.cc (right):

https://codereview.chromium.org/2797583002/diff/80001/cc/paint/discardable_im...
cc/paint/discardable_image_map.cc:220: gfx::ColorSpace target_color_space;
On 2017/04/04 17:30:08, vmpstr wrote:
> Is there something to initialize this to like ColorSpace::Whatever, or is the
> default ctor smart enough to make this a reasonable value?

Default is a special "invalid" value which is ignored.

The sk_sp<SkColorSpace> equivalent is nullptr.

https://codereview.chromium.org/2797583002/diff/80001/cc/tiles/software_image...
File cc/tiles/software_image_decode_cache.cc (right):

https://codereview.chromium.org/2797583002/diff/80001/cc/tiles/software_image...
cc/tiles/software_image_decode_cache.cc:1043: hash_ = base::HashInts(hash_,
target_color_space.GetHash());
On 2017/04/04 17:30:08, vmpstr wrote:
> leave a comment here please

Sure -- if I leave it in, that is...

If I make I'll take a stab at making target_color_space be an
sk_sp<SkColorSpace> (in which case it won't enter the hash, cause it has no hash
function).

[ccameron, 2017-04-04 19:23:18]

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

[commit-bot: I haz the power, 2017-04-04 19:24:17]

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, 2017-04-04 19:29:09]

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

[commit-bot: I haz the power, 2017-04-04 19:29:10]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ccameron, 2017-04-04 19:31:08]

So, I started replacing this with sk_sp<SkColorSpace> and ... it just gets
messy, cause we have to convert back and forth (as opposed to always using
gfx::ColorSpace and only converting at the last minute).

gfx::ColorSpace is the way that these things are transported around (especially
in the compositor, where we need to represent YUV spaces, and other stuff that
isn't represented by SkColorSpace), so if we're going to worry about its size,
let's fix its size, not work around it.

The current weight for a gfx::ColorSpace is 88 bytes. It wouldn't be
unreasonable to make a gfx::ColorSpace wrap a refcounted ColorSpaceInternals,
and have a global map of those (in fact, it might be a good idea to do that, to
ensure that we re-use the SkColorSpace objects when we do need them).

[ericrk, 2017-04-05 22:05:05]

lgtm

I'm OK landing this if we have a plan to convert to ref-counted gfx::ColorSpace
(or something similar) in the near future.

[ccameron, 2017-04-05 22:19:03]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2017-04-05 22:19:54]

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-05 22:24:29]

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

[commit-bot: I haz the power, 2017-04-05 22:24:30]

Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[ccameron, 2017-04-05 23:00:34]

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

[commit-bot: I haz the power, 2017-04-05 23:01:24]

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, 2017-04-05 23:14:38]

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

[commit-bot: I haz the power, 2017-04-05 23:14:39]

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

[ccameron, 2017-04-05 23:44:58]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2017-04-05 23:44:58]

The patchset sent to the CQ was uploaded after l-g-t-m from ericrk@chromium.org
Link to the patchset: https://codereview.chromium.org/2797583002/#ps160001
(title: "Rebase but don't break the build")

[commit-bot: I haz the power, 2017-04-05 23:45:59]

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

[vmpstr, 2017-04-06 00:18:50]

lgtm with comments

https://codereview.chromium.org/2797583002/diff/140001/cc/tiles/gpu_image_dec...
File cc/tiles/gpu_image_decode_cache.cc (right):

https://codereview.chromium.org/2797583002/diff/140001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_cache.cc:1110: DLOG(ERROR) << "scalePixels failed.";
Are DLOGs equivalent to DCHECKs?

https://codereview.chromium.org/2797583002/diff/140001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_cache.cc:1123: DLOG(ERROR) <<
"getDeferredTextureImageData failed despite params "
The comment above makes me believe that this error might still be happening?

https://codereview.chromium.org/2797583002/diff/140001/cc/tiles/gpu_image_dec...
File cc/tiles/gpu_image_decode_cache.h (right):

https://codereview.chromium.org/2797583002/diff/140001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_cache.h:288: InUseCacheKey FromDrawImage(const
DrawImage& draw_image);
This is unused?

[ccameron, 2017-04-06 01:32:20]

The CQ bit was unchecked by ccameron@chromium.org

[ccameron, 2017-04-06 01:36:28]

Oop. Fixed.

https://codereview.chromium.org/2797583002/diff/140001/cc/tiles/gpu_image_dec...
File cc/tiles/gpu_image_decode_cache.cc (right):

https://codereview.chromium.org/2797583002/diff/140001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_cache.cc:1110: DLOG(ERROR) << "scalePixels failed.";
On 2017/04/06 00:18:50, vmpstr wrote:
> Are DLOGs equivalent to DCHECKs?

They just complain, they don't crash.

https://codereview.chromium.org/2797583002/diff/140001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_cache.cc:1123: DLOG(ERROR) <<
"getDeferredTextureImageData failed despite params "
On 2017/04/06 00:18:50, vmpstr wrote:
> The comment above makes me believe that this error might still be happening?

Yeah, this was a failure mode that I hit sometimes, and so I found it better to
not silently fail (at least on debug buidls).

https://codereview.chromium.org/2797583002/diff/140001/cc/tiles/gpu_image_dec...
File cc/tiles/gpu_image_decode_cache.h (right):

https://codereview.chromium.org/2797583002/diff/140001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_cache.h:288: InUseCacheKey FromDrawImage(const
DrawImage& draw_image);
On 2017/04/06 00:18:50, vmpstr wrote:
> This is unused?

Oop, yes, it can be killed.

[ccameron, 2017-04-06 01:36:33]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2017-04-06 01:36:34]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org,
ericrk@chromium.org
Link to the patchset: https://codereview.chromium.org/2797583002/#ps180001
(title: "Remove dead code")

[commit-bot: I haz the power, 2017-04-06 01:37:09]

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-06 01:42:53]

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

[commit-bot: I haz the power, 2017-04-06 01:42:55]

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_...)

[ccameron, 2017-04-06 04:25:43]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2017-04-06 04:25:56]

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-06 06:27:26]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1491452743177050,
"parent_rev": "fa6adcb94c4d1db18dbe1c91f780f6a1c785f978", "commit_rev":
"bdb2737223ac58596bffeb66a079dc6a80c3e5cc"}

[commit-bot: I haz the power, 2017-04-06 06:29:17]

Description was changed from

==========
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
==========

to

==========
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}
Committed:
https://chromium.googlesource.com/chromium/src/+/bdb2737223ac58596bffeb66a079...
==========

[commit-bot: I haz the power, 2017-04-06 06:29:18]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/bdb2737223ac58596bffeb66a079...