twitter reply transparent image is not so transparent on trunk

VERIFIED FIXED in mozilla1.9.3a1

Status

()

Core
ImageLib
P1
major
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: Robert Sayre, Assigned: Joe Drew (not getting mail))

Tracking

(5 keywords)

Trunk
mozilla1.9.3a1
regression, verified1.9.0.15, verified1.9.0.16, verified1.9.1, verified1.9.2
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 +
blocking1.9.0.15 +
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta1-fixed, blocking1.9.1 .4+, status1.9.1 .4-fixed)

Details

Attachments

(7 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
I'll attach a screenshot
(Reporter)

Comment 1

8 years ago
Created attachment 403655 [details]
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 ?
(Reporter)

Comment 3

8 years ago
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+
Keywords: regression, regressionwindow-wanted
OS: Mac OS X → All
Priority: -- → P1
Version: unspecified → Trunk
Created attachment 406453 [details]
Here's the image that's broken

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+

Comment 8

8 years ago
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+
status1.9.1: --- → ?
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.
Blocks: 514776
Keywords: regressionwindow-wanted
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?

Updated

8 years ago
Duplicate of this bug: 522155
Joe's going to come up with a patch, I'll get a reftest together for this instance.

Comment 17

8 years ago
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... ;-)
(Assignee)

Comment 18

8 years ago
In that case, I'll let Alfred come up with the patch.
Assignee: nobody → alfredkayser

Updated

8 years ago
Blocks: 522155

Comment 19

8 years ago
Created attachment 406500 [details] [diff] [review]
Undo the realDepth/transparent part

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

Comment 20

8 years ago
Created attachment 406502 [details] [diff] [review]
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.
Assignee: alfredkayser → joe
Attachment #406500 - Attachment is obsolete: true
Attachment #406502 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #406502 - Flags: review? → review?(alfredkayser)
(Assignee)

Comment 21

8 years ago
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?
Created attachment 406511 [details]
here's a twitterless version of the same problem
(Assignee)

Comment 23

8 years ago
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

8 years ago
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

8 years ago
Created attachment 406529 [details]
This image shows more clearly the problem...
(Assignee)

Comment 26

8 years ago
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.
(Assignee)

Updated

8 years ago
Attachment #406500 - Attachment is obsolete: false
Attachment #406500 - Flags: review?(joe)
(Assignee)

Updated

8 years ago
Attachment #406502 - Attachment is obsolete: true
Attachment #406502 - Flags: review?(alfredkayser)
(Assignee)

Comment 27

8 years ago
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+
(Assignee)

Comment 28

8 years ago
Created attachment 406558 [details] [diff] [review]
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.
Attachment #406558 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #406558 - Flags: review? → review?(alfredkayser)
(Assignee)

Comment 29

8 years ago
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
(Assignee)

Updated

8 years ago
status1.9.2: --- → beta1-fixed
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+
(Assignee)

Comment 32

8 years ago
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
Last Resolved: 8 years ago
status1.9.1: ? → .4-fixed
Keywords: fixed1.9.0.15
Resolution: --- → FIXED
Created attachment 406622 [details] [diff] [review]
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
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).
Keywords: fixed1.9.0.15 → verified1.9.0.15
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
Keywords: verified1.9.1, verified1.9.2
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.3a1
Severity: blocker → major
I've pushed the tests:
http://hg.mozilla.org/mozilla-central/rev/1005021e95bd
zomg, GIF tests finally?! \o/ <3 \o/ <3
Flags: in-testsuite? → in-testsuite+
Attachment #406622 - Flags: review?(joe)
Keywords: fixed1.9.0.16
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).
Keywords: fixed1.9.0.16 → verified1.9.0.16
You need to log in before you can comment on or make changes to this bug.