[webkit-reviews] review denied: [Bug 27933] SVG Filter premultiplied color support for getImageDate/putImageData : [Attachment 34037] premultiplied color support for getImageData/putImageData

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Dirk Schulze <krit at webkit.org>'s
request for review:
Bug 27933: SVG Filter premultiplied color support for getImageDate/putImageData
https://bugs.webkit.org/show_bug.cgi?id=27933

Attachment 34037: premultiplied color support for getImageData/putImageData
https://bugs.webkit.org/attachment.cgi?id=34037&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
+== 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?