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)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9beta3

People

(Reporter: alfredkayser, Assigned: alfredkayser)

Details

Attachments

(1 file)

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)
this looks right -- can we get a little extra testing on this before we land it?
Outside of regular animated GIFs and PNGs, are there any specific test images I should try?
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
Attachment #294782 - Flags: review?(pavlov) → review+
Attachment #294782 - Flags: superreview?(tor)
Attachment #294782 - Flags: approval1.9?
Assignee: akayser → alfredkayser
(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+
Attachment #294782 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: