Closed Bug 546272 Opened 11 years ago Closed 11 years ago

(apng) Second frame of APNG is cropped

Categories

(Core :: ImageLib, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- .17-fixed

People

(Reporter: newstop, Assigned: joe)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(4 files, 2 obsolete files)

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
Attached image spin5.png example
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
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.
Attached patch small patch (obsolete) — Splinter Review
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
Attachment #427310 - Flags: review?(joe)
No longer depends on: 433047
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.
Attachment #427310 - Flags: review?(joe) → review-
Attached patch correct boundsRect (obsolete) — Splinter Review
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+
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.
Attached image very simple testcase
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.
Assignee: glennrp+bmo → joe
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)
(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.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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?
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
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.