Review Request 109998: kjs: Fix CanvasImageData eating the color value by doing wrong alpha premultiply

[Michael Pyne]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109998/#review31004
-----------------------------------------------------------


At this point you're probably better suited to review KHTML and KJS than many of the rest of the KDE devs, but I would say in general that it is more important that the code be correct than to be optimized, so I would say to "Ship It" based on the problem description, if no one else with KHTML/KJS experience is able to review. I agree that it's very difficult to "un-premultiply" the colors once you have an alpha of 0 introduced. Even without an alpha of 0, there is probably precision errors introduced with repeated changes to the color attributes the way we have it now, which would be a separate bug.

- Michael Pyne


On April 13, 2013, 3:34 p.m., Bernd Buschinski wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109998/
> -----------------------------------------------------------
> 
> (Updated April 13, 2013, 3:34 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> -------
> 
> previous knowledge:
> - the CanvasImageData allows setting the single color red/green/blue/alpha-values for each pixel
> - the default for all pixel is Rgba 0,0,0,0 and must not be changed
> - you can set the color-rgba values in any order, there is no fixed order
> 
> the bug:
> We use QImage::Format_ARGB32_Premultiplied beacuse it faster for many operations.
> As we directly set the red/green/blue/alpha color value via QImage::scanLine,
> we must premultiply it, to keep it in the QImage::Format_ARGB32_Premultiplied format.
> 
> But as CanvasImageData does not specify any order of setting the values or does not know what values are set,
> we might end up doing a wrong premultiply.
> For premultiply we MUST have the correct alpha value, if it is not set it is assumed 0 ( the default),
> but if we premultiply a given (for example) red value with 0, its get zero, the red color value is gone.
> Gone and no way to recover it.
> So for example if we set for each pixel (in this order) red=255,green=0,blue=0,alpha=255,
> we get a black image instead of a red one.
> 
> my solution:
> As I don't want to introduce a "is set" check for the alpha value, I suggest we use ARG32 instead of ARGB32_Premultiplied.
> This allows us to set the color values in any order we want.
> 
> possible problems:
> ARG32 is slower than ARGB32_Premultiplied.
> Yes, but as far as I can see we do not display the image directly, it is "only" used as in
>   QPainter::setCompositionMode(QPainter::CompositionMode_Source);
>   QPainter::drawImage(x,y,ourImage);
> to get painted on another image.
> I hope QPainter uses some sse magic, so the paint is faster than our custom premultiply.
> 
> btw:
> While (afaik) it is possible to re-use that not-displayed ImageData, I only found javascript code 
> creating new ImageData for each animation-frame. In this case ARGB32_Premultiplied would be more performant.
> 
> 
> Diffs
> -----
> 
>   khtml/html/html_canvasimpl.cpp c7fce27 
> 
> Diff: http://git.reviewboard.kde.org/r/109998/diff/
> 
> 
> Testing
> -------
> 
> http://www.w3schools.com/tags/canvas_createimagedata.asp
> shows the problem, as it shows a black image but it should be red.
> 
> 
> Thanks,
> 
> Bernd Buschinski
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20130413/8c079ea3/attachment.htm>