546272 - (apng) Second frame of APNG is cropped

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Max Stepin]

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

Comment 1

[Max Stepin]

Attachment: spin5.png example

Comment 2

[Glenn Randers-Pehrson]

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.

Comment 3

[Glenn Randers-Pehrson]

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

Comment 4

[Glenn Randers-Pehrson]

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?

Comment 5

[Max Stepin]

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.

Comment 6

[Glenn Randers-Pehrson]

(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.

Comment 7

[Max Stepin]

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.

Comment 8

[Matt]

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

Comment 9

[Glenn Randers-Pehrson]

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

Comment 10

[Matt]

Could someone please remove "regressionwindow-wanted"

Comment 11

[Joe Drew (not getting mail)]

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.

Comment 12

[Max Stepin]

Attachment: small patch

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

Comment 13

[Max Stepin]

Glenn, what do you think of the patch?

Comment 14

[Glenn Randers-Pehrson]

Max's one-line patch fixes this bug, at least on Windows XP.

Tryserver builds are at
http://build.mozilla.org/tryserver-builds/[e-mail]-v01a-1.9.3-546272-Feb18

Comment 15

[Max Stepin]

Joe, review?

Comment 16

[Max Stepin]

Joe, review?

Comment 17

[Joe Drew (not getting mail)]

(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.

Comment 18

[Joe Drew (not getting mail)]

Attachment: correct boundsRect

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

Comment 19

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 431402 [details] [diff] [review]
correct boundsRect

r+ as long as this comes with a test.

Comment 20

[Max Stepin]

Joe, can you test the patch? Tryserver build maybe?

Because it doesn't fix the bug for me.

Comment 21

[Joe Drew (not getting mail)]

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.

Comment 22

[Joe Drew (not getting mail)]

Attachment: very simple testcase

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

Comment 23

[Max Stepin]

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?

Comment 24

[Joe Drew (not getting mail)]

Attachment: tests for sub-rectangle images

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.

Comment 25

[Joe Drew (not getting mail)]

Attachment: fix several sub-frame issues

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.

Comment 26

[Joe Drew (not getting mail)]

(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.

Comment 27

[Max Stepin]

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

Comment 28

[Joe Drew (not getting mail)]

Yeah, I tested it this time :)

Comment 29

[Joe Drew (not getting mail)]

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.

Comment 30

[Max Stepin]

2 months = baked enough?

How about getting this into 3.6.x as well?

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 431566 [details] [diff] [review]
fix several sub-frame issues

Approved for 1.9.2.14, a=dveditz for release-drivers

Comment 32

[Daniel Veditz [:dveditz]]

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.

Comment 33

[Jonathan Watt [:jwatt]]

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

Comment 34

[Max Stepin]

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 35

[Daniel Veditz [:dveditz]]

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.

Comment 36

[Joe Drew (not getting mail)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/537b80ba533e