Bug 21575 - Cairo's ImageBuffer::getImageData() does not handle alpha=0 case correctly
Summary: Cairo's ImageBuffer::getImageData() does not handle alpha=0 case correctly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-10-13 12:26 PDT by Eric Roman
Modified: 2009-02-03 11:51 PST (History)
2 users (show)

See Also:


Attachments
imageData (2.05 KB, patch)
2008-10-13 15:46 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
canvas-getImageData.html output (7.78 KB, text/plain)
2008-10-29 03:25 PDT, Dirk Schulze
no flags Details
color's given to GraphicsContext (2.26 KB, text/plain)
2008-10-29 15:55 PDT, Dirk Schulze
no flags Details
imageData respects pre-multiplied colors (2.87 KB, patch)
2009-01-31 07:19 PST, Dirk Schulze
zecke: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Roman 2008-10-13 12:26:29 PDT
Cairo buffer data is stored in ARGB, and the following loop in ImageBufferCairo.cpp converts it to RGBA:

            if (unsigned int alpha = (*pixel & 0xff000000) >> 24) {
                destRows[basex] = (*pixel & 0x00ff0000) >> 16;
                destRows[basex + 1] = (*pixel & 0x0000ff00) >> 8;
                destRows[basex + 2] = (*pixel & 0x000000ff);
                destRows[basex + 3] = alpha;
            } else
                reinterpret_cast<uint32_t*>(destRows + basex)[0] = pixel[0];


The else clause looks suspicious, as it is assigning a ARGB value to where RGBA is expected. So instead of RGB0 you get 0RGB.
Comment 1 Eric Roman 2008-10-13 12:29:16 PDT
There is also concern of endianness with how color components are extracted from uint32.
Comment 2 Dirk Schulze 2008-10-13 12:31:39 PDT
Need to respect premultiplied colors:

                destRows[basex] = ((*pixel & 0x00ff0000) >> 16) * 255 / alpha;
                destRows[basex + 1] = ((*pixel & 0x0000ff00) >> 8) * 255 / alpha;
                destRows[basex + 2] = ((*pixel & 0x000000ff)) * 255 / alpha;
                destRows[basex + 3] = alpha;

(the same on putImageData)
Comment 3 Dirk Schulze 2008-10-13 15:46:52 PDT
Created attachment 24331 [details]
imageData

This corrects the bugs and adds support for premultiplied colors.

Todo: make it endian-save
Comment 4 Dirk Schulze 2008-10-29 03:25:29 PDT
Created attachment 24743 [details]
canvas-getImageData.html output

This is the output of canvas-getImageData.html after applying the patch. Like you can see many tests fail, because alpha channel is 1 "degree" lower than it should be. And perhaps thats why the other colors are wrong too. Because we use the alpha to get premultiplied colors.
Comment 5 Dirk Schulze 2008-10-29 15:55:19 PDT
Created attachment 24758 [details]
color's given to GraphicsContext

I checked the color that are given to GraphicsContext. I added the original alpha value on fist column and the second column includes the alpha multiplied with 255.
As you can see (if you compare the values of the first list with the values of the second list): cairo gives the right values back (for the alpha channel).

That means first: There is something wrong in Color.cpp
and second: it shouldn't work on Cg either.
Comment 6 Dirk Schulze 2009-01-31 07:19:33 PST
Created attachment 27216 [details]
imageData respects pre-multiplied colors

This fixes the pre-multiplied colors bug, as well as the alpha=0 case. It reads the colors, given to the context, correctly but fails on the canvas-getImageData test.
But the fact, that the context gets the wrong color and getImageData reads exactly this color-settings, tells that getImageData works correctly. (see https://bugs.webkit.org/show_bug.cgi?id=22150)
You can see the results of the test on the second attachment and the alpha of the colors, given to the context and therefor to the ImageBuffer, on the third attachement. You'll see, that the alpha channel is identical. (Haven't added all colors, but the result is the same)

This patch passes canvas-putImageData.

I talked about platform dependencies on IRC (#cairo). And they assure me, that this code is already endian safe.
Comment 7 Holger Freyther 2009-02-02 06:08:32 PST
Comment on attachment 27216 [details]
imageData respects pre-multiplied colors

Looks good. The +254 needs some thinking but I see where you got this code from (ImageBufferCG.cpp)
Comment 8 Dirk Schulze 2009-02-03 11:51:55 PST
landed in r40542