419277 - Artifacts with animated GIF and CSS px != device px

Status: RESOLVED WORKSFORME

(Core :: Graphics, defect, P2)

Description

[Thomas Goldstein]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008022320 Minefield/3.0b4pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008022320 Minefield/3.0b4pre

It seems animated GIF can have display problems when changing the zoom level. Details below.

Reproducible: Always

Steps to Reproduce:
1. Go to http://stifu.free.fr/pics/zabu/wip/templateman.gif
2. Change the zoom level (anything but 100%)
3. Let the animation loop
Actual Results:  
A black rectangle appears on the right of the image

Expected Results:  
No black rectangle should be there

Comment 1

[Thomas Goldstein]

I went back to yesterday's nightly, and the bug isn't in it, as expected. So this is most likely a regression from bug 381661.

Comment 2

[Steve England [:stevee]]

Works: 20080223_1426_firefox-3.0b4pre.en-US.win32
Broken: 20080223_1459_firefox-3.0b4pre.en-US.win32

Checkins to module PhoenixTinderbox between 2008-02-23 14:26 and 2008-02-23 14:58 : 
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1203805560&maxdate=1203807539
--> bug 381661

Comment 3

[Steve England [:stevee]]

Attachment: testcase from reporter's URL

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

We're not blocking at this point on any zoom artifacts, but I think what's going on is that the gif compositing code is drawing subframes possibly using Paint() and using EXTEND_PAD... that's a guess though, without looking at code.

Comment 5

[Ben Frisch]

It's just the selection cursor for me.  Try clicking to the right of the image.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Is this guaranteed to only affect zoom and not affect high-DPI screens?  Does this issue appear if layout.css.dpi is set to a large value?

Comment 7

[Martijn Wargers (dead)]

Yes, this issue also appears with a large value for layout.css.dpi (192) for me.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Renominating, since high-DPI issues are much more something we should block on than zoom issues...

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: fix

What's going on here is a little odd -- stuart tells me that old Image impls used to work around this as well.  Basically, we have a first frame that's smaller than the actual gif canvas dimensions... but for some reason, the nsThebesImage that's created has the right mWidth/mHeight, but its decodedSize is the size of the canvas, and when Draw is called, the source rectangle assumes that it's the bigger size as well.

When we turned on EXTEND_PAD, all of a sudden we were getting the padding effect from where the fist of the animation was hitting the right edge of the frame, because we were drawing it into a bigger rectangle.  This patch corrects for this.

One thing that I'm not sure of is that this might have invalidation problems, since we're not actually going to draw over the entire area that we'd be expected to.  However, since images can be transparent, the background should be drawn anyway, and the part that's not drawn would normally just have a transparent region drawn on top... so I think that this should be safe.

Comment 10

[Peter Weilbacher]

Vlad, can you please change the patch to exclude OS/2, too, from the workaround in ThebesDrawTile? Like this:
   #if !defined(XP_MACOSX) && !defined(XP_WIN) && !defined(XP_OS2)

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #4)
> We're not blocking at this point on any zoom artifacts

I think we should block on some zoom artifacts, depending on how severe they are.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Wouldn't it be slightly cleaner (and work better with my patch in bug 403181, which I'm going to land in an hour or two) to do this fixup around where we clip the source rect to the mDecoded rect? You should just need to generalize the code there to remove the implicit assumption that aSourceRect is contained in (0,0,mWidth,mHeight).

Does your imgContainer change work when blendMethod != imgIContainer::kBlendSource? Seems like the Fill() in that if-block will consume the path.

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Nope, our Fill()/Stroke() are always fill_preserve()/stroke_preserve().  We had a really good reason for defining it that way way back, but I forget what it was now :p

But yeah, I'll wait until you land 403181 and update this.

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

So now that roc's patch has landed and stuck, I don't seem to be able to reproduce this any more.  Can someone confirm?

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yep, pretty sure this was fixed by roc's patch here:

http://mxr.mozilla.org/seamonkey/source/gfx/src/thebes/nsThebesImage.cpp#492

subimageRect won't be != 0,0,mWidth,mHeight, so we'll get the temporary surface treatment.  The temporary is created with subimageRect's size (which is bigger), but that's ok, since we end up painting into it essentially enlarging the frame, and everything then works correctly.

So, going to call this WORKSFORME.

Comment 16

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I'm not sure how to do a test for this, FWIW, since it relies on animated GIFs and specific frame sizes.  It might be possible to create a testcase for it by trying to get the error to happen in the first frame, and then disabling animation.. but I think the first frame will always be drawn at full canvas size.