User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a2pre) Gecko/20100211 Minefield/3.7a2pre Build Identifier: See the attached example. Second frame parameters: x,y = (54,54) w,h=(148,148) Left-top corner should be at (54,54), but it's drawn at (108,108) The bug exists in FF 3.6 and in latest Minefield. No bug in FF 3.5.x. Reproducible: Always

I see the rendering error with a current build on WinXP. Only the lower left quadrant of one of the frames is shown, with the rest rendered in the browser's background color.

This is related to bug #433047. I'm marking a dependency for now, although it could be a dupe, dependent, or a separate problem.

The bug is not apparent in Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.7) Gecko/20100106 Ubuntu/9.10 (karmic) Firefox/3.5.7. When in FF-3.6 does it first appear?

Probably not a dupe, that #433047 is pretty old. This one is new. The earliest version I tested is FirefoxPortable36beta1. Not exactly the best way of testing. But the bug does appear on that beta 1.

(In reply to comment #5) > Probably not a dupe, that #433047 is pretty old. This one is new. It's most likely a regression from fixing other APNG bugs.

imgFrame.cpp (line 717): mDecoded.IntersectRect(mDecoded, boundsRect); before IntersectRect: mDecoded=(54,54,148,148) boundsRect=(0,0,148,148) after IntersectRect: mDecoded=(54,54,94,94) That's too small, so it's displayed cropped.

Regression range: works: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090720 Minefield/3.6a1pre http://hg.mozilla.org/mozilla-central/rev/247524e19d0c broken: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090721 Minefield/3.6a1pre http://hg.mozilla.org/mozilla-central/rev/f2a58ffcd00c http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=247524e19d0c&tochange=f2a58ffcd00c State in latest builds: broken: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a2pre) Gecko/20100216 Minefield/3.7a2pre works: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.0.19pre) Gecko/2010021606 GranParadiso/3.0.19pre works: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.9pre) Gecko/20100216 Shiretoko/3.5.9pre broken: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.2pre) Gecko/20100216 Namoroka/3.6.2pre

Within that range, the large patch in bug #753 is the only one that looks to be relevant.

Could someone please remove "regressionwindow-wanted"

This is almost certainly a regression from bug 753. I'm actually sort of surprised that a) it didn't come up sooner and b) we didn't have any test coverage of it until now.

nsPNGDecoder.cpp : restores 3.5 behavior of FrameUpdated(,rect)

Glenn, what do you think of the patch?

Max's one-line patch fixes this bug, at least on Windows XP. Tryserver builds are at http://build.mozilla.org/tryserver-builds/glennrp@gmail.com-v01a-1.9.3-546272-Feb18

Joe, review?

Joe, review?

(In reply to comment #7) > imgFrame.cpp (line 717): > > mDecoded.IntersectRect(mDecoded, boundsRect); > > before IntersectRect: > mDecoded=(54,54,148,148) > boundsRect=(0,0,148,148) > > after IntersectRect: > mDecoded=(54,54,94,94) > > That's too small, so it's displayed cropped. Ah, yes, this is a regression from bug 753, but the patch as posted just works around the issue. The correct patch will change the boundsRect to include mOffset.x and mOffset.y. I have such a patch, but can't test it because right now mozilla-central doesn't build on my machine.

I'm not convinced this was ever correct, but it certainly isn't now that imgFrame has an offset.

Comment on attachment 431402 [details] [diff] [review] correct boundsRect r+ as long as this comes with a test.

Joe, can you test the patch? Tryserver build maybe? Because it doesn't fix the bug for me.

You're totally right. (I was being bitten by bug 550646, but I got a patch to that and was able to compile.) I think the patch is still right, but it obviously doesn't fix this problem. I'm still not in love with the earlier patch - we shouldn't send our entire width and height every time. Max, how did you create this image? I've been trying to create a simpler testcase (just rectangles) but failing.

Ah, finally figured it out. The important part is that the frames have PREVIOUS or BACKGROUND disposals.

No, my patch doesn't send the entire (256x256) frame. It only sends (148,148). I don't touch width and height at all, I only make offsets zero. --- I know how the bug happens. Let me try to explain... Important point - the same animation, but in GIF, doesn't have a bug. So I patch nsPNGDecoder.cpp to make it work just like GIF decoder. --- Now, in details: The actual doubling of offsets happens in imgFrame::Draw() Offset (54,54) turns into (108,108) offset in here: gfxRect available = gfxRect(mDecoded.x, mDecoded.y, mDecoded.width, mDecoded.height) + gfxPoint(aPadding.left, aPadding.top); It doesn't affect GIF animations, because mDecoded.x and mDecoded.y are always zero for all GIFs. My patch makes sure mDecoded.x and mDecoded.y are always zero for APNG as well. Makes sense?

Here are some simple testcases, both for this bug and for a bug that arose when I had made some mistakes in trying to fix this bug.

There are several problems here. Some are less urgent than others, but we may as well get them resolved all at once. The first is that we were "correcting" the available area by the padding in imgFrame::Draw(), which made no sense in general. However, this did make GIF files work, because GIF files didn't report their real sizes when decoding. So I fixed that too. Also, there are several places in imgFrame where we were assuming that the frame's extent was (0, 0)-(width, height), which simply isn't true. I've fixed those that I found.

(In reply to comment #23) > My patch makes sure mDecoded.x and mDecoded.y are always zero for APNG as well. > > Makes sense? Totally makes sense, and thank you for explaining. I don't agree with this direction, though, since it makes mDecoded in PNG *and* GIF imgFrames wrong. Instead, I went the opposite way, and corrected the problem in imgContainer *and* in the GIF decoder.

I tried your patch, Joe, and it works fine.

Yeah, I tested it this time :)

http://hg.mozilla.org/mozilla-central/rev/dc5674a71f28 http://hg.mozilla.org/mozilla-central/rev/f70db887a173 Once this has baked a bit, we'll try getting this into 3.6.

2 months = baked enough? How about getting this into 3.6.x as well?

Comment on attachment 431566 [details] [diff] [review] fix several sub-frame issues Approved for 1.9.2.14, a=dveditz for release-drivers

Comment on attachment 431566 [details] [diff] [review] fix several sub-frame issues Missed non-blocker code-freeze for 1.9.2.14 -- rescinding approval, you can try again next time.

The test for bug546272.png is intermittently failing, see bug 629632. Would it be okay to increase the 100ms timeout to, say, 200ms?

Are you sure the increase to 200ms would help? The frame delay in bug546272.png is only 20ms. But it wouldn't hurt, I guess. The timeout for GIF tests was recently increased to 500ms. Also, I remember there were some problems with delaytest itself: https://bugzilla.mozilla.org/show_bug.cgi?id=580182#c24

Comment on attachment 431566 [details] [diff] [review] fix several sub-frame issues Approved for 1.9.2.16, a=dveditz for release-drivers Code-freeze for 1.9.2.16 non-blockers is TOMORROW -- please land this ASAP if you intend to.