I'll attach a screenshot

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

Did you already tried to disable color correction ?

see it on 1.9.2 as well.

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?

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

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.

Flagging, for the sake of thoroughness.

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

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

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

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

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.

Wild guessing that it might be caused by 1.41 + mGIFStruct.is_transparent = PR_FALSE;

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?

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

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... ;-)

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

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

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.

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?

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?

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.

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 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.

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.

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 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 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.

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

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

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).

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

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

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

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).