403423 - PNG files with sRGB chunk displayed improperly when color_management is enabled

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Glenn Randers-Pehrson]

Attachment: panel from pmt.sf.net/gamma_test that is displayed improperly

The attached image, which is a grayscale PNG with only IHDR, sRGB, IDAT, and IEND chunks, is displayed improperly when gfx.color_management is enabled.  See bug #403239, comments 14-18.  The bug is probably in libpr0n/decoders/png/nsPNGDecoder.cpp at line 409 which fails to test for the presence of a valid sRGB chunk.

Comment 1

[Glenn Randers-Pehrson]

Attachment: Fixes bug in sRGB handling in nsPNGDecoder.cpp

This patch and the patch in bug #403239 will bit-rot each other.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

I think your diff is horked.

Comment 3

[Glenn Randers-Pehrson]

Comment on attachment 288246 [details] [diff] [review]
Fixes bug in sRGB handling in nsPNGDecoder.cpp

Horked, indeed.  No idea how that happened.

Comment 4

[Glenn Randers-Pehrson]

Attachment: Fixes bug in sRGB handling in nsPNGDecoder.cpp dehorked (v1)

The other patch was horked by my fat finger hitting "]" and <ENTER> together instead of just <ENTER>.

Comment 5

[Glenn Randers-Pehrson]

Attachment: Fixes bug in sRGB handling in nsPNGDecoder.cpp (v2)

Resolves merge conflict with checkin from bug #403239

Comment 6

[Glenn Randers-Pehrson]

Comment on attachment 288497 [details] [diff] [review]
Fixes bug in sRGB handling in nsPNGDecoder.cpp (v2)

The problem seems not to be the lack of a gAMA chunk but the handling of a grayscale PNG image with an sRGB chunk.

See http://www.simplesystems.org/users/glennrp/srgb.

Comment 7

[Glenn Randers-Pehrson]

Attachment: Fixes bug in sRGB handling in nsPNGDecoder.cpp (v3)

Changes grayscale to RGBA when sRGB is present and color_management is enabled.

Comment 8

[Reed Loden [:reed]]

Are you going to request review on your patch?

Comment 9

[Glenn Randers-Pehrson]

After I hear from someone that the patch works.  Can't try it myself because I haven't got access to a platform with a sufficiently recent version of libpango installed, nor to the TriServer.

Comment 10

[Chris Lawson (gone)]

Hardware/OS -> All/All, since I see this in recent trunk Camino and Minefield on OS X PPC.

I'll try to give this a test as soon as I finish pulling an updated tree.

Comment 11

[Chris Lawson (gone)]

I can confirm the patch in comment 7 fixes this bug in a homemade build from this afternoon. Go ahead and target someone for review, as I have absolutely no idea whether or not this is the right approach. I just know that it worked. :)

Comment 12

[tor]

Comment on attachment 290035 [details] [diff] [review]
Fixes bug in sRGB handling in nsPNGDecoder.cpp (v3)

I think that line should be inside the "if profile {}" block", but otherwise it seems correct.

Comment 13

[Glenn Randers-Pehrson]

I agree, inside the "if (profile) {}" block would be better.  New patch coming shortly.

Comment 14

[Glenn Randers-Pehrson]

Attachment: Fixes bug in sRGB handling in nsPNGDecoder.cpp (v4)

Moved inside "if (profile) {}"

Comment 15

[Glenn Randers-Pehrson]

tor: Note that several other places the code has

    if (color_type == PNG_COLOR_TYPE_GRAY ||
        color_type == PNG_COLOR_TYPE_GRAY_ALPHA)
      png_set_gray_to_rgb(png_ptr);

or similar.

I omitted the test from the patch because it's redundant with tests
inside libpng.  I recommend removing all of them to save a few bytes
of footprint.  I'll post a new patch that does that if you like.

Comment 16

[Glenn Randers-Pehrson]

Attachment: Fixes bug in sRGB handling in nsPNGDecoder.cpp (v5)

tor: The "v5" patch also removes the redundant tests -- take your pick of v4 or v5.

Comment 18

[Reed Loden [:reed]]

Checking in modules/libpr0n/decoders/png/nsPNGDecoder.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/png/nsPNGDecoder.cpp,v  <--  nsPNGDecoder.cpp
new revision: 1.76; previous revision: 1.75
done