155939 - Frames not replaced correctly on animated GIF (offset calculation incorrectly includes border/padding)

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Aaron Kaluszka]

On http://kontek.net/ the MODPlug animated GIF button leaves some pieces behind
when animating.  Oddly enough, it does not do this when the image is viewed by
itself.  It looks fine on other browsers and on other pages.  I have seen the
same behavior on different machines.  I do not remember seeing it before the
checkin for bug 22607, but have seen it for a while now.
2002070304-trunk

Comment 1

[ArronM (:paper)]

exactly which gif are you having problems with?

Is it the one on the very bottom right?  If that's the one, it's not an animated
gif, it's a plugin..

Comment 2

[Aaron Kaluszka]

The one second from left.

Comment 3

[ArronM (:paper)]

ah, I see it now :)
Confirming on Build 2002070204.  Little bits of blue get left behind

Comment 4

[Aaron Kaluszka]

I think I found the problem:

Example:
data:text/html,<img src="http://www.vg-network.com:8080/pics/gvgaboxbanner.gif"
border="1">

shows the problem.

Note that:
data:text/html,<img src="http://www.vg-network.com:8080/pics/gvgaboxbanner.gif"
border="0">

does NOT exhibit the problem.

However,

data:text/html,<img src="http://www.vg-network.com:8080/pics/gvgaboxbanner.gif"
border="10">

shows an even worse problem.

Basically, the frame replacement offset is calculated using the outer frame
(including the border) when it should only be calculated from the frame of the
image itself.  This is really bad.

Comment 5

[ArronM (:paper)]

-> me

Comment 6

[ArronM (:paper)]

-> Taking

I have a fix, but I don't trust it yet.
It appears nsImageFrame::Paint expects the aDirtyRect to include
borders/padding, whereas imgContainerGIF doesn't know anything about that, so it
just passes  aDirtyRect without a border.  Doesn't effect 1 frame gifs because
imgContainerGIF doesn't send out a dirty notification (a FrameChanged call) for
them. 

If imgContainerGIF is the only one sending FrameChanged calls, then I can simply
add the border to aDirtyRect in FrameChanged before it gets sent to Paint.  If
not, I'll have to look for another alternative.

Also confirming bug present in 1.0.1 and nominating this bug for 1.0.2.  
Regression is almost definitely Bug 85595, or rather the check-in there caused
this bug to show itself. (Before 85585 we always passed the full frame's to
FrameChanged, even if only a partial area was changed)

Comment 7

[ArronM (:paper)]

Bug existed in 1.0.0 as well, and can be confirmed by using the following:
data:text/html, <img src="http://www.vg-network.com:8080/pics/gvgaboxbanner.gif"
border="50" />

Fortunately, people rarely use borders on animated gifs :)

Comment 8

[ArronM (:paper)]

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

Comment 9

[ArronM (:paper)]

Another testcase, using CSS padding instead of border:
http://bugzilla.mozilla.org/attachment.cgi?id=105197&action=view

Comment 10

[ArronM (:paper)]

Attachment: Adjust dirtyrect before Invalidating

Haven't had any problems with this patch in the 2+ weeks I've been using it. 
Code comments should explain all. :)

Comment 11

[ArronM (:paper)]

Attachment: Adjust rect before Invalidating

After bz pointed me towards the similar code in OnDataAvailable, I dug around
and found Bug 87550.  Same issue, different spot. 

So I made this patch look like the other. :)

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 105281 [details] [diff] [review]
Adjust rect before Invalidating

r/sr=bzbarsky

Comment 13

[tor]

Comment on attachment 105281 [details] [diff] [review]
Adjust rect before Invalidating

Hey, that looks familiar. :)

sr=tor

Comment 14

[tor]

Comment on attachment 105281 [details] [diff] [review]
Adjust rect before Invalidating

Hey, that looks familiar. :)

sr=tor

Comment 15

[tor]

Checked in.