Closed Bug 85595 Opened 24 years ago Closed 23 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)

(deleted), image/gif
Details
(deleted), image/gif
Details
(deleted), image/gif
Details
(deleted), image/gif
Details
(deleted), image/gif
Details
(deleted), patch
pavlov
: review+
Details | Diff | Splinter Review
(deleted), 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) (deleted) — 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
Attached patch Patch to fix a bunch of GIF animation stuff (obsolete) (deleted) — Splinter Review
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) (deleted) — 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
Attached patch Revision 5; negative size checking added. (obsolete) (deleted) — Splinter Review
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
Attached patch Revision 6; standardizations & alloc check (obsolete) (deleted) — Splinter Review
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
Attached image Here is a GIF illustrating the problem (deleted) —
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.
Attached image problematic gif for r7 patch (deleted) —
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: 23 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: