Closed Bug 519589 Opened 10 years ago Closed 10 years ago

twitter reply transparent image is not so transparent on trunk

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- beta1-fixed
blocking1.9.1 --- .4+
status1.9.1 --- .4-fixed

People

(Reporter: sayrer, Assigned: joe)

References

Details

(5 keywords)

Attachments

(7 files, 1 obsolete file)

I'll attach a screenshot
Attached image 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.
Flags: blocking1.9.2?
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?
Flags: blocking1.9.2? → blocking1.9.2+
OS: Mac OS X → All
Priority: -- → P1
Version: unspecified → Trunk
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.
blocking1.9.1: --- → .4+
Flags: blocking1.9.0.15+
There are definitely transparent GIFs that actually work, so this might not affect all of them.
blocking1.9.1: .4+ → ---
It affects at least one high-profile case, so restoring the blocking flag.
blocking1.9.1: --- → .4+
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;
Component: Graphics → ImageLib
Flags: in-testsuite?
QA Contact: thebes → imagelib
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?
Duplicate of this bug: 522155
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.
Assignee: nobody → alfredkayser
Blocks: 522155
This patch revert the part that caused this problem,
but keeps the part that fixes the OOB issue itself.
Attached patch fix v2 (obsolete) — Splinter Review
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.
Assignee: alfredkayser → joe
Attachment #406500 - Attachment is obsolete: true
Attachment #406502 - Flags: review?
Attachment #406502 - Flags: review? → review?(alfredkayser)
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.
Attachment #406500 - Attachment is obsolete: false
Attachment #406500 - Flags: review?(joe)
Attachment #406502 - Attachment is obsolete: true
Attachment #406502 - Flags: review?(alfredkayser)
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.
Attachment #406500 - Flags: review?(joe) → review+
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.
Attachment #406558 - Flags: review?
Attachment #406558 - Flags: review? → review?(alfredkayser)
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
Attachment #406558 - Flags: review?(alfredkayser) → review+
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.
Attachment #406558 - Flags: approval1.9.1.5+
Attachment #406558 - Flags: approval1.9.1.4+
Attachment #406558 - Flags: approval1.9.0.16+
Attachment #406558 - Flags: approval1.9.0.15+
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
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: fixed1.9.0.15
Resolution: --- → FIXED
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
Attachment #406622 - Flags: review?(joe)
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
Severity: normal → blocker
Status: RESOLVED → VERIFIED
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.3a1
Severity: blocker → major
zomg, GIF tests finally?! \o/ <3 \o/ <3
Flags: in-testsuite? → in-testsuite+
Attachment #406622 - Flags: review?(joe)
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).
You need to log in before you can comment on or make changes to this bug.