Closed Bug 85595 Opened 23 years ago Closed 22 years ago

Transparent animated GIFs have white background

Categories

(Core :: Graphics: ImageLib, defect)

defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla1.0.1

People

(Reporter: markushuebner, Assigned: pavlov)

References

()

Details

(Keywords: regression, testcase, topembed, Whiteboard: [driver:brendan] [adt2 RTM] [Need 06/21])

Attachments

(7 files, 11 obsolete files)

4.23 KB, image/gif
Details
9.62 KB, image/gif
Details
124.79 KB, image/gif
Details
8.00 KB, image/gif
Details
99.11 KB, image/gif
Details
912 bytes, patch
pavlov
: review+
Details | Diff | Splinter Review
27.12 KB, patch
pavlov
: review+
Details | Diff | Splinter Review
At
http://www.world-direct.com/central
the transparent animated GIFs have a white background
and don't animate. Just the latest frame is displayed.

On this page there is also bug 79650 causing the image
display problems ... normaly moving over a rectangle
displays the text below and if you move out of it the
text disappears.
When viewing one of the animated GIFs directly via
http://www.world-direct.com/centralimgs/btn/iinfo_1.gif
you can see the animation (the smooth fading of the text).




partial DUP bug 17126
possibly also DUP bug 77914
Has the site gone under a redesign ?
http://www.world-direct.com/centralimgs/btn/iinfo_1.gif is now 404 not found.
no, it's still there ... you just had a typo in your URL.
the correct URL is

http://www.world-direct.com/central/imgs/btn/iinfo_1.gif
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: --- → mozilla0.9.3
Do Linux/Mac users also see that the transparent animated gif has a white 
background?!
2001070206 Linux: I don't see a white background, but I also see a lack of
animating gifs.  I'm not sure I even see
http://www.world-direct.com/central/imgs/btn/iinfo_1.gif appear at all on that page.
Doesn't look like this is getting fixed before the freeze tomorrow night.
Pushing out a milestone.  Please correct if I'm mistaken.
Target Milestone: mozilla0.9.3 → mozilla0.9.4
*** Bug 90901 has been marked as a duplicate of this bug. ***
I found the same problem while viewing an animated gif in linux (redhat 6.2, 
mozilla 0.9.2 build ID20010711).
I'll attach another example of animated transparent gif image, i.e. a counter, 
which is displayed as '7' in moz, while it should be '822677'.
changing to ALL
Keywords: testcase
OS: Windows 2000 → All
Hardware: PC → All
updaging keyword.
any news on the progress of this one?
Keywords: mozilla0.9.4
Target Milestone: mozilla0.9.4 → mozilla0.9.5
bug 93524 is having the same problem.
Summary: Transparent animated GIFs have white background and don't animate → Transparent animated GIFs have white background
providing the smallest testcase possible :)
Hi
When I went here: http://www.world-direct.com/central/imgs/btn/iinfo_1.gif
and didn't except the cookie that they wanted to set I didn't see anything.
Might be a dupe of 22607 or 84080.
Make that bug 22607 or bug 84080.
This is another (rather heavy, though visually nice) example:
http://www.andi.com/pictures/handanim_gross.gif

The background isn't white, but it's certainly corrupted.
Keywords: nsbranch
There aren't any comments on this bug since the 16th of
Sept.  Can QA regess against the Netscape commercial builds and determine if
this is still a valid bug?  If so, and we can get fixes/reviews in the next day
or two, please mark as nsbranch+ which will get this on the PDT radar. Else,
mark is as nsbranch-. Also, can someone comment in the bug how serious you think
this is?  PDT is only accepting "stop ship" bugs such as data loss and loss of
major functionality.
This one is serious since some of the most widely used GIFs (w3c compliant, 
etc.) are having this problem.

What is the current status of this one?
no fix yet. cc'ing saari to see if he can help. pav feels this may be a dup of
saari's bugs.
Keywords: nsbranchnsbranch-
.
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Blocks: 104166
Blocks: 107067
Keywords: nsbranch-
Keywords: mozilla0.9.6
Any news on that? We should really try to get this one fixed.
Target Milestone: mozilla0.9.6 → mozilla0.9.8
Keywords: mozilla1.0
pav, what's the deal here?
Blocks: 115520
Blocks: 119597
Dunno if this is the same... this page came up on yahoo's "Random link":

http://www.geocities.com/TimesSquare/7419/

When you click the 'cheats' link, all 3 animated GIFs lose their transparency.
Pretty ugly effect. Not that the page was any prize to begin with.
Whiteboard: [driver:brendan]
Keywords: nsbeta1
Target Milestone: mozilla0.9.8 → mozilla0.9.9
this is out for 0.9.8.  I'll be seeing to this getting fixed early in the 0.9.9 
timeframe.
No longer blocks: 115520
I recently fixed this bug in an image decompression library completely unrelated
to mozilla.  I'd submit the same fix here, except that I'm totally unfamiliar
with the way the mozilla stuff actually works.  But I can describe the actual
technical problem and what needs to be done to repair it.

It turns out that the GIF disposal method known as "Restore Background" is only
supposed to restore the pixels which correspond to the "window" of the frame
being disposed, instead of the "screen" (which is what appears to be happening
in the current version of mozilla).  ("window" and "screen" being GIF terms for
the size and position of the particular frame, and of the complete animation,
respectively.)  The GIF spec itself is somewhat unclear on this, but experience
with the testcases reported here shows that to be the case.
over to nivedita
Assignee: pavlov → nivedita
Keywords: mozilla0.9.4
Attached patch proposed patch for animated gif (obsolete) — Splinter Review
the patch sets the dirty rect as the current frame rect size opposed to the
entire frame rect.  

Pavlov,
Can you please verify the patch and convey your thoughts on them.
This is in regard to the patch (id=67710). Though, the patch seems to work, I 
have doubts about the way we are handling the transparency in gif. 
> http://www.andi.com/pictures/handanim_gross.gif

This doesn't load properly for me, it starts loading but then switches to the
broken image rect :-(
this patch looks ok to me and my tests havn't found any regressions.... I will 
sit down with Saari as soon as possible and verify that this is the right fix.
Keywords: nsbeta1nsbeta1+
Whiteboard: [driver:brendan] → [driver:brendan] [needs r=]
Keywords: mozilla0.9.9
what is the current status?
need update from pavlov
Target Milestone: mozilla0.9.9 → mozilla1.0
This looks okay to me... trying to think if there is any case where the
background color should be maintained when the image is transparent. Probably
just me being pedantic and misinterpreting the spec :-)

Let me run this through a couple tests and then I'll r=
Whiteboard: [driver:brendan] [needs r=] → [driver:brendan] [needs r=][adt2]
It seems to me the problem is that frames marked to "replace" rather than
combine should replace only the previous frame (at least with GIFs optimized for
size and use transparencies).  I believe GIMP has the same issue with GIFs
created in this manner.  Is it possible to keep the frames in a stack and when a
frame is marked to "replace" then the previous frame is just popped of the stack
and the current frame replaces it?  I believe that would fix the issue.
we already keep all the frames in the list, so that should be easily doable
This is my first patch, so please have patience :)

As well as this bug, the patch fixes the following bugs:
Bug 8409  (Verified Invalid) Animated gif leaves trails
Bug 22607  GIF decoder doesn't support all frame replacement options  
Bug 84080  Animated GIFs w/transparency do not properly animate/overclip 
Bug 102670 (possibly) problems rendering animated GIF on page 
Bug 130376 Certain animated image doesn't render quite right  
Bug 125904 Animated GIFs don't always redraw non-animated portions of the image


On my Windows machine, I've tested each gif in those bugs, and they render
properly for me.  I don't have a Mac or Unix, so could people with these
systems please try it?	(Mac esp, since they seem to store images differently)

I haven't done any memory leak tests (I don't know how to yet)

And of course, don't hesitate to point out problems with the patch.
tor, can you look over this.  I'm off on vacation starting a few hours ago for a
little while.  Hopefully we can get some people banging on this patch since it
seems like it will fix a lot of the gif problems.
For the most part, I just tried to clean up the code.

With this patch, http://corp.pixel-industries.com/index2.html , goes from 50%
CPU usage to 10% CPU usage on my system.  (The site has many animated gifs)

Please mark my previous patch obsolete. (Attachment 78695 [details] [diff])
Attachment #78695 - Attachment is obsolete: true
Keywords: patch, review
pavlov,
I have tested the patch id=79132 with quite some animated gifs, and this patch 
seems to work fine for all of them. 
Aside from any changes requested by reviewers, this should be my last revision.


Changes made since the last patch (Rev 2):

1) DoComposite; Cleanup/Optimization; Moved nextFrame->DrawTo and
BuildCompositeMask out of switch since each switch had it.

2) DoComposite; Cleanup/Optimization; Skip disposal switch if on first frame
(since there is nothing to dispose of)

3) DoComposite; Bug Fix; Moved "Previous Frame Storing" code block down to
after disposing of previous frame (like IE and Opera)
   Solved "AnimatedTest.html"'s Image 14 and Image 15 rendering problem

4) DoComposite; Bug Fix; Changed "Previous Frame Storing" code to memcopy
CompositingFrame (Data & Alpha) instead of trying to use DrawTo &
BuildComposite
   Solved "AnimatedTest2.html"'s Image 5 rendering problem
   
5) AppendFrame; Removed my invalid comments

--

I made a few very (very) ugly gif testing pages, which can be found at
http://animecity.nu/mozilla/IMGTest/

Please mark Rev 2 patch as obsolete :)
Attachment #79132 - Attachment is obsolete: true
Attached patch The Correct Revision 4 (obsolete) — Splinter Review
How embarassing, that was the wrong patch and won't compile.
This one will. x.x

-Arron
Attachment #79894 - Attachment is obsolete: true
After patching Bug 137128, this patch revealed some problems.
Changes from last revision:

- Check to see if width & height are > 0 in FillAreaWithColor, OneMaskArea, and
ZeroMaskArea
Attachment #79942 - Attachment is obsolete: true
Changes since last patch (All minor):

1) check to see if alloc succeded
2) quit immediately if calculated height or width <= 0
Comment on attachment 80671 [details] [diff] [review]
Revision 5; negative size checking added.

Arron, next time on the file attachment page, could you please tick which
patches your attachment obsoletes.
Attachment #80671 - Attachment is obsolete: true
Right now I don't have rights to obsolete patches, but I hear the next time
bugzilla upgrades, I'll be able to. (the ability to obsolete your own patches
will exist then)

Sorry for creating all the administrative work.
.
Assignee: nivedita → pavlov
Will this also help bug 86319 ?
The only thing I added what a call to ImageUpdated after copying the composite
frame to the previous composite frame.	Windows doesn't require it (which is
why I missed it), but Linux can't live without it. 

Oh, and I also trimmed trailing spaces. :)

---

Markus, there will definitely be some better performance with regards to bug
86319.	It will depend on the gif, however.  Some will see a huge performance
gain, while others will get zero.  The majority should see a slight reduction
in CPU usage.
Attachment #81327 - Attachment is obsolete: true
Comment on attachment 83301 [details] [diff] [review]
Revision 7; Call ImageUpdated after memcpy to prevcomposite

r=pavlov

nothing in the patch pops out as me as a problem and the patch fixes a lot of
problems.  I suggest we get this in to the trunk and see if there are any
regressions.

tor can you sr=?
Attachment #83301 - Flags: review+
*** Bug 130376 has been marked as a duplicate of this bug. ***
Comment on attachment 83301 [details] [diff] [review]
Revision 7; Call ImageUpdated after memcpy to prevcomposite

Some initial comments:

  * changing the replacement mode magic numbers with enums would
    help people without the gif spec memorized.  :)

  * FillAreaWithColor() is always being called with a color of zero.
    Isn't the background color of a GIF colormap entry zero or
    something?

  * ZeroMaskArea() and OneMaskArea() are almost identical code,
    differing by a constant.  Probably should be combined into
    a SetMaskArea().

>+          // Calculate area that we need to redraw
>+          // which is the combination of the previous frame and this one
>+
>+          // This is essentially nsRect::UnionRect()
>+          nscoord xmost1 = x + width;
>+          nscoord xmost2 = xDispose + widthDispose;
>+          nscoord ymost1 = y + height;
>+          nscoord ymost2 = yDispose + heightDispose;
>+
>+          (*aDirtyRect).x = PR_MIN(x, xDispose);
>+          (*aDirtyRect).y = PR_MIN(y, yDispose);
>+          (*aDirtyRect).width = PR_MAX(xmost1, xmost2) - (*aDirtyRect).x;
>+          (*aDirtyRect).height = PR_MAX(ymost1, ymost2) - (*aDirtyRect).y;

   * If this is essentially nsRect::UnionRect(), why not use that?
>(1) * changing the replacement mode magic numbers with enums would

The only reason I didn't was because disposal method 3 and 4 do the same thing,
and I couldn't think of seperate names for them.  Would
DISPOSE_RESTORE_PREVIOUS1 and DISPOSE_RESTORE_PREVIOUS2 do?

>(2) * FillAreaWithColor() is always being called with a color of zero.

zero == transparent when mask for that pixel is 0.  So when I call
FillAreaWithColor with a 0 and the frame has am alpha layer, I'm really making
the area transparent.

>(3) * ZeroMaskArea() and OneMaskArea() are almost identical code,

Almost, but not quite.  Zero does some different bit shifting and uses &=, while
One uses |=.  I'm 1/2 way through recoding these two functions anyway, so would
it be okay if I file another bug to deal with combining/optimizing these functions?

>(4) * If this is essentially nsRect::UnionRect(), why not use that?

I built the code first, then realized it was a dup of UnionRect ;P  However,
UnionRect requires nsRect parameters.  I already had 4 individual PRUInts for
the frame area, and needed 4 individual PRUInts for prevFrame, and I didn't want
to waste another 2 calls to fill 2 nsRects that would be only used once.
I *do* use UnionRect as part of my "clean-up of imgContainer.cpp" patch, which
will be posted to a bug as soon as I create one.  However, I can put UnionRect
in *now* if you wish.

I took the liberty of numbering the comments 1-4 so you can tell me what you
want done with each of the 4.
tor, could you describe the problem you are seeing with Attachment 85089 [details]?

I don't see any artifacts or noticable problems on my system with the patch
I'm seeing garbage from previous frames - not updating the mask properly,
maybe?
tor, I hope I've found the problem.  Please try this additional patch and see
if it solves the problem.

Rationale: Image is 50 in width. nsImageGTK sets LineStride (mRowBytes) to 152.
 When I call SetImageData in FillAreaWithColor, I pass 150 as # of bytes to
write (aLength).  At the end of SetImageData, a calculation is done to
determine the area changed and ImageUpdated is called with that area.  Without
this patch, it sets the "height changed" to 0 (Calculation: aLength /
row_stride == 150 / 152 == 0).	The patch correct this by rounding it up
[Calculation: ((aLength-1) / row_stride)+1 == 1].

This does not effect Windows because ImageUpdated is not used on that
platform.. assuming this is really the problem and not a red herring. :P
Comment on attachment 85140 [details] [diff] [review]
Additional Test Patch: Round up height for nsRect passed to ImageUpdated

sr=tor

Add a one line comment explaining you're adjusting for aligned strides.
Attachment #85140 - Flags: superreview+
(1) - aren't they enumerated in a spec somewhere?

(2) - the code pulls color (always zero) apart into r,g,b and fills the area.

(3), (4) - file bugs, CC me.


      for (PRInt32 y=0; y<height; y++) {
#ifdef XP_PC
        aFrame->SetImageData(foo, bprToWrite, ((y+aY) * bpr) - xOffset);
#else
        aFrame->SetImageData(foo, bprToWrite, ((y+aY) * bpr) + xOffset);
#endif
      }

Shouldn't that be "#if defined(XP_WIN) || defined(XP_OS2)"?  I know win32
stores images in row-reversed order, but didn't think they had totally
lost their minds and stored them right-to-left.
Attachment #67710 - Attachment is obsolete: true
Depends on: 148551
(1) There are some definitions in GIF2.cpp. The same ones that are in comments
after each of the first 3 case statements in DoComposite.  I don't know why
these definitions are in comment and aren't being used, so I played it safe and
left them as is.  If you see no ramifications to including GIF2.h in
imgContainer.cpp, and using the definitions in the comments, then I can do so. 
I'd still have to make up another definition, since 3 is not defined in
GIF2.cpp.  The alternative is to leave them as numbers until my "clean-up of
imgContainer.cpp" is in place.

(2) correct. In both cases FillAreaWithColor is used, ZeroMaskArea is called
immediately afterwards, effectively making that area invisible.  What's the
issue here? Do we need a comment somewhere better explaining something?

(3) (4) Done & CC'd.  For everyone elses reference, these are Bug 148634 and Bug
148637, respectively.

(5) "The #ifdef XP_PC thing"
You are correct, win32 doesn't store rtl, but SetImageData's was incorrectly
recalculating offset.  At the time of making this patch my knowledge was limited
and this was how I corrected the problem.  Now that I more experienced with
gfxImageFrame & ImgLib in general, I understand where the real bug is and filed
Bug 148551 to fix it.  I will marked this bug as depending on the new bug, and
remove the #ifdef XP_PC.  Please SR Bug 148551 

pav, I'll need a r= for Bug 148551 before this one can be checked-in.
No longer depends on: 148551
Attachment #83301 - Attachment is obsolete: true
(2) - the point is the function is doing needless work.  If you're only going
to call it with black, then change it to not have that argument and make
judicious use of memset().
Comment on attachment 85140 [details] [diff] [review]
Additional Test Patch: Round up height for nsRect passed to ImageUpdated

tor, will this do for a comment?

+ // adjust for aLength < row_stride
+  PRInt32 numnewrows = ((aLength - 1) / row_stride) + 1;
Depends on: 148551
(2)
Thanks for the clarification, tor.
I removed the color parameter & related code from both FillWithColor and
FillAreaWithColor, and renamed them both to BlackenFrame.

In my defense, the only reason I added the color parameter to FillAreaWithColor
was because FillWithColor had it.  Neither pass in any other color than black,
as you pointed out.  Looks much cleaner now. :)
Attachment #85965 - Attachment is obsolete: true
imgContainer::DoComposite, case 2 (restore to background) - why aren't you
using the background color from the screen descriptor block?  Shouldn't it
also be setting the alpha bits instead of clearing them?

imgContainer::BlackenFrame(gfxIImageFrame*) - why are you only clearing for
the *_A1 formats?

imgContainer::BlackenFrame(gsIImageFrame *, FRInt32*4) - you're zeroing in
all formats, so the switch() is rather pointless.
tor:
"restore to background" means restore to the background image that was there
before the gif was, not restore to background color of the gif.  The disposal
#define's are misleading, and that's one reason why I don't like them.  My fix
of Bug 148637 already includes more consice defines.

In imgContainer::BlackenFrame(gsIImageFrame *, FRInt32*4) I'm skipping 2 formats
that I can't guarantee to work (RGBA and BGRA) because I don't know how those
formats work.  From the sounds of it, RBGA stores the alphalevel within the
imageData block.. if it does, then the existing code definitely will not work
properly.

In imgContainer::BlackenFrame(gfxIImageFrame*), I left the format switch code
as-is.  Now that the function is considerably smaller from what it originally
was, I can see that the switch could include the same ones as the other
BlackenFrame does.. but in the end, it really doesn't matter.
Reading through the gif89m spec, the description of the graphics control
extension seem to have a different opinion about mode 2:

            iv) Disposal Method - Indicates the way in which the graphic is to
            be treated after being displayed.

            Values :    0 -   No disposal specified. The decoder is
                              not required to take any action.
                        1 -   Do not dispose. The graphic is to be left
                              in place.
                        2 -   Restore to background color. The area used by the
                              graphic must be restored to the background color.
                        3 -   Restore to previous. The decoder is required to
                              restore the area overwritten by the graphic with
                              what was there prior to rendering the graphic.
                     4-7 -    To be defined.
The best comment I've seen on "Dispose to Background" is from the help file of
GIF Construction Set by Alchemy Mindworks, where they say:

"Background: This is the background colour used to display GIF files in GIF
Construction Set Professional. It’s ignored by web browsers, but it’s handy to
make sure your transparency is working properly, as discussed earlier."

I've made a gif to illustrate this:
http://www.animecity.nu/mozilla/imgtest/Mode2-2.gif

The image is 200x100.  It has 2 frames:
Header: Background color is set to aqua
Frame 1: 200x100 image of a red fading to pink.  Disposal is 2/Background.
Frame 2: 30x30 image at 90x40.  Image is a green box with an invisible center. 
The invisible color is a dark blue.

If we followed the spec, Frame 1 should dispose fully to aqua, and frame 2
should draw a green box with it's center being aqua.  If we look at how NS2 -
NS7, IE, and Opera handle it, they all dispose of Frame 1 to "transparent", and
draw frame 2 with a green box with it's center being transparent.
Answers to various review questions (in no particular order).

  Realistically you don't need to deal with the RGBA or A8 varients,
  since this compositing code will only be used for animated GIFs.
  BlackenFrame(gfxIImageFrame*) should handle {RGB, BGR).

  Yes, that comment is fine.

  Ok, sounds like a shortcoming in the GIF spec (written before the era of
  browsers).
Patch with comments required/accepted by tor
Attachment #85140 - Attachment is obsolete: true
As per Comment #73 and conversation with tor, the "switch (format)" stuff has
been removed from the 2 BlackenFrame functions.  It's not needed since gifs are
only in RBG_A1.
Attachment #85969 - Attachment is obsolete: true
Comment on attachment 86689 [details] [diff] [review]
Patch #2: Round up height for nsRect passed to ImageUpdated

sr=tor
Attachment #86689 - Flags: superreview+
Comment on attachment 86692 [details] [diff] [review]
Patch #1, Rev 10; Removed switches from BlackenFrame

Actually they can be RGB or RGB_A1, which are equivalent for the purposes of
those two functions.

sr=tor
Attachment #86692 - Flags: superreview+
Can't be TM 1.0.

/be
Keywords: mozilla1.0mozilla1.0.1
Target Milestone: mozilla1.0 → mozilla1.0.1
Comment on attachment 86692 [details] [diff] [review]
Patch #1, Rev 10; Removed switches from BlackenFrame

r=pavlov
Attachment #86692 - Flags: review+
Blocks: 143047
Keywords: adt1.0.1, topembed
Whiteboard: [driver:brendan] [needs r=][adt2] → [driver:brendan] [needs a=] [adt2 RTM] [Need ETA]
Whiteboard: [driver:brendan] [needs a=] [adt2 RTM] [Need ETA] → [driver:brendan] [needs a=] [adt2 RTM] [Need ETA] [ETA is whenever this gets a=]
Comment on attachment 86689 [details] [diff] [review]
Patch #2: Round up height for nsRect passed to ImageUpdated

r=pavlov
Attachment #86689 - Flags: review+
checked in the last 2 patches in the bug in to the trunk.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Blocks: 22607
Verified fixed win XP trunk build 2002061808, Mac OS X trunk build 2002061808
and linux trunk build 2002061808
Status: RESOLVED → VERIFIED
adt1.0.1+ (on ADT's behalf) approval for checkin to the 1.0 branch, pending
drivers' approval. pls check this in asap, then add the "fixed1.0.1" keyword.
Keywords: adt1.0.1adt1.0.1+
Whiteboard: [driver:brendan] [needs a=] [adt2 RTM] [Need ETA] [ETA is whenever this gets a=] → [driver:brendan] [adt2 RTM] [Need 06/21]
*** Bug 153612 has been marked as a duplicate of this bug. ***
*** Bug 154026 has been marked as a duplicate of this bug. ***
*** Bug 152054 has been marked as a duplicate of this bug. ***
blake is offering to land this (in pav's absence). Thanks and take it away blake
:-) 
Attachment #86689 - Flags: approval+
Attachment #86692 - Flags: approval+
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.
Just a reminder that Bug 148551 will need to be checked-in to the trunk at the
same time as this one.
sorry for the bug spam.  I meant the other bug needs to be applied to the 1.0.x
branch, not trunk (It's already in the trunk)
Fixed on branch.
*** Bug 152974 has been marked as a duplicate of this bug. ***
Verified fixed win xp branch build 2002072208, linux branch build 2002072306,
and Mac OS X branch build 2002072305
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: