WebGL shows PNG Textures with indexed colors too dark

• Component: WebGL
• Status: RESOLVED FIXED

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.

[Zhenyao Mo]

Reproduced on my SnowLeopard.  I'll have a look.  Thanks for reporting this and the nice test.

[Zhenyao Mo]

It's confirmed indexed PNG files are not dealt with in the current code path.

[Zhenyao Mo]

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]

In this case it might be advantageous for Safari to switch to use the WebKit image decoders too.

[Simon Fraser (smfr)]

The indexed PNG file renders just fine when used in an <img> tag. Why is WebGL different?

[Zhenyao Mo]

(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]

Created attachment 80533 [details]
Patch

[Kenneth Russell]

*** Bug 53269 has been marked as a duplicate of this bug. ***

[Chris Marrin]

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]

(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]

Committed r77108: <http://trac.webkit.org/changeset/77108>

[WebKit Review Bot]

http://trac.webkit.org/changeset/77108 might have broken SnowLeopard Intel Release (Tests)