Bug 27933

Created attachment 34037 [details] premultiplied color support for getImageData/putImageData premultiplied color support for getImageData/putImageData. Still not sure if a simple boolean in getImageData/putImageData for premultiplied or not is enough.

Comment on attachment 34037 [details] premultiplied color support for getImageData/putImageData +== Rolled over to ChangeLog-2009-06-16 ==SVG Filter premultiplied color support for getImageDate/putImageData ?? is "NotPreMultiplied" the normal terminolgy? "unmultiplied"? Google seems to suggest that "straight" is the opposite of PreMultiplied I'm not sure that would be more clear here though. Unmultiplied might be clearer than NotPreMultiplied. I'm not a graphics expert though. No argument names: PassRefPtr<ImageData> getNotPreMultipliedImageData(const IntRect& rect) const; 73 PassRefPtr<ImageData> getPreMultipliedImageData(const IntRect& rect) const; they don't add anything. Likewise here: ImageData* source Seems we should add a "colorFrom..." for whatever type of pixel this is: destRows[basex] = (*pixel & 0x00FF0000) >> 16; 171 destRows[basex + 1] = (*pixel & 0x0000FF00) >> 8; 172 destRows[basex + 2] = (*pixel & 0x0000FF00); 173 destRows[basex + 3] = (*pixel & 0xFF000000) >> 24; Then we just create the Color in an if and the pixel assignment is shared. Same here: *pixel = srcRows[basex + 3] << 24 | srcRows[basex] << 16 | srcRows[basex + 1] << 8 | srcRows[basex + 2]; Better to use inlines to share more code. They also are are more self-documenting. Seems the toImage() result could be in a local: if (multiplied == NotPreMultiplied) 134 image = data.m_pixmap.toImage().convertToFormat(QImage::Format_ARGB32); 135 else 136 image = data.m_pixmap.toImage().convertToFormat(QImage::Format_ARGB32_Premultiplied) How do we test this?

(In reply to comment #2) > is "NotPreMultiplied" the normal terminolgy? "unmultiplied"? Google seems to > suggest that "straight" is the opposite of PreMultiplied I'm not sure that > would be more clear here though. Unmultiplied might be clearer than > NotPreMultiplied. I'm not a graphics expert though. The SVG specification call it not premultiplied, but I also found straight and unmultiplied. There is no consistent term. I'll use "Unmultiplied" and "Premultiplied" for the next patch.

Created attachment 34365 [details] premultiplied color support for getImageData/putImageData Corrected patch. Testing is a bit difficult since only SVG Filters make use of premultiplied colors, as far as I know (at least for geImageData/putImageData). SVG Filters are neither activated nor does a effect exist, that makes already use of premultiplied colors.

landed in r47099.

Created attachment 34651 [details] premultiplied color support for Skia This is a untested suggestion for Skia.

Comment on attachment 34365 [details] premultiplied color support for getImageData/putImageData clearing review flag.

I think this would need a chromium person to try this locally. How would they test if this worked?

(In reply to comment #8) > I think this would need a chromium person to try this locally. How would they > test if this worked? Hm... a posibility is to apply the last patch of https://bugs.webkit.org/show_bug.cgi?id=28133 replace getImageData with getPremultipliedImageData() and putImageData with putPremultipliedImageData(), build with --filter enabled and look at http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-filters-blend-01-b.html (ignore the first line, it's a bug in the filter implementation). Sorry, I know that this is a bit complex just for testing, but it was a preperation for SVG filters.

Comment on attachment 34651 [details] premultiplied color support for Skia Seems this should be re-posted when it's easier to test.

Landed a build fix for Skia in r47126 (r47099 did not compile).

Comment on attachment 34651 [details] premultiplied color support for Skia > Index: WebCore/platform/graphics/skia/SkiaUtils.cpp > =================================================================== > --- WebCore/platform/graphics/skia/SkiaUtils.cpp (revision 47098) > +++ WebCore/platform/graphics/skia/SkiaUtils.cpp (working copy) > @@ -80,7 +80,7 @@ static U8CPU InvScaleByte(U8CPU componen > return (component * scale + 0x8000) >> 16; > } > > -SkColor SkPMColorToColor(SkPMColor pm) > +SkColor SkUMColorToColor(SkPMColor pm) > { > if (0 == pm) > return 0; > @@ -94,6 +94,17 @@ SkColor SkPMColorToColor(SkPMColor pm) > InvScaleByte(SkGetPackedB32(pm), scale)); > } > > +SkColor SkPMColorToColor(SkPMColor pm) > +{ > + if (0 == pm) > + return 0; > + > + return SkColorSetARGB(SkGetPackedA32(pm), > + SkGetPackedR32(pm), > + SkGetPackedG32(pm), > + SkGetPackedB32(pm)); > +} It seems like you have the name of this function backwards. UMColor->Color should just be re-ordering the bytes, PMColor->Color needs scaling. Also, having SKUMColorToColor take a PMColor argument is a bit confusing. I actually think writing it out like this is easier to follow, even though it looks longer: unsigned char* destPixel = &destRow[x * 4]; if (multiplied == Unmultiplied) { SkColor color = SkPMColorToColor(srcRow[x]); destPixel[0] = SkColorGetR(color); destPixel[1] = SkColorGetG(color); destPixel[2] = SkColorGetB(color); destPixel[3] = SkColorGetA(color); } else { // Input and output are both pre-multiplied, we just need to re-arrange the bytes from the bitmap format to RGBA. destPixel[0] = SkGetPackedR32(srcRow[x]); destPixel[1] = SkGetPackedG32(srcRow[x]); destPixel[2] = SkGetPackedB32(srcRow[x]); destPixel[3] = SkGetPackedA32(srcRow[x]); } When we want pre-multiplied case, we just need to re-arrange the output. Having the extra function in there (especially one that takes the "wrong" type) makes this harder to see. Brett

Created attachment 35113 [details] premultiplied color support for Skia changed the patch according to brett's comment.

Comment on attachment 35113 [details] premultiplied color support for Skia Given no further commentary from Brett, this looks sane to me. Dirk is a committer, so cq-. He can change it to cq+ if he'd prefer the bot to commit this for him.

Looks good to me, sorry if you were waiting for me.

Comment on attachment 35113 [details] premultiplied color support for Skia Rejecting patch 35113 from commit-queue. This patch will require manual commit. WebKitTools/Scripts/build-webkit failed with exit code 1

Comment on attachment 35113 [details] premultiplied color support for Skia build-webkit was broken for a moment. Not sure why the bots weren't showing it.

Comment on attachment 35113 [details] premultiplied color support for Skia Clearing flags on attachment: 35113 Committed r47991: <http://trac.webkit.org/changeset/47991>

All reviewed patches have been landed. Closing bug.

Broke chromium build. Needs SkColorPriv.h include. Fix coming up.

Build fix committed as http://trac.webkit.org/changeset/47995.

Argh. There's no such value -- "Unpremultiplied". Should we just roll out? Ping.

Another fix committed http://trac.webkit.org/changeset/47996.