Bug 80321 – WebCore::ImageBuffer.cpp has broken color table code

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Stephen Chenney]

The current code for computing color space conversions in WebCore/platform/graphics/ImageBuffer.cpp looks like this:

In the header:
        Vector<int> m_linearRgbLUT;
        Vector<int> m_deviceRgbLUT;

In the class code:

void ImageBuffer::transformColorSpace(ColorSpace srcColorSpace, ColorSpace dstColorSpace)
{
    if (srcColorSpace == dstColorSpace)
        return;

    // only sRGB <-> linearRGB are supported at the moment
    if ((srcColorSpace != ColorSpaceLinearRGB && srcColorSpace != ColorSpaceDeviceRGB)
        || (dstColorSpace != ColorSpaceLinearRGB && dstColorSpace != ColorSpaceDeviceRGB))
        return;

    if (dstColorSpace == ColorSpaceLinearRGB) {
        if (m_linearRgbLUT.isEmpty()) {
            for (unsigned i = 0; i < 256; i++) {
                float color = i  / 255.0f;
                color = (color <= 0.04045f ? color / 12.92f : pow((color + 0.055f) / 1.055f, 2.4f));
                color = std::max(0.0f, color);
                color = std::min(1.0f, color);
                m_linearRgbLUT.append(static_cast<int>(color * 255));
            }
        }
        platformTransformColorSpace(m_linearRgbLUT);
    } else if (dstColorSpace == ColorSpaceDeviceRGB) {
        if (m_deviceRgbLUT.isEmpty()) {
            for (unsigned i = 0; i < 256; i++) {
                float color = i / 255.0f;
                color = (powf(color, 1.0f / 2.4f) * 1.055f) - 0.055f;
                color = std::max(0.0f, color);
                color = std::min(1.0f, color);
                m_deviceRgbLUT.append(static_cast<int>(color * 255));
            }
        }
        platformTransformColorSpace(m_deviceRgbLUT);
    }
}

There are two major problems with this. 1. The color space LUT are constants, and should be static. 2. The numerical code to compute the color space transforms has numerical imprecision, meaning that the Linear -> Device LUT maps 255 to 254. So every filter that is applied in SVG, and maybe CSS, drops the intensity of the resulting pixels.

Unfortunately, fixing this will cause massive rebaseline needs as lots of images will change imperceptibly.

[Tim Horton]

> Unfortunately, fixing this will cause massive rebaseline needs as lots of images will change imperceptibly.

Keep in mind that this code isn't used by CG/Mac, so when rebaselining, please don't change results in platform/mac.

[Stephen Chenney]

Created attachment 146097 [details]
Patch

[Stephen Chenney]

I have not attempted to rebaseline images, and have just set expectations for Chromium. Mac does not use this code path and is unaffected. I imagine everyone else will have some pain when this goes in, so I'll be sure to announce widely on IRC when it is imminent.

I still have to make a basic ref test for this. Should have it by end of day.

[Dirk Schulze]

Comment on attachment 146097 [details]
Patch

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

r=me. Good catch!

> Source/WebCore/ChangeLog:18
> +        Second, the code for fill ign the table was mapping 255 to 254, thus

s/ign/ignore/?

[WebKit Review Bot]

Comment on attachment 146097 [details]
Patch

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

New failing tests:
platform/chromium/compositing/accelerated-drawing/svg-filters.html

[WebKit Review Bot]

Created attachment 146205 [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

[Stephen Chenney]

Created attachment 146297 [details]
Patch

New patch to run through ews, with the new test. Otherwise unchanged.

[Stephen Chenney]

Committed r119831: <http://trac.webkit.org/changeset/119831>

[Stephen Chenney]

Bug still open pending test rebaselines.

(In reply to comment #8)
> Committed r119831: <http://trac.webkit.org/changeset/119831>

The test added in this revision is failing in OS X Lion: <http://build.webkit.org/results/Lion%20Debug%20(Tests)/r119833%20(7472)/results.html>.

[Stephen Chenney]

(In reply to comment #10)
> The test added in this revision is failing in OS X Lion: <http://build.webkit.org/results/Lion%20Debug%20(Tests)/r119833%20(7472)/results.html>.

It seems the test should be skipped or a failure bug created for Mac. All it's doing is verifying that a basic filter operation does not modify the colors. Mac uses a different code path, which is apparently modifying the colors.

Will you create the bug, or me?

[Stephen Chenney]

Committed r119841: <http://trac.webkit.org/changeset/119841>

(In reply to comment #11)
> (In reply to comment #10)
> > The test added in this revision is failing in OS X Lion: <http://build.webkit.org/results/Lion%20Debug%20(Tests)/r119833%20(7472)/results.html>.
> 
> It seems the test should be skipped or a failure bug created for Mac. All it's doing is verifying that a basic filter operation does not modify the colors. Mac uses a different code path, which is apparently modifying the colors.
> 
> Will you create the bug, or me?

I filed bug 88672.

[Stephen Chenney]

Committed r119860: <http://trac.webkit.org/changeset/119860>