Closed Bug 692573 Opened 13 years ago Closed 13 years ago

Incorrect rendering of resized animated gif images

Categories

(Core :: Graphics: ImageLib, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
firefox10 - ---

People

(Reporter: jwir3, Assigned: jwir3)

References

(Blocks 1 open bug, )

Details

(Keywords: regression)

Attachments

(2 files, 3 obsolete files)

Some animated gifs, such as the hyena in: http://www.furaffinity.net/journal/2263796/ appear to be animating incorrectly. This could be related to bug 666446.
See also bug 666446 comment 376.
Summary: Incorrect rendering of some animated gif images → Incorrect rendering of resized animated gif images
So it looks like this is a result of the image frames coming out of order. When I view the image in GIMP, it looks like there is a 0ms time per frame. I wonder if maybe changes that were made in bug 666446 might be causing a frame to be skipped, since the refresh driver is now coalescing paint events. 

The correct display of the image is actually dependent on the ordering of images, as some of the frames aren't reproductions of the whole image, simply sprite animations.
(In reply to Scott Johnson (:jwir3) from comment #2)
> The correct display of the image is actually dependent on the ordering of
> images, as some of the frames aren't reproductions of the whole image,
> simply sprite animations.

I meant 'frames' when I said 'images' in:  '... dependent on the ordering of images...'.
Depends on: 666446
Blocks: 666446
No longer depends on: 666446
Attached patch Patch to fix this bug (obsolete) — Splinter Review
So, it looks like the problem here was that the first frame of the animation wasn't being composited correctly. Instead of being composited against the last frame of the animation after it had looped once, the logic of AdvanceFrame() was detecting that it was the first frame of the animation, and thus not correctly calling DoComposite. I modified this to only skip DoComposite (that is, redraw everything from scratch) only if we're on the first frame and first iteration of the animated image.

This is going to be integrated into bug 666446's patches for landing, but I wanted to get it reviewed on its own before integrating it. As long as things look ok, I'm going to add this to bug 666446 and close this bug.

Note that if you intend to apply this patch, it has as a prerequisite that the patches from bug 666446 be applied first.
Attachment #565990 - Flags: review?(joe)
Attached file Test Case
Posting a more concise test case. This fails after patches for bug 666446 is pushed (visible problems), but looks fine after the patch for this bug is pushed.
Comment on attachment 565990 [details] [diff] [review]
Patch to fix this bug

Review of attachment 565990 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpr0n/src/RasterImage.cpp
@@ +388,5 @@
>    imgFrame *frameToUse = nsnull;
>  
> +  // We need to treat the first frame as special (no compositing), but only if
> +  // it's the first time through the animation. (See bug 692573).
> +  if (nextFrameIndex == 0 && mLoopCount == 0) {

mLoopCount == 0 is true precisely on the *last* frame of an animation, not the first, because we *decrement* mLoopCount in AdvanceFrame.
Attachment #565990 - Flags: review?(joe) → review-
(In reply to Joe Drew (:JOEDREW!) from comment #6)
> mLoopCount == 0 is true precisely on the *last* frame of an animation, not
> the first, because we *decrement* mLoopCount in AdvanceFrame.
(s/frame/loop/)
Hmmm.. yes, you're right of course. It must be that its now compositing EVERY frame, so that's why this patch works. Basically, what it seems to me is that the first frame isn't getting composited correctly. I'll try it again with the correct logic, and see if it's still working.
So, why are we treating the first frame as special anyway? Wouldn't it make sense to call DoComposite() on every frame, regardless of whether it's the first frame of the animation or not? 

I suppose there is probably a performance gain by not calling DoComposite() if we don't have to - i.e. if the dirty region is such that we're going to be repainting the entire area of the image anyway, but in that case it seems like we should be detecting that, rather than this method of trying to detect if we're on the absolute first frame of the animation.

Even for a single-looped image, our performance gains are going to decrease multiplicatively with the number of frames in the image, so it doesn't seem like a large performance gain in that case. 

Or perhaps I'm missing some subtle detail here?
(In reply to Scott Johnson (:jwir3) from comment #9)
> So, why are we treating the first frame as special anyway? Wouldn't it make
> sense to call DoComposite() on every frame, regardless of whether it's the
> first frame of the animation or not?

The first frame of a GIF is always stored in 32-bit colour, so we don't need to do any composition to draw it (except, I guess, in this case!!) It's also a little complex to define the "previous frame" in general for the first frame, especially for the very first time we're drawing it. Also, the first frame has to be refreshed to clear all the changes that have been made while drawing the animation.

But aside from those caveats, I am supportive of removing special cases!

> Even for a single-looped image, our performance gains are going to decrease
> multiplicatively with the number of frames in the image, so it doesn't seem
> like a large performance gain in that case. 

I don't really understand what this means.
(In reply to Joe Drew (:JOEDREW!) from comment #10)
> 
> The first frame of a GIF is always stored in 32-bit colour, so we don't need
> to do any composition to draw it (except, I guess, in this case!!) It's also
> a little complex to define the "previous frame" in general for the first
> frame, especially for the very first time we're drawing it. Also, the first
> frame has to be refreshed to clear all the changes that have been made while
> drawing the animation.
> 
> But aside from those caveats, I am supportive of removing special cases!

Ok, I'll see what I can do to remove this special case. I am optimistic that it can be removed. :)

> I don't really understand what this means.

Yeah, you probably don't need to worry about it. It was actually more me just thinking out loud toward an argument against it being a performance gain. What I was trying to say is that if we have an animated image with 10 frames, that loops 100 times, then the probability that we are able to take advantage of any performance gain by not compositing the _very_ first frame is 1 / 10 * 100 = 1/1000, and that this decreases by a factor of 100 for each frame we add to the animation. I guess it was more just to convince myself that there probably wasn't a performance gain in effect here. :)
Blocks: 694374
Attached patch Patch (v2) (obsolete) — Splinter Review
Attachment #565990 - Attachment is obsolete: true
Attachment #566896 - Flags: review?(joe)
Attachment #566896 - Flags: review?(joe)
After some debugging, I've noticed that RasterImage::AdvanceFrame() isn't actually called prior to painting the first frame of an animated image. That is to say, it appears to only be called when we want to actually ADVANCE to the next frame. It is called, of course, when currentFrame = numFrames -1, and nextFrame = 0, but this is included in the general case where we want to call DoComposite().

Thus, my suggestion is to simply remove the code for the special case (the if statement) and always call DoComposite(), because AFAICT, we never actually use the special case code at all (or, rather, we're using it right now every time we want to paint frame 0, but this is incorrect).

I've been performing some tests on the effects of removing this if statement, and it appears that all of our current unit tests are passing, as well as all of my interactive test cases for bug 666446. I'm running the entire test suite right now, just to verify I didn't miss anything.

Joe originally recommended that we move frameChanged() back into AdvanceFrame(), rather than have it in RequestRefresh(), but I don't especially like this idea for two reasons - 

1) if we are updating our image at a slower pace (e.g. if the tab with animations is in the background, and the refresh driver is throttled back), then we're going to be calling frameChanged() for every frame advancement we perform, even though we really only need to do it for the final resulting frame. 

2) I recall that we originally did this when developing the architecture for bug 666446, and I know there was a reason we choose to go with the frameChanged() being in the requestRefresh(), call, rather than in AdvanceFrame(). I can't remember the exact details, but I know that there is a problem waiting to hit us in the face. (Yeah, this argument doesn't hold as much water as I'd like, but I just have this gut feeling...)

At any rate, I'd like to discuss this and see what interested parties think about it.
Attached patch Patch (v3) (obsolete) — Splinter Review
After speaking with Joe in IRC, we came to the conclusion that it would probably be better if we could just accumulate the dirty rectangle during consecutive AdvanceFrame() calls, and then use this for the dirty rectangle that's passed to FrameChanged() at the end of RequestRefresh(). This method seems to work well, so it's what I'm posting for review.
Attachment #566896 - Attachment is obsolete: true
Attachment #567189 - Flags: review?(joe)
Comment on attachment 567189 [details] [diff] [review]
Patch (v3)

Review of attachment 567189 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpr0n/src/RasterImage.cpp
@@ +416,5 @@
>  
>    // Set currentAnimationFrameIndex at the last possible moment
>    mAnim->currentAnimationFrameIndex = nextFrameIndex;
>    mAnim->currentAnimationFrameTime = aTime;
> +  *aDirtyRect = aDirtyRect->Union(dirtyRectCopy);

I think the Union should be external to AdvanceFrame(). Since AdvanceFrame() only ever advances one frame, there's no reason it should care about external state. RequestRefresh can accumulate the dirty rect.

I think this'll also result in cleaner code.

@@ +471,2 @@
>  #ifdef DEBUG
>    mFramesNotified++;

FWIW, this should be indented correctly in patch 9 in bug 666446.
Attachment #567189 - Flags: review?(joe) → review-
Attached patch Patch (v4)Splinter Review
(In reply to Joe Drew (:JOEDREW!) from comment #15)
> I think the Union should be external to AdvanceFrame(). Since AdvanceFrame()
> only ever advances one frame, there's no reason it should care about
> external state. RequestRefresh can accumulate the dirty rect.
> 
> I think this'll also result in cleaner code.
> 
Agreed. I made that change.

> FWIW, this should be indented correctly in patch 9 in bug 666446.
I corrected this, but the entire patch is going to be integrated into PATCH9 of bug 666446. I'm just pulling it out in this bug so that it's easier to review & you don't have to look through the entire PATCH9 every time there's a simple change to this guy. ;)
Attachment #567189 - Attachment is obsolete: true
Attachment #567309 - Flags: review?(joe)
Comment on attachment 567309 [details] [diff] [review]
Patch (v4)

Review of attachment 567309 [details] [diff] [review]:
-----------------------------------------------------------------

Hooray! \o/

::: modules/libpr0n/src/RasterImage.cpp
@@ -388,5 @@
>    imgFrame *frameToUse = nsnull;
>  
>    if (nextFrameIndex == 0) {
>      frameToUse = nextFrame;
> -    aDirtyRect = &(mAnim->firstFrameRefreshArea);

Oh man - this is something I definitely should have caught in review!

@@ +466,5 @@
>      }
>  
> +    // Notify listeners that our frame has actually changed, but do this only
> +    // once for all frames that we've now passed (if AdvanceFrame() was called
> +    // more than once.

Close parenthesis missing.
Attachment #567309 - Flags: review?(joe) → review+
> Oh man - this is something I definitely should have caught in review!

No worries. I caught it. You can't be expected to catch everything in review. Two sets of eyes are better than one. :)

> Close parenthesis missing.

Fixed.

I've integrated this into PATCH9 for bug 666446. Should I leave this bug open until 666446 lands, or just close it now that it's actually been integrated into bug 666446's patches?
(In reply to Scott Johnson (:jwir3) from comment #18)
> I've integrated this into PATCH9 for bug 666446. Should I leave this bug
> open until 666446 lands, or just close it now that it's actually been
> integrated into bug 666446's patches?

Well, this bug actually doesn't exist on trunk anymore, so this was fixed by the backout, and we won't have a similar bug when we reland bug 666446.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: