111037 - browser won't show Mac PNG

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Stephen Compall]

tested in 2001112008

The url is a PNG created with Adobe Photoshop 5.0le on Mac OS 9.0.4. I uploaded
a series of PNGs to illustrate a bug in the location bar, but I found that they
would not appear. I opened them again in APS, fine; N4, fine; what's wrong, Moz?

Comment 1

[Stephen Compall]

I just tested this in Netscape 6.1-linux, and the image is terribly mangled.
Perhaps taken out of tree?

Comment 2

[Greg K.]

Confirmed using Mac/2001112011 (0.9.6). Nothing is displayed at all when this image is 
accessed either via the URL provided or by saving it as a local PNG file.

Mac IE partially displays the image, but in black and white.

The applications Photoshop, GraphicConverter, and QuickTime Player all display the 
image properly.

I have no idea what's going on here. I even resaved a copy from GraphicConverter and it 
still wouldn't display in Mozilla. FWIW, it's a RGB, 8-BPC image.

Comment 3

[Stephen Compall]

Linux testers: this probably won't show up if you did --use-system-png=. The
testcase does not work on any platform, but it works in IE5-Win98. That's a
definite sign it should work properly, when MS supports open PNG better than
Mozilla...

Changing plat/os to all-maybe somebody should consider renaming this bug. Is
there just something wrong with non-standard file formats or something, anybody?

Comment 4

[Stuart Parmenter]

giving to nivedita

Comment 5

[Nivedita (gone)]

This image is displayed properly in Netscape 4.7, but appears corrupted in 
IE6.0 , mozilla on windows 98  and windows NT platforms. The image viewed in  
NS4.7 is in sync to the image when viewed in photoshop.

Comment 6

[Spiros Ioannou]

With build 20020202 on Win2k it displays correctly (it uses the quicktime plugin).
Maby that's because I've quicktime installed?

Comment 7

[Nivedita (gone)]

Attachment: patch file fixing the incorrect gamma value

png_get_uint_32 for gamma is returning a value( 2^32 -1 ) in this case, which
when type casted to png_fixed_point, results in an overflow, hence gamma
results in having a  value of -1. Having an incorrect gamma results in garbled
display of the colors.	

Hence added a check if overflow occurs set the gamma to the default value
0.45455.

We can run in overflow error for the following parameters as well, which use
png_get_uint_32 and are later are type casted to png_fixed_point.

a) length ->	   chunk_length
b) width  ->	   width of the image
c) height ->	   height of the image
d) int_x_white & int_y_white ->  Chromaticity of the white
e) int_x_red & int_y_red     ->  Chromaticity of the red
f) int_x_green & int_y_green ->  Chromaticity of the green
g) int_x_blue & int_y_blue   ->  Chromaticity of the blue
h) idat_size

Comment 8

[Nivedita (gone)]

Attachment: patch file fixing the incorrect gamma value

png_get_uint_32 for gamma is returning a value( 2^32 -1 ) in this case, which
when type casted to png_fixed_point, results in an overflow, hence gamma
results in having a  value of -1. Having an incorrect gamma results in garbled
display of the colors.	

Hence added a check if overflow occurs set the gamma to the default value
0.45455.

We could run into overflow error for the following parameters as well, which
use png_get_uint_32 and are later are type casted to png_fixed_point.

a) length ->	   chunk_length
b) width  ->	   width of the image
c) height ->	   height of the image
d) int_x_white & int_y_white ->  Chromaticity of the white
e) int_x_red & int_y_red     ->  Chromaticity of the red
f) int_x_green & int_y_green ->  Chromaticity of the green
g) int_x_blue & int_y_blue   ->  Chromaticity of the blue
h) idat_size		     ->  size of the IDAT 

Should we return an png_error for the above if they have invalid value?

Comment 9

[Stuart Parmenter]

nivedita:  I'd prefer to not change the libpng source if possible.  Is it
possible to fix this in the decoder code?

Comment 10

[Nivedita (gone)]

Attachment: patch file incorporating comments given by Stuart

settting the invalid gamma to a default gamma value in the nsPNGDecoder.

Comment 11

[Stuart Parmenter]

Comment on attachment 68837 [details] [diff] [review]
patch file incorporating comments given by Stuart

r=pavlov

Comment 12

[tor]

Comment on attachment 68837 [details] [diff] [review]
patch file incorporating comments given by Stuart

sr=tor

Comment 13

[Stuart Parmenter]

checked in fix

Comment 14

[Greg Roelofs]

Um...guys?  png_get_gAMA() puts the value in aGamma.  What the heck are you
poking around in info_ptr->gamma for?  (Tor, you should have caught this!)

Greg

Comment 15

[tor]

Reopen to clean up fix.

Comment 16

[tor]

Attachment: don't look inside info_ptr

Comment 17

[Stuart Parmenter]

Comment on attachment 70746 [details] [diff] [review]
don't look inside info_ptr

r=pavlov

Comment 18

[Christopher Blizzard (:blizzard)]

Comment on attachment 70746 [details] [diff] [review]
don't look inside info_ptr

sr=blizzard

Comment 19

[Glenn Randers-Pehrson]

> a) length ->	   chunk_length
> b) width  ->	   width of the image
> c) height ->	   height of the image
> d) int_x_white & int_y_white ->  Chromaticity of the white
> e) int_x_red & int_y_red     ->  Chromaticity of the red
> f) int_x_green & int_y_green ->  Chromaticity of the green
> g) int_x_blue & int_y_blue   ->  Chromaticity of the blue
> h) idat_size		     ->  size of the IDAT 
>
> Should we return an png_error for the above if they have invalid value?

png_error should be issued for bad width, height, or idat_size.  For
bad chromaticity, a png_warning should be issued and the cHRM chunk
abandoned.

I'll look into modifying libpng to handle these problems.

Glenn

Comment 20

[Asa Dotzler [:asa]]

a=asa (on behalf of drivers) for checkin to 0.9.9

Comment 21

[tor]

Checked in.

Comment 22

[Asa Dotzler [:asa]]

I don't see where this got checked in. I'm probably overlooking it but thought
I'd doublecheck. tor, has this really landed?

Comment 23

[tor]

Yes, it's in:

revision 1.28
date: 2002/02/22 01:29:55;  author: tor%cs.brown.edu;  state: Exp;  lines: +2 -1
Bug 111037 - don't look inside png info_ptr.  r=pavlov, sr=blizzard, a=asa.

Comment 24

[Glenn Randers-Pehrson]

>We could run into overflow error for the following parameters as well, which
>use png_get_uint_32 and are later are type casted to png_fixed_point.
>
>a) length ->	   chunk_length
>b) width  ->	   width of the image
>c) height ->	   height of the image
>d) int_x_white & int_y_white ->  Chromaticity of the white
>e) int_x_red & int_y_red     ->  Chromaticity of the red
>f) int_x_green & int_y_green ->  Chromaticity of the green
>g) int_x_blue & int_y_blue   ->  Chromaticity of the blue
>h) idat_size		     ->  size of the IDAT 

Libpng-1.2.2 will check for all of these and issue warnings or errors
as appropriate.  I'd appreciated someone trying libpng-1.2.2beta2, which
is available from libpng.sf.net, with mozilla.

Glenn

Comment 25

[tor]

libpng1.2.2beta2 seems to work fine with mozilla.  The version of libpng in the
mozilla tree has a few local changes (see bug 125845).  Should any of these be
folded into the official tree?

Comment 26

[Glenn Randers-Pehrson]

Png_zalloc, png_zfree, and png_handle_as_unknown will be modified as
suggested in libpng-1.2.2.  I hope you are not using png_info_init,
png_read_destroy, or png_write_destroy because they are deprecated in
applications.

Glenn

Comment 27

[tor]

None of the latter three functions are used by nsPNGDecoder.

Comment 28

[Glenn Randers-Pehrson]

Your patch that adds PNGAPI to png_info_init, png_read_destroy, and
png_write_destroy shouldn't be necessary then.  It shouldn't have been
needed in version 1.0.9 either.