Closed
Bug 546272
Opened 15 years ago
Closed 15 years ago
(apng) Second frame of APNG is cropped
Categories
(Core :: Graphics: ImageLib, defect)
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)
117.17 KB,
image/png
|
Details | |
1.33 KB,
image/png
|
Details | |
7.52 KB,
patch
|
Details | Diff | Splinter Review | |
3.65 KB,
patch
|
roc
:
review+
vlad
:
review+
dveditz
:
approval1.9.2.14-
dveditz
:
approval1.9.2.17+
|
Details | Diff | Splinter Review |
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•15 years ago
|
||
Comment 2•15 years ago
|
||
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
Comment 3•15 years ago
|
||
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•15 years ago
|
Keywords: regression,
regressionwindow-wanted
Comment 4•15 years ago
|
||
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•15 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.
Comment 6•15 years ago
|
||
(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•15 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
Comment 9•15 years ago
|
||
Within that range, the large patch in bug #753 is the only one that looks to be relevant.
Comment 10•15 years ago
|
||
Could someone please remove "regressionwindow-wanted"
Assignee | ||
Comment 11•15 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•15 years ago
|
||
nsPNGDecoder.cpp : restores 3.5 behavior of FrameUpdated(,rect)
Reporter | ||
Comment 13•15 years ago
|
||
Glenn, what do you think of the patch?
Comment 14•15 years ago
|
||
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•15 years ago
|
Attachment #427310 -
Flags: review?(joe)
Reporter | ||
Comment 15•15 years ago
|
||
Joe, review?
Reporter | ||
Comment 16•15 years ago
|
||
Joe, review?
Assignee | ||
Comment 17•15 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•15 years ago
|
Attachment #427310 -
Flags: review?(joe) → review-
Assignee | ||
Comment 18•15 years ago
|
||
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 19•15 years ago
|
||
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•15 years ago
|
||
Joe, can you test the patch? Tryserver build maybe?
Because it doesn't fix the bug for me.
Assignee | ||
Comment 21•15 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•15 years ago
|
||
Ah, finally figured it out. The important part is that the frames have PREVIOUS or BACKGROUND disposals.
Reporter | ||
Comment 23•15 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•15 years ago
|
||
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•15 years ago
|
||
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•15 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•15 years ago
|
||
I tried your patch, Joe, and it works fine.
Assignee | ||
Comment 28•15 years ago
|
||
Yeah, I tested it this time :)
Attachment #431566 -
Flags: review?(vladimir) → review+
Attachment #431566 -
Flags: review?(roc) → review+
Assignee | ||
Comment 29•15 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
Closed: 15 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 30•15 years ago
|
||
2 months = baked enough?
How about getting this into 3.6.x as well?
Updated•14 years ago
|
Attachment #431566 -
Flags: feedback?(bobbyholley+bmo)
Updated•14 years ago
|
Attachment #431566 -
Flags: approval1.9.2.14?
Comment 31•14 years ago
|
||
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+
Comment 32•14 years ago
|
||
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-
Comment 33•14 years ago
|
||
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•14 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
Comment 35•14 years ago
|
||
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+
Assignee | ||
Comment 36•14 years ago
|
||
status1.9.2:
--- → .16-fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•