73978 - transparent animating gif problems

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[James Paige]

(using build 2001032904 on Windows 95)

Thanks to yesterdays fix for bug 73328, we can now see that the new imagelib is
not handling transparent animating gifs properly.

To reproduce, visit http://HamsterRepublic.com and look at the Hamster at the
top of the page. note the black rectangle around him. note the way his arm
leaves a trail of after-images behind it.

He displays just fine in ns 4.x and IE and pre-libpr0n mozilla builds.

Comment 1

[James Paige]

adding to libpr0n meta bug depend list...

Comment 2

[ajbu]

*** Bug 74090 has been marked as a duplicate of this bug. ***

Comment 3

[ajbu]

Also seeing this on http://www.hamsterdance.org

Comment 4

[Terri Preston]

I see this behavior on W2k 2001032904
Linux 2001032908 appears to work correctly
(Was checking Mac but have to work on another bug ASAP)

Comment 5

[saari (gone)]

Yeah, I need to add an alpha channel to the compositing frame. Hopefully I'll
get to this somethime this weekend.

Comment 6

[Jubal Kessler]

FYI, I am seeing the glitches in transparent animating gifs as well. Linux build
20010404. Looking forward to the alpha channel fix! :-)

Comment 7

[Boris Zbarsky [:bzbarsky]]

*** Bug 74876 has been marked as a duplicate of this bug. ***

Comment 8

[Martin Horwath]

*** Bug 74894 has been marked as a duplicate of this bug. ***

Comment 9

[Boris Zbarsky [:bzbarsky]]

*** Bug 75016 has been marked as a duplicate of this bug. ***

Comment 10

[Matthias Versen [:Matti]]

*** Bug 75100 has been marked as a duplicate of this bug. ***

Comment 11

[Boris Zbarsky [:bzbarsky]]

*** Bug 75322 has been marked as a duplicate of this bug. ***

Comment 12

[R.K.Aa.]

*** Bug 73342 has been marked as a duplicate of this bug. ***

Comment 13

[R.K.Aa.]

pavlov filed bug 73342 with this comment:
"animations don't look right because DrawToImage wasn't implemented."
It was filed for sun/solaris bug, and is also seen under linux.
Changing OS/platform to all.

Comment 14

[R.K.Aa.]

*** Bug 75553 has been marked as a duplicate of this bug. ***

Comment 15

[Terri Preston]

*** Bug 75763 has been marked as a duplicate of this bug. ***

Comment 16

[R.K.Aa.]

*** Bug 75962 has been marked as a duplicate of this bug. ***

Comment 17

[R.K.Aa.]

*** Bug 75969 has been marked as a duplicate of this bug. ***

Comment 18

[R.K.Aa.]

At least 11 dups and they keep pouring in. mostfreq keyw.

Comment 19

[Cesar Eduardo Barros]

*** Bug 76037 has been marked as a duplicate of this bug. ***

Comment 20

[Ken Snider]

Attachment: Simplified Testcase

Comment 21

[R.K.Aa.]

*** Bug 76115 has been marked as a duplicate of this bug. ***

Comment 22

[Bradley Baetz]

*** Bug 76270 has been marked as a duplicate of this bug. ***

Comment 23

[saari (gone)]

pulling together a patch ontop of an updated tree... (updating to tonights tip
hosed my working tree beyond repair)

Comment 24

[Tim Powell]

*** Bug 76666 has been marked as a duplicate of this bug. ***

Comment 25

[tor]

*** Bug 76753 has been marked as a duplicate of this bug. ***

Comment 26

[Peter Trudelle]

changing from critical to 'would like', not a blocker but would take a fix in
the next 2 days.

Comment 27

[Brendan Eich [:brendan]]

saari: prognosis?  We'd like not to release this regression.

/be

Comment 28

[saari (gone)]

Working on focus regressions first, but this is largly in my tree. Still needs
clean up and testing on mac and linux. Hopefully I can get a patch posted
tonight or tomorrow.

Comment 29

[saari (gone)]

*** Bug 76750 has been marked as a duplicate of this bug. ***

Comment 30

[Igor Izyumin]

Same thing, on some slashdot.org banner ads, linux 2001042106.  Saw this in
pretty much all linux builds since libpr0n.

Comment 31

[Doron Rosenberg (IBM)]

*** Bug 77092 has been marked as a duplicate of this bug. ***

Comment 32

[saari (gone)]

Attachment: gfx2 patches for animations

Comment 33

[saari (gone)]

Attachment: modules/libpr0n patches for animations

Comment 34

[saari (gone)]

Attachment: gfx patch for animations (windows only)

Comment 35

[Stuart Parmenter]

i will review this later today (monday).

Comment 36

[Terri Preston]

*** Bug 46995 has been marked as a duplicate of this bug. ***

Comment 37

[saari (gone)]

Attachment: updated patch to imgContainer.cpp to fix masking problem on mac and linux

Comment 38

[Bradley Baetz]

*** Bug 77255 has been marked as a duplicate of this bug. ***

Comment 39

[Dan Rosen]

Attachment: combined patch, on saari's behalf. also replaces XP_PC with XP_WIN.

Comment 40

[Dan Rosen]

saari: you should be able to apply this patch with the usual patch-fu without
problems. i'll try to get online tonight, if you need r= after fixing the linux
alpha problem...

Comment 41

[Terri Preston]

*** Bug 77290 has been marked as a duplicate of this bug. ***

Comment 42

[saari (gone)]

Attachment: patch for gtk only, doesn't fix all problems, just the vast majority. Good enough for now

Comment 43

[Giovanni Gatti]

Check my page also http://www.fuelcat.it thank you

Comment 44

[Dan Rosen]

woohoo! r=dr on the union of the patches from 4/23 (20:09) and 4/24 (2:49).

note: you probably want to hunt through the rest of libpr0n at some point to
replace XP_PC with XP_WIN...

Comment 45

[David Hyatt]

[s]r=hyatt

Comment 46

[Stuart Parmenter]

r=pavlov with the things i pointed out to saari

Comment 47

[saari (gone)]

Attachment: updated w. pav's changes. need a=

Comment 48

[tor]

Some comments:

  * nsImageWin::CleanUpDIB() simply returns now.  does this mean we leak
      the mHBitmap?

  * win32 gfx code should be reviewed by a win32 person (dcone, mcclusk?)

  * file a bug for splitting imgContainer

  * large change - would like to see it thrashed for a couple days before
       going into the 0.9 branch

Comment 49

[saari (gone)]

tor, thanks for looking at this

>nsImageWin::CleanUpDIB() simply returns now.  does this mean we leak
> the mHBitmap?

Good catch, that shouldn't have been in there. New patch coming.

> win32 gfx code should be reviewed by a win32 person (dcone, mcclusk?)

okay, I've CC'd them

> file a bug for splitting imgContainer

done. bug 77497

> large change - would like to see it thrashed for a couple days before
> going into the 0.9 branch

Okay, then someone else will have to land it. I'm going on vacation, but should 
still be within contact if you need me.

Comment 50

[saari (gone)]

Attachment: update for windows drawing code, fixed the leak

Comment 51

[timeless]

+      if (nsnull != imgWin->mAlphaBits) {
+        if ( 1==imgWin->mAlphaDepth) {
+               MONOBITMAPINFO  bmi(imgWin->mAlphaWidth, imgWin->mAlphaHeight);
are those tabs?

+        if (decoder->mGIFStruct->is_transparent) {
+          gfx_color transColor = 0;
+            transColor |= cmap[decoder->mGIFStruct->tpixel].red;
why the indentation?

Comment 52

[Roland Mainz]

Xlib-toolkit for Unix needs to be updated, too...

Comment 53

[Stuart Parmenter]

ok, saari is on vacation until monday.  I'm going to be in and out.  So either 
I check this in in a few hours when someone to a= wakes up or it probably won't 
go in until early next month.

Comment 54

[Chris Petersen]

This problem is also occuring on Adobe's site too (www.adobe.com.) The image ( 
"Foundation Imaging") is near the center of page.

Comment 55

[/\/\arcio Galli]

*** Bug 77468 has been marked as a duplicate of this bug. ***

Comment 56

[Joseph Elwell]

Attachment: new patch, fixes the merge conflict with nsGIFDecoder2.cpp

Comment 57

[Joseph Elwell]

Since the owners are on vacation I checked in an updated version to the trunk
(fixes merge conflict) with Asa's permission. Mozilla.org would like to get this
fix into the 0.9 branch, so the more time this can bake on the trunk the better.

Comment 58

[Terri Preston]

*** Bug 74283 has been marked as a duplicate of this bug. ***

Comment 59

[Terri Preston]

Well, the fix looks great on Mac build 2001042605 and windows build 2001042604
but animations not animating on Linux build 2001042608, changing os to Linux

Comment 60

[ajbu]

I just checked all duplicates on this bug on build 2001042604 Windows 2000.
attachment 12153 [details] (http://bugzilla.mozilla.org/showattachment.cgi?
attach_id=12153) on bug 46995 still seems to have problems, left part of the 
counter renders invisible every cycle. 
Seems to be a different bug and should be reopened?

Comment 61

[saari (gone)]

Terri, I saw this running on linux myself on 2 different machines. Are you 
sure you had the changes?

Yes, if you find specific examples that don't look right, open new bugs on 
me.

Comment 62

[Terri Preston]

saari, mayben didn't get in the build today?  I will try first thing tomorrow
with tomorrow's build

Comment 63

[timeless]

Attachment: simplify background logic.

Comment 64

[Asa Dotzler [:asa]]

The two testcases, 
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=30899
and
http://HamsterRepublic.com
work absolutely fine for me with today's linux 042606 mozilla builds.

Comment 65

[Asa Dotzler [:asa]]

like ajbanck, I tested all of the duplicates and found only that same one still
failing.  It is very likely a different, much older (and XP) bug.  I've reopened
it and I'm resolving this as Fixed again.  

Comment 66

[Stuart Parmenter]

timeless: that patch is pretty useless.

Comment 67

[Stuart Parmenter]

*** Bug 77824 has been marked as a duplicate of this bug. ***

Comment 68

[Terri Preston]

Verified fixed in linux build 2001042706, great job saari!

Comment 69

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

This has been checked into the 0.9 branch.

I have some questions remaining, which I'd like to get on the record here:

 - where is the Win32 review that tor asked for?
 - where is the r or sr for the last two versions of the patch (given, 
   especially, that the first r and sr missed the Win32 leak)?
 - did anyone run this patch under Purify?  given our recent experiences with
   libpr0n and memory management, it would seem prudent.

Comment 70

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

Reopening to get Win32 review.

Comment 71

[Jacek Piskozub]

Was the fix checked into the trunk as well? If so it has created a new bug.

Today Win32 trunk installer build (2001-04-27-09) shows the transparent animated
GIFs with no trails but the backgroung is black, not transparent as expected. 

This has already been filed as bug 77914.

Comment 72

[Jacek Piskozub]

I retract my remark on creating a new bug, after checking with Win32 build
2001042604. That build had animated transpared GIFs completely fixed. The black
backgroung (bug 77914) came later.

Comment 73

[Stuart Parmenter]

shaver: I reviewed the last patch and the win32 code.  I think is it pretty
silly to leave a bug that has been checked in as open.  If you want someone else
to look at the win32 code, please file a new bug of "look at win32 code in ..."

as for the background not being right, i've seen this the first time through the
animation loop.. every time after the first seems to get the right transparent
background.  I know saari is aware of this... although new bugs assigned to him
are probably ok.

Comment 74

[sujay]

Is this fixed in the trunk? doesn't appear to be...I just downloaded
5/2 trunk build and I still get the black box around the GIF.

Comment 75

[sujay]

oops ignore last comment....that issue is 77914.

Comment 76

[Terri Preston]

This appears to be fixed, although a few of the duplicates are still messed up,
I will re-open those but I'm verifying fixed on W2k build 2001050204, mac build
2001050208 and linux build 2001050108 (Please don't re-open this bug for the
black background, that is bug 77914)

Comment 77

[Stuart Parmenter]

*** Bug 74359 has been marked as a duplicate of this bug. ***