RESOLVED FIXED 47477
WebGL shows PNG Textures with indexed colors too dark
https://bugs.webkit.org/show_bug.cgi?id=47477
Summary WebGL shows PNG Textures with indexed colors too dark
q3drbones
Reported 2010-10-10 15:15:32 PDT
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.
Attachments
Test case for WebGL bug with indexed color PNG files (52.71 KB, application/zip)
2010-10-10 15:15 PDT, q3drbones
no flags
Patch (13.38 KB, patch)
2011-01-28 18:08 PST, Kenneth Russell
cmarrin: review+
kbr: commit-queue-
Zhenyao Mo
Comment 1 2010-10-14 17:36:20 PDT
Reproduced on my SnowLeopard. I'll have a look. Thanks for reporting this and the nice test.
Zhenyao Mo
Comment 2 2010-10-20 12:21:36 PDT
It's confirmed indexed PNG files are not dealt with in the current code path.
Zhenyao Mo
Comment 3 2010-11-05 11:00:04 PDT
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.
Kenneth Russell
Comment 4 2010-11-05 11:12:21 PDT
In this case it might be advantageous for Safari to switch to use the WebKit image decoders too.
Simon Fraser (smfr)
Comment 5 2011-01-27 22:24:05 PST
The indexed PNG file renders just fine when used in an <img> tag. Why is WebGL different?
Zhenyao Mo
Comment 6 2011-01-28 08:07:09 PST
(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.
Kenneth Russell
Comment 7 2011-01-28 18:08:06 PST
Kenneth Russell
Comment 8 2011-01-28 18:09:00 PST
*** Bug 53269 has been marked as a duplicate of this bug. ***
Chris Marrin
Comment 9 2011-01-29 06:09:33 PST
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
Kenneth Russell
Comment 10 2011-01-30 21:08:37 PST
(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.
Kenneth Russell
Comment 11 2011-01-30 21:11:20 PST
WebKit Review Bot
Comment 12 2011-01-31 04:14:37 PST
http://trac.webkit.org/changeset/77108 might have broken SnowLeopard Intel Release (Tests)
Note You need to log in before you can comment on or make changes to this bug.