Closed
Bug 410111
Opened 17 years ago
Closed 16 years ago
Catch more cases of frame clipping in AGIF/APNG animations
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
VERIFIED
FIXED
mozilla1.9beta3
People
(Reporter: alfredkayser, Assigned: alfredkayser)
Details
Attachments
(1 file)
6.47 KB,
patch
|
pavlov
:
review+
tor
:
superreview+
mtschrep
:
approval1.9+
|
Details | Diff | Splinter Review |
bug 408288 fixed a nasty issue in clipping of animation frames against the 'composition' frame. However, there are more cases to caught to prevent misuse. Also, the third parameter of DrawFrameTo is actually the rectangle of the 'source' frame (the frame being drawn) providing the size of the source, and the offset where it needs to be. The callers: DrawFrameTo(aPrevFrame, mAnim->compositingFrame, prevFrameRect); and DrawFrameTo(aNextFrame, mAnim->compositingFrame, nextFrameRect); This means that: 1. srcRect is same as the aDstRect passed as parameter 2. that 'GetRect' is thus not needed. 3. the parameters of dst.Scale are actually always 1,1. So this call is then not needed. Annotation to the patch: part 1: explain the real use of the third parameter Part 2: use NS_ENSURE_ARG_PARAM part 3: skip one GetRect part 4: move the checks for out of bounds before the if (format) as they apply for all cases part 5: use PR_MIN for code simplicity (and the compiler does smart tricks with PR_MIN). part 6. move the .Translate/.Rectangle before the blend operation (so we can remove .NewPath) part 7. only do .CurrentOperator and .SetOperator(defaultOperator) when really needed part 8. remove the .Scale as it never did anything except for some overhead
Attachment #294782 -
Flags: review?(pavlov)
Comment 1•16 years ago
|
||
this looks right -- can we get a little extra testing on this before we land it?
Comment 2•16 years ago
|
||
Outside of regular animated GIFs and PNGs, are there any specific test images I should try?
Comment 3•16 years ago
|
||
This patch seems to be working OK. I haven't noticed any issues with animated PNGs or GIFs with it. I also looked at some of the other problematic GIFs in some of the other clipping bugs of late and they all appear to be working correctly as well.
Assignee: nobody → akayser
Version: unspecified → Trunk
Updated•16 years ago
|
Attachment #294782 -
Flags: review?(pavlov) → review+
Assignee | ||
Updated•16 years ago
|
Attachment #294782 -
Flags: superreview?(tor)
Attachment #294782 -
Flags: approval1.9?
Updated•16 years ago
|
Assignee: akayser → alfredkayser
Comment 4•16 years ago
|
||
(In reply to comment #3) > This patch seems to be working OK. I haven't noticed any issues with animated > PNGs or GIFs with it. I also looked at some of the other problematic GIFs in > some of the other clipping bugs of late and they all appear to be working > correctly as well. > Can we get those added to a test suite?
Attachment #294782 -
Flags: superreview?(tor) → superreview+
Updated•16 years ago
|
Attachment #294782 -
Flags: approval1.9? → approval1.9+
Updated•16 years ago
|
Keywords: checkin-needed
Comment 5•16 years ago
|
||
Checking in modules/libpr0n/src/imgContainer.cpp; /cvsroot/mozilla/modules/libpr0n/src/imgContainer.cpp,v <-- imgContainer.cpp new revision: 1.65; previous revision: 1.64 done
Status: NEW → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M11
Assignee | ||
Updated•15 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•