(apng) Second frame of APNG is cropped

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: newstop, Assigned: joe)

Tracking

(Blocks: 1 bug, {regression})

unspecified
x86
Windows XP
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(status1.9.2 .17-fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

9 years ago
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
(Reporter)

Comment 1

9 years ago
Created attachment 426963 [details]
spin5.png example

Updated

9 years ago
Blocks: 495609
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.
Assignee: nobody → glennrp+bmo
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
This is related to bug #433047.  I'm marking a dependency for now, although it could be a dupe, dependent, or a separate problem.
Depends on: 433047

Updated

9 years ago
Keywords: regression, regressionwindow-wanted
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?
(Reporter)

Comment 5

9 years ago
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.
(Reporter)

Comment 7

9 years ago
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"
(Assignee)

Comment 11

9 years ago
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.
Keywords: regressionwindow-wanted
(Reporter)

Comment 12

9 years ago
Created attachment 427310 [details] [diff] [review]
small patch

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

Comment 13

9 years ago
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
(Reporter)

Updated

9 years ago
Attachment #427310 - Flags: review?(joe)

Updated

9 years ago
No longer depends on: 433047
(Reporter)

Comment 15

9 years ago
Joe, review?
(Reporter)

Comment 16

9 years ago
Joe, review?
(Assignee)

Comment 17

9 years ago
(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.
(Assignee)

Updated

9 years ago
Attachment #427310 - Flags: review?(joe) → review-
(Assignee)

Comment 18

9 years ago
Created attachment 431402 [details] [diff] [review]
correct boundsRect

I'm not convinced this was ever correct, but it certainly isn't now that imgFrame has an offset.
Attachment #427310 - Attachment is obsolete: true
Attachment #431402 - Flags: review?(jmuizelaar)
Comment on attachment 431402 [details] [diff] [review]
correct boundsRect

r+ as long as this comes with a test.
Attachment #431402 - Flags: review?(jmuizelaar) → review+
(Reporter)

Comment 20

9 years ago
Joe, can you test the patch? Tryserver build maybe?

Because it doesn't fix the bug for me.
(Assignee)

Comment 21

9 years ago
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.
(Assignee)

Comment 22

9 years ago
Created attachment 431549 [details]
very simple testcase

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

Comment 23

9 years ago
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?
(Assignee)

Comment 24

9 years ago
Created attachment 431565 [details] [diff] [review]
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.
Assignee: glennrp+bmo → joe
(Assignee)

Comment 25

9 years ago
Created attachment 431566 [details] [diff] [review]
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.
Attachment #431402 - Attachment is obsolete: true
Attachment #431566 - Flags: review?(vladimir)
Attachment #431566 - Flags: review?(roc)
Attachment #431566 - Flags: feedback?(bobbyholley+bmo)
(Assignee)

Comment 26

9 years ago
(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.
(Reporter)

Comment 27

9 years ago
I tried your patch, Joe, and it works fine.
(Assignee)

Comment 28

9 years ago
Yeah, I tested it this time :)
(Assignee)

Comment 29

9 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Reporter)

Comment 30

9 years ago
2 months = baked enough?

How about getting this into 3.6.x as well?
Attachment #431566 - Flags: feedback?(bobbyholley+bmo)
Attachment #431566 - Flags: approval1.9.2.14?
Comment on attachment 431566 [details] [diff] [review]
fix several sub-frame issues

Approved for 1.9.2.14, a=dveditz for release-drivers
Attachment #431566 - Flags: approval1.9.2.14? → approval1.9.2.14+
Blocks: 597174
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.
Attachment #431566 - Flags: approval1.9.2.14+ → approval1.9.2.14-
The test for bug546272.png is intermittently failing, see bug 629632. Would it be okay to increase the 100ms timeout to, say, 200ms?
(Reporter)

Comment 34

8 years ago
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

Updated

8 years ago
Depends on: 629632
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.
Attachment #431566 - Flags: approval1.9.2.16+
You need to log in before you can comment on or make changes to this bug.