<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/109998/">http://git.reviewboard.kde.org/r/109998/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<p>- Michael</p>
<br />
<p>On April 13th, 2013, 3:34 p.m. UTC, Bernd Buschinski wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kdelibs.</div>
<div>By Bernd Buschinski.</div>
<p style="color: grey;"><i>Updated April 13, 2013, 3:34 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">http://www.w3schools.com/tags/canvas_createimagedata.asp
shows the problem, as it shows a black image but it should be red.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>khtml/html/html_canvasimpl.cpp <span style="color: grey">(c7fce27)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/109998/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>