WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Patch
(13.38 KB, patch)
2011-01-28 18:08 PST
,
Kenneth Russell
cmarrin
: review+
kbr
: commit-queue-
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 80533
[details]
Patch
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
Committed
r77108
: <
http://trac.webkit.org/changeset/77108
>
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.
Top of Page
Format For Printing
XML
Clone This Bug