Refactor image decoders to use 'colorSpace' instead of 'colorProfile'

Refactor image decoders to use 'colorSpace' instead of 'colorProfile'

This is a little bit more logical given that images (PNGs) can
encode color space information without actually using a color
profile.

This CL started as an attempt to improve support for these PNGs,
but I think it's useful to refactor first.

TBR=aelias@chromium.org
TBR=esprehn@chromium.org

BUG=

Committed: https://crrev.com/ae4f0f021e5e13154580b2ae57a71cbb573930b2
Cr-Commit-Position: refs/heads/master@{#428237}

[msarett, 2016-10-27 15:26:44]

Patchset #1 (id:1) has been deleted

[msarett, 2016-10-27 15:43:21]

msarett@google.com changed reviewers:
+ ccameron@chromium.org, fmalita@chromium.org, scroggo@chromium.org

[msarett, 2016-10-27 15:43:22]

scroggo@:
third_party/WebKit/Source/platform/image-decoders/

fmalita@:
Hopefully trivial...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
third_party/WebKit/Source/platform/graphics/test/MockImageDecoder.h

ccameron@:
See comment on slight behavior change in color correct
rendering mode.
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp

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

https://codereview.chromium.org/2454123002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:120:
DCHECK(colorSpace);
We lose the debug check for icc profiles that we fail to parse.

Maybe I should add this back in ImageDecoder.cpp (guarded or not by the color
correct rendering flag)?

I find it a little strange because there definitely are a small number of
garbage "profiles" on the web that we will correctly not parse.  But it could
also help catch bugs.

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

https://codereview.chromium.org/2454123002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:241:
// TODO (msarett):
I plan to follow up with this...

[f(malita), 2016-10-27 15:57:28]

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

https://codereview.chromium.org/2454123002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:183: return
m_actualDecoder ? m_actualDecoder->hasColorSpace() : m_hasColorProfile;
If we're renaming ImageDecoder::hasColorProfile -> hasColorSpace, would it make
sense to also rename DeferredImageDecoder::hasColorProfile?

[msarett, 2016-10-27 16:42:57]

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

https://codereview.chromium.org/2454123002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:183: return
m_actualDecoder ? m_actualDecoder->hasColorSpace() : m_hasColorProfile;
On 2016/10/27 15:57:28, f(malita) wrote:
> If we're renaming ImageDecoder::hasColorProfile -> hasColorSpace, would it
make
> sense to also rename DeferredImageDecoder::hasColorProfile?

Yeah I think so.  In general, it probably makes sense to push the profile->space
change outside of the decoders (which I was avoiding a little bit).

Patch Set 2 does this.

[f(malita), 2016-10-27 17:57:24]

On 2016/10/27 16:42:57, msarett wrote:
>
https://codereview.chromium.org/2454123002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
> (right):
> 
>
https://codereview.chromium.org/2454123002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:183:
return
> m_actualDecoder ? m_actualDecoder->hasColorSpace() : m_hasColorProfile;
> On 2016/10/27 15:57:28, f(malita) wrote:
> > If we're renaming ImageDecoder::hasColorProfile -> hasColorSpace, would it
> make
> > sense to also rename DeferredImageDecoder::hasColorProfile?
> 
> Yeah I think so.  In general, it probably makes sense to push the
profile->space
> change outside of the decoders (which I was avoiding a little bit).
> 
> Patch Set 2 does this.

Thanks!

core/, platform/graphics/ LGTM

You may need some extra owners for platform/exported and modules/, or feel free
to TBR since those are trivial changes.

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

https://codereview.chromium.org/2454123002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:368:
setColorSpaceAndComputeTransform(std::move(srcSpace));
nit: I would just inline these and skip the move.

https://codereview.chromium.org/2454123002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:373: if
(!srcSpace) {
Why is this early out needed?  Don't we want to clear m_srcSpace in this case?

[msarett, 2016-10-27 18:21:42]

Many thanks for the big review Florin!

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

https://codereview.chromium.org/2454123002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:368:
setColorSpaceAndComputeTransform(std::move(srcSpace));
On 2016/10/27 17:57:24, f(malita) wrote:
> nit: I would just inline these and skip the move.

sgtm

https://codereview.chromium.org/2454123002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:373: if
(!srcSpace) {
On 2016/10/27 17:57:24, f(malita) wrote:
> Why is this early out needed?  Don't we want to clear m_srcSpace in this case?

I think it makes sense to check before attempting to acquire the SpinLock, but I
agree that this is too soon to check.  I'll move this back where it was before.

Also, I think it makes sense to set the transform to nullptr up at the top.

[msarett, 2016-10-27 18:21:59]

The CQ bit was checked by msarett@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 18:22:31]

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

[scroggo_chromium, 2016-10-27 18:24:13]

image-decoders code lgtm

[commit-bot: I haz the power, 2016-10-27 19:20:18]

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

[commit-bot: I haz the power, 2016-10-27 19:20:19]

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

[msarett, 2016-10-27 20:24:59]

The CQ bit was checked by msarett@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 20:25:42]

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-27 22:36:12]

lgtm

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

https://codereview.chromium.org/2454123002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:120:
DCHECK(colorSpace);
On 2016/10/27 15:43:22, msarett wrote:
> We lose the debug check for icc profiles that we fail to parse.
> 
> Maybe I should add this back in ImageDecoder.cpp (guarded or not by the color
> correct rendering flag)?
> 
> I find it a little strange because there definitely are a small number of
> garbage "profiles" on the web that we will correctly not parse.  But it could
> also help catch bugs.

This doesn't worry me ATM -- indeed we may want to do histogramming in
ImageDecoder about what we failed to parse and how.

[commit-bot: I haz the power, 2016-10-27 23:54:48]

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

[commit-bot: I haz the power, 2016-10-27 23:54:49]

Dry run: This issue passed the CQ dry run.

[msarett, 2016-10-28 01:09:16]

The CQ bit was checked by msarett@google.com

[msarett, 2016-10-28 01:09:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
fmalita@chromium.org, scroggo@chromium.org
Link to the patchset: https://codereview.chromium.org/2454123002/#ps80001
(title: "Fix legacy ImageFrame")

[commit-bot: I haz the power, 2016-10-28 01:09:50]

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

[msarett, 2016-10-28 01:11:38]

Description was changed from

==========
Refactor image decoders to use 'colorSpace' instead of 'colorProfile'

This is a little bit more logical given that images (PNGs) can
encode color space information without actually using a color
profile.

This CL started as an attempt to improve support for these PNGs,
but I think it's useful to refactor first.

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

to

==========
Refactor image decoders to use 'colorSpace' instead of 'colorProfile'

This is a little bit more logical given that images (PNGs) can
encode color space information without actually using a color
profile.

This CL started as an attempt to improve support for these PNGs,
but I think it's useful to refactor first.

TBR=aelias@chromium.org
TBR=esprehn@chromium.org

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

[commit-bot: I haz the power, 2016-10-28 01:42:54]

Description was changed from

==========
Refactor image decoders to use 'colorSpace' instead of 'colorProfile'

This is a little bit more logical given that images (PNGs) can
encode color space information without actually using a color
profile.

This CL started as an attempt to improve support for these PNGs,
but I think it's useful to refactor first.

TBR=aelias@chromium.org
TBR=esprehn@chromium.org

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

to

==========
Refactor image decoders to use 'colorSpace' instead of 'colorProfile'

This is a little bit more logical given that images (PNGs) can
encode color space information without actually using a color
profile.

This CL started as an attempt to improve support for these PNGs,
but I think it's useful to refactor first.

TBR=aelias@chromium.org
TBR=esprehn@chromium.org

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

[commit-bot: I haz the power, 2016-10-28 01:42:55]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-10-28 01:45:56]

Description was changed from

==========
Refactor image decoders to use 'colorSpace' instead of 'colorProfile'

This is a little bit more logical given that images (PNGs) can
encode color space information without actually using a color
profile.

This CL started as an attempt to improve support for these PNGs,
but I think it's useful to refactor first.

TBR=aelias@chromium.org
TBR=esprehn@chromium.org

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

to

==========
Refactor image decoders to use 'colorSpace' instead of 'colorProfile'

This is a little bit more logical given that images (PNGs) can
encode color space information without actually using a color
profile.

This CL started as an attempt to improve support for these PNGs,
but I think it's useful to refactor first.

TBR=aelias@chromium.org
TBR=esprehn@chromium.org

BUG=

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

[commit-bot: I haz the power, 2016-10-28 01:45:57]

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