Bug 81974 – Adds color management support to Chromium port.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tony Payne]

Adds color management support to Chromium port.

[Tony Payne]

Created attachment 134126 [details]
Patch for crbug.com/143, Part 1 of 2

This also includes a hack for using git on windows. I'll remove before review.

So far, no attempt at any optimization.

[Tony Payne]

Created attachment 134127 [details]
Fix for crbug.com/143, Part 2 of 2

See comment for Part 1

[Tony Payne]

Created attachment 135635 [details]
Patch for crbug.com/143 part 1

[Tony Payne]

Created attachment 135636 [details]
Pat

[Tony Payne]

Created attachment 135637 [details]
Patch for crbug.com/143 part 2

[Tony Payne]

Created attachment 135891 [details]
Patch for crbug.com/143 part 1

[Tony Payne]

Created attachment 135892 [details]
Patch for crbug.com/143 part 2

[Tony Payne]

Created attachment 135893 [details]
Patch for crbug.com/143 part 1

[Tony Payne]

Created attachment 136755 [details]
Patch

[WebKit Review Bot]

Attachment 136755 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Pl..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 136755 [details]
Patch

Attachment 136755 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12392063

New failing tests:
css3/filters/effect-blur-hw.html
css3/filters/effect-combined.html
css3/filters/effect-sepia.html
css3/filters/crash-filter-change.html
css3/filters/effect-blur.html
css3/filters/effect-sepia-hw.html
css3/filters/effect-invert-hw.html
css3/filters/effect-grayscale.html
css3/filters/custom/missing-custom-filter-shader.html
accessibility/aria-disabled.html
css3/filters/effect-brightness-hw.html
css3/filters/effect-saturate.html
css3/filters/effect-hue-rotate.html
css3/filters/effect-opacity-hw.html
css3/filters/effect-invert.html
css3/filters/effect-combined-hw.html
css3/filters/effect-contrast.html
css3/filters/effect-brightness.html
css3/filters/effect-opacity.html
css3/filters/crash-hw-sw-switch.html
css3/filters/effect-drop-shadow.html
css3/filters/effect-hue-rotate-hw.html
compositing/visibility/visibility-image-layers.html
css3/filters/effect-drop-shadow-hw.html
css3/filters/effect-grayscale-hw.html
compositing/masks/direct-image-mask.html
compositing/color-matching/image-color-matching.html
css3/filters/effect-contrast-hw.html
css3/filters/effect-saturate-hw.html
compositing/reflections/simple-composited-reflections.html

[WebKit Review Bot]

Created attachment 136785 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Tony Payne]

(In reply to comment #11)
> (From update of attachment 136755 [details])
> Attachment 136755 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12392063
> 
> New failing tests:
> css3/filters/effect-blur-hw.html
> css3/filters/effect-combined.html
> css3/filters/effect-sepia.html
> css3/filters/crash-filter-change.html
> css3/filters/effect-blur.html
> css3/filters/effect-sepia-hw.html
> css3/filters/effect-invert-hw.html
> css3/filters/effect-grayscale.html
> css3/filters/custom/missing-custom-filter-shader.html
> accessibility/aria-disabled.html
> css3/filters/effect-brightness-hw.html
> css3/filters/effect-saturate.html
> css3/filters/effect-hue-rotate.html
> css3/filters/effect-opacity-hw.html
> css3/filters/effect-invert.html
> css3/filters/effect-combined-hw.html
> css3/filters/effect-contrast.html
> css3/filters/effect-brightness.html
> css3/filters/effect-opacity.html
> css3/filters/crash-hw-sw-switch.html
> css3/filters/effect-drop-shadow.html
> css3/filters/effect-hue-rotate-hw.html
> compositing/visibility/visibility-image-layers.html
> css3/filters/effect-drop-shadow-hw.html
> css3/filters/effect-grayscale-hw.html
> compositing/masks/direct-image-mask.html
> compositing/color-matching/image-color-matching.html
> css3/filters/effect-contrast-hw.html
> css3/filters/effect-saturate-hw.html
> compositing/reflections/simple-composited-reflections.html

These failures are expected. Will rebaseline once https://bugs.webkit.org/show_bug.cgi?id=83799 lands.

[Tony Payne]

Created attachment 137101 [details]
Patch

[Tony Payne]

Created attachment 137569 [details]
Patch

[WebKit Review Bot]

Comment on attachment 137569 [details]
Patch

Attachment 137569 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12424124

New failing tests:
svg/as-border-image/svg-as-border-image.html

[WebKit Review Bot]

Created attachment 137591 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Review Bot]

Comment on attachment 137569 [details]
Patch

Attachment 137569 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12415962

New failing tests:
svg/as-border-image/svg-as-border-image.html

[WebKit Review Bot]

Created attachment 137608 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Eric Seidel (no email)]

Comment on attachment 137569 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137569&action=review

How much of a perf hit to we take on the perf cyclers?  This seems reasonable, but w/o assuming sRGB for untagged images or matching to the monitor profile, I'm surprised this helps much.

> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:35
> +#include "third_party/qcms/src/qcms.h"

Sigh.  At some point we may want some CMS abstraction for WebCore/platform to share more of this code between ports?

[Tony Payne]

(In reply to comment #20)
> (From update of attachment 137569 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137569&action=review
> 
> How much of a perf hit to we take on the perf cyclers?  This seems reasonable, but w/o assuming sRGB for untagged images or matching to the monitor profile, I'm surprised this helps much.

I don't know the answer to this. I couldn't find any good information on how to run/add performance tests, even after asking around. I'd love some guidance. I did a totally inadequate manual test with a large image tagged with AdobeRGB 1998. Before my patch, this image took an average of 37ms to render on a release build of chromium linux. With my patch, it rendered in an average of 42ms.

The vast majority of monitors are sRGB. The outstanding items will only help users who have wide gamut monitors. I don't intend to leave those items outstanding for very long.

> > Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:35
> > +#include "third_party/qcms/src/qcms.h"
> 
> Sigh.  At some point we may want some CMS abstraction for WebCore/platform to share more of this code between ports?

This thought definitely crossed my mind, especially as I was swapping between lcms and qcms to compare results (lcms was 5x slower than qcms in my inadequate microbenchmark). Most CMS APIs look quite similar, so it should be quite doable.

[noel gordon]

(In reply to comment #21)
> (In reply to comment #20)

> This thought definitely crossed my mind, especially as I was swapping between lcms and qcms to compare results (lcms was 5x slower than qcms in my inadequate microbenchmark). Most CMS APIs look quite similar, so it should be quite doable.

Your benchmark seems to match http://muizelaar.blogspot.com.au/2009/10/qcms-now-faster.html (that comparison was done on x64 mac build).  Let's agree that qcms is fast.

[noel gordon]

(In reply to comment #21)
> (In reply to comment #20)
> > (From update of attachment 137569 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=137569&action=review
> > 
> > How much of a perf hit to we take on the perf cyclers?  This seems reasonable, but w/o assuming sRGB for untagged images or matching to the monitor profile, I'm surprised this helps much.
> 
> I don't know the answer to this. I couldn't find any good information on how to run/add performance tests, even after asking around. I'd love some guidance.

In the past, I created a representative corpus of decent size and used it to study the performance impact of changes to the image decoders (bug 78323 for example).

> I did a totally inadequate manual test with a large image tagged with AdobeRGB 1998. Before my patch, this image took an average of 37ms to render on a release build of chromium linux. With my patch, it rendered in an average of 42ms.

I assume this result was is with parallel jobs, 8 threads?  One image is not much to go on, but there's a 13% hit here.  The performance aspect is the most worrying part of this change for me.

It seems that qcms only supports rgba pixels.  That's the reason you have to swizzle the bytes of all image rows (twice) in the inadequately named flip() routine if I'm reading correctly?  Those row bytes swizzles are expensive - they were removed from the JPEG encoder for a 2x increase in decoding performance (bug 59670).

[Tony Payne]

(In reply to comment #23)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > (From update of attachment 137569 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=137569&action=review
> > > 
> > > How much of a perf hit to we take on the perf cyclers?  This seems reasonable, but w/o assuming sRGB for untagged images or matching to the monitor profile, I'm surprised this helps much.
> > 
> > I don't know the answer to this. I couldn't find any good information on how to run/add performance tests, even after asking around. I'd love some guidance.
> 
> In the past, I created a representative corpus of decent size and used it to study the performance impact of changes to the image decoders (bug 78323 for example).
> 
> > I did a totally inadequate manual test with a large image tagged with AdobeRGB 1998. Before my patch, this image took an average of 37ms to render on a release build of chromium linux. With my patch, it rendered in an average of 42ms.
> 
> I assume this result was is with parallel jobs, 8 threads?  One image is not much to go on, but there's a 13% hit here.  The performance aspect is the most worrying part of this change for me.

Yes, that was with the current state of the patch. My personal opinion is that it's better to be somewhat slower than to render incorrectly.

> It seems that qcms only supports rgba pixels.  That's the reason you have to swizzle the bytes of all image rows (twice) in the inadequately named flip() routine if I'm reading correctly?  Those row bytes swizzles are expensive - they were removed from the JPEG encoder for a 2x increase in decoding performance (bug 59670).

lcms also supports only rgba. From an API perspective, it supports bgra but internally it does the exact same swizzle. I think it's important to remember that this only affects images with an embedded color profile (for now, at least). These images are not nearly as common as images without profiles and usually are only included when the publisher cares about colors matching correctly. Ironically, the more the publisher cares about correct rendering, the worse we actually render.

[Tony Payne]

Created attachment 138159 [details]
Patch

[Tony Payne]

Could we start the review? I'm still working through the layout test issues, but the code is ready for review.

As for performance, keep in mind that the Mac chromium port already implements this functionality and this change should actually *speed up* Mac rendering. Windows and Linux should be impacted less than Mac has already been impacted by this feature.

[WebKit Review Bot]

Comment on attachment 138159 [details]
Patch

Attachment 138159 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12474333

New failing tests:
svg/as-border-image/svg-as-border-image.html

[WebKit Review Bot]

Created attachment 138181 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Review Bot]

Comment on attachment 138159 [details]
Patch

Attachment 138159 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12472323

New failing tests:
svg/as-border-image/svg-as-border-image.html

[WebKit Review Bot]

Created attachment 138201 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[noel gordon]

(In reply to comment #20)
> How much of a perf hit to we take on the perf cyclers?

Perhaps the cyclers won't see anything unless there are lots of ICC profiled images in the test suite ...

[noel gordon]

(In reply to comment #21)
> (In reply to comment #20)
> > (From update of attachment 137569 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=137569&action=review
> > 
> > How much of a perf hit to we take on the perf cyclers?  This seems reasonable, but w/o assuming sRGB for untagged images or matching to the monitor profile, I'm surprised this helps much.
> 
> I don't know the answer to this. I couldn't find any good information on how to run/add performance tests, even after asking around. I'd love some guidance. I did a totally inadequate manual test with a large image tagged with AdobeRGB 1998. Before my patch, this image took an average of 37ms to render on a release build of chromium linux. With my patch, it rendered in an average of 42ms.
> 
> The vast majority of monitors are sRGB. The outstanding items will only help users who have wide gamut monitors. I don't intend to leave those items outstanding for very long.

You'll want to get monitor profiles sooner rather than later because that minority with profiled monitors are the ones that really care about color correction.

[noel gordon]

(In reply to comment #24)
> > It seems that qcms only supports rgba pixels.  That's the reason you have to swizzle the bytes of all image rows (twice) in the inadequately named flip() routine if I'm reading correctly?  Those row bytes swizzles are expensive - they were removed from the JPEG encoder for a 2x increase in decoding performance (bug 59670).
> 
> lcms also supports only rgba. From an API perspective, it supports bgra but internally it does the exact same swizzle. I think it's important to remember that this only affects images with an embedded color profile (for now, at least). These images are not nearly as common as images without profiles and usually are only included when the publisher cares about colors matching correctly. Ironically, the more the publisher cares about correct rendering, the worse we actually render.

lcms is interesting, but my question was about qcms rgba pixels, so let's see why that matters.

[noel gordon]

Comment on attachment 138159 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138159&action=review

> Source/WTF/ChangeLog:6
> +        Reviewed by Noel Gordon.

Should read "Reviewed by NOBODY (OOPS!)." and in WebCore/ChangeLog.  WebKit's commit queue automatically fills this in on submit.

> Source/WTF/ChangeLog:8
> +        Enables ICCJPEG for all chromium platforms for accessing embedded

"all"?  Does that include ANDROID chromium?

> Source/WebCore/ChangeLog:8
> +        Tests updated so they pass with color management.

A test based on http://crbug.com/143, using it's farbkreis.jpg image, would help prevent regressions.

<image src="farbkreis.jpg" width="50%">
<script>
if (window.layoutTestController)
    window.layoutTestController.dumpAsText(pixelTest = true);
</script>

> Source/WebCore/ChangeLog:21
> +        (WebCore::flip):

flip() does not match the code.  Perhaps you forgot to regenerate your ChangeLog.

> Source/WebCore/WebCore.gyp/WebCore.gyp:1069
> +        '<(chromium_src_dir)/third_party/qcms/qcms.gyp:qcms',

You need to change this file in three places.  Refer to bug 48977.

> Source/WebCore/platform/image-decoders/ImageDecoder.h:190
> +#if PLATFORM(CHROMIUM)

Does chromium ANDROID want/need this?

> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:27
> +#include <algorithm>

Is this used anywhere?

> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:34
> +#if PLATFORM(CHROMIUM)

Again, does chromium ANDROID want/need this?

> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:35
> +#include "third_party/qcms/src/qcms.h"

We #include "iccjpeg.h" without any third_party prefix.  Shouldn't qcms.h work the same?

> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:129
> +    for (int index = 0; index < width; index++) {

What's index for, maybe while (--width >=0) would do.

> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:130
> +        char c1 = pixels[2];

Abbreviated name: prefer meaningful variable names.

> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:142
> +    qcms_transform_data(rowJob->transform, rowJob->pixels, rowJob->pixels, rowJob->width);

The comment at line 161 (left) was removed, but it hinted at the problem with this approach.  Let's stop here.

[noel gordon]

> my question was about qcms rgba pixels, so let's see why that matters.

The data you're sending qcms_transform_data() here is premultiplied rgba in the common case.  The qcms documentation cleary states that it requires rgba input (raw rgba).  Since color transforms are in general non-linear, premultiplied rgba input does not produce the same results as rgba input.  We render wrong.

This breaks PNG images at least.  Another factor is that the PNG decoder applies the image gamma correction if specified and then the color transform applies it's gamma correction to pixels that are already gamma corrected.  And I see no progression with your patch at http://www.libpng.org/pub/png/png-colortest.html

The question is, how do you propose to fix it?  If the answer is continue down the current track and un-premultiply the data before qcms_transform_data() and then premultiply the output, then that'll require even more CPU to give good performance because those steps involve integer multiplication, and perhaps division, on every pixel, twice!

> My personal opinion is that it's better to be somewhat slower than to render incorrectly.

What's your view on even slower to get correct rendering?  Consider your stated intentions to apply color correction to even more image types eventually, not just those with color profiles.

qcms wants rgba input, so the trick would be to pass rgba to qcms with a minimum of fuss and make qcms produce bgra output (or rgba output on Android, but that's another story).

[Tony Payne]

Created attachment 140624 [details]
Patch

[WebKit Review Bot]

Comment on attachment 140624 [details]
Patch

Attachment 140624 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12645265

New failing tests:
fast/reflections/reflection-masks-opacity.html
fast/borders/inline-mask-overlay-image-outset-vertical-rl.html
fast/reflections/reflection-masks-outset.html
compositing/images/direct-image-background-color.html
svg/as-border-image/svg-as-border-image.html
fast/borders/block-mask-overlay-image.html
fast/borders/inline-mask-overlay-image-outset.html
fast/borders/block-mask-overlay-image-outset.html
fast/borders/inline-mask-overlay-image.html
fast/backgrounds/mask-composite.html
fast/reflections/reflection-masks.html

[WebKit Review Bot]

Created attachment 140653 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Review Bot]

Comment on attachment 140624 [details]
Patch

Attachment 140624 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12644345

New failing tests:
fast/reflections/reflection-masks-opacity.html
fast/borders/inline-mask-overlay-image-outset-vertical-rl.html
fast/reflections/reflection-masks-outset.html
compositing/images/direct-image-background-color.html
svg/as-border-image/svg-as-border-image.html
fast/borders/block-mask-overlay-image.html
fast/borders/inline-mask-overlay-image-outset.html
fast/borders/block-mask-overlay-image-outset.html
fast/borders/inline-mask-overlay-image.html
fast/backgrounds/mask-composite.html
fast/reflections/reflection-masks.html

[WebKit Review Bot]

Created attachment 140659 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Tony Payne]

Created attachment 140828 [details]
Patch

[WebKit Review Bot]

Comment on attachment 140828 [details]
Patch

Attachment 140828 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12653439

[noel gordon]

Comment on attachment 140828 [details]
Patch

Clearly a WIP patch, ChangeLogs out of date, etc.

View in context: https://bugs.webkit.org/attachment.cgi?id=140828&action=review

> Source/WTF/wtf/Platform.h:467
> +#define WTF_USE_ICCJPEG

#define WTF_USE_ICCJPEG 1

> Source/WebCore/WebCore.gyp/WebCore.gyp:1087
> +        '<(chromium_src_dir)/third_party/qcms/qcms.gyp:qcms',

Again you need to modify this file in three places, not one.

> Source/WebCore/platform/image-decoders/ImageDecoder.h:48
> +#if USE(QCMS)

Remove these lines 48-51.

> Source/WebCore/platform/image-decoders/ImageDecoder.h:195
>          ColorProfile m_colorProfile;

Is this ImageFrame member used anymore?  See comments below for the skia/ImageDecoderSkia.cpp ImageFrame::setColorProfile() function.  Think this variable can go, and its USE(QCMS) wrapping with it.

> Source/WebCore/platform/image-decoders/ImageDecoder.h:229
> +#if USE(QCMS)

Remove these lines: 229-234, 236

> Source/WebCore/platform/image-decoders/ImageDecoder.h:361
>          ColorProfile m_colorProfile;

Prefix this line with // FIXME: Do we need m_colorProfile anymore, on any port?

> Source/WebCore/platform/image-decoders/ImageDecoder.h:365
> +        qcms_transform* m_transform;

Note an ImageDecoder has a long life-time compared to it's image reader.  Do these variables need to out-live the reader class for some reason?  Did you consider making them members of JPEGImageReader and PNGImageReader so they could be deleted as soon as an image is decoded?  If we don't do that, then by rights we need to compute the size of the data held by these color correction variables and tell the browser's live decoded resources cache about that.  This is a failing of the existing design I note: no one tells the live decoded resources cache about ImageFrame::m_colorProfile.size() or ImageDecoder::m_colorProfile.size(), and they can be much larger than the decoded image bytes.

For now, move these out of here: make them members of JPEGImageDecoder and PNGImageDecoder respectively.  That'll allow us to move them into their reader classes if needed, and also minimise the code changes to the current file.

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:50
> +#include "third_party/qcms/src/qcms.h"

#include "qcms.h" and everywhere else you see third_party/qcms/...

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:541
> +                return false;

Say we decode 100 rows and then bail (return false) awaiting more network data.  100 rows of uncorrected image data will we painted to the screen. http://crbug/115079

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:544
> +          if (m_transform) {

Modulo the above, this looks like a great place to measure qcms transform throughput during your testing.

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:569
> +            qcms_transform_data(m_transform, samples, samples, width);

|samples| has an extra level of indirection to the row data, so I think you want

#if USE(QCMS)
    if (m_transform)
        qcms_transform_data(m_transform, *samples, *samples, info->output_width);
#endif

    int width = m_scaled ? m_scaledColumns.size() : info->output_width;
    ...

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:611
> +    m_colorProfile = colorProfile;

How is m_colorProfile used anymore? I suppose write the code as

#if USE(QCMS)
    ...
#else
    // FIXME: Do we need m_colorProfile anymore, on all ports?
    m_colorProfile = colorProfile;
#endif

for now until your sure, and then ...

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:613
> +    if (m_colorProfile.isEmpty())

... use the input variable colorProfile here and in the rest of the transform creation code that follows, not m_colorProfile.

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:621
> +                                        QCMS_DATA_BGRA_8,

QCMS_DATA_BGRA_8 is not defined by qcms.  Know you're aware of qcms's pixel format issues (no BGRX support) as something to be fixed, and that this was your first time dealing with the PNG and JPEG decoders.  I must say you've made a decent stab at add color correction to them both.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:117
> +        , m_cmsRow(0)

Good you've spotted the need for this (the color correction of interlaced PNG would be broken without it).

Please call it m_rowBuffer.  Let's remove all the #if USE(QCMS) wrapping from it here and then the same ...

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:196
> +#endif

... through to here.  It's just an m_rowBuffer we have available if we need it.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:321
> +    if ((colorType == PNG_COLOR_TYPE_PALETTE || colorType == PNG_COLOR_TYPE_RGB || colorType == PNG_COLOR_TYPE_RGB_ALPHA) && !m_ignoreGammaAndColorProfile) {

Why add PNG_COLOR_TYPE_PALETTE, why change behavior?  Unless you intend add a test now, I'd fix this later.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:330
> +#if USE(QCMS)

Looks similar to the JPEGImageDecoder::setColorProfile() function body though here.  Seems the PNGImageDecoder wants a similar function?  I should say that neither want it as member function.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:422
> +#if USE(QCMS)

Create this temporary row buffer only if we have a transform.

if (m_transform) {
    m_reader->createCmsRow(colorChannels * size().width());
    if (!m_reader->cmsRow()) {
        ...

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:499
> +#endif

These lines (487-499) could be simplified:

#if USE(QCMS)
    if (m_transform) {
        qcms_transform_data(m_transform, row, m_reader->cmsRow(), size().width());
        row = m_reader->cmsRow();
    }
#endif

and then none of changes below would be needed.

> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:27
> +#include <algorithm>

You're not using this, let's remove it.

> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:32
>  #include "NotImplemented.h"

Remove this include.

> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:117
>      m_colorProfile = colorProfile;

Is ImageFrame::m_colorProfile used anymore?  I think this function might now be written as

void ImageFrame::setColorProfile(const ColorProfile& colorProfile)
{
    // FIXME: Do we need this ImageFrame function anymore, on any port?
    UNUSED_PARAM(colorProfile);
}

> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:126
>      if (m_status == FrameComplete) {

Style nit: one line if body, don't use { in that case.

> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:-162
> -        // premultiplied, so don't apply the color profile if it isn't.

Hope you've read an understood that comment.

[Tony Payne]

Created attachment 141813 [details]
Patch

[Tony Payne]

Comment on attachment 140828 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140828&action=review

>> Source/WTF/wtf/Platform.h:467
>> +#define WTF_USE_ICCJPEG
> 
> #define WTF_USE_ICCJPEG 1

Done

>> Source/WebCore/WebCore.gyp/WebCore.gyp:1087
>> +        '<(chromium_src_dir)/third_party/qcms/qcms.gyp:qcms',
> 
> Again you need to modify this file in three places, not one.

Done, I think. qcms is included in each of the places iccjpeg is included.

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:48
>> +#if USE(QCMS)
> 
> Remove these lines 48-51.

Done.

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:195
>>          ColorProfile m_colorProfile;
> 
> Is this ImageFrame member used anymore?  See comments below for the skia/ImageDecoderSkia.cpp ImageFrame::setColorProfile() function.  Think this variable can go, and its USE(QCMS) wrapping with it.

Removed.

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:229
>> +#if USE(QCMS)
> 
> Remove these lines: 229-234, 236

Done.

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:361
>>          ColorProfile m_colorProfile;
> 
> Prefix this line with // FIXME: Do we need m_colorProfile anymore, on any port?

It looks to me like it is still needed for http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp&exact_package=chromium&q=m_colorProfile&type=cs&l=96

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:365
>> +        qcms_transform* m_transform;
> 
> Note an ImageDecoder has a long life-time compared to it's image reader.  Do these variables need to out-live the reader class for some reason?  Did you consider making them members of JPEGImageReader and PNGImageReader so they could be deleted as soon as an image is decoded?  If we don't do that, then by rights we need to compute the size of the data held by these color correction variables and tell the browser's live decoded resources cache about that.  This is a failing of the existing design I note: no one tells the live decoded resources cache about ImageFrame::m_colorProfile.size() or ImageDecoder::m_colorProfile.size(), and they can be much larger than the decoded image bytes.
> 
> For now, move these out of here: make them members of JPEGImageDecoder and PNGImageDecoder respectively.  That'll allow us to move them into their reader classes if needed, and also minimise the code changes to the current file.

Okay. I'd actually like m_sRGB to have a much longer lifetime, roughly the extent of the life of the application.

>> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:50
>> +#include "third_party/qcms/src/qcms.h"
> 
> #include "qcms.h" and everywhere else you see third_party/qcms/...

Done.

>> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:541
>> +                return false;
> 
> Say we decode 100 rows and then bail (return false) awaiting more network data.  100 rows of uncorrected image data will we painted to the screen. http://crbug/115079

Fixed. Transforming by-row or all at once has no effect on performance as long as the output transform is precached.

>> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:544
>> +          if (m_transform) {
> 
> Modulo the above, this looks like a great place to measure qcms transform throughput during your testing.

I don't trust manual testing. I'd really like a performance test that can be run as part of the normal suite of tests. Still have no idea what the right way to do that is...

>> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:569
>> +            qcms_transform_data(m_transform, samples, samples, width);
> 
> |samples| has an extra level of indirection to the row data, so I think you want
> 
> #if USE(QCMS)
>     if (m_transform)
>         qcms_transform_data(m_transform, *samples, *samples, info->output_width);
> #endif
> 
>     int width = m_scaled ? m_scaledColumns.size() : info->output_width;
>     ...

Done.

>> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:621
>> +                                        QCMS_DATA_BGRA_8,
> 
> QCMS_DATA_BGRA_8 is not defined by qcms.  Know you're aware of qcms's pixel format issues (no BGRX support) as something to be fixed, and that this was your first time dealing with the PNG and JPEG decoders.  I must say you've made a decent stab at add color correction to them both.

I took this stab at fixing the missing BGRA feature: http://codereview.chromium.org/10384114/

Yours looks like it only takes RGBx input. Not sure how that is going to work with the JPEG turbo-swizzle

>> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:117
>> +        , m_cmsRow(0)
> 
> Good you've spotted the need for this (the color correction of interlaced PNG would be broken without it).
> 
> Please call it m_rowBuffer.  Let's remove all the #if USE(QCMS) wrapping from it here and then the same ...

Since we are only allocating the rowBuffer if transform exists, rowBuffer will only be used when USE(QCMS). Are you sure you want to define it when !USE(QCMS)?

>> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:196
>> +#endif
> 
> ... through to here.  It's just an m_rowBuffer we have available if we need it.

Done

>> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:321
>> +    if ((colorType == PNG_COLOR_TYPE_PALETTE || colorType == PNG_COLOR_TYPE_RGB || colorType == PNG_COLOR_TYPE_RGB_ALPHA) && !m_ignoreGammaAndColorProfile) {
> 
> Why add PNG_COLOR_TYPE_PALETTE, why change behavior?  Unless you intend add a test now, I'd fix this later.

The sample page you linked (http://www.libpng.org/pub/png/png-colortest.html) uses palette colors. Without this change, we'll still fail that test.

>> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:330
>> +#if USE(QCMS)
> 
> Looks similar to the JPEGImageDecoder::setColorProfile() function body though here.  Seems the PNGImageDecoder wants a similar function?  I should say that neither want it as member function.

Created a createColorTransform(colorProfile) in both of the Readers.

>> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:422
>> +#if USE(QCMS)
> 
> Create this temporary row buffer only if we have a transform.
> 
> if (m_transform) {
>     m_reader->createCmsRow(colorChannels * size().width());
>     if (!m_reader->cmsRow()) {
>         ...

Done.

>> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:499
>> +#endif
> 
> These lines (487-499) could be simplified:
> 
> #if USE(QCMS)
>     if (m_transform) {
>         qcms_transform_data(m_transform, row, m_reader->cmsRow(), size().width());
>         row = m_reader->cmsRow();
>     }
> #endif
> 
> and then none of changes below would be needed.

The only difference is we'll be pulling alpha from the output of qcms_transform_data. I do not know why Mozilla makes a point of reading alpha from the original buffer. I have not seen any place where qcms fails to pass the alpha through to the output.

Done

>> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:27
>> +#include <algorithm>
> 
> You're not using this, let's remove it.

Done

>> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:32
>>  #include "NotImplemented.h"
> 
> Remove this include.

Done

>> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:117
>>      m_colorProfile = colorProfile;
> 
> Is ImageFrame::m_colorProfile used anymore?  I think this function might now be written as
> 
> void ImageFrame::setColorProfile(const ColorProfile& colorProfile)
> {
>     // FIXME: Do we need this ImageFrame function anymore, on any port?
>     UNUSED_PARAM(colorProfile);
> }

Done. It looks to me like ImageDecoderCG still uses this method.

>> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:126
>>      if (m_status == FrameComplete) {
> 
> Style nit: one line if body, don't use { in that case.

Done.

>> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:-162
>> -        // premultiplied, so don't apply the color profile if it isn't.
> 
> Hope you've read an understood that comment.

I think I understand it, but I could be sorely mistaken.

[WebKit Review Bot]

Comment on attachment 141813 [details]
Patch

Attachment 141813 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12704116

[noel gordon]

Well it's coming along.

(In reply to comment #45)

> >> Source/WebCore/WebCore.gyp/WebCore.gyp:1087
> >> +        '<(chromium_src_dir)/third_party/qcms/qcms.gyp:qcms',
> > 
> > Again you need to modify this file in three places, not one.
> 
> Done, I think. qcms is included in each of the places iccjpeg is included.

Perfect.

 
> >> Source/WebCore/platform/image-decoders/ImageDecoder.h:361
> >>          ColorProfile m_colorProfile;
> > 
> > Prefix this line with // FIXME: Do we need m_colorProfile anymore, on any port?
> 
> It looks to me like it is still needed for http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp&exact_package=chromium&q=m_colorProfile&type=cs&l=96

Added you to bug 86346, I've removed ImageDecoderCG.  I'd add the comment I asked for (some other port might use it).  It's just a reminder that we need to take it out, but to not break other ports by doing that right now.


> >> Source/WebCore/platform/image-decoders/ImageDecoder.h:365
> >> +        qcms_transform* m_transform;
> > 
> > For now, move these out of here: make them members of JPEGImageDecoder and PNGImageDecoder respectively.  That'll allow us to move them into their reader classes if needed, and also minimise the code changes to the current file.
> 
> Okay. I'd actually like m_sRGB to have a much longer lifetime, roughly the extent of the life of the application.

Yes, and it could then be used by the PNGDecoder, JPEGDecoder.

Somewhere near here http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/ImageDecoder.h?rev=117355#L302 for now?

You could re-add your TODOs (webkit calls them FIXME) and you could ensure the ICC4 profile initialization code qcms_enable_iccv4() is called if needed.  That was dropped from the current patch so I'm not sure if that was intentional or not.  Something like:

#if USE(QCMS)
         static qcms_profile* qcmsDeviceProfile()
         {
             static qcms_profile* deviceProfile = 0;
 
             static bool qcmsInitalised = false;
             if (!qcmsInitalised) {
                qcms_enable_iccv4();
                // FIXME: sRGB profiles don't add much value. Use the users monitor profile for the sake of humanity.
                deviceProfile = qcms_profile_sRGB();
                // FIXME: Check the profile is valid and sensible and CRASH() if not.
                qcms_profile_precache_output_transform(deviceProfile);
                qcmsInitalised = true;
            }

            ASSERT(deviceProfile);
            return deviceProfile;
        }
#endif

where caller does not own the returned result, nor does caller need to release it.


> >> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:541
> >> +                return false;
> > 
> > Say we decode 100 rows and then bail (return false) awaiting more network data.  100 rows of uncorrected image data will we painted to the screen. http://crbug/115079
> 
> Fixed. Transforming by-row or all at once has no effect on performance as long as the output transform is precached.

Transform row-by-row, good and another bug closes.  Would a users monitor profile be precached too?

 
> >> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:544
> >> +          if (m_transform) {
> > 
> > Modulo the above, this looks like a great place to measure qcms transform throughput during your testing.
> 
> I don't trust manual testing.
>
> I'd really like a performance test that can be run as part of the normal suite of tests. Still have no idea what the right way to do that is.

A reliable, trustworthy test environment is the right way.

Have you talked to the perf team folks?  The Androids also care about perf (image decoding, and so on) and they run perf bots so that's another good avenue to explore.  They'll have advice and might have something you could re-use, or maybe even help you get it up and running.

Mentioned before about having a corpus of images you trust.  A small corpus can help get an idea on costs.  A palleted 256 x 256 (google maps) png is ~1ms to decode.  A 512 x 512 JPEG (street view map) image is also ~1ms to decode.  A 4000 x 3000 JPEG family snap is ~104ms decode swizzled, or ~195ms with no swizzle, and produces 45MByte of bgra pixels (too much to fit in your CPUs L1 cache).  You can estimate the rate: ~45Mbyte per 100ms.  Same 4000 x 3000 image encoded as PNG takes way longer to decode.

Known-good color profiles are available in the resources tab at www.color.org.  Find a tool (look around) that takes an image and adds/removes its color profile.  You can upload an image to http://regex.info/exif.cgi and click a button to display it's color profile information as a cross check.

That en-block transform code you had in the JPEG decoder was a handy place to do

   double start = currentTime();

   if (m_transform) {
        unsigned char* data = reinterpret_cast<unsigned char*>(buffer.getAddr(0, 0));
        qcms_transform_data(m_transform, data, data, info->output_height * info->output_width);
   }

   fprintf(stderr, "%f ms\n", currentTime() - start);

Take a 4000 x 3000 JPEG with a color profile that's in your browser cache and run it through here to find out how long qcms takes to process 45Mbytes of pixel data.  Is the test repeatable?  Does image size matter?  Is en-block (done right) better than row-by-row?  And so on.  Get curious: with it comes the comfort of knowing the facts.


> >> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:569
> >> +            qcms_transform_data(m_transform, samples, samples, width);
> > 
> > |samples| has an extra level of indirection to the row data, so I think you want
> > 
> > #if USE(QCMS)
> >     if (m_transform)
> >         qcms_transform_data(m_transform, *samples, *samples, info->output_width);
> > #endif
> > 
> >     int width = m_scaled ? m_scaledColumns.size() : info->output_width;
> >     ...
> 
> Done.

Did a test save us, looked possibly insecure.  Image decoding is a very security-sensitive area of webkit if I've not mentioned it before.


> >> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:621
> >> +                                        QCMS_DATA_BGRA_8,
> > 
> > QCMS_DATA_BGRA_8 is not defined by qcms.  Know you're aware of qcms's pixel format issues (no BGRX support) as something to be fixed, and that this was your first time dealing with the PNG and JPEG decoders.  I must say you've made a decent stab at add color correction to them both.
> 
> I took this stab at fixing the missing BGRA feature: http://codereview.chromium.org/10384114/
> 
> Yours looks like it only takes RGBx input. Not sure how that is going to work with the JPEG turbo-swizzle

(last paragraph of comment #35) and in the code:

http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp#L75  JCS_EXT_RGBA swizzle
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp#L77  JCS_EXT_BGRA swizzle

If you create an m_transform, then you're color correcting.  Tell the JPEG decoder to use a JCS_EXT_RGBA swizzle in that case during decoder setup.  Technically the transform and the decoding swizzle must be setup prior to calling jpeg_start_decompress().  Anyway, send the decoded data (it's RGBA, right?) into qcms to keep it happy.  Make qcms output BGRA format and we're done.  Works for you?


> >> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:117
> >> +        , m_cmsRow(0)
> > 
> > Good you've spotted the need for this (the color correction of interlaced PNG would be broken without it).
> > 
> > Please call it m_rowBuffer.  Let's remove all the #if USE(QCMS) wrapping from it here and then the same ...
> 
> Since we are only allocating the rowBuffer if transform exists, rowBuffer will only be used when USE(QCMS). Are you sure you want to define it when !USE(QCMS)?

Don't want USE(QCMS) all over.  You've moved most the QCMS code closer together now.  The next step should be obvious.


> >> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:321
> >> +    if ((colorType == PNG_COLOR_TYPE_PALETTE || colorType == PNG_COLOR_TYPE_RGB || colorType == PNG_COLOR_TYPE_RGB_ALPHA) && !m_ignoreGammaAndColorProfile) {
> > 
> > Why add PNG_COLOR_TYPE_PALETTE, why change behavior?  Unless you intend add a test now, I'd fix this later.
> 
> The sample page you linked (http://www.libpng.org/pub/png/png-colortest.html) uses palette colors. Without this change, we'll still fail that test.

Filed bug 86722.  Lots of images on that page: could you add the specific image(s) you're talking about to that bug please?


> >> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:499
> >> +#endif
> > 
> > These lines (487-499) could be simplified:
> > 
> > #if USE(QCMS)
> >     if (m_transform) {
> >         qcms_transform_data(m_transform, row, m_reader->cmsRow(), size().width());
> >         row = m_reader->cmsRow();
> >     }
> > #endif
> > 
> > and then none of changes below would be needed.
> 
> The only difference is we'll be pulling alpha from the output of qcms_transform_data. I do not know why Mozilla makes a point of reading alpha from the original buffer. I have not seen any place where qcms fails to pass the alpha through to the output.

Agree: alpha is a pass-through in qcms from my reading of it.


> > Is ImageFrame::m_colorProfile used anymore?  I think this function might now be written as
> > 
> > void ImageFrame::setColorProfile(const ColorProfile& colorProfile)
> > {
> >     // FIXME: Do we need this ImageFrame function anymore, on any port?
> >     UNUSED_PARAM(colorProfile);
> > }
> 
> Done. It looks to me like ImageDecoderCG still uses this method.

Not anymore.  Other ports might still use it, hence the FIXME comment.

 
> >> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:-162
> >> -        // premultiplied, so don't apply the color profile if it isn't.
> > 
> > Hope you've read an understood that comment.
> 
> I think I understand it, but I could be sorely mistaken.

You don't send alpha premultiplied data into qcms anymore, so you can relax.

[noel gordon]

Comment on attachment 141813 [details]
Patch

I'm seeing good progression here.  Appreciate your not adding all those tests to the review patch while it's still WIP.  Makes it faster to load for me.

View in context: https://bugs.webkit.org/attachment.cgi?id=141813&action=review

> Source/WebCore/ChangeLog:1
> [phone]  Tony Payne  <[e-mail]>

I'm looking forward to a good ChangeLog one day.

> Source/WTF/wtf/Platform.h:464
>  #if OS(DARWIN)

All good through here.

> Source/WebCore/WebCore.gyp/WebCore.gyp:1086
>          '<(chromium_src_dir)/third_party/libxslt/libxslt.gyp:libxslt',

All correct now.

> Source/WebCore/platform/image-decoders/ImageDecoder.h:-190
> -#if PLATFORM(CHROMIUM) && OS(DARWIN)

Make a mental note only of these lines.  If I have to break this patch up and land it parts, we might need this again.

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:49
> +#if USE(QCMS)

There is an extern "C" section below, try moving these lines (no C++ in qcms as I understand it).

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:311
>              if (!m_decoder->ignoresGammaAndColorProfile()) {

All these lines 311-319 become

   if (!m_decoder->ignoresGammaAndColorProfile())
       createColorTransform(readColorProfile(info()));

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:428
> +#if USE(QCMS)

Move the #if USE(QCMS) inside the createColorTransform() body (see readColorProfile) and add a

#else
    UNUSED_PARAM(colorProfile);
#endif

at then end to silence the webkit DEBUG bots.

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:442
> +        m_transform = qcms_transform_create(qcmsColorProfile,

Nicely written function.  This line can be one line (webkit has no real line limit, we use our judgment).

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:583
> +             else if (transform)

if (transform)

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:606
> +        if (transform && !m_scaled)

m_scaled doesn't matter here, make it if (transform)

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:189
> +#if USE(QCMS)

Write this routine as you did the JPEG function of the same name, with the #if USE(QCMS) inside etc, prefer the early return (otherwise the code starts dancing off to the right with nested ifs).

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:198
> +                    m_transform = qcms_transform_create(qcmsColorProfile,

Line limits.  This one is harder because the ternary ? makes it somewhat harder to read if it's one line.  Use your judgment.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:212
>      void createInterlaceBuffer(int size) { m_interlaceBuffer = new png_byte[size]; }

The two lines 212-213 seem out of place now.  Can you maybe move them up near line 182?

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:350
> +#if USE(QCMS)

Don't need #if ... assuming you follow line 189.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:430
> +#if USE(QCMS)

Space before this line.  We're starting a new paragraph.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:498
> +    qcms_transform* transform = m_reader->colorTransform();

m_scaled again.  Ditch it and write

if (qcms_transform* transform = m_reader->colorTransform()) {
    ...

[noel gordon]

Comment on attachment 141813 [details]
Patch

Maybe you accidentally set r+, a reviewer will do that for you.

[Tony Payne]

(In reply to comment #47)

> > >> Source/WebCore/platform/image-decoders/ImageDecoder.h:361
> > >>          ColorProfile m_colorProfile;
> > > 
> > > Prefix this line with // FIXME: Do we need m_colorProfile anymore, on any port?
> > 
> > It looks to me like it is still needed for http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp&exact_package=chromium&q=m_colorProfile&type=cs&l=96
> 
> Added you to bug 86346, I've removed ImageDecoderCG.  I'd add the comment I asked for (some other port might use it).  It's just a reminder that we need to take it out, but to not break other ports by doing that right now.

Added comment. For now, I'm keeping m_colorProfile populated until we remove it.

> 
> > >> Source/WebCore/platform/image-decoders/ImageDecoder.h:365
> > >> +        qcms_transform* m_transform;
> > > 
> > > For now, move these out of here: make them members of JPEGImageDecoder and PNGImageDecoder respectively.  That'll allow us to move them into their reader classes if needed, and also minimise the code changes to the current file.
> > 
> > Okay. I'd actually like m_sRGB to have a much longer lifetime, roughly the extent of the life of the application.
> 
> Yes, and it could then be used by the PNGDecoder, JPEGDecoder.
> 
> Somewhere near here http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/ImageDecoder.h?rev=117355#L302 for now?
> 
> You could re-add your TODOs (webkit calls them FIXME) and you could ensure the ICC4 profile initialization code qcms_enable_iccv4() is called if needed.  That was dropped from the current patch so I'm not sure if that was intentional or not.  Something like:

Done. I purposefully removed iccv4 support since that is not performing well yet in qcms. Later, I'd like to enable it via a flag.

> 
> > >> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:541
> > >> +                return false;
> > > 
> > > Say we decode 100 rows and then bail (return false) awaiting more network data.  100 rows of uncorrected image data will we painted to the screen. http://crbug/115079
> > 
> > Fixed. Transforming by-row or all at once has no effect on performance as long as the output transform is precached.
> 
> Transform row-by-row, good and another bug closes.  Would a users monitor profile be precached too?

Yes.

> 
> > >> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:544
> > >> +          if (m_transform) {
> > > 
> > > Modulo the above, this looks like a great place to measure qcms transform throughput during your testing.
> > 
> > I don't trust manual testing.
> >
> > I'd really like a performance test that can be run as part of the normal suite of tests. Still have no idea what the right way to do that is.
> 
> A reliable, trustworthy test environment is the right way.
> 
> Have you talked to the perf team folks?  The Androids also care about perf (image decoding, and so on) and they run perf bots so that's another good avenue to explore.  They'll have advice and might have something you could re-use, or maybe even help you get it up and running.
> 
> Mentioned before about having a corpus of images you trust.  A small corpus can help get an idea on costs.  A palleted 256 x 256 (google maps) png is ~1ms to decode.  A 512 x 512 JPEG (street view map) image is also ~1ms to decode.  A 4000 x 3000 JPEG family snap is ~104ms decode swizzled, or ~195ms with no swizzle, and produces 45MByte of bgra pixels (too much to fit in your CPUs L1 cache).  You can estimate the rate: ~45Mbyte per 100ms.  Same 4000 x 3000 image encoded as PNG takes way longer to decode.
> 

On my machine, a 2100x1400 JPG with color profile takes about 55ms according to the Timeline. That's roughly the same rate as the non-swizzled JPEG, which makes sense since we need to hit each pixel twice.

> Known-good color profiles are available in the resources tab at www.color.org.  Find a tool (look around) that takes an image and adds/removes its color profile.  You can upload an image to http://regex.info/exif.cgi and click a button to display it's color profile information as a cross check.
> 
> That en-block transform code you had in the JPEG decoder was a handy place to do
> 
>    double start = currentTime();
> 
>    if (m_transform) {
>         unsigned char* data = reinterpret_cast<unsigned char*>(buffer.getAddr(0, 0));
>         qcms_transform_data(m_transform, data, data, info->output_height * info->output_width);
>    }
> 
>    fprintf(stderr, "%f ms\n", currentTime() - start);
> 
> Take a 4000 x 3000 JPEG with a color profile that's in your browser cache and run it through here to find out how long qcms takes to process 45Mbytes of pixel data.  Is the test repeatable?  Does image size matter?  Is en-block (done right) better than row-by-row?  And so on.  Get curious: with it comes the comfort of knowing the facts.
> 
> 
> > >> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:569
> > >> +            qcms_transform_data(m_transform, samples, samples, width);
> > > 
> > > |samples| has an extra level of indirection to the row data, so I think you want
> > > 
> > > #if USE(QCMS)
> > >     if (m_transform)
> > >         qcms_transform_data(m_transform, *samples, *samples, info->output_width);
> > > #endif
> > > 
> > >     int width = m_scaled ? m_scaledColumns.size() : info->output_width;
> > >     ...
> > 
> > Done.
> 
> Did a test save us, looked possibly insecure.  Image decoding is a very security-sensitive area of webkit if I've not mentioned it before.

No, you saved us. How can I trigger this code path so I can test it?

> > >> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:621
> > >> +                                        QCMS_DATA_BGRA_8,
> > > 
> > > QCMS_DATA_BGRA_8 is not defined by qcms.  Know you're aware of qcms's pixel format issues (no BGRX support) as something to be fixed, and that this was your first time dealing with the PNG and JPEG decoders.  I must say you've made a decent stab at add color correction to them both.
> > 
> > I took this stab at fixing the missing BGRA feature: http://codereview.chromium.org/10384114/
> > 
> > Yours looks like it only takes RGBx input. Not sure how that is going to work with the JPEG turbo-swizzle
> 
> (last paragraph of comment #35) and in the code:
> 
> http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp#L75  JCS_EXT_RGBA swizzle
> http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp#L77  JCS_EXT_BGRA swizzle
> 
> If you create an m_transform, then you're color correcting.  Tell the JPEG decoder to use a JCS_EXT_RGBA swizzle in that case during decoder setup.  Technically the transform and the decoding swizzle must be setup prior to calling jpeg_start_decompress().  Anyway, send the decoded data (it's RGBA, right?) into qcms to keep it happy.  Make qcms output BGRA format and we're done.  Works for you?

Getting it to work with your version of qcms bgra support was tricky. Please review the JPEG changes carefully.

> 
> > >> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:117
> > >> +        , m_cmsRow(0)
> > > 
> > > Good you've spotted the need for this (the color correction of interlaced PNG would be broken without it).
> > > 
> > > Please call it m_rowBuffer.  Let's remove all the #if USE(QCMS) wrapping from it here and then the same ...
> > 
> > Since we are only allocating the rowBuffer if transform exists, rowBuffer will only be used when USE(QCMS). Are you sure you want to define it when !USE(QCMS)?
> 
> Don't want USE(QCMS) all over.  You've moved most the QCMS code closer together now.  The next step should be obvious.

And if it isn't obvious...?

> 
> > >> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:321
> > >> +    if ((colorType == PNG_COLOR_TYPE_PALETTE || colorType == PNG_COLOR_TYPE_RGB || colorType == PNG_COLOR_TYPE_RGB_ALPHA) && !m_ignoreGammaAndColorProfile) {
> > > 
> > > Why add PNG_COLOR_TYPE_PALETTE, why change behavior?  Unless you intend add a test now, I'd fix this later.
> > 
> > The sample page you linked (http://www.libpng.org/pub/png/png-colortest.html) uses palette colors. Without this change, we'll still fail that test.
> 
> Filed bug 86722.  Lots of images on that page: could you add the specific image(s) you're talking about to that bug please?

Done.

> 
> > >> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:499
> > >> +#endif
> > > 
> > > These lines (487-499) could be simplified:
> > > 
> > > #if USE(QCMS)
> > >     if (m_transform) {
> > >         qcms_transform_data(m_transform, row, m_reader->cmsRow(), size().width());
> > >         row = m_reader->cmsRow();
> > >     }
> > > #endif
> > > 
> > > and then none of changes below would be needed.
> > 
> > The only difference is we'll be pulling alpha from the output of qcms_transform_data. I do not know why Mozilla makes a point of reading alpha from the original buffer. I have not seen any place where qcms fails to pass the alpha through to the output.
> 
> Agree: alpha is a pass-through in qcms from my reading of it.
> 
> 
> > > Is ImageFrame::m_colorProfile used anymore?  I think this function might now be written as
> > > 
> > > void ImageFrame::setColorProfile(const ColorProfile& colorProfile)
> > > {
> > >     // FIXME: Do we need this ImageFrame function anymore, on any port?
> > >     UNUSED_PARAM(colorProfile);
> > > }
> > 
> > Done. It looks to me like ImageDecoderCG still uses this method.
> 
> Not anymore.  Other ports might still use it, hence the FIXME comment.
> 
> 
> > >> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:-162
> > >> -        // premultiplied, so don't apply the color profile if it isn't.
> > > 
> > > Hope you've read an understood that comment.
> > 
> > I think I understand it, but I could be sorely mistaken.
> 
> You don't send alpha premultiplied data into qcms anymore, so you can relax.

[Tony Payne]

Created attachment 142605 [details]
Patch

[Tony Payne]

Comment on attachment 141813 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141813&action=review

>> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:49
>> +#if USE(QCMS)
> 
> There is an extern "C" section below, try moving these lines (no C++ in qcms as I understand it).

Done.

>> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:311
>>              if (!m_decoder->ignoresGammaAndColorProfile()) {
> 
> All these lines 311-319 become
> 
>    if (!m_decoder->ignoresGammaAndColorProfile())
>        createColorTransform(readColorProfile(info()));

Until I'm sure no port is using m_colorProfile, I feel better continuing to set it.

>> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:428
>> +#if USE(QCMS)
> 
> Move the #if USE(QCMS) inside the createColorTransform() body (see readColorProfile) and add a
> 
> #else
>     UNUSED_PARAM(colorProfile);
> #endif
> 
> at then end to silence the webkit DEBUG bots.

Done.

>> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:442
>> +        m_transform = qcms_transform_create(qcmsColorProfile,
> 
> Nicely written function.  This line can be one line (webkit has no real line limit, we use our judgment).

Since we're now using the ternary operator, keeping this way for readability.

>> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:583
>> +             else if (transform)
> 
> if (transform)

Done.

>> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:606
>> +        if (transform && !m_scaled)
> 
> m_scaled doesn't matter here, make it if (transform)

Done.

>> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:189
>> +#if USE(QCMS)
> 
> Write this routine as you did the JPEG function of the same name, with the #if USE(QCMS) inside etc, prefer the early return (otherwise the code starts dancing off to the right with nested ifs).

Done.

>> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:198
>> +                    m_transform = qcms_transform_create(qcmsColorProfile,
> 
> Line limits.  This one is harder because the ternary ? makes it somewhat harder to read if it's one line.  Use your judgment.

Prefer this way for readability.

>> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:212
>>      void createInterlaceBuffer(int size) { m_interlaceBuffer = new png_byte[size]; }
> 
> The two lines 212-213 seem out of place now.  Can you maybe move them up near line 182?

Done.

>> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:350
>> +#if USE(QCMS)
> 
> Don't need #if ... assuming you follow line 189.

Done.

>> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:430
>> +#if USE(QCMS)
> 
> Space before this line.  We're starting a new paragraph.

Done.

>> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:498
>> +    qcms_transform* transform = m_reader->colorTransform();
> 
> m_scaled again.  Ditch it and write
> 
> if (qcms_transform* transform = m_reader->colorTransform()) {
>     ...

Done.

[WebKit Review Bot]

Comment on attachment 142605 [details]
Patch

Attachment 142605 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12720733

[Gyuyoung Kim]

Comment on attachment 142605 [details]
Patch

Attachment 142605 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12726316

[Gustavo Noronha (kov)]

Comment on attachment 142605 [details]
Patch

Attachment 142605 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12716782

[Tony Payne]

Created attachment 142739 [details]
Patch

[WebKit Review Bot]

Comment on attachment 142739 [details]
Patch

Attachment 142739 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12724534

[Gustavo Noronha (kov)]

Comment on attachment 142739 [details]
Patch

Attachment 142739 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12726513

[Gyuyoung Kim]

Comment on attachment 142739 [details]
Patch

Attachment 142739 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12722874

[noel gordon]

(In reply to comment #50)
> (In reply to comment #47)
> 
> > > >> Source/WebCore/platform/image-decoders/ImageDecoder.h:361
> > > >>          ColorProfile m_colorProfile;

> > Added you to bug 86346, I've removed ImageDecoderCG.  I'd add the comment I asked for (some other port might use it).  It's just a reminder that we need to take it out, but to not break other ports by doing that right now.
> 
> Added comment. For now, I'm keeping m_colorProfile populated until we remove it.

Ok.
 
> > You could re-add your TODOs (webkit calls them FIXME) and you could ensure the ICC4 profile initialization code qcms_enable_iccv4() is called if needed.  That was dropped from the current patch so I'm not sure if that was intentional or not.  Something like:
> 
> Done. I purposefully removed iccv4 support since that is not performing well yet in qcms. Later, I'd like to enable it via a flag.

Ok.
 
> > Mentioned before about having a corpus of images you trust.  A small corpus can help get an idea on costs.  A palleted 256 x 256 (google maps) png is ~1ms to decode.  A 512 x 512 JPEG (street view map) image is also ~1ms to decode.  A 4000 x 3000 JPEG family snap is ~104ms decode swizzled, or ~195ms with no swizzle, and produces 45MByte of bgra pixels (too much to fit in your CPUs L1 cache).  You can estimate the rate: ~45Mbyte per 100ms.  Same 4000 x 3000 image encoded as PNG takes way longer to decode.
> > 
> 
> On my machine, a 2100x1400 JPG with color profile takes about 55ms according to the Timeline. That's roughly the same rate as the non-swizzled JPEG, which makes sense since we need to hit each pixel twice.

So qcms adds 50%?

 
> > > > #if USE(QCMS)
> > > >     if (m_transform)
> > > >         qcms_transform_data(m_transform, *samples, *samples, info->output_width);
> > > > #endif
> > > > 
> > > >     int width = m_scaled ? m_scaledColumns.size() : info->output_width;
> > > >     ...
> > > 
> > > Done.
> > 
> > Did a test save us, looked possibly insecure.  Image decoding is a very security-sensitive area of webkit if I've not mentioned it before.
> 
> No, you saved us. How can I trigger this code path so I can test it?

This path won't be triggered on chrome which only uses this path for JPEG transform=0 RGB images and they don't have color profiles.  If another port uses/enables qcms but is not using libjpeg-turbo, then existing tests will exercise this path.

 
> > If you create an m_transform, then you're color correcting.  Tell the JPEG decoder to use a JCS_EXT_RGBA swizzle in that case during decoder setup.  Technically the transform and the decoding swizzle must be setup prior to calling jpeg_start_decompress().  Anyway, send the decoded data (it's RGBA, right?) into qcms to keep it happy.  Make qcms output BGRA format and we're done.  Works for you?
> 
> Getting it to work with your version of qcms bgra support was tricky. Please review the JPEG changes carefully.

Noted.

[noel gordon]

(In reply to comment #52)
> (From update of attachment 141813 [details])
> >> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:311
> >>              if (!m_decoder->ignoresGammaAndColorProfile()) {
> > 
> > All these lines 311-319 become
> > 
> >    if (!m_decoder->ignoresGammaAndColorProfile())
> >        createColorTransform(readColorProfile(info()));
> 
> Until I'm sure no port is using m_colorProfile, I feel better continuing to set it.

Ok.
 
> >> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:198
> >> +                    m_transform = qcms_transform_create(qcmsColorProfile,
> > 
> > Line limits.  This one is harder because the ternary ? makes it somewhat harder to read if it's one line.  Use your judgment.
> 
> Prefer this way for readability.

Ok, and the style checker didn't complain.

[noel gordon]

Comment on attachment 142739 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142739&action=review

> Source/WTF/wtf/Platform.h:473
> +#define WTF_USE_QMCS 1

We'll need to change this name, the GTK seems to already use it.

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:315
> +                    bool swizzled = turboSwizzled(m_info.out_color_space);

Guess this is the part you referred to as "tricky", and indeed it is.  You're using turboSwizzled() unguarded here (broke the EFL bot) and using |swizzled| to mean the output color space has alpha which is not quite right.  Seems a helper function is needed to clarify your intent.

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:440
> +        qcms_profile* outputProfile = WebCore::ImageDecoder::qcmsOutputDeviceProfile();

No need for the WebCore:: prefix.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:181
> +#if USE(QCMS)

> And if it wasn't obvious ...?

I think we should try to move the new QCMS one-liners and other function code together and group them in #if USE(QCMS).  There seems to be no ryhme or reason to pre-existing placement of the code though these lines 177-189, and that's not helping you.

[Tony Payne]

> > > Mentioned before about having a corpus of images you trust.  A small corpus can help get an idea on costs.  A palleted 256 x 256 (google maps) png is ~1ms to decode.  A 512 x 512 JPEG (street view map) image is also ~1ms to decode.  A 4000 x 3000 JPEG family snap is ~104ms decode swizzled, or ~195ms with no swizzle, and produces 45MByte of bgra pixels (too much to fit in your CPUs L1 cache).  You can estimate the rate: ~45Mbyte per 100ms.  Same 4000 x 3000 image encoded as PNG takes way longer to decode.
> > > 
> > 
> > On my machine, a 2100x1400 JPG with color profile takes about 55ms according to the Timeline. That's roughly the same rate as the non-swizzled JPEG, which makes sense since we need to hit each pixel twice.
> 
> So qcms adds 50%?

Roughly speaking and with only a single data point, yes, images with profiles take about 50% longer. ParallelJobs cut that significantly in my early testing, but I understand there are serious drawbacks to that approach.

[noel gordon]

(In reply to comment #63)
> > So qcms adds 50%?
> 
> Roughly speaking and with only a single data point, yes, images with profiles take about 50% longer. ParallelJobs cut that significantly in my early testing, but I understand there are serious drawbacks to that approach.

I've measured similar results from this patch (did some minor fix-ups, alpha handling and such).  Also measured no speed regressions for images without profiles.  And finally measured, without your patch on a MAC, the speed of the old implementation (skia coreGraphics color correction).  It adds 200% to the decoding time for the 800 x 800 JPEG profiled farbkreis.jpg image from crbug/143 (ignore profile ~7ms, don't ~21ms), qcms adds 50%.

One difference I noted was for interlaced PNG.  We color correct those images now for each interlace pass.  That prevents color correction flashing, but chews CPU.  It's mentioned in qcms bug list:

  https://bugzilla.mozilla.org/buglist.cgi?quicksearch=GFX%3A+Color+Management

[Tony Payne]

Created attachment 143663 [details]
Patch

[WebKit Review Bot]

Comment on attachment 143663 [details]
Patch

Attachment 143663 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12778340

[Gyuyoung Kim]

Comment on attachment 143663 [details]
Patch

Attachment 143663 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12792010

[Tony Payne]

Created attachment 143694 [details]
Patch

[WebKit Review Bot]

Comment on attachment 143694 [details]
Patch

Attachment 143694 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12794038

[noel gordon]

(In reply to comment #64)
>   https://bugzilla.mozilla.org/buglist.cgi?quicksearch=GFX%3A+Color+Management

Suggests there will be some image rendering regressions.  For example, not drawing the image at http://4walled.cc/show-729990 with qcms active http://bugzil.la/756877

[noel gordon]

I think you have some details on the http://4walled.cc/show-729990 image issues now.  Could you add those to http://bugzil.la/756877 to help them diagnose?

[noel gordon]

Adam asked off-line if would be possible to use native API per platform.  We have results for CoreGraphics, but not for win32, so I investigated that ...

[noel gordon]

Created attachment 144201 [details]
Patch ICM2.0 jpeg decoder

[noel gordon]

Created attachment 144202 [details]
screen shot jpeg-decode-with-icm2.0-chrome-win

[noel gordon]

So the ICM2.0 Win API seems mature and capable and also draws http://4walled.cc/show-729990 fine I note.  For the 800 x 800 JPEG profiled farbkreis.jpg image from crbug/143, the decode time is ~19ms with color transforms enabled (qcms ~14ms, coreGraphics skia ~21ms) so ICM2.0 is intermediate in terms of speed for that image.

Use qcms everywhere and good speed will result, but with some loss of capability compared to the existing platform APIs.  It's the usual trade-off of speed with quality, and dealing with three libraries instead of one, etc.  qcms has bugs (who doesn't) so I see room for improvement there.

[noel gordon]

Comment on attachment 143694 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143694&action=review

> Source/WTF/ChangeLog:2
> +

ChangeLogs should have a title.

> Source/WebCore/ChangeLog:17
> +       two known output color spaces for which the decoder use a data swizzle.

Sentences. s/use/uses/

> Source/WebCore/ChangeLog:19
> +       libjpeg-turbo, alpha may be present in the swizzled output color data.

s/color data/color space/

> Source/WebCore/ChangeLog:54
> +       color transform is needed for decoding. Apply color transform to each

s/color transform is/a color transform is/

> Source/WebCore/platform/image-decoders/ImageDecoder.h:322
> +                // FIXME: Check that the profile is valid and fallback to sRGB if not.

Is this fallback a prescription or an option?  Is a fallback to no-color-correction not a viable option?

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:317
> +                // Input RGBA data to qcms. Note: restored to BGRA on output from qcms_transform_data.

s/ from qcms_transform_data//

The following lines were the "tricky" ones.  Should we patch qcms once more and make these lines (316-320) disappear?

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:458
> +        // FIXME: We currently only support color profiles for RGB profiled images.

s/FIXME: // (see the PNG decoder).

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:460
> +        // FIXME: Don't force perceptual intent if the image profile contains an intent.

qcms_profile_get_rendering_intent() could be used to resolve this FIXME right now, no?

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:461
> +        m_transform = qcms_transform_create(inputProfile,

Is a qcms_profile_is_bogus() call on the input profile warranted here?  Maybe the qcms bug list suggests too many false positives or other issues?

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:468
> +    qcms_transform* m_transform;

Space before this member would fit the style of this file.

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:605
> +        if (info->out_color_space == JCS_RGB && m_reader->colorTransform())

Test m_reader->colorTransform() first?  The transform will be 0 in the common case.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:189
> +        // FIXME: Add support for gray profiles.

I'd remove this FIXME.  There's no such comment in the JPEG decoder, and this file already has a large comment about RGB|RGBA-only support.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:202
> +        // FIXME: We currently only support color profiles for RGB and RGBA images.

s/FIXME: // :)

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:204
> +        // FIXME: Don't force perceptual intent if the image profile contains an intent.

qcms_profile_get_rendering_intent() could be used to resolve this FIXME now?

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:302
> +    // FIXME: Add support for grayscale color profiles.

I'd remove this FIXME, per line 189.

[Tony Payne]

Created attachment 144620 [details]
Patch

[Tony Payne]

Comment on attachment 143694 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143694&action=review

>> Source/WTF/ChangeLog:2
>> +
> 
> ChangeLogs should have a title.

Done.

>> Source/WebCore/ChangeLog:17
>> +       two known output color spaces for which the decoder use a data swizzle.
> 
> Sentences. s/use/uses/

Done.

>> Source/WebCore/ChangeLog:19
>> +       libjpeg-turbo, alpha may be present in the swizzled output color data.
> 
> s/color data/color space/

Done.

>> Source/WebCore/ChangeLog:54
>> +       color transform is needed for decoding. Apply color transform to each
> 
> s/color transform is/a color transform is/

Done.

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:322
>> +                // FIXME: Check that the profile is valid and fallback to sRGB if not.
> 
> Is this fallback a prescription or an option?  Is a fallback to no-color-correction not a viable option?

Updated FIXME to note that fallback is an option.

>> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:317
>> +                // Input RGBA data to qcms. Note: restored to BGRA on output from qcms_transform_data.
> 
> s/ from qcms_transform_data//
> 
> The following lines were the "tricky" ones.  Should we patch qcms once more and make these lines (316-320) disappear?

This is no longer "tricky" for me.

>> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:458
>> +        // FIXME: We currently only support color profiles for RGB profiled images.
> 
> s/FIXME: // (see the PNG decoder).

Done.

>> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:460
>> +        // FIXME: Don't force perceptual intent if the image profile contains an intent.
> 
> qcms_profile_get_rendering_intent() could be used to resolve this FIXME right now, no?

Currently, qcms does not honor the rendering intent parameter.

>> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:461
>> +        m_transform = qcms_transform_create(inputProfile,
> 
> Is a qcms_profile_is_bogus() call on the input profile warranted here?  Maybe the qcms bug list suggests too many false positives or other issues?

jrmuizel noted in a similar mozilla bug that bogus profiles only affect the images in which they are embedded.

>> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:468
>> +    qcms_transform* m_transform;
> 
> Space before this member would fit the style of this file.

Done.

>> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:605
>> +        if (info->out_color_space == JCS_RGB && m_reader->colorTransform())
> 
> Test m_reader->colorTransform() first?  The transform will be 0 in the common case.

Done.

>> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:189
>> +        // FIXME: Add support for gray profiles.
> 
> I'd remove this FIXME.  There's no such comment in the JPEG decoder, and this file already has a large comment about RGB|RGBA-only support.

Done.

>> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:202
>> +        // FIXME: We currently only support color profiles for RGB and RGBA images.
> 
> s/FIXME: // :)

Done.

>> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:204
>> +        // FIXME: Don't force perceptual intent if the image profile contains an intent.
> 
> qcms_profile_get_rendering_intent() could be used to resolve this FIXME now?

See response above.

>> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:302
>> +    // FIXME: Add support for grayscale color profiles.
> 
> I'd remove this FIXME, per line 189.

Done.

[Tony Payne]

Created attachment 144809 [details]
Patch

[Early Warning System Bot]

Comment on attachment 144809 [details]
Patch

Attachment 144809 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12809011

[Early Warning System Bot]

Comment on attachment 144809 [details]
Patch

Attachment 144809 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12848904

[Tony Payne]

Created attachment 144838 [details]
Patch

[Tony Payne]

Created attachment 145158 [details]
Patch

[noel gordon]

Comment on attachment 144838 [details]
Patch

Patch is all green bubbles now (congrats) and looks good to me.  Peter has done a lot of work on the ImageDecoders and is interested in color correction.  I'd like to hear his thoughts on this patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=144838&action=review

> Source/WebCore/ChangeLog:8
> +       No new tests (OOPS!).

Nit: remember to fill out this line before landing now you've updated test expectations.

[Adam Barth]

Comment on attachment 144838 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144838&action=review

Ok.  I'm ready to rs this patch after Peter takes a look.

> Source/WTF/wtf/Platform.h:470
>  #if USE(SKIA_ON_MAC_CHROMIUM)

We can get rid of this USE macro since we always use Skia on chromium-mac now.

> Source/WebCore/platform/image-decoders/ImageDecoder.h:301
> +            static bool qcmsInitialized = false;
> +            if (!qcmsInitialized) {
> +                qcmsInitialized = true;

Does this create a race condition?  Do we never decode images off the main thread (e.g., for workers or in the compositor)?  Please add an ASSERT that we're on the main thread onside the if-block.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:142
> +        delete[] m_rowBuffer;

Can we use OwnArrayPtr to avoid manually calling delete[] ?

[Stephen White]

Comment on attachment 144838 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144838&action=review

> LayoutTests/platform/chromium/test_expectations.txt:3656
> +BUGWK87761 MAC LINUX WIN : css3/filters/effect-blur-hw.html = IMAGE
> +BUGWK87761 MAC LINUX WIN : css3/filters/effect-blur.html = IMAGE
> +BUGWK87761 MAC LINUX WIN : css3/filters/effect-brightness-hw.html = IMAGE
> +BUGWK87761 MAC LINUX WIN : css3/filters/effect-brightness.html = IMAGE
> +BUGWK87761 MAC LINUX WIN : css3/filters/effect-combined-hw.html = IMAGE
> +BUGWK87761 MAC LINUX WIN : css3/filters/effect-combined.html = IMAGE
> +BUGWK87761 MAC LINUX WIN : css3/filters/effect-contrast-hw.html = IMAGE
> +BUGWK87761 MAC LINUX WIN : css3/filters/effect-contrast.html = IMAGE
> +BUGWK87761 MAC LINUX WIN : css3/filters/effect-drop-shadow-hw.html = IMAGE
> +BUGWK87761 MAC LINUX WIN : css3/filters/effect-drop-shadow.html = IMAGE
> +BUGWK87761 MAC LINUX WIN : css3/filters/effect-grayscale-hw.html = IMAGE
> +BUGWK87761 MAC LINUX WIN : css3/filters/effect-grayscale.html = IMAGE
> +BUGWK87761 MAC LINUX WIN : css3/filters/effect-hue-rotate-hw.html = IMAGE
> +BUGWK87761 MAC LINUX WIN : css3/filters/effect-hue-rotate.html = IMAGE
> +BUGWK87761 MAC LINUX WIN : css3/filters/effect-invert-hw.html = IMAGE
> +BUGWK87761 MAC LINUX WIN : css3/filters/effect-invert.html = IMAGE
> +BUGWK87761 MAC LINUX WIN : css3/filters/effect-opacity-hw.html = IMAGE
> +BUGWK87761 MAC LINUX WIN : css3/filters/effect-opacity.html = IMAGE
> +BUGWK87761 MAC LINUX WIN : css3/filters/effect-saturate-hw.html = IMAGE
> +BUGWK87761 MAC LINUX WIN : css3/filters/effect-saturate.html = IMAGE
> +BUGWK87761 MAC LINUX WIN : css3/filters/effect-sepia-hw.html = IMAGE
> +BUGWK87761 MAC LINUX WIN : css3/filters/effect-sepia.html = IMAGE
> +BUGWK87761 MAC LINUX WIN : css3/filters/regions-expanding.html = IMAGE

Any idea why the filters tests have been universally affected?  Is there color profile information in the reference.png file that's now being respected and wasn't before?  Or is this actually affecting filter processing in some way?

[Tony Payne]

(In reply to comment #86)
> (From update of attachment 144838 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144838&action=review
> 
> > LayoutTests/platform/chromium/test_expectations.txt:3656
> > +BUGWK87761 MAC LINUX WIN : css3/filters/effect-blur-hw.html = IMAGE
> > +BUGWK87761 MAC LINUX WIN : css3/filters/effect-blur.html = IMAGE
> > +BUGWK87761 MAC LINUX WIN : css3/filters/effect-brightness-hw.html = IMAGE
> > +BUGWK87761 MAC LINUX WIN : css3/filters/effect-brightness.html = IMAGE
> > +BUGWK87761 MAC LINUX WIN : css3/filters/effect-combined-hw.html = IMAGE
> > +BUGWK87761 MAC LINUX WIN : css3/filters/effect-combined.html = IMAGE
> > +BUGWK87761 MAC LINUX WIN : css3/filters/effect-contrast-hw.html = IMAGE
> > +BUGWK87761 MAC LINUX WIN : css3/filters/effect-contrast.html = IMAGE
> > +BUGWK87761 MAC LINUX WIN : css3/filters/effect-drop-shadow-hw.html = IMAGE
> > +BUGWK87761 MAC LINUX WIN : css3/filters/effect-drop-shadow.html = IMAGE
> > +BUGWK87761 MAC LINUX WIN : css3/filters/effect-grayscale-hw.html = IMAGE
> > +BUGWK87761 MAC LINUX WIN : css3/filters/effect-grayscale.html = IMAGE
> > +BUGWK87761 MAC LINUX WIN : css3/filters/effect-hue-rotate-hw.html = IMAGE
> > +BUGWK87761 MAC LINUX WIN : css3/filters/effect-hue-rotate.html = IMAGE
> > +BUGWK87761 MAC LINUX WIN : css3/filters/effect-invert-hw.html = IMAGE
> > +BUGWK87761 MAC LINUX WIN : css3/filters/effect-invert.html = IMAGE
> > +BUGWK87761 MAC LINUX WIN : css3/filters/effect-opacity-hw.html = IMAGE
> > +BUGWK87761 MAC LINUX WIN : css3/filters/effect-opacity.html = IMAGE
> > +BUGWK87761 MAC LINUX WIN : css3/filters/effect-saturate-hw.html = IMAGE
> > +BUGWK87761 MAC LINUX WIN : css3/filters/effect-saturate.html = IMAGE
> > +BUGWK87761 MAC LINUX WIN : css3/filters/effect-sepia-hw.html = IMAGE
> > +BUGWK87761 MAC LINUX WIN : css3/filters/effect-sepia.html = IMAGE
> > +BUGWK87761 MAC LINUX WIN : css3/filters/regions-expanding.html = IMAGE
> 
> Any idea why the filters tests have been universally affected?  Is there color profile information in the reference.png file that's now being respected and wasn't before?  Or is this actually affecting filter processing in some way?

Exactly. The reference PNG contains a Generic RGB (not sRGB!) color profile.

[Tony Payne]

Created attachment 145641 [details]
Patch

[Tony Payne]

Comment on attachment 144838 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144838&action=review

>> Source/WebCore/ChangeLog:8
>> +       No new tests (OOPS!).
> 
> Nit: remember to fill out this line before landing now you've updated test expectations.

Done.

>> Source/WTF/wtf/Platform.h:470
>>  #if USE(SKIA_ON_MAC_CHROMIUM)
> 
> We can get rid of this USE macro since we always use Skia on chromium-mac now.

Done.

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:301
>> +                qcmsInitialized = true;
> 
> Does this create a race condition?  Do we never decode images off the main thread (e.g., for workers or in the compositor)?  Please add an ASSERT that we're on the main thread onside the if-block.

Added atomic check.

>> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:142
>> +        delete[] m_rowBuffer;
> 
> Can we use OwnArrayPtr to avoid manually calling delete[] ?

Yes. Done.

[WebKit Review Bot]

Comment on attachment 145641 [details]
Patch

Attachment 145641 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12899451

New failing tests:
http/tests/media/media-source/video-media-source-event-attributes.html

[WebKit Review Bot]

Created attachment 145715 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[noel gordon]

(In reply to comment #87)
> > > +BUGWK87761 MAC LINUX WIN : css3/filters/effect-sepia-hw.html = IMAGE
> > > +BUGWK87761 MAC LINUX WIN : css3/filters/effect-sepia.html = IMAGE
> > > +BUGWK87761 MAC LINUX WIN : css3/filters/regions-expanding.html = IMAGE
> > 
> > Any idea why the filters tests have been universally affected?  Is there color profile information in the reference.png file that's now being respected and wasn't before?  Or is this actually affecting filter processing in some way?
> 
> Exactly. The reference PNG contains a Generic RGB (not sRGB!) color profile.

@senorblanco: is having a color profile in the reference image specifically needed for the filter tests?  Just curious.

[noel gordon]

(In reply to comment #89)
> >> Source/WebCore/platform/image-decoders/ImageDecoder.h:301
> >>
> > Does this create a race condition?  Do we never decode images off the main thread (e.g., for workers or in the compositor)?  Please add an ASSERT that we're on the main thread onside the if-block.
> 
> Added atomic check.

The increment always succeeds, returning 1 on the initial call to qcmsOutputDeviceProfile() if I'm reading correctly.  Seems we don't initialize at all.  What am I missing?

[Tony Payne]

(In reply to comment #93)
> (In reply to comment #89)
> > >> Source/WebCore/platform/image-decoders/ImageDecoder.h:301
> > >>
> > > Does this create a race condition?  Do we never decode images off the main thread (e.g., for workers or in the compositor)?  Please add an ASSERT that we're on the main thread onside the if-block.
> > 
> > Added atomic check.
> 
> The increment always succeeds, returning 1 on the initial call to qcmsOutputDeviceProfile() if I'm reading correctly.  Seems we don't initialize at all.  What am I missing?

(In reply to comment #93)
> (In reply to comment #89)
> > >> Source/WebCore/platform/image-decoders/ImageDecoder.h:301
> > >>
> > > Does this create a race condition?  Do we never decode images off the main thread (e.g., for workers or in the compositor)?  Please add an ASSERT that we're on the main thread onside the if-block.
> > 
> > Added atomic check.
> 
> The increment always succeeds, returning 1 on the initial call to qcmsOutputDeviceProfile() if I'm reading correctly.  Seems we don't initialize at all.  What am I missing?

My understanding is that the increment returns the previous value.

[Tony Payne]

Created attachment 145789 [details]
Patch

[Tony Payne]

(In reply to comment #94)
> (In reply to comment #93)
> > (In reply to comment #89)
> > > >> Source/WebCore/platform/image-decoders/ImageDecoder.h:301
> > > >>
> > > > Does this create a race condition?  Do we never decode images off the main thread (e.g., for workers or in the compositor)?  Please add an ASSERT that we're on the main thread onside the if-block.
> > > 
> > > Added atomic check.
> > 
> > The increment always succeeds, returning 1 on the initial call to qcmsOutputDeviceProfile() if I'm reading correctly.  Seems we don't initialize at all.  What am I missing?
> 
> (In reply to comment #93)
> > (In reply to comment #89)
> > > >> Source/WebCore/platform/image-decoders/ImageDecoder.h:301
> > > >>
> > > > Does this create a race condition?  Do we never decode images off the main thread (e.g., for workers or in the compositor)?  Please add an ASSERT that we're on the main thread onside the if-block.
> > > 
> > > Added atomic check.
> > 
> > The increment always succeeds, returning 1 on the initial call to qcmsOutputDeviceProfile() if I'm reading correctly.  Seems we don't initialize at all.  What am I missing?
> 
> My understanding is that the increment returns the previous value.

You are correct. I was misled by the following comment in Atomics.h:

// Note, atomic_{add, sub}_value() return the previous value of addend's content.

It goes on to add 1 to that value. The documentation for InterlockedIncrement and OSAtomicIncrement32Barrier concur that the return is the incremented value.

Fixed.

[Stephen White]

(In reply to comment #92)
> (In reply to comment #87)
> > > > +BUGWK87761 MAC LINUX WIN : css3/filters/effect-sepia-hw.html = IMAGE
> > > > +BUGWK87761 MAC LINUX WIN : css3/filters/effect-sepia.html = IMAGE
> > > > +BUGWK87761 MAC LINUX WIN : css3/filters/regions-expanding.html = IMAGE
> > > 
> > > Any idea why the filters tests have been universally affected?  Is there color profile information in the reference.png file that's now being respected and wasn't before?  Or is this actually affecting filter processing in some way?
> > 
> > Exactly. The reference PNG contains a Generic RGB (not sRGB!) color profile.
> 
> @senorblanco: is having a color profile in the reference image specifically needed for the filter tests?  Just curious.

No, I don't think so (although I didn't write the tests).

One thing about SVG filters (and possibly CSS filters, although that hasn't been written into the CSS filters spec yet AFAIK) is that they have the option to process either in sRGB or linear space.  By default, it's done in sRGB.  In the CoreGraphics port, this is done setting the desired colorspace on both the Image and GraphicsContext objects, and CoreGraphics figures out if any LUT needs to be applied.

On all the other ports, it's done by applying a LUT before and after each filter application, which is slow and also introduces some nasty colour banding.  One of my long-term projects was to implement proper color space handling in Skia, as it's done in CoreGraphics.  I think this would allow us to skip unnecessary LUT application.

Would this current patch need to be revisited in that case, or is it compatible with that goal?

[noel gordon]

(In reply to comment #96)

> You are correct. I was misled by the following comment in Atomics.h:
> 
> // Note, atomic_{add, sub}_value() return the previous value of addend's content.
> 
> It goes on to add 1 to that value. The documentation for InterlockedIncrement and OSAtomicIncrement32Barrier concur that the return is the incremented value.
> 
> Fixed.

OK thanks, and an example of WebKit policy about commenting.  Comments are so often confused, equivocal, plain wrong, or so poorly written, that I tend to ignore them.  The code is the only real documentation and it's always up-to-date.

Also try to be in the habit of running layout tests in your client prior to webkit-patch upload.  For example, comment out your TestExpectations changes locally and run tests.  You'll see image diffs sure, but the diff images should be color corrected.  That would not have be so with the increment bug just discussed and your local test results would've looked broken.

Minor: the increment test eventually wraps to 0 (takes a long time), should/could you prevent that?

[WebKit Review Bot]

Comment on attachment 145789 [details]
Patch

Attachment 145789 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12901718

New failing tests:
svg/wicd/test-rightsizing-b.xhtml
fast/css/background-shorthand-invalid-url.html
fast/reflections/reflection-masks-opacity.html
fast/reflections/reflection-with-zoom.html
fast/reflections/reflection-masks-outset.html
tables/mozilla/bugs/bug82946-2.html
compositing/masks/direct-image-mask.html
platform/chromium/virtual/threaded/compositing/visibility/visibility-image-layers.html
svg/custom/image-with-transform-clip-filter.svg
fast/reflections/reflection-direction.html
fast/reflections/reflection-masks.html
svg/as-border-image/svg-as-border-image-2.html
svg/custom/pointer-events-image-css-transform.svg
compositing/visibility/visibility-image-layers.html
svg/custom/pointer-events-image.svg
fast/media/mq-min-pixel-ratio.html
compositing/reflections/simple-composited-reflections.html

[WebKit Review Bot]

Created attachment 145942 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Tony Payne]

Created attachment 146197 [details]
Patch

[Tony Payne]

Latest patch has all green bubbles.

[Adam Barth]

Comment on attachment 146197 [details]
Patch

This looks good as far as I can tell.  I'm relying on Noel's expertise in this area.  Please make sure Noel is happy before landing.

[Tony Chang]

Comment on attachment 146197 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146197&action=review

> Source/WebCore/platform/image-decoders/ImageDecoder.h:301
> +            static int volatile qcmsInitialized = 0;
> +            if (atomicIncrement(&qcmsInitialized) == 1) {

Do we need to be worried about 2 threads racing here?

[Tony Payne]

Comment on attachment 146197 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146197&action=review

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:301
>> +            if (atomicIncrement(&qcmsInitialized) == 1) {
> 
> Do we need to be worried about 2 threads racing here?

According to Noel, the decoders are currently thread-safe and he would prefer they remain thread-safe.

[Tony Chang]

(In reply to comment #105)
> (From update of attachment 146197 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146197&action=review
> 
> >> Source/WebCore/platform/image-decoders/ImageDecoder.h:301
> >> +            if (atomicIncrement(&qcmsInitialized) == 1) {
> > 
> > Do we need to be worried about 2 threads racing here?
> 
> According to Noel, the decoders are currently thread-safe and he would prefer they remain thread-safe.

I see, that's fine with me. Should this patch be added to the commit queue?

[Tony Payne]

(In reply to comment #106)
> (In reply to comment #105)
> > (From update of attachment 146197 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=146197&action=review
> > 
> > >> Source/WebCore/platform/image-decoders/ImageDecoder.h:301
> > >> +            if (atomicIncrement(&qcmsInitialized) == 1) {
> > > 
> > > Do we need to be worried about 2 threads racing here?
> > 
> > According to Noel, the decoders are currently thread-safe and he would prefer they remain thread-safe.
> 
> I see, that's fine with me. Should this patch be added to the commit queue?

Noel is to add cq+ once he is comfortable with the patch. At this point, I'm not aware of any outstanding concerns, so I do not know what the delay is.

[noel gordon]

(In reply to comment #105)
> (From update of attachment 146197 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146197&action=review
> 
> >> Source/WebCore/platform/image-decoders/ImageDecoder.h:301
> >> +            if (atomicIncrement(&qcmsInitialized) == 1) {
> > 
> > Do we need to be worried about 2 threads racing here?
> 
> According to Noel, the decoders are currently thread-safe and he would prefer they remain thread-safe.

That wasn't the question - the question Tony (tc) asked was about this _new atomicIncrement code_ you added and whether it races.  Well, does it?

[noel gordon]

(In reply to comment #97)
> > > > Any idea why the filters tests have been universally affected?  Is there color profile information in the reference.png file that's now being respected and wasn't before?  Or is this actually affecting filter processing in some way?
> > > 
> > > Exactly. The reference PNG contains a Generic RGB (not sRGB!) color profile.
> > 
> > @senorblanco: is having a color profile in the reference image specifically needed for the filter tests?  Just curious.
> 
> No, I don't think so (although I didn't write the tests).
 
Thanks Stephen, attached example differences for reference test image.  GenericRGB is nor the same as sRGB.

[noel gordon]

Created attachment 147080 [details]
example-css3-filter-test-diffs

[noel gordon]

(In reply to comment #107)
> Noel is to add cq+ once he is comfortable with the patch. At this point, I'm not aware of any outstanding concerns, so I do not know what the delay is.

I ran chromium-mac layout tests with your patch and fast/reflections/reflection-with-zoom.html failed for me.  I loaded the test and compared safari, chrome-mac, and chrome-mac with you change (attached).

[noel gordon]

Created attachment 147082 [details]
safari-5.1.7, chrome-mac, chrome-mac-81797

Renderings of fast/reflections/reflection-with-zoom.html

[noel gordon]

On chrome-mac, Cary's code painted profiled images to a target rendering bitmap with deviceRGBColorSpaceRef().  The patch uses sRGB, and that changes rendering on chrome-mac as shown in the attachment.  I'm not sure it's progression, and I think chrome-mac users will notice.

[Tony Payne]

Created attachment 147219 [details]
Patch

[noel gordon]

Interesting approach.  Grab the colorSpace off the primary monitor ID, which is exactly what the chrome layers of Chrome do, and the sandbox sanctions these particular CG API calls.

You'll probably have some bugs with users who fiddle profiles often, or when starting chrome from a second monitor and such, but we already have those bugs.  

I'd consider trying to use a genericRGB profile and compare, but if the test results on mac-chrome look good, this patch looks promising.

[Tony Payne]

(In reply to comment #115)
> Interesting approach.  Grab the colorSpace off the primary monitor ID, which is exactly what the chrome layers of Chrome do, and the sandbox sanctions these particular CG API calls.
> 
> You'll probably have some bugs with users who fiddle profiles often, or when starting chrome from a second monitor and such, but we already have those bugs.  
> 
> I'd consider trying to use a genericRGB profile and compare, but if the test results on mac-chrome look good, this patch looks promising.

I have a try job running to see what it looks like with the genericRGB profile: http://build.chromium.org/p/tryserver.chromium/builders/mac_layout_rel/builds/444

[noel gordon]

Comment on attachment 147219 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147219&action=review

> Source/WebCore/platform/image-decoders/ImageDecoder.h:306
> +            if (atomicIncrement(&qcmsInitialized) == 1) {

Race day.  Let's make qcmsInitialized a bool initialized to false, remove the Atomics.h, and make it

    if (!qcmsInitialized) {
        ASSERT(isMainThread());
        qcmsInitialized = true;
        ...

> Source/WebCore/platform/image-decoders/ImageDecoder.h:310
> +                CGColorSpaceRef monitorColorSpace(CGDisplayCopyColorSpace(CGMainDisplayID()));

I'd maybe use a RetainPtr here

RetainPtr<CGColorSpaceRef> monitorColorSpace(AdoptCF, CGDisplayCopyColorSpace(CGMainDisplayID()));

> Source/WebCore/platform/image-decoders/ImageDecoder.h:317
> +                        outputDeviceProfile = 0;

qcms_profile_release the outputDeviceProfile here before setting it to 0, you'll leak memory otherwise.

> Source/WebCore/platform/image-decoders/ImageDecoder.h:321
> +                CGColorSpaceRelease(monitorColorSpace);

With a RetainPtr<> on monitorColorSpace, this line is not needed (the colorSpace is auto released when it falls out of scope).

[noel gordon]

(In reply to comment #116)
> I have a try job running to see what it looks like with the genericRGB profile: http://build.chromium.org/p/tryserver.chromium/builders/mac_layout_rel/builds/444

Awesome.  I think we can address the genericRGB issue on another bug.  This patch is looking good.  So if you're happy with tests, attend to the nitsabove, and prepare a patch for landing.  I'd be happy to see Adam r+ and cq+ that patch.

[Tony Payne]

Created attachment 147395 [details]
Patch

[Adam Barth]

Comment on attachment 147395 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147395&action=review

> Source/WebCore/platform/image-decoders/ImageDecoder.h:307
> +            if (!qcmsInitialized) {

I would have put the body of branch out-of-line since it's kind of silly to inline it into each call site.

[WebKit Review Bot]

Comment on attachment 147395 [details]
Patch

Rejecting attachment 147395 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
s/Pattern.o
In file included from Source/WebCore/platform/graphics/ImageSource.cpp:32:
Source/WebCore/platform/image-decoders/ImageDecoder.h: In static member function 'static qcms_profile* WebCore::ImageDecoder::qcmsOutputDeviceProfile()':
Source/WebCore/platform/image-decoders/ImageDecoder.h:308: error: 'isMainThread' was not declared in this scope
make: *** [out/Debug/obj.target/webcore_platform/Source/WebCore/platform/graphics/ImageSource.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/12950579

[Tony Payne]

Created attachment 147642 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 147642 [details]
Patch for landing

Rejecting attachment 147642 [details] from commit-queue.

New failing tests:
media/video-playing-and-pause.html
media/video-zoom-controls.html
Full output: http://queues.webkit.org/results/12956524

[WebKit Review Bot]

Created attachment 147703 [details]
Archive of layout-test-results from ec2-cq-03

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: ec2-cq-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Review Bot]

Comment on attachment 147642 [details]
Patch for landing

Clearing flags on attachment: 147642

Committed r120393: <http://trac.webkit.org/changeset/120393>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 89163

[Tony Payne]

Created attachment 147860 [details]
Fixed CG include that no longer works

[WebKit Review Bot]

Comment on attachment 147860 [details]
Fixed CG include that no longer works

Clearing flags on attachment: 147860

Committed r120485: <http://trac.webkit.org/changeset/120485>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 89286

[WebKit Review Bot]

Re-opened since this is blocked by 89345

[Tony Payne]

Created attachment 148093 [details]
Patch

[Tony Payne]

Created attachment 148094 [details]
Patch

[Tony Payne]

I put back the atomics code, since this patch breaks some browser_tests. According to hbono, browser tests run the renderer on other threads.

[Csaba Osztrogonác]

(In reply to comment #129)
> (From update of attachment 147860 [details])
> Clearing flags on attachment: 147860
> 
> Committed r120485: <http://trac.webkit.org/changeset/120485>

Additionally it broke the Chromium build on ARM, because
the thirdparty qcms has an ugly bug:

WebKit/chromium/third_party/qcms/qcms.gyp:
...
'conditions': [
        [ 'OS in ["linux", "freebsd", "openbsd", "solaris"]', {
          'cflags': [
            '-msse',
            '-msse2',
          ],
        }],
        [ 'target_arch != "arm"', {
          'sources': [
            'src/transform-sse1.c',
            'src/transform-sse2.c',
          ],
...

Passing -msse and -msse2 caused build error on ARM. I don't know how is it
possible to fix this 3rdparty thing. It would be great if a chromium expert
can pick up this bug.

[Tony Payne]

I have a fix for the cros_arm compilation problem here: http://codereview.chromium.org/10561030/

[Tony Payne]

Chromium change committed to fix sse/sse2 on cros_arm. This should be safe to go back in.

[Tony Payne]

Created attachment 148111 [details]
Removed test expectations

[Tony Payne]

Created attachment 148113 [details]
Removed atomics and assert

[WebKit Review Bot]

Comment on attachment 148113 [details]
Removed atomics and assert

Attachment 148113 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12981156

New failing tests:
platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html
fast/text/international/thai-line-breaks.html

[WebKit Review Bot]

Created attachment 148134 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Adam Barth]

Comment on attachment 148113 [details]
Removed atomics and assert

Clearing flags on attachment: 148113

Committed r120613: <http://trac.webkit.org/changeset/120613>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.