Summary: | gl-teximage.html fails on chromium webkit mac bot | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Zhenyao Mo <zmo> | ||||||||||||
Component: | WebGL | Assignee: | Zhenyao Mo <zmo> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | cmarrin, dglazkov, enne, kbr, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | OS X 10.5 | ||||||||||||||
Bug Depends on: | 46947 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
Zhenyao Mo
2010-10-01 17:51:38 PDT
It turns out to be an endian issue. Created attachment 70914 [details]
patch
Tested on Vangelis's laptop which is a Leopard.
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.
Created attachment 70921 [details]
revised patch: fixed the style issue
Created attachment 70923 [details]
revised patch: remove some temporary code I added for debugging (forgot to remove them)
Attachment 70914 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4396055 Attachment 70923 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4469051 Created attachment 71050 [details]
revised patch: fixed the skia port compile error
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 */ }"
Created attachment 71091 [details]
revised patch: responding to kbr's review
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. Committed r70073: <http://trac.webkit.org/changeset/70073> |