Bug 47034 – gl-teximage.html fails on chromium webkit mac bot

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Zhenyao Mo]

Below is the diff (for the 16-bit image test):

-PASS pixel 128, 8 should be within 10 units of 15, 121, 0, 255 was 15, 121, 0, 255
+FAIL pixel 128, 8 should be within 10 units of 15, 121, 0, 255 was 73, 177, 0, 255

I guess there is some difference between Leopard and SnowLeopard image decoding, because on my local SnowLeopard the test runs fine, but the bot is Leopard.

[Zhenyao Mo]

It turns out to be an endian issue.

[Zhenyao Mo]

Created attachment 70914 [details]
patch

Tested on Vangelis's laptop which is a Leopard.

[WebKit Review Bot]

Attachment 70914 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/GraphicsContext3D.cpp:45:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zhenyao Mo]

Created attachment 70921 [details]
revised patch: fixed the style issue

[Zhenyao Mo]

Created attachment 70923 [details]
revised patch: remove some temporary code I added for debugging (forgot to remove them)

[WebKit Review Bot]

Attachment 70914 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4396055

[WebKit Review Bot]

Attachment 70923 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4469051

[Zhenyao Mo]

Created attachment 71050 [details]
revised patch: fixed the skia port compile error

[Kenneth Russell]

Comment on attachment 71050 [details]
revised patch: fixed the skia port compile error

There are two issues with this code as written. One is that it does not properly handle the case where the machine's native byte order is big endian rather than little endian. The second and more significant is that it unnecessarily complicates the code for a relatively rare case of 16-bit PNGs with byte order opposite of the machine's native byte order. The endianness switch does not need to be passed all the way down to the unpacking routine. Instead, another few unpacking routines for 16-bit source values can be added like unpackR16ToRGBA8Swapped, where that would be used if the source byte order was big endian and the native machine's byte order is little endian. A run-time test can be added to packPixels to determine whether the machine is big or little endian. "uint16_t tempValue = 1; if (reinterpret_cast<uint8_t*>(&tempValue)[0] == 1) { /* little endian */ } else { /* big endian */ }"

[Zhenyao Mo]

Created attachment 71091 [details]
revised patch: responding to kbr's review

[Kenneth Russell]

Comment on attachment 71091 [details]
revised patch: responding to kbr's review

View in context: https://bugs.webkit.org/attachment.cgi?id=71091&action=review

This looks better. I note that it looks like there was a preexisting bug in the calls to computeIncrementParameters for the 16-bit-per-pixel formats. It would be worth adding an assert in computeIncrementParameters of the form "ASSERT(!(bytesPerPixel % elementSizeInBytes))".

> WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp:76
> +    bool srcByteOrder16Big;

I don't think the compiler is going to be able to figure out that this is always initialized before use. Please initialize it to false.

[Zhenyao Mo]

Committed r70073: <http://trac.webkit.org/changeset/70073>