455169 - PNG rendering problem with color management

Status: VERIFIED FIXED

(Core :: Graphics: Color Management, defect, P3)

Description

[tor]

Attachment: screenshot of incorrect rendering

If the test URL is loaded with gfx.color_management.mode=1 (all images corrected), the right portion has random colors.  Without color correction the image loads fine.

Comment 1

[tor]

Attachment: problem file, for reference

Comment 2

[tor]

pngcheck information:
  chunk IHDR at offset 0x0000c, length 13
    96 x 96 image, 32-bit RGB+alpha, interlaced
  chunk tEXt at offset 0x00025, length 25, keyword: Software
  chunk IDAT at offset 0x0004a, length 15153
    zlib:  deflated, 32K window, maximum compression
  chunk IEND at offset 0x03b87, length 0

Maybe a problem with how we handle interlaced files?

Comment 3

[Bill Gianopoulos [:WG9s]]

I see similar issue with setting gfx.color_management.mode=1 with the blue background on the selected item in the Download or add-ons manager.

Comment 4

[Glenn Randers-Pehrson]

WFM: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080912031847 Minefield/3.1b1pre Creative ZENcast v1.02.11

Comment 5

[Glenn Randers-Pehrson]

Attachment: noninterlaced version of problem image

Comment 6

[Bill Gianopoulos [:WG9s]]

(In reply to comment #4)
> WFM: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre)
> Gecko/20080912031847 Minefield/3.1b1pre Creative ZENcast v1.02.11

I only see this issue with Linux builds.

Comment 7

[Glenn Randers-Pehrson]

Attachment: Make sure inType is defined in the PNG decoder

There seems to be a path through the PNG decoder that uses inType but does not define it.  That might give the symptom you saw.  This patch ensures that inType is defined even if there is no profile built by the gAMA, sRGB, or iCCP chunk.

Comment 8

[tor]

Still seeing the rendering error with your patch.

Comment 9

[Glenn Randers-Pehrson]

Comment on attachment 338478 [details] [diff] [review]
Make sure inType is defined in the PNG decoder

Patch does not do the job so I'm marking it obsolete.

Comment 10

[Glenn Randers-Pehrson]

(In reply to comment #0)
> 
> If the test URL is loaded with gfx.color_management.mode=1 (all images
> corrected), the right portion has random colors.  Without color correction the
> image loads fine.

They don't look random.  They are the same in each row.  It looks as though
the png_combine_row() function is working on the 3-channel width instead of
the 4-channel width, and leaving the right 1/4 of each row unchanged.

(In reply to comment #2)
> Maybe a problem with how we handle interlaced files?

Has anyone verified that the noninterlaced version of the image does not
exhibit the problem?

Comment 11

[Bill Gianopoulos [:WG9s]]

(In reply to comment #10)
> Has anyone verified that the noninterlaced version of the image does not
> exhibit the problem?

I do not see the issue with the non-interlaced version of the image that you attached.

Comment 12

[Bill Gianopoulos [:WG9s]]

Also the left eye (right as you face it) eye in the acid2 test display incorrectly if gfx.color_management.mode=1.

Comment 14

[Daniel Holbert [:dholbert]]

FWIW, here are a few more places I've run into this bug recently:
* The Ubuntu logo used at the top of many pages on Ubuntu's issue tracker:
   Image:      https://launchpadlibrarian.net/6100649/ubuntu_emblem.png
   Context:    http://launchpad.net/ubuntu

* The Logos for NoScript and FlashGot addons, used on their web pages
   Image:      http://software.informaction.com/data/noscript/logo.png
   Context:    http://noscript.net/   (also http://maone.net/ )
   Image:      http://software.informaction.com/data/flashgot/logo.png
   Context:    http://flashgot.net/   (also http://maone.net/ )

Comment 15

[Daniel Holbert [:dholbert]]

One more broken PNG, posted in today's Platform meeting notes:
  http://people.mozilla.org/~beltzner/images/2008-11-18-blocker-report.png
(sorry to bugspam -- this PNG just hits close to home, so I thought it merited mentioning :))

Comment 16

[Daniel Holbert [:dholbert]]

See also bug 465593, which is about similar corruption in the Add-ons manager.

Comment 17

[Glenn Randers-Pehrson]

I am not seeing the problem with Firefox 3.0.4 on Linux, so it seems to be some kind of regression in 3.1.

Comment 18

[Glenn Randers-Pehrson]

Attachment: Copy alpha channel over to Cairo array

This may fix the problem but I'm not able to test it.

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: Copy alpha channel over to Cairo array (compiles, but doesn't fix this bug)

I just tried the patch on a Linux mozilla-central build, with some typo fixes (original version didn't compile), and it doesn't fix this bug.

(I'm attaching the patch with the compile fixes for reference -- I had to remove a stray ")" in the first chunk, and also add 2 close-brackets, to close the "if (channels == 2)" clause and the "  if (decoder->mCMSLine)" clause.)

Comment 20

[Glenn Randers-Pehrson]

What a mysterious bug.  It seems that the bug manifests
itself for
  1. Linux only.
  2. Interlaced PNG files only.
  3. Transparent PNG files only.
  4. Only PNG files without iCCP or cHRM
  5. gfx.color_management.mode=1
  6. Both indexed-color and alpha-channel PNGs

Comment 21

[Daniel Holbert [:dholbert]]

FWIW, this bug also appears for the header of http://quotes.burntelectrons.org
Image URL: http://quotes.burntelectrons.org/res/themes/mash/img/title.png

Comment 22

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Sigh.  I see this in a nightly, I do not see it on a local build I just did.  My build was a debug build though, will try an opt build in a second.  If I still can't reproduce it, then it's time to start looking at compiler config or something :(

Comment 23

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Ok, it shows up in opt, but not in debug.  Hmm.

Comment 24

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

No, I'm lying, it shows up in debug also.  Ignore me.

Comment 25

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

bholley: dunno, if you saw my comments on irc, this looks like NullXFORM brokenness.

it only memmoves

  Size * T_BYTES(p -> InputFormat) * T_CHANNELS(p -> InputFormat)

bytes; if the input is RGBx, T_CHANNELS is 3, so it doesn't ever copy the last 1/4 of the input to the output.

This is probably never being hit on non-linux because on win32 and OSX, we always get a profile from system, and chances are it's not going to be sRGB.

I have to run, should be a simple fix, just not sure what the right T_FOO is to get the actual number of bytes/bits/whatever from a format is.

Comment 26

[Bobby Holley (:bholley)]

vlad - thanks for looking into that. I'll put together a patch in the next day or two.

Comment 27

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: use T_EXTRA as well as T_CHANNELS

Just grabbed this since I had it open when I got back this morning -- looks like it's just T_EXTRA.  So T_BYTES * (T_CHANNELS + T_EXTRA) is the right number of bytes per row.

Comment 28

[Joe Drew (not getting mail)]

Comment on attachment 351211 [details] [diff] [review]
use T_EXTRA as well as T_CHANNELS

Are we pushing these changes upstream?

Comment 29

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Don't think so, bobby knows more what's happening there -- I think upstream isn't interested in changes to current lcms since they're working on lcms2 or something similar.

Comment 30

[Bobby Holley (:bholley)]

Comment on attachment 351211 [details] [diff] [review]
use T_EXTRA as well as T_CHANNELS

Looks right - my initial reaction was that the macro would be something like T_EXTRA, so this should be right. Vlad, have you tested if this fixes the test cases? If not, we can throw up a tryserver build to test before we land.

Comment 31

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yep, was testing/debugging on linux.  Fixes all the cases here (and doesn't break anything else).

Comment 32

[Bobby Holley (:bholley)]

(In reply to comment #28)
> Are we pushing these changes upstream?

Eh, I'm not actually that sure on the status on this stuff. I've pinged the maintainer every now and then when we fix a bug that I think they'd care about, but I haven't heard back (last I heard was in regards the precaching patch, which he mentioned using but I'm not sure if that ever went anywhere).

Once things settle down a bit with lcms I'll probably run an hg blame in the cms directory and send him all commits that have affected lcms, and let him figure out which ones he wants (not sure how things play with lcms2).

Comment 33

[Bobby Holley (:bholley)]

Comment on attachment 351211 [details] [diff] [review]
use T_EXTRA as well as T_CHANNELS

Great - I'm guessing we want this for 1.9.1, so I'm flagging for approval.

This patch fixes a problem where we weren't considering the full variety of pixel formats when doing color transforms. The chance for regression is pretty low, and I think that if we're saying we ship with full cms support (just disabled by default) we should support it and not have it break in really ugly ways.

Comment 34

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 351211 [details] [diff] [review]
use T_EXTRA as well as T_CHANNELS

'tis a blocker, so doesn't need approval :)

Comment 35

[Bobby Holley (:bholley)]

pushed in 6eb4f7167d23.

Letting it bake. All goes well and I'll push it to 191 tomorrow.

Comment 37

[Daniel Holbert [:dholbert]]

Resolving as fixed, since it's fixed on trunk (mozilla-central).

Fix confirmed in today's nightly:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20081207 Minefield/3.2a1pre ID:20081207021514

Comment 38

[Bobby Holley (:bholley)]

don't think it got resolved as fixed last time - flagging for 191 and resolving

Comment 39

[Bobby Holley (:bholley)]

pushed to releases/mozilla-1.9.1 in 9b896a3006fe

Comment 40

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US;
rv:1.9.1b3pre) Gecko/20090122 Shiretoko/3.1b3pre 
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre)
Gecko/20090122 Minefield/3.2a1pre