Bug 3438 – incorrect display of transparent 1x1 PNGs

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Benjamin Frere]

Hello, I noticed that the transparent (alpha 24 bit) PNG use on this website:

http://www.monauto.fr/fiche_auto.php?id=3

use for the alternance lines in the tables, displays in a strange gray colour on Tiger's Sarfari 2 instead of 
blue (it works as it should on Panther's Safari 1.x, though).

This is the complete url of the png:
http://www.monauto.fr/images/oddbg.png

It works perfectly well in FireFox under Tiger, so it looks like a WebCore bug.
Of course it works very well on Windows machines, but who cares?

But there's also something strange. The PNG file was generated by Photoshop. If we use another soft 
like Gimp or similar, the bug disapears.

Best regards,

Benjamin Frere

[Mathieu Massebœuf]

You may have the explanation here : http://hsivonen.iki.fi/png-gamma/

I'm curently redesigning my corporate website (http://sports.fr/) and it's a pain to work with PNGs - first I 
need to remove the color profile so they work fine for most browsers (working with photoshop cs) - and 
then it doesn't work well on safari pre 10.4 as it seems to apply a default color profile to the PNGs (which 
is not correct and darken the colors).

In your case I'd say that the previous Safari is not correct in its PNG handling, and the new one is.

NB : PNG 8bit have the same issue.

[Stéphane Rieppi]

I'd like to have other opinions about that. _Every_ other browser I've tried handles it correctly. If Safari really does something better than all those other browsers (which I doubt), maybe it's a case of trying too much.

[Alexey Proskuryakov]

Well, I don't think this issue is about PNG gamma. Only 1x1 images are affected - resaving in Photoshop as any other size fixes the problem, and resaving as 1x1 again makes it appear back (so, the workaround is to use larger images).

This appears to be fixed in current nightly builds. I'm not closing as WORKSFORME yet because:
1) someone more familiar with image decoding might want to take a look at this bug;
2) looks like the problem remains for images loaded using a direct URL, not as a part of a document (though it's hard to tell, even using Pixie :-))) );
3) might be worth investigating in order to file a Radar.

[Alexey Proskuryakov]

Created attachment 7072 [details]
test case

Besides (1)-(3), landing a test case could be useful.

[Darin Adler]

We do have an optimization specific to 1x1 images that was new to Tiger. Sounds like that was going wrong.

[Darin Adler]

Comment on attachment 7072 [details]
test case

I think the description "Should be uniform color" is a little terse, but seems like a good test.

[Darin Adler]

(In reply to comment #3)
> 2) looks like the problem remains for images loaded using a direct URL, not as
> a part of a document (though it's hard to tell, even using Pixie :-))) );

The standalone image case is still using the old code -- that's a temporary situation so we don't have to worry about it for much longer, so I think there's no need to worry about this.

> 3) might be worth investigating in order to file a Radar.

I'm pretty sure there's no need. The 1x1 code is inside WebKit.

[Alexey Proskuryakov]

The 1x1 case optimization was restored today (r15529), and so was this bug. Reopening.

Furthermore, my test case was apparently not quite right - I'm now seeing that the colors didn't quite match even with a yesterday's nightly (presumably because of PNG color correction). Should be easy to fix by removing colorspace-related chunks.

[Alexey Proskuryakov]

Comment on attachment 7072 [details]
test case

clearing the review flag

[Alexey Proskuryakov]

Created attachment 9583 [details]
fixed test case

This bug turns out to be about premultiplied vs. non-premultiplied alpha! The m_solidColor member of Image is supposed to be non-premul, but in Image::checkForSolidColor() it is initialized from a premul bitmap context.

Dividing out by the alpha (if it's non-zero) in checkForSolidColor() fixes the test case. A more straightforward approach (and more accurate, if the image format is non-premul) would be to use a non-premul bitmap context, however when I change the kCGImageAlphaPremultipliedLast to kCGImageAlphaLast in the CGBitmapContextCreate I get a NULL context -- I have no idea why. Next thing I'm going to try is use 8-bit unsigned instead of float components and see if that changes anything.

Created attachment 9764 [details]
Use non-premul, 8-bit integer bitmap context

Patch is missing a test and a change log, but I'd like to get feedback on switching to integers (is there any reason to use floats when you convert to int anyway?) and on whether I should use another type for the 8-bit quantity instead of unsigned char.

[Dave Hyatt]

Comment on attachment 9764 [details]
Use non-premul, 8-bit integer bitmap context

This is totally fine.

r=me

[Darin Adler]

Comment on attachment 9764 [details]
Use non-premul, 8-bit integer bitmap context

We don't need kCGBitmapByteOrder32Host any more since the components are now single bytes -- byte order does not matter in that case.

Should probably omit sizeof(unsigned char) -- that's defined by the language to be 1 and I think it's clear enough to just pass 8.

[Darin Adler]

(In reply to comment #12)
> whether I should use another type for the 8-bit quantity instead of unsigned char

I thnk unsigned char is fine for this.

Created attachment 9773 [details]
Patch, including change log and ap's test

Also addressed Darin's comments.

[Alexey Proskuryakov]

(In reply to comment #16)
> Patch, including change log and ap's test

The test is supposed to replace fast/replaced/img-1x1.html.

[David Kilzer (:ddkilzer)]

Comment on attachment 9764 [details]
Use non-premul, 8-bit integer bitmap context

Clearing review flag.

[Darin Adler]

Comment on attachment 9773 [details]
Patch, including change log and ap's test

r=me

Comment on attachment 9773 [details]
Patch, including change log and ap's test

I didn't test the patch correctly. Alexey just did, and it turns out that just like in the float case, you can't create a bitmap context with non-premul alpha http://developer.apple.com/qa/qa2001/qa1037.html .

[Darin Adler]

(In reply to comment #20)
> I didn't test the patch correctly. Alexey just did, and it turns out that just
> like in the float case, you can't create a bitmap context with non-premul alpha
> http://developer.apple.com/qa/qa2001/qa1037.html .
Maybe we can get a pixel out of the bitmap in some way other than rendering it.

Created attachment 9776 [details]
Undo the premultiplication

> Maybe we can get a pixel out of the bitmap in some way other than rendering it.
In case that doesn't work out, you can try to undo the premultiplication. For image formats that are natively premultiplied, this is as accurate as you can get anyway.

Comment on attachment 9776 [details]
Undo the premultiplication

I couldn't find a way to extract color from a CGImage without rendering it, so I'm proposing this fix for now.

[Darin Adler]

Comment on attachment 9776 [details]
Undo the premultiplication

OK. Lets do it this way, then. r=me

[Alexey Proskuryakov]

Committed revision 15758.