Created attachment 70415 [details] Test case for WebGL bug with indexed color PNG files A problem with handling textures occurs when using PNG with indexed colors. While true color PNG images work well, one gets significantly too dark results when using a PNG with indexed colors. The PNG with indexed colors is a result of running optiPNG as an attempt to reduce the file size. Both images look identically when opening in WebKit without the WebGL context. I've attached a simple test case which is taken from http://learningwebgl.com/blog/?p=507 only with modified textures. The default case is using the 24bit true color PNG file. By replacing a string from 24 to IDX, the texture with the indexed colors will be used.
Reproduced on my SnowLeopard. I'll have a look. Thanks for reporting this and the nice test.
It's confirmed indexed PNG files are not dealt with in the current code path.
After this patch https://bugs.webkit.org/show_bug.cgi?id=47974, chrome switched to use webkit decoder, and indexed PNG files are working now. It's still failing for Safari.
In this case it might be advantageous for Safari to switch to use the WebKit image decoders too.
The indexed PNG file renders just fine when used in an <img> tag. Why is WebGL different?
(In reply to comment #5) > The indexed PNG file renders just fine when used in an <img> tag. Why is WebGL different? WebGL decodes the images by itself because we want control over whether to apply meta settings to the RGBA data (like gamma correction, colorspace profile, etc). Only we haven't handled the case where colors are indexed.
Created attachment 80533 [details] Patch
*** Bug 53269 has been marked as a duplicate of this bug. ***
Comment on attachment 80533 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80533&action=review Looks good. r=me with minor nits > Source/WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp:132 > + // seem to work unless it's premultiplied. There's no need to explain other ways this could be done, unless this is a FIXME in which case it should be marked as such. Otherwise a shorter comment (like the last sentence) would suffice > Source/WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp:140 > + CGContextDrawImage(bitmapContext.get(), CGRectMake(0, 0, width, height), cgImage); The style guidelines don't say anything about it, but I think a blank here and after the 'return false' above would be more clear. > LayoutTests/fast/canvas/webgl/gl-teximage.html:206 > + for (var ii = 0; ii < 2; ++ii) { extra space here > LayoutTests/fast/canvas/webgl/gl-teximage.html:208 > + if (ii == 0) { wrong indenting > LayoutTests/fast/canvas/webgl/gl-teximage.html:232 > + } No braces on either clause here > LayoutTests/fast/canvas/webgl/gl-teximage.html:238 > + assertMsg(ge128Count[jj] > 256 * 0.45, It would be more clear to put the 0.45 into a const > LayoutTests/fast/canvas/webgl/gl-teximage.html:389 > + // The image should be red. I think a line between the comment and the line above would be more clear
(In reply to comment #9) > (From update of attachment 80533 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=80533&action=review > > Looks good. r=me with minor nits > > > Source/WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp:132 > > + // seem to work unless it's premultiplied. > > There's no need to explain other ways this could be done, unless this is a FIXME in which case it should be marked as such. Otherwise a shorter comment (like the last sentence) would suffice I've changed this to a FIXME because we can't properly implement the WebGL flag for not premultiplying the alpha channel with indexed PNGs without it. > > Source/WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp:140 > > + CGContextDrawImage(bitmapContext.get(), CGRectMake(0, 0, width, height), cgImage); > > The style guidelines don't say anything about it, but I think a blank here and after the 'return false' above would be more clear. OK. > > LayoutTests/fast/canvas/webgl/gl-teximage.html:206 > > + for (var ii = 0; ii < 2; ++ii) { > > extra space here Fixed tabs here and upstream. > > LayoutTests/fast/canvas/webgl/gl-teximage.html:208 > > + if (ii == 0) { > > wrong indenting Fixed tabs here and upstream. > > LayoutTests/fast/canvas/webgl/gl-teximage.html:232 > > + } > > No braces on either clause here We aren't following WebKit style in the WebGL tests in the Khronos repo, so I've left this as is. > > LayoutTests/fast/canvas/webgl/gl-teximage.html:238 > > + assertMsg(ge128Count[jj] > 256 * 0.45, > > It would be more clear to put the 0.45 into a const I didn't add this part of the test (it's pulled verbatim from the Khronos repo) so I've left this as is. > > LayoutTests/fast/canvas/webgl/gl-teximage.html:389 > > + // The image should be red. > > I think a line between the comment and the line above would be more clear Added.
Committed r77108: <http://trac.webkit.org/changeset/77108>
http://trac.webkit.org/changeset/77108 might have broken SnowLeopard Intel Release (Tests)