Plumb color space to DecodingImageGenerator and ImageFrameGenerator

Plumb color space to DecodingImageGenerator and ImageFrameGenerator

Image creation was failing in ExternalMemoryAllocator::allocPixelRef
because the SkImageInfo specified to the ExternalMemoryAllocator via
the ImageFrameGenerator that creates it did not include the color space
of the image. Populate this information by querying it from the decoder
in DecodingImageGenerator::create.

Change ImageDecoder::colorSpace to be the single place for enforcing
the rule that images that do not have embedded color profiles should be
interpreted as sRGB.

Change all callers to ImageFrame::setSizeAndColorSpace to query the
ImageDecoder::colorSpace, and enforce that the color space specified be
non-null.

Rename ImageDecoder::hasColorSpace to hasEmbeddedColorSpace to make it
clear that this refers only to an embedded color space (all images
have an explicit or implicit color space).

Rename ImageDecoder::setColorSpaceAndComputeTransform's overload to
setColorProfileAndComputeTransform, when the argument is a full ICC
profile.

BUG=44872
Committed: https://crrev.com/65402021deb370b590ec564d93f941733308f477
Cr-Commit-Position: refs/heads/master@{#428864}

[ccameron, 2016-10-29 02:17:56]

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

[commit-bot: I haz the power, 2016-10-29 02:18:09]

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

[ccameron, 2016-10-29 02:40:31]

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

[ccameron, 2016-10-29 02:40:32]

This is from the work that we did in NC a few weeks back.

[commit-bot: I haz the power, 2016-10-29 03:26:37]

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

[commit-bot: I haz the power, 2016-10-29 03:26:38]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ccameron, 2016-10-29 05:32:26]

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

[commit-bot: I haz the power, 2016-10-29 05:32:31]

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-10-29 06:50:02]

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

[commit-bot: I haz the power, 2016-10-29 06:50:03]

Dry run: This issue passed the CQ dry run.

[Justin Novosad, 2016-10-31 14:55:13]

https://codereview.chromium.org/2462803002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
(right):

https://codereview.chromium.org/2462803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:110:
m_colorSpace(colorSpace),
std::move

https://codereview.chromium.org/2462803002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h (right):

https://codereview.chromium.org/2462803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:70: new
ImageFrameGenerator(fullSize, colorSpace, isMultiFrame));
Should use std::move here

https://codereview.chromium.org/2462803002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right):

https://codereview.chromium.org/2462803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:372:
sk_sp<SkColorSpace> colorSpace = SkColorSpace::NewICC(iccData, iccLength);
I feel that at this point the nullness of ColorSpace should be checked and if
necessary a warning should be logged, stating that an undecodable color profile
was encountered.

https://codereview.chromium.org/2462803002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/2462803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:116:
m_colorSpace = colorSpace;
std::move

https://codereview.chromium.org/2462803002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp
(right):

https://codereview.chromium.org/2462803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:125:
m_parent->colorSpace())) {
don't we want to do this only if colorCorrectRendering is enabled?  same
question for all other decoders.

[ccameron, 2016-10-31 18:09:27]

Description was changed from

==========
Plumb color space to DecodingImageGenerator and ImageFrameGenerator

Image creation was failing in ExternalMemoryAllocator::allocPixelRef
because the SkImageInfo specified to the ExternalMemoryAllocator via
the ImageFrameGenerator that creates it did not include the color space
of the image. Populate this information by querying it from the decoder
in DecodingImageGenerator::create.

Change ImageDecoder::colorSpace to be the single place for enforcing
the rule that images that do not have embedded color profiles should be
interpreted as sRGB.

Change all callers to ImageFrame::setSizeAndColorSpace to query the
ImageDecoder::colorSpace, and enforce that the color space specified be
non-null.

Rename ImageDecoder::hasColorSpace to hasEmbeddedColorSpace to make it
clear that this refers only to an embedded color space (all images
have an explicit or implicit color space).

Rename ImageDecoder::setColorSpaceAndComputeTransform's overload to
setColorProfileAndComputeTransform, when the argument is a full ICC
profile.

BUG=
==========

to

==========
Plumb color space to DecodingImageGenerator and ImageFrameGenerator

Image creation was failing in ExternalMemoryAllocator::allocPixelRef
because the SkImageInfo specified to the ExternalMemoryAllocator via
the ImageFrameGenerator that creates it did not include the color space
of the image. Populate this information by querying it from the decoder
in DecodingImageGenerator::create.

Change ImageDecoder::colorSpace to be the single place for enforcing
the rule that images that do not have embedded color profiles should be
interpreted as sRGB.

Change all callers to ImageFrame::setSizeAndColorSpace to query the
ImageDecoder::colorSpace, and enforce that the color space specified be
non-null.

Rename ImageDecoder::hasColorSpace to hasEmbeddedColorSpace to make it
clear that this refers only to an embedded color space (all images
have an explicit or implicit color space).

Rename ImageDecoder::setColorSpaceAndComputeTransform's overload to
setColorProfileAndComputeTransform, when the argument is a full ICC
profile.

BUG=44872
==========

[ccameron, 2016-10-31 18:10:18]

Thanks -- updated.

I'd like to keep tag all inputs even when the flag is off (but bugs are
preventing this).

https://codereview.chromium.org/2462803002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
(right):

https://codereview.chromium.org/2462803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:110:
m_colorSpace(colorSpace),
On 2016/10/31 14:55:13, Justin Novosad wrote:
> std::move

Done.

https://codereview.chromium.org/2462803002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h (right):

https://codereview.chromium.org/2462803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h:70: new
ImageFrameGenerator(fullSize, colorSpace, isMultiFrame));
On 2016/10/31 14:55:13, Justin Novosad wrote:
> Should use std::move here

Done.

https://codereview.chromium.org/2462803002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right):

https://codereview.chromium.org/2462803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:372:
sk_sp<SkColorSpace> colorSpace = SkColorSpace::NewICC(iccData, iccLength);
On 2016/10/31 14:55:13, Justin Novosad wrote:
> I feel that at this point the nullness of ColorSpace should be checked and if
> necessary a warning should be logged, stating that an undecodable color
profile
> was encountered.

Yes, definitely. Added. We should also consider some UMAs about what sorts of
ICCs we encounter in images -- this will be the place to add them cause all
decoders go through here.

https://codereview.chromium.org/2462803002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/2462803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:116:
m_colorSpace = colorSpace;
On 2016/10/31 14:55:13, Justin Novosad wrote:
> std::move

Done.

I also added a comment clarifying the plan to always require that |colorSpace|
be non-null.

https://codereview.chromium.org/2462803002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp
(right):

https://codereview.chromium.org/2462803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/bmp/BMPImageReader.cpp:125:
m_parent->colorSpace())) {
On 2016/10/31 14:55:13, Justin Novosad wrote:
> don't we want to do this only if colorCorrectRendering is enabled?  same
> question for all other decoders.

Tagging input color spaces should have no effect, because only output color
space tagging triggers color correction.

That is, in theory. There is a bug
(https://bugs.chromium.org/p/skia/issues/detail?id=5907) where we fail to decode
scaled images when the color space isn't renderable, resulting in some images
ending up blank. I've added comments referencing that bug to the places where we
toss this value away.

Once that bug is fixed, I'll want to remove the checks, so that our inputs are
always tagged (just to limit the divergence hidden behind the flag).

[Justin Novosad, 2016-10-31 18:15:58]

lgtm

[ccameron, 2016-10-31 22:01:17]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2016-10-31 22:02:16]

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-10-31 23:30:53]

Description was changed from

==========
Plumb color space to DecodingImageGenerator and ImageFrameGenerator

Image creation was failing in ExternalMemoryAllocator::allocPixelRef
because the SkImageInfo specified to the ExternalMemoryAllocator via
the ImageFrameGenerator that creates it did not include the color space
of the image. Populate this information by querying it from the decoder
in DecodingImageGenerator::create.

Change ImageDecoder::colorSpace to be the single place for enforcing
the rule that images that do not have embedded color profiles should be
interpreted as sRGB.

Change all callers to ImageFrame::setSizeAndColorSpace to query the
ImageDecoder::colorSpace, and enforce that the color space specified be
non-null.

Rename ImageDecoder::hasColorSpace to hasEmbeddedColorSpace to make it
clear that this refers only to an embedded color space (all images
have an explicit or implicit color space).

Rename ImageDecoder::setColorSpaceAndComputeTransform's overload to
setColorProfileAndComputeTransform, when the argument is a full ICC
profile.

BUG=44872
==========

to

==========
Plumb color space to DecodingImageGenerator and ImageFrameGenerator

Image creation was failing in ExternalMemoryAllocator::allocPixelRef
because the SkImageInfo specified to the ExternalMemoryAllocator via
the ImageFrameGenerator that creates it did not include the color space
of the image. Populate this information by querying it from the decoder
in DecodingImageGenerator::create.

Change ImageDecoder::colorSpace to be the single place for enforcing
the rule that images that do not have embedded color profiles should be
interpreted as sRGB.

Change all callers to ImageFrame::setSizeAndColorSpace to query the
ImageDecoder::colorSpace, and enforce that the color space specified be
non-null.

Rename ImageDecoder::hasColorSpace to hasEmbeddedColorSpace to make it
clear that this refers only to an embedded color space (all images
have an explicit or implicit color space).

Rename ImageDecoder::setColorSpaceAndComputeTransform's overload to
setColorProfileAndComputeTransform, when the argument is a full ICC
profile.

BUG=44872
==========

[commit-bot: I haz the power, 2016-10-31 23:30:54]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-31 23:34:00]

Description was changed from

==========
Plumb color space to DecodingImageGenerator and ImageFrameGenerator

Image creation was failing in ExternalMemoryAllocator::allocPixelRef
because the SkImageInfo specified to the ExternalMemoryAllocator via
the ImageFrameGenerator that creates it did not include the color space
of the image. Populate this information by querying it from the decoder
in DecodingImageGenerator::create.

Change ImageDecoder::colorSpace to be the single place for enforcing
the rule that images that do not have embedded color profiles should be
interpreted as sRGB.

Change all callers to ImageFrame::setSizeAndColorSpace to query the
ImageDecoder::colorSpace, and enforce that the color space specified be
non-null.

Rename ImageDecoder::hasColorSpace to hasEmbeddedColorSpace to make it
clear that this refers only to an embedded color space (all images
have an explicit or implicit color space).

Rename ImageDecoder::setColorSpaceAndComputeTransform's overload to
setColorProfileAndComputeTransform, when the argument is a full ICC
profile.

BUG=44872
==========

to

==========
Plumb color space to DecodingImageGenerator and ImageFrameGenerator

Image creation was failing in ExternalMemoryAllocator::allocPixelRef
because the SkImageInfo specified to the ExternalMemoryAllocator via
the ImageFrameGenerator that creates it did not include the color space
of the image. Populate this information by querying it from the decoder
in DecodingImageGenerator::create.

Change ImageDecoder::colorSpace to be the single place for enforcing
the rule that images that do not have embedded color profiles should be
interpreted as sRGB.

Change all callers to ImageFrame::setSizeAndColorSpace to query the
ImageDecoder::colorSpace, and enforce that the color space specified be
non-null.

Rename ImageDecoder::hasColorSpace to hasEmbeddedColorSpace to make it
clear that this refers only to an embedded color space (all images
have an explicit or implicit color space).

Rename ImageDecoder::setColorSpaceAndComputeTransform's overload to
setColorProfileAndComputeTransform, when the argument is a full ICC
profile.

BUG=44872

Committed: https://crrev.com/65402021deb370b590ec564d93f941733308f477
Cr-Commit-Position: refs/heads/master@{#428864}
==========

[commit-bot: I haz the power, 2016-10-31 23:34:01]

Patchset 3 (id:??) landed as
https://crrev.com/65402021deb370b590ec564d93f941733308f477
Cr-Commit-Position: refs/heads/master@{#428864}