Bug 80321 - WebCore::ImageBuffer.cpp has broken color table code
Summary: WebCore::ImageBuffer.cpp has broken color table code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Stephen Chenney
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-05 13:15 PST by Stephen Chenney
Modified: 2012-06-08 13:16 PDT (History)
10 users (show)

See Also:


Attachments
Patch (20.78 KB, patch)
2012-06-06 13:28 PDT, Stephen Chenney
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (487.43 KB, application/zip)
2012-06-06 23:07 PDT, WebKit Review Bot
no flags Details
Patch (23.38 KB, patch)
2012-06-07 08:53 PDT, Stephen Chenney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephen Chenney 2012-03-05 13:15:15 PST
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.
Comment 1 Tim Horton 2012-03-07 20:00:44 PST
> 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.
Comment 2 Stephen Chenney 2012-06-06 13:28:32 PDT
Created attachment 146097 [details]
Patch
Comment 3 Stephen Chenney 2012-06-06 13:32:34 PDT
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.
Comment 4 Dirk Schulze 2012-06-06 13:37:32 PDT
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/?
Comment 5 WebKit Review Bot 2012-06-06 23:07:41 PDT
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
Comment 6 WebKit Review Bot 2012-06-06 23:07:55 PDT
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
Comment 7 Stephen Chenney 2012-06-07 08:53:02 PDT
Created attachment 146297 [details]
Patch

New patch to run through ews, with the new test. Otherwise unchanged.
Comment 8 Stephen Chenney 2012-06-08 08:01:14 PDT
Committed r119831: <http://trac.webkit.org/changeset/119831>
Comment 9 Stephen Chenney 2012-06-08 08:01:48 PDT
Bug still open pending test rebaselines.
Comment 10 mitz 2012-06-08 10:28:48 PDT
(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>.
Comment 11 Stephen Chenney 2012-06-08 10:37:21 PDT
(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?
Comment 12 Stephen Chenney 2012-06-08 10:52:17 PDT
Committed r119841: <http://trac.webkit.org/changeset/119841>
Comment 13 mitz 2012-06-08 10:57:16 PDT
(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.
Comment 14 Stephen Chenney 2012-06-08 13:16:36 PDT
Committed r119860: <http://trac.webkit.org/changeset/119860>