If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Catch more cases of frame clipping in AGIF/APNG animations

VERIFIED FIXED in mozilla1.9beta3

Status

()

Core
ImageLib
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: Alfred Kayser, Assigned: Alfred Kayser)

Tracking

Trunk
mozilla1.9beta3
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

10 years ago
Created attachment 294782 [details] [diff] [review]
V1: Clean up the clipping in drawFrameTo

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

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

Updated

10 years ago
Attachment #294782 - Flags: review?(pavlov) → review+
(Assignee)

Updated

10 years ago
Attachment #294782 - Flags: superreview?(tor)
Attachment #294782 - Flags: approval1.9?
Assignee: akayser → alfredkayser

Comment 4

10 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?

Updated

10 years ago
Attachment #294782 - Flags: superreview?(tor) → superreview+

Updated

10 years ago
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
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M11
(Assignee)

Updated

9 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.