Bug 81974 - Adds color management support to Chromium port.
: Adds color management support to Chromium port.
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 89163 89286 89345
: 87761 87781 88565
  Show dependency treegraph
 
Reported: 2012-03-22 14:59 PST by
Modified: 2012-06-18 11:53 PST (History)


Attachments
Patch for crbug.com/143, Part 1 of 2 (1.73 MB, patch)
2012-03-27 13:42 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Fix for crbug.com/143, Part 2 of 2 (1.13 MB, patch)
2012-03-27 13:42 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Patch for crbug.com/143 part 1 (1017.44 KB, patch)
2012-04-04 11:17 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Pat (1.38 MB, patch)
2012-04-04 11:17 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Patch for crbug.com/143 part 2 (1.38 MB, patch)
2012-04-04 11:18 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Patch for crbug.com/143 part 1 (1017.42 KB, patch)
2012-04-05 13:36 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Patch for crbug.com/143 part 2 (1.38 MB, patch)
2012-04-05 13:37 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Patch for crbug.com/143 part 1 (1017.42 KB, patch)
2012-04-05 13:37 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Patch (8.75 KB, patch)
2012-04-11 14:23 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.06 MB, application/zip)
2012-04-11 16:29 PST, WebKit Review Bot
no flags Details
Patch (992.10 KB, patch)
2012-04-13 10:20 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Patch (984.13 KB, patch)
2012-04-17 11:36 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (6.33 MB, application/zip)
2012-04-17 13:26 PST, WebKit Review Bot
no flags Details
Archive of layout-test-results from ec2-cr-linux-04 (6.30 MB, application/zip)
2012-04-17 14:35 PST, WebKit Review Bot
no flags Details
Patch (984.18 KB, patch)
2012-04-20 14:09 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (5.99 MB, application/zip)
2012-04-20 15:52 PST, WebKit Review Bot
no flags Details
Archive of layout-test-results from ec2-cr-linux-04 (5.86 MB, application/zip)
2012-04-20 16:55 PST, WebKit Review Bot
no flags Details
Patch (990.76 KB, patch)
2012-05-07 17:05 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (10.32 MB, application/zip)
2012-05-07 19:56 PST, WebKit Review Bot
no flags Details
Archive of layout-test-results from ec2-cr-linux-04 (9.41 MB, application/zip)
2012-05-07 20:41 PST, WebKit Review Bot
no flags Details
Patch (994.01 KB, patch)
2012-05-08 17:36 PST, Tony Payne
noel.gordon: review-
tpayne: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch (18.05 KB, patch)
2012-05-14 16:55 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Patch (21.63 KB, patch)
2012-05-17 18:45 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Patch (21.72 KB, patch)
2012-05-18 11:01 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Patch (22.80 KB, patch)
2012-05-23 15:33 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Patch (25.85 KB, patch)
2012-05-23 18:25 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Patch ICM2.0 jpeg decoder (156.40 KB, patch)
2012-05-26 02:55 PST, noel gordon
no flags Review Patch | Details | Formatted Diff | Diff
screen shot jpeg-decode-with-icm2.0-chrome-win (942.40 KB, image/jpeg)
2012-05-26 03:32 PST, noel gordon
no flags Details
Patch (32.34 KB, patch)
2012-05-29 14:26 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Patch (32.16 KB, patch)
2012-05-30 07:31 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Patch (32.09 KB, patch)
2012-05-30 09:32 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Patch (34.01 KB, patch)
2012-05-31 14:37 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Patch (32.37 KB, patch)
2012-06-04 15:33 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (875.76 KB, application/zip)
2012-06-05 00:46 PST, WebKit Review Bot
no flags Details
Patch (32.37 KB, patch)
2012-06-05 07:30 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (8.59 MB, application/zip)
2012-06-05 23:29 PST, WebKit Review Bot
no flags Details
Patch (33.11 KB, patch)
2012-06-06 22:20 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
example-css3-filter-test-diffs (58.06 KB, image/png)
2012-06-12 07:24 PST, noel gordon
no flags Details
safari-5.1.7, chrome-mac, chrome-mac-81797 (479.41 KB, image/png)
2012-06-12 07:32 PST, noel gordon
no flags Details
Patch (34.15 KB, patch)
2012-06-12 19:47 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Patch (34.19 KB, patch)
2012-06-13 12:58 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (34.22 KB, patch)
2012-06-14 13:52 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cq-03 (1.82 MB, application/zip)
2012-06-14 18:47 PST, WebKit Review Bot
no flags Details
Fixed CG include that no longer works (34.11 KB, patch)
2012-06-15 11:06 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Patch (34.14 KB, patch)
2012-06-18 07:04 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Patch (34.08 KB, patch)
2012-06-18 07:13 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Removed test expectations (27.04 KB, patch)
2012-06-18 09:55 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Removed atomics and assert (27.05 KB, patch)
2012-06-18 10:01 PST, Tony Payne
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.40 MB, application/zip)
2012-06-18 11:30 PST, WebKit Review Bot
no flags Details


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-03-22 14:59:39 PST
Adds color management support to Chromium port.
------- Comment #1 From 2012-03-27 13:42:22 PST -------
Created an attachment (id=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.
------- Comment #2 From 2012-03-27 13:42:53 PST -------
Created an attachment (id=134127) [details]
Fix for crbug.com/143, Part 2 of 2

See comment for Part 1
------- Comment #3 From 2012-04-04 11:17:02 PST -------
Created an attachment (id=135635) [details]
Patch for crbug.com/143 part 1
------- Comment #4 From 2012-04-04 11:17:22 PST -------
Created an attachment (id=135636) [details]
Pat
------- Comment #5 From 2012-04-04 11:18:19 PST -------
Created an attachment (id=135637) [details]
Patch for crbug.com/143 part 2
------- Comment #6 From 2012-04-05 13:36:38 PST -------
Created an attachment (id=135891) [details]
Patch for crbug.com/143 part 1
------- Comment #7 From 2012-04-05 13:37:02 PST -------
Created an attachment (id=135892) [details]
Patch for crbug.com/143 part 2
------- Comment #8 From 2012-04-05 13:37:45 PST -------
Created an attachment (id=135893) [details]
Patch for crbug.com/143 part 1
------- Comment #9 From 2012-04-11 14:23:03 PST -------
Created an attachment (id=136755) [details]
Patch
------- Comment #10 From 2012-04-11 14:27:44 PST -------
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.
------- Comment #11 From 2012-04-11 16:29:07 PST -------
(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
------- Comment #12 From 2012-04-11 16:29:10 PST -------
Created an attachment (id=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
------- Comment #13 From 2012-04-12 15:04:03 PST -------
(In reply to comment #11)
> (From update of attachment 136755 [details] [details])
> Attachment 136755 [details] [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.
------- Comment #14 From 2012-04-13 10:20:12 PST -------
Created an attachment (id=137101) [details]
Patch
------- Comment #15 From 2012-04-17 11:36:26 PST -------
Created an attachment (id=137569) [details]
Patch
------- Comment #16 From 2012-04-17 13:26:49 PST -------
(From update of attachment 137569 [details])
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
------- Comment #17 From 2012-04-17 13:26:55 PST -------
Created an attachment (id=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
------- Comment #18 From 2012-04-17 14:35:01 PST -------
(From update of attachment 137569 [details])
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
------- Comment #19 From 2012-04-17 14:35:07 PST -------
Created an attachment (id=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
------- Comment #20 From 2012-04-19 16:07:22 PST -------
(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.

> 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?
------- Comment #21 From 2012-04-19 16:19:48 PST -------
(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.

> > 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.
------- Comment #22 From 2012-04-19 18:21:48 PST -------
(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.
------- Comment #23 From 2012-04-19 18:52:44 PST -------
(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.

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).
------- Comment #24 From 2012-04-19 18:58:50 PST -------
(In reply to comment #23)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > (From update of attachment 137569 [details] [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.
------- Comment #25 From 2012-04-20 14:09:01 PST -------
Created an attachment (id=138159) [details]
Patch
------- Comment #26 From 2012-04-20 14:11:19 PST -------
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.
------- Comment #27 From 2012-04-20 15:52:14 PST -------
(From update of attachment 138159 [details])
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
------- Comment #28 From 2012-04-20 15:52:20 PST -------
Created an attachment (id=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
------- Comment #29 From 2012-04-20 16:55:03 PST -------
(From update of attachment 138159 [details])
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
------- Comment #30 From 2012-04-20 16:55:10 PST -------
Created an attachment (id=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
------- Comment #31 From 2012-04-22 21:17:50 PST -------
(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 ...
------- Comment #32 From 2012-04-23 05:25:02 PST -------
(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. 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.
------- Comment #33 From 2012-04-23 05:27:50 PST -------
(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.
------- Comment #34 From 2012-04-23 05:52:04 PST -------
(From update of attachment 138159 [details])
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.
------- Comment #35 From 2012-04-23 06:46:20 PST -------
> 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).
------- Comment #36 From 2012-05-07 17:05:22 PST -------
Created an attachment (id=140624) [details]
Patch
------- Comment #37 From 2012-05-07 19:56:23 PST -------
(From update of attachment 140624 [details])
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
------- Comment #38 From 2012-05-07 19:56:32 PST -------
Created an attachment (id=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
------- Comment #39 From 2012-05-07 20:41:49 PST -------
(From update of attachment 140624 [details])
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
------- Comment #40 From 2012-05-07 20:41:57 PST -------
Created an attachment (id=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
------- Comment #41 From 2012-05-08 17:36:38 PST -------
Created an attachment (id=140828) [details]
Patch
------- Comment #42 From 2012-05-08 18:41:12 PST -------
(From update of attachment 140828 [details])
Attachment 140828 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12653439
------- Comment #43 From 2012-05-13 08:43:59 PST -------
(From update of attachment 140828 [details])
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.
------- Comment #44 From 2012-05-14 16:55:40 PST -------
Created an attachment (id=141813) [details]
Patch
------- Comment #45 From 2012-05-14 16:56:31 PST -------
(From update of attachment 140828 [details])
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.
------- Comment #46 From 2012-05-14 17:35:06 PST -------
(From update of attachment 141813 [details])
Attachment 141813 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12704116
------- Comment #47 From 2012-05-17 10:04:14 PST -------
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.
------- Comment #48 From 2012-05-17 10:49:37 PST -------
(From update of attachment 141813 [details])
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
> +2012-05-14  Tony Payne  <tpayne@chromium.org>

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()) {
    ...
------- Comment #49 From 2012-05-17 10:52:08 PST -------
(From update of attachment 141813 [details])
Maybe you accidentally set r+, a reviewer will do that for you.
------- Comment #50 From 2012-05-17 18:42:45 PST -------
(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.
------- Comment #51 From 2012-05-17 18:45:38 PST -------
Created an attachment (id=142605) [details]
Patch
------- Comment #52 From 2012-05-17 18:48:33 PST -------
(From update of attachment 141813 [details])
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.
------- Comment #53 From 2012-05-17 20:59:11 PST -------
(From update of attachment 142605 [details])
Attachment 142605 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12720733
------- Comment #54 From 2012-05-17 21:07:12 PST -------
(From update of attachment 142605 [details])
Attachment 142605 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12726316
------- Comment #55 From 2012-05-17 22:37:32 PST -------
(From update of attachment 142605 [details])
Attachment 142605 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12716782
------- Comment #56 From 2012-05-18 11:01:12 PST -------
Created an attachment (id=142739) [details]
Patch
------- Comment #57 From 2012-05-18 11:35:19 PST -------
(From update of attachment 142739 [details])
Attachment 142739 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12724534
------- Comment #58 From 2012-05-18 11:43:20 PST -------
(From update of attachment 142739 [details])
Attachment 142739 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12726513
------- Comment #59 From 2012-05-18 12:20:07 PST -------
(From update of attachment 142739 [details])
Attachment 142739 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12722874
------- Comment #60 From 2012-05-21 06:19:54 PST -------
(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.
------- Comment #61 From 2012-05-21 06:21:57 PST -------
(In reply to comment #52)
> (From update of attachment 141813 [details] [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.
------- Comment #62 From 2012-05-21 06:41:08 PST -------
(From update of attachment 142739 [details])
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.
------- Comment #63 From 2012-05-22 14:20:03 PST -------
> > > 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.
------- Comment #64 From 2012-05-23 09:11:05 PST -------
(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
------- Comment #65 From 2012-05-23 15:33:39 PST -------
Created an attachment (id=143663) [details]
Patch
------- Comment #66 From 2012-05-23 16:46:45 PST -------
(From update of attachment 143663 [details])
Attachment 143663 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12778340
------- Comment #67 From 2012-05-23 16:49:22 PST -------
(From update of attachment 143663 [details])
Attachment 143663 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12792010
------- Comment #68 From 2012-05-23 18:25:02 PST -------
Created an attachment (id=143694) [details]
Patch
------- Comment #69 From 2012-05-23 19:11:45 PST -------
(From update of attachment 143694 [details])
Attachment 143694 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12794038
------- Comment #70 From 2012-05-25 02:21:13 PST -------
(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
------- Comment #71 From 2012-05-26 02:48:42 PST -------
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?
------- Comment #72 From 2012-05-26 02:51:15 PST -------
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 ...
------- Comment #73 From 2012-05-26 02:55:39 PST -------
Created an attachment (id=144201) [details]
Patch ICM2.0 jpeg decoder
------- Comment #74 From 2012-05-26 03:32:11 PST -------
Created an attachment (id=144202) [details]
screen shot jpeg-decode-with-icm2.0-chrome-win
------- Comment #75 From 2012-05-26 04:59:37 PST -------
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.
------- Comment #76 From 2012-05-26 07:59:17 PST -------
(From update of attachment 143694 [details])
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.
------- Comment #77 From 2012-05-29 14:26:13 PST -------
Created an attachment (id=144620) [details]
Patch
------- Comment #78 From 2012-05-29 14:27:08 PST -------
(From update of attachment 143694 [details])
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.
------- Comment #79 From 2012-05-30 07:31:25 PST -------
Created an attachment (id=144809) [details]
Patch
------- Comment #80 From 2012-05-30 09:05:33 PST -------
(From update of attachment 144809 [details])
Attachment 144809 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12809011
------- Comment #81 From 2012-05-30 09:29:02 PST -------
(From update of attachment 144809 [details])
Attachment 144809 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12848904
------- Comment #82 From 2012-05-30 09:32:13 PST -------
Created an attachment (id=144838) [details]
Patch
------- Comment #83 From 2012-05-31 14:37:38 PST -------
Created an attachment (id=145158) [details]
Patch
------- Comment #84 From 2012-06-01 21:48:59 PST -------
(From update of attachment 144838 [details])
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.
------- Comment #85 From 2012-06-02 23:31:53 PST -------
(From update of attachment 144838 [details])
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[] ?
------- Comment #86 From 2012-06-04 12:10:52 PST -------
(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?
------- Comment #87 From 2012-06-04 13:43:28 PST -------
(In reply to comment #86)
> (From update of attachment 144838 [details] [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.
------- Comment #88 From 2012-06-04 15:33:27 PST -------
Created an attachment (id=145641) [details]
Patch
------- Comment #89 From 2012-06-04 15:34:31 PST -------
(From update of attachment 144838 [details])
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.
------- Comment #90 From 2012-06-05 00:46:14 PST -------
(From update of attachment 145641 [details])
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
------- Comment #91 From 2012-06-05 00:46:30 PST -------
Created an attachment (id=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
------- Comment #92 From 2012-06-05 07:09:24 PST -------
(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.
------- Comment #93 From 2012-06-05 07:18:53 PST -------
(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?
------- Comment #94 From 2012-06-05 07:20:16 PST -------
(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.
------- Comment #95 From 2012-06-05 07:30:03 PST -------
Created an attachment (id=145789) [details]
Patch
------- Comment #96 From 2012-06-05 07:31:31 PST -------
(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.
------- Comment #97 From 2012-06-05 07:45:41 PST -------
(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?
------- Comment #98 From 2012-06-05 17:56:22 PST -------
(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?
------- Comment #99 From 2012-06-05 23:29:37 PST -------
(From update of attachment 145789 [details])
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
------- Comment #100 From 2012-06-05 23:29:59 PST -------
Created an attachment (id=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
------- Comment #101 From 2012-06-06 22:20:03 PST -------
Created an attachment (id=146197) [details]
Patch
------- Comment #102 From 2012-06-07 08:33:11 PST -------
Latest patch has all green bubbles.
------- Comment #103 From 2012-06-08 11:35:39 PST -------
(From update of attachment 146197 [details])
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.
------- Comment #104 From 2012-06-08 11:48:40 PST -------
(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
> +            static int volatile qcmsInitialized = 0;
> +            if (atomicIncrement(&qcmsInitialized) == 1) {

Do we need to be worried about 2 threads racing here?
------- Comment #105 From 2012-06-08 13:41:11 PST -------
(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.
------- Comment #106 From 2012-06-11 11:42:30 PST -------
(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?
------- Comment #107 From 2012-06-11 11:44:08 PST -------
(In reply to comment #106)
> (In reply to comment #105)
> > (From update of attachment 146197 [details] [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.
------- Comment #108 From 2012-06-12 00:35:33 PST -------
(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.

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?
------- Comment #109 From 2012-06-12 07:22:20 PST -------
(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.
------- Comment #110 From 2012-06-12 07:24:22 PST -------
Created an attachment (id=147080) [details]
example-css3-filter-test-diffs
------- Comment #111 From 2012-06-12 07:30:04 PST -------
(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).
------- Comment #112 From 2012-06-12 07:32:27 PST -------
Created an attachment (id=147082) [details]
safari-5.1.7, chrome-mac, chrome-mac-81797

Renderings of fast/reflections/reflection-with-zoom.html
------- Comment #113 From 2012-06-12 07:49:15 PST -------
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.
------- Comment #114 From 2012-06-12 19:47:21 PST -------
Created an attachment (id=147219) [details]
Patch
------- Comment #115 From 2012-06-13 10:21:24 PST -------
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.
------- Comment #116 From 2012-06-13 10:22:27 PST -------
(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
------- Comment #117 From 2012-06-13 11:47:48 PST -------
(From update of attachment 147219 [details])
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).
------- Comment #118 From 2012-06-13 11:53:14 PST -------
(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.
------- Comment #119 From 2012-06-13 12:58:18 PST -------
Created an attachment (id=147395) [details]
Patch
------- Comment #120 From 2012-06-13 13:20:40 PST -------
(From update of attachment 147395 [details])
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.
------- Comment #121 From 2012-06-13 21:42:51 PST -------
(From update of attachment 147395 [details])
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
------- Comment #122 From 2012-06-14 13:52:20 PST -------
Created an attachment (id=147642) [details]
Patch for landing
------- Comment #123 From 2012-06-14 18:47:22 PST -------
(From update of attachment 147642 [details])
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
------- Comment #124 From 2012-06-14 18:47:30 PST -------
Created an attachment (id=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
------- Comment #125 From 2012-06-14 19:53:34 PST -------
(From update of attachment 147642 [details])
Clearing flags on attachment: 147642

Committed r120393: <http://trac.webkit.org/changeset/120393>
------- Comment #126 From 2012-06-14 19:53:47 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #127 From 2012-06-14 21:02:23 PST -------
Re-opened since this is blocked by 89163
------- Comment #128 From 2012-06-15 11:06:55 PST -------
Created an attachment (id=147860) [details]
Fixed CG include that no longer works
------- Comment #129 From 2012-06-15 12:52:05 PST -------
(From update of attachment 147860 [details])
Clearing flags on attachment: 147860

Committed r120485: <http://trac.webkit.org/changeset/120485>
------- Comment #130 From 2012-06-15 12:52:20 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #131 From 2012-06-16 12:40:25 PST -------
Re-opened since this is blocked by 89286
------- Comment #132 From 2012-06-18 06:37:52 PST -------
Re-opened since this is blocked by 89345
------- Comment #133 From 2012-06-18 07:04:42 PST -------
Created an attachment (id=148093) [details]
Patch
------- Comment #134 From 2012-06-18 07:13:01 PST -------
Created an attachment (id=148094) [details]
Patch
------- Comment #135 From 2012-06-18 07:13:53 PST -------
I put back the atomics code, since this patch breaks some browser_tests. According to hbono, browser tests run the renderer on other threads.
------- Comment #136 From 2012-06-18 07:19:05 PST -------
(In reply to comment #129)
> (From update of attachment 147860 [details] [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.
------- Comment #137 From 2012-06-18 07:20:28 PST -------
I have a fix for the cros_arm compilation problem here: http://codereview.chromium.org/10561030/
------- Comment #138 From 2012-06-18 09:07:10 PST -------
Chromium change committed to fix sse/sse2 on cros_arm. This should be safe to go back in.
------- Comment #139 From 2012-06-18 09:55:30 PST -------
Created an attachment (id=148111) [details]
Removed test expectations
------- Comment #140 From 2012-06-18 10:01:08 PST -------
Created an attachment (id=148113) [details]
Removed atomics and assert
------- Comment #141 From 2012-06-18 11:30:27 PST -------
(From update of attachment 148113 [details])
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
------- Comment #142 From 2012-06-18 11:30:36 PST -------
Created an attachment (id=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
------- Comment #143 From 2012-06-18 11:52:37 PST -------
(From update of attachment 148113 [details])
Clearing flags on attachment: 148113

Committed r120613: <http://trac.webkit.org/changeset/120613>
------- Comment #144 From 2012-06-18 11:53:10 PST -------
All reviewed patches have been landed.  Closing bug.