Summary: | SVG Filter premultiplied color support for getImageDate/putImageData | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dirk Schulze <krit> | ||||||||||
Component: | SVG | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | ariya.hidayat, brettw, dglazkov, eric, jeffschiller, pkasting | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 68469, 26389, 28133 | ||||||||||||
Attachments: |
|
Description
Dirk Schulze
2009-08-02 03:37:12 PDT
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.
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.
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. |