----------------------------------------------------------- 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 > >
