In ImageBufferQt we do some conversion from RGB to BGR. While the code doesn't say why, the reason it is needed is because Qt uses ARGB and JavaScript canvas expect RGBA, and storing ABGR as a 32bit vector happens to produce a RGBA byte-order on a little endian machine. This is ofcourse wrong on a bit endian machine. The conversion should be fixed, and the code and its reason made clearer.
Created attachment 183997 [details] Patch
Created attachment 183999 [details] Patch Fix copyRGBAToPremultipliedQRgb path
Comment on attachment 183999 [details] Patch Removing review. The code ihas a few identified mistakes that only trigger with canvas code.
Comment on attachment 183999 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183999&action=review > Source/WebCore/ChangeLog:10 > + Replace the conversion to methods that make it clearer what is going on. > + The routines are also optimized compared to the existing by avoiding going > + over the WebCore Color class, and unswitching branches from the loop. Tests? > Source/WebCore/platform/graphics/qt/ImageBufferQt.cpp:277 > + const QRgb* srcData = (const QRgb*)image.constScanLine(originy); reinterpret_cast please.
(In reply to comment #4) > (From update of attachment 183999 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=183999&action=review > > > Source/WebCore/ChangeLog:10 > > + Replace the conversion to methods that make it clearer what is going on. > > + The routines are also optimized compared to the existing by avoiding going > > + over the WebCore Color class, and unswitching branches from the loop. > > Tests? > It is tested by existing tests in fast/canvas for instance. I would keep a watch on http://build.webkit.sed.hu/results/ARMv7 Linux Qt5 Release (Test)/r140405 (7739)/fast/canvas/canvas-putImageData-pretty-diff.html for instance.
(In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 183999 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=183999&action=review > > > > > Source/WebCore/ChangeLog:10 > > > + Replace the conversion to methods that make it clearer what is going on. > > > + The routines are also optimized compared to the existing by avoiding going > > > + over the WebCore Color class, and unswitching branches from the loop. > > > > Tests? > > > It is tested by existing tests in fast/canvas for instance. I would keep a watch on http://build.webkit.sed.hu/results/ARMv7 Linux Qt5 Release (Test)/r140405 (7739)/fast/canvas/canvas-putImageData-pretty-diff.html for instance. OK, would be good to mention it in the changelog.
Created attachment 184005 [details] Patch Fixed problem with non-zero x offsets
Created attachment 184221 [details] Patch Use reinterpret_cast, and other cleanup
Created attachment 186386 [details] Patch
Comment on attachment 186386 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186386&action=review LGTM beside 2 nitpicks. > Source/WebCore/platform/graphics/qt/ImageBufferQt.cpp:201 > + to[0] = from.red(); > + to[1] = from.green(); > + to[2] = from.blue(); > + to[3] = from.alpha(); Somebody in the future might come here and rewrite this using bit shifting if he feels like it. A comment would be great to explain why this is the only right way. Maybe quote the spec at http://www.w3.org/TR/2011/WD-2dcontext-20110525/#pixel-manipulation ? > Source/WebCore/platform/graphics/qt/ImageBufferQt.cpp:253 > + const unsigned* srcRow = reinterpret_cast<const unsigned*>(image.constScanLine(originy + y)) + originx; This could use RGBA32 to keep the type abstraction of colorFromPremultipliedARGB and some Color methods. > Source/WebCore/platform/graphics/qt/ImageBufferQt.cpp:316 > + unsigned* destRow = destData + y * numColumns; ditto
Committed r141886: <http://trac.webkit.org/changeset/141886>