Bug 107560 – [Qt] RGB -> BGR is wrong on big endian

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Allan Sandfeld Jensen]

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.

[Allan Sandfeld Jensen]

Created attachment 183997 [details]
Patch

[Allan Sandfeld Jensen]

Created attachment 183999 [details]
Patch

Fix copyRGBAToPremultipliedQRgb path

[Allan Sandfeld Jensen]

Comment on attachment 183999 [details]
Patch

Removing review. The code ihas a few identified mistakes that only trigger with canvas code.

[Noam Rosenthal]

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.

[Allan Sandfeld Jensen]

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

[Noam Rosenthal]

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

[Allan Sandfeld Jensen]

Created attachment 184005 [details]
Patch

Fixed problem with non-zero x offsets

[Allan Sandfeld Jensen]

Created attachment 184221 [details]
Patch

Use reinterpret_cast, and other cleanup

[Allan Sandfeld Jensen]

Created attachment 186386 [details]
Patch

[Jocelyn Turcotte]

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

[Allan Sandfeld Jensen]

Committed r141886: <http://trac.webkit.org/changeset/141886>