Closed Bug 148637 Opened 23 years ago Closed 22 years ago

Clean-up & Optimization of imgContainerGIF

Categories

(Core :: Graphics: ImageLib, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.4alpha

People

(Reporter: paper, Assigned: paper)

References

Details

(Keywords: perf)

Attachments

(3 files, 10 obsolete files)

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.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: perf
Target Milestone: --- → mozilla1.1beta
Depends on: 77497
Blocks: 86319
Target Milestone: mozilla1.1beta → mozilla1.2alpha
Target Milestone: mozilla1.2alpha → Future
Attached patch What I have (obsolete) — Splinter Review
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.
Attachment #102129 - Flags: needs-work+
Patch smaller now that Bug 152756 is checked in.
Attachment #102129 - Attachment is obsolete: true
Attachment #102660 - Flags: needs-work+
Attached patch Cleanup #1 (obsolete) — Splinter Review
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.
Attached patch Cleanup #1: diff -uw version (obsolete) — Splinter Review
Attachment #107302 - Flags: review?(cbiesinger)
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
Attachment #107302 - Flags: review?(cbiesinger) → review-
Attached patch Cleanup #1 (obsolete) — Splinter Review
- 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! :)
Attachment #107302 - Attachment is obsolete: true
Attached patch Cleanup #1: diff -uw version (obsolete) — Splinter Review
Attachment #107303 - Attachment is obsolete: true
Attachment #107491 - Attachment is obsolete: true
Attached patch Cleanup #1 (obsolete) — Splinter Review
I changed the logic of BuildCompositeMask to exit out of the overlay is beyond the area of the composite.
Attachment #107488 - Attachment is obsolete: true
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
Attached patch Cleanup #1Splinter Review
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 :)
Attachment #107498 - Attachment is obsolete: true
Attachment #107503 - Flags: review?(cbiesinger)
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
Attachment #107503 - Flags: review?(cbiesinger) → review+
Attachment #107503 - Flags: superreview?(tor)
Comment on attachment 107503 [details] [diff] [review] Cleanup #1 sr=tor with warning change
Attachment #107503 - Flags: superreview?(tor) → superreview+
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
Attachment #107503 - Flags: checkin+
Depends on: 185773
Depends on: 185775
Target Milestone: Future → mozilla1.4alpha
Attached patch Optimizations (obsolete) — Splinter Review
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.
Attachment #102660 - Attachment is obsolete: true
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.
Attachment #114132 - Flags: review?(cbiesinger)
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.
Attachment #114132 - Flags: review?(cbiesinger) → review-
Attached patch Optimizations (obsolete) — Splinter Review
Everything except the NS_ENSURE_ARG_POINTER changes, as per discussion on IRC. A bit more documentation too.
Attachment #114132 - Attachment is obsolete: true
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.
Attachment #114411 - Flags: review+
Attached patch Optimizations (obsolete) — Splinter Review
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
Attachment #114411 - Attachment is obsolete: true
Attachment #114850 - Flags: superreview?(tor)
Attachment #114850 - Flags: review?(cbiesinger)
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
Attachment #114850 - Flags: review?(cbiesinger) → review+
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
Attachment #114850 - Flags: superreview?(tor) → superreview+
Attachment #114850 - Attachment is obsolete: true
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
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
*** Bug 177948 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: