Closed Bug 455169 Opened 12 years ago Closed 12 years ago

PNG rendering problem with color management

Categories

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

x86
Linux
defect

Tracking

()

VERIFIED FIXED
mozilla1.9.1b3

People

(Reporter: tor, Assigned: vlad)

References

(Blocks 1 open bug, )

Details

(Keywords: verified1.9.1)

Attachments

(4 files, 3 obsolete files)

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.
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?
Component: ImageLib → GFX
QA Contact: imagelib → general
Component: GFX → ImageLib
QA Contact: general → imagelib
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.
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
Blocks: 455077
(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.
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.
Assignee: nobody → glennrp
Status: NEW → ASSIGNED
Still seeing the rendering error with your patch.
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.
Attachment #338478 - Attachment is obsolete: true
(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?
(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.
Also the left eye (right as you face it) eye in the acid2 test display incorrectly if gfx.color_management.mode=1.
Duplicate of this bug: 464385
Flags: blocking1.9.1?
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/ )
Flags: blocking1.9.1? → blocking1.9.1+
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 :))
See also bug 465593, which is about similar corruption in the Add-ons manager.
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.
This may fix the problem but I'm not able to test it.
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.)
Attachment #349946 - Attachment is obsolete: true
Attachment #349972 - Attachment is obsolete: true
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
Priority: -- → P3
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 :(
Ok, it shows up in opt, but not in debug.  Hmm.
No, I'm lying, it shows up in debug also.  Ignore me.
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.
vlad - thanks for looking into that. I'll put together a patch in the next day or two.
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.
Assignee: glennrp → vladimir
Attachment #351211 - Flags: superreview?(joe)
Attachment #351211 - Flags: review?(bholley)
Attachment #351211 - Flags: superreview?(joe) → superreview+
Comment on attachment 351211 [details] [diff] [review]
use T_EXTRA as well as T_CHANNELS

Are we pushing these changes upstream?
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.
Attachment #351211 - Flags: review+
Attachment #351211 - Flags: review?(bholley) → review+
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.
Yep, was testing/debugging on linux.  Fixes all the cases here (and doesn't break anything else).
(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).
Attachment #351211 - Flags: approval1.9.1?
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 on attachment 351211 [details] [diff] [review]
use T_EXTRA as well as T_CHANNELS

'tis a blocker, so doesn't need approval :)
Attachment #351211 - Flags: approval1.9.1? → approval1.9.1+
pushed in 6eb4f7167d23.

Letting it bake. All goes well and I'll push it to 191 tomorrow.
Component: ImageLib → GFX: Color Management
QA Contact: imagelib → color-management
Duplicate of this bug: 465593
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
don't think it got resolved as fixed last time - flagging for 191 and resolving
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [needs 191 landing]
pushed to releases/mozilla-1.9.1 in 9b896a3006fe
Whiteboard: [needs 191 landing]
Keywords: fixed1.9.1
Target Milestone: --- → mozilla1.9.1b3
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.