Bug 47477 - WebGL shows PNG Textures with indexed colors too dark
Summary: WebGL shows PNG Textures with indexed colors too dark
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.6
: P2 Normal
Assignee: Kenneth Russell
URL:
Keywords:
: 53269 (view as bug list)
Depends on:
Blocks: 53531
  Show dependency treegraph
 
Reported: 2010-10-10 15:15 PDT by q3drbones
Modified: 2011-02-01 14:12 PST (History)
10 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description q3drbones 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.
Comment 1 Zhenyao Mo 2010-10-14 17:36:20 PDT
Reproduced on my SnowLeopard.  I'll have a look.  Thanks for reporting this and the nice test.
Comment 2 Zhenyao Mo 2010-10-20 12:21:36 PDT
It's confirmed indexed PNG files are not dealt with in the current code path.
Comment 3 Zhenyao Mo 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.
Comment 4 Kenneth Russell 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.
Comment 5 Simon Fraser (smfr) 2011-01-27 22:24:05 PST
The indexed PNG file renders just fine when used in an <img> tag. Why is WebGL different?
Comment 6 Zhenyao Mo 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.
Comment 7 Kenneth Russell 2011-01-28 18:08:06 PST
Created attachment 80533 [details]
Patch
Comment 8 Kenneth Russell 2011-01-28 18:09:00 PST
*** Bug 53269 has been marked as a duplicate of this bug. ***
Comment 9 Chris Marrin 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
Comment 10 Kenneth Russell 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.
Comment 11 Kenneth Russell 2011-01-30 21:11:20 PST
Committed r77108: <http://trac.webkit.org/changeset/77108>
Comment 12 WebKit Review Bot 2011-01-31 04:14:37 PST
http://trac.webkit.org/changeset/77108 might have broken SnowLeopard Intel Release (Tests)