imgContainerGIF needs some cleaning up and general optimization. I'll try to create seperate bugs where I can, but a bulk of the changes are "all or none" As per comments in Bug 85595, this bug will contain at a minumum changing the code that is commented to be "essentially UnionRect" to actually use UnionRect. Some of the optimizations which will be included here or in a new bug if I can seperate it: - Timer Creation Clean-up. Timers only need to be created in one place. - Skip building a composite for frames that really don't need it (ie. one where the previous frame is a full frame and it disposed of itself by clearing itself to background) - Reduce the need for mCompositing frame by copying composited image back to mFrames if certain criteria is met - Free memory used by mCompositing frame when no longer in use. - Build a nsRect during first loop (appendframe) to determine what area of the first frame really needs updating.

This is what I have so far. I'm not seeking a r= on it. I'll be splitting it up into smaller patches and shoving them off into seperate bugs. I'm attaching the patch in case anyone wants to play. Plus there's a lot of junk in the patch, like trailing spaces, debug code, and silly comments.

Patch smaller now that Bug 152756 is checked in.

Mostly reformatting. Some minor code changes, the biggest one being replacement of "switch (format)" with a check&exit if alpha layer isn't 1 bit.

Comment on attachment 107303 [details] [diff] [review] Cleanup #1: diff -uw version why did you remove the second part of the BuildCompositeMask comment? - aOverlayFrame->LockAlphaData(); er, can you remove that just like that? -/// Windows and OS/2 have the funky bottom up data storage we need to account for any reason you're removing the comment here? or just because you mentioned it 100 lines above? + nsWeakPtr mObserver; maybe you should add a comemnt to this member as well, esp. what type the observer is? + //! All the frames of the GIF + nsSupportsArray mFrames; maybe mention what types the frames are? (gfxIImageFrame I guess?) rest looks good

Comment on attachment 107302 [details] [diff] [review] Cleanup #1 marking review- to get it off my queue

- aOverlayFrame->LockAlphaData(); Look 10 lines above.. I replaced "NS_FAILED(aOverlayFrame->GetTransparentColor.." with "NS_FAILED(aOverlayFrame->LockAlphaData..". Both fail if there's not Alpha Data. -/// Windows and OS/2 Yeah, mentioning it once per function is enough, I feel. Anymore than that and it gets annoying. The other two changes have been done! :)

I changed the logic of BuildCompositeMask to exit out of the overlay is beyond the area of the composite.

Comment on attachment 107497 [details] [diff] [review] Cleanup #1: diff -uw version const PRUint32 width = PR_MIN(widthOverlay,(widthComposite-overlayXOffset)); this could do with a few more spaces, same for the next line

I forgot for a moment there that I was doing a code cleanup patch, and went for not modifying more lines than I needed. Silly me. Spaced, and split over two lines :)

Comment on attachment 107503 [details] [diff] [review] Cleanup #1 I've also moved Line 41 ("mSize(0,0),") down a line to shut up biesi's compiler warning. I'm leaving the unsigned mismatch compiler warning, because the WIP patch in this bug will is rewriting that function (and two other functions that I did not clean up)

Comment on attachment 107503 [details] [diff] [review] Cleanup #1 r=biesi with the move of mSize initialization

Comment on attachment 107503 [details] [diff] [review] Cleanup #1 sr=tor with warning change

Comment on attachment 107503 [details] [diff] [review] Cleanup #1 Checking in imgContainerGIF.cpp; /cvsroot/mozilla/modules/libpr0n/decoders/gif/imgContainerGIF.cpp,v <-- imgContainerGIF.cpp new revision: 1.7; previous revision: 1.6 done Checking in imgContainerGIF.h; /cvsroot/mozilla/modules/libpr0n/decoders/gif/imgContainerGIF.h,v <-- imgContainerGIF.h new revision: 1.3; previous revision: 1.2 done Leaving Bug open for Optimization part

Rewrite of AppendFrame, Notify, and DoComposite Probably best to just apply the patch and review those 3 functions (and compare it to the pre-patched functions if need be). Trying to read the diff will be a nightmare. Even a -uw is ugly, but I'll attach one if requested.

Comment on attachment 114132 [details] [diff] [review] Optimizations ack, there's a bunch of trailing spaces. Consider them removed. I'll wait until biesi finds something wrong before I post a new patch though.

Comment on attachment 114132 [details] [diff] [review] Optimizations + * (aNextFrame,mCompositingFrame, or mCompositingPrevFrame) nit: space missing after aNextFrame + void CopyFrameImage(gfxIImageFrame *aSrcFrame, gfxIImageFrame *aDstFrame); hm, can this be a static method? as mentioned on irc, please remove NewFrameData, given that it's empty. + // aFrameNum is 1 based, mCurrentDecodingFrameIndex is not. maybe mention what mCurrentDecodingFrameIndex _is_ based on (presumably 0) +void imgContainerGIF::CopyFrameImage(gfxIImageFrame *aSrcFrame, what if an error happens in this function? does the caller not care? more comments later...

Comment on attachment 114132 [details] [diff] [review] Optimizations NS_ASSERTION(item, "imgContainerGIF::AppendFrame: item is null"); if (!item) return NS_ERROR_NULL_POINTER; NS_ENSURE_ARG_POINTER(item); instead of this->some_member_function() just use some_member_function (mostly, maybe only, affects StopAnimation) not sure if I mentioned it already, but |if NS_FAILED(...)| should have an additional pair of (), several times. more tomorrow

Comment on attachment 114132 [details] [diff] [review] Optimizations please also add an assertion in ::DoComposite for aFrameToUse

Comment on attachment 114132 [details] [diff] [review] Optimizations mCompositingFrame = do_CreateInstance("@mozilla.org/gfx/image/frame;2"); if (!mCompositingFrame) return NS_ERROR_OUT_OF_MEMORY; might want to use this isntead: mCompositingFrame = do_CreateInstance("@mozilla.org/gfx/image/frame;2", &rv); if (NS_FAILED(rv)) return rv; nsCOMPtr<nsIInterfaceRequestor> ireq(do_QueryInterface(aDstFrame)); Given that you commented the other two code blocks in this (CopyFrameImage) function, please also add a comment to this one, like: // Tell the image that it's data has been updated (copied from the original code) And it would be a good idea to null-check ireq and img before using them. Marking review- because I'd like to see a new patch with the changes I mentioned.

Everything except the NS_ENSURE_ARG_POINTER changes, as per discussion on IRC. A bit more documentation too.

Comment on attachment 114411 [details] [diff] [review] Optimizations +static void CopyFrameImage(gfxIImageFrame *aSrcFrame, gfxIImageFrame *aDstFrame); um, I really meant a static method on the class... but this is also ok, I guess. + // XXX - This could be done it multiple framechanged calls do you mean "with" instead of "it"? thanks for the patch, looks good.

Changes from last patch: - Changed CopyFrameImage() to return PRBool, and fail if size is different, etc. - The CopyFrameImage that copies a composited frame back into mFrames now checks the results and skips optimization. The other 2 calls to CopyFrameImage won't break on failure, so they don't check the result. - Made CopyFrameImage static as originally intended by biesi's request (I hope) - Fixed that "it" == "with" typo

Comment on attachment 114850 [details] [diff] [review] Optimizations + if (NS_SUCCEEDED(aDstFrame->LockImageData())) { and + if (NS_SUCCEEDED(aDstFrame->LockAlphaData())) { shouldn't you return PR_FALSE if this fails? looks good otherwise, r=biesi

Comment on attachment 114850 [details] [diff] [review] Optimizations Remove the extra prototype of CopyFrameImage in the .cpp, follow biesi's last comment, and attach a patch for future reference. sr=tor

checked in!! Let there be rejoicing and lots of dancing, but please, no more sites with dancing GIFs. :) Checking in imgContainerGIF.h; /cvsroot/mozilla/modules/libpr0n/decoders/gif/imgContainerGIF.h,v <-- imgContainerGIF.h new revision: 1.6; previous revision: 1.5 done Checking in imgContainerGIF.cpp; /cvsroot/mozilla/modules/libpr0n/decoders/gif/imgContainerGIF.cpp,v <-- imgContainerGIF.cpp new revision: 1.13; previous revision: 1.12 done Checking in nsGIFDecoder2.cpp; /cvsroot/mozilla/modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp,v <-- nsGIFDecoder2.cpp new revision: 1.47; previous revision: 1.46 done

*** Bug 177948 has been marked as a duplicate of this bug. ***