519589 - twitter reply transparent image is not so transparent on trunk

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect, P1)

Description

[Robert Sayre]

I'll attach a screenshot

Comment 1

[Robert Sayre]

Attachment: screenshot

it's been like this for a while. was wondering if anyone else would file it...

Comment 2

[Matthias Versen [:Matti]]

Did you already tried to disable color correction ?

Comment 3

[Robert Sayre]

see it on 1.9.2 as well.

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

And on Win7.  This is going to be crappy to have in our beta, and could mask other problems with visual stuff, so if there's an easy fix then let's try to get it in?

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Attachment: Here's the image that's broken

My guess is the breakage is from one of the security fixes to our gif decoder.

Comment 6

[Johnathan Nightingale [:johnath]]

And that security fix has been backported, right?  This is broken on most recent nightlies for:

1.9.0:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.16pre) Gecko/2009101504 GranParadiso/3.0.16pre

1.9.1:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.5pre) Gecko/20091015 Shiretoko/3.5.5pre

It is NOT broken on 3.5.3:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.3) Gecko/20090824 Firefox/3.5.3

If we've actually broken GIF transparency - this blocks basically everything.

Comment 7

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Flagging, for the sake of thoroughness.

Comment 8

[Robert Kaiser]

There are definitely transparent GIFs that actually work, so this might not affect all of them.

Comment 9

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

It affects at least one high-profile case, so restoring the blocking flag.

Comment 10

[Reed Loden [:reed]]

Bug 514776 talks about GIF transparency, so that might be a possible place to start looking.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Bisection shows that it was likely Bug 514776 (http://hg.mozilla.org/mozilla-central/rev/ff3496b1f6c7) that caused this.

Comment 12

[Johnathan Nightingale [:johnath]]

As jeff says, this broke on mozilla-central between Sep 11 and Sep 12
nightlies, giving us this range:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4d4979399774&tochange=ff3496b1f6c7

And one obvious candidate.

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Wild guessing that it might be caused by

1.41 +        mGIFStruct.is_transparent = PR_FALSE;

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Looking at the code more thoroughly it looks like the problem might be that the
transparent pixel color is not in the color map. Previously we let gifs like
this have transparency, now we don't?

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Joe's going to come up with a patch, I'll get a reftest together for this instance.

Comment 17

[Alfred Kayser]

Mid-air collision: I was just writing:
Patch coming up real soon.
Trick is to not reset the is_transparent flag, 
but to make sure that invalid tpixel is never poking behind the colormap... ;-)

Comment 18

[Joe Drew (not getting mail)]

In that case, I'll let Alfred come up with the patch.

Comment 19

[Alfred Kayser]

Attachment: Undo the realDepth/transparent part

This patch revert the part that caused this problem,
but keeps the part that fixes the OOB issue itself.

Comment 20

[Joe Drew (not getting mail)]

Attachment: fix v2

This patch is logically equivalent to Alfred's, but it's a little more explanatory (more comments) and it handles the case (that I'm not sure is possible) where it's not possible to represent the transparent pixel in a colormap.

I'm in the middle of running this through some valgrind tests.

Comment 21

[Joe Drew (not getting mail)]

Alfred, one thing I'm a bit concerned about with my patch is the memset in the local-colormap case; first, why are we using 3 << depth, and second, is it logically correct to do that as I have without a separate realDepth variable?

Comment 22

[Jeff Muizelaar [:jrmuizel]]

Attachment: here's a twitterless version of the same problem

Comment 23

[Joe Drew (not getting mail)]

Things I would like a test for:
 - Regular transparency (transparent pixel inside the colormap's range)
 - Transparent pixel outside the possible bounds for colormaps, if possible (>= 256)
 - 1 bit of color, transparent pixel at 255/256

Questions I would like an answer for:
 - What should we do to colors that would be outside the range of the colormap except for the transparent pixel enlarging the colormap? Leave them mapped to 0?

Comment 24

[Alfred Kayser]

Stick with v1 because it reverts to something that is known to work.

Answers to the questions:
1. tpixel can never be outside the possible range (0..255) as it is a one byte value. 
2. tpixel is only set once for the whole animated image (so for all frames!)
3. every frame can have its own local colormap, which can be smaller than tpixel.
4. The source colormap is 3 bytes per pixel (therefor the 3<<depth).
5. When tpixel is larger than the current (local) map, the old code extended the colormap to include the tpixel as well.
6. The colormap is cleared to zero, so any pixels outside the source range automatically become transparent as well.

Comment 25

[Alfred Kayser]

Attachment: This image shows more clearly the problem...

Comment 26

[Joe Drew (not getting mail)]

Alfred, thanks for this feedback. I agree that we should commit your change rather than mine - I'm going to reroll my build with your patch and check for some of the bugs we've fixed in the interim.

For posterity: GIFs store their colormaps as 3 bytes per pixel, but we operate on 4 bytes per pixel. We copy the data directly into the relevant arrays, then ConvertColormap in nsGIFDecoder2 expands these packed pixels (in-place) into unpacked 4-byte ARGB pixels as required by Cairo.

Comment 27

[Joe Drew (not getting mail)]

Comment on attachment 406500 [details] [diff] [review]
Undo the realDepth/transparent part

Jeff's working on some tests for the GIF decoder, but I've verified that this is good. Let's get this in.

Comment 28

[Joe Drew (not getting mail)]

Attachment: 1.9.1/1.9.0 branch patch

This is a preliminary 1.9.1/1.9.0 patch. I need to run it through a build (my 1.9.1 tree was out of date) and tryserver, but it looks right on inspection.

Comment 29

[Joe Drew (not getting mail)]

Pushed Alfred's patch to 1.9.2 and the 1.9.2 beta 1 relbranch:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/6ff84d80826d
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7b46a9704eb8

Comment 30

[Bobby Holley (:bholley)]

Comment on attachment 406558 [details] [diff] [review]
1.9.1/1.9.0 branch patch

It sure would be nice if there were more comments about gif decoder magic. Since this is just a branch patch, I won't insist on it though. r=bholley

Comment 31

[Samuel Sidler (old account; do not CC)]

Comment on attachment 406558 [details] [diff] [review]
1.9.1/1.9.0 branch patch

Approved for 1.9.1.4, 1.9.1.5, 1.9.0.15, and 1.9.0.16. a=ss for release-drivers.

Please land on... mozilla-1.9.1, GECKO1914_20091006_RELBRANCH, CVS HEAD, and GECKO190_20091006_RELBRANCH as soon as possible.

Comment 32

[Joe Drew (not getting mail)]

Checked in to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/9605f7ad200c

Checked in to 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b21a4de3706b

Checked in to 1.9.1.4 relbranch: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/49342a1d9d93

Checked in to 1.9.0:
Checking in nsGIFDecoder2.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp,v  <--  nsGIFDecoder2.cpp
new revision: 1.107; previous revision: 1.106
done

and 1.9.0.15 relbranch:
Checking in nsGIFDecoder2.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp,v  <--  nsGIFDecoder2.cpp
new revision: 1.106.2.1; previous revision: 1.106
done

Comment 33

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add the begining of a test suite for GIF

I've sent this off to tryserver.

Joe, these match the three tests you wanted:
 - Regular transparency: covered by in-colormap-trans.gif
 - Transparent pixel outside the possible bounds for colormaps: this is the case that the twitter image has and is covered by the out-of-colormap-trans.gif
 - 1 bit of color, transparent pixel at 255/256: the transparent pixel field is only 8 bits wide so the maximum it can be is 255 and this situation is covered by: 1bit-255-trans.gif

Comment 34

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0 on twitter with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.15) Gecko/2009101601 Firefox/3.0.15 (.NET CLR 3.5.30729).

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on 1.9.1 on all platforms and on 1.9.2 and trunk with the following builds on OS X and Windows:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20091016 Minefield/3.7a1pre ID:20091016035239

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b2pre) Gecko/20091016 Namoroka/3.6b2pre ID:20091016042840

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.4) Gecko/20091016 Firefox/3.5.4 ID:20091016081620

Comment 36

[Jeff Muizelaar [:jrmuizel]]

I've pushed the tests:
http://hg.mozilla.org/mozilla-central/rev/1005021e95bd

Comment 37

[Justin Dolske [:Dolske]]

zomg, GIF tests finally?! \o/ <3 \o/ <3

Comment 38

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0.16 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.16pre) Gecko/2009111921 GranParadiso/3.0.16pre (.NET CLR 3.5.30729).