We can easily merge imgContainerGIF into imgContainer. The main difference seems to be that imgContainer only maintains one frame, while imgContainerGIF multiple (using an COMarray). By merging the classes into one, we can save one class, and also pave the way for APNG (see bug 257197). (even the animation modes for GIF and APNG are the same). The Init could differentiate between the normal 'one frame' versus the real animation mode, so that only for reajl animated images, a nsCOMArray is used. Currently imgContainer.obj is 8K, and imgContainerGIF.obj is 16K, so merging these will probably save about 8K, not counting the savings elsewhere (in libpr0n/build: imgContainerGIF factory). Analysis notes: Not realy used: GetPreferredAlphaChannelFormat (can we remove this one?) More or less the same: ::Init (GIF version does keep ref to aObserver...) ::GetPreferredAlphaChannelFormat - returns different format, but it seems only used in PNGDecoder?? ::GetWidth, ::GetHeight : exactly the same ::GetCurrentFrame, ::GetNumFrames, ::GetFrameAt, ::AppendFrame, ::Clear -GIF used an array of frames, parent only maintains one frame... ::RemoveFrame(gfxIImageFrame *item) both: return NS_ERROR_NOT_IMPLEMENTED (so let's remove this one?) ::EndFrameDecode(framenum, timeout) parent sets the timeout in the frame, GIF edition just updates currentDecodingFrameIndex ?? Note: gifdecoder calls 'SetTimeout' explicitely before calling EndFrameDecode..., so set it through EndFrameDecode, and remove SetTimeout. ::DecodingComplete GIF: if (#frames==1) SetMutable(FALSE), parent: NOOP ::GetAnimationMode GIF: return gif animation, parent: kDontAnimMode ::SetAnimationMode, ::StartAnimation, ::StopAnimation parent: NOOP, GIF: do the animation stuff... ::GetLoopCount, ::SetLoopCount, ::ResetAnimation parent: NOOP, GIF: do the animation stuff... The related bugs: * bug 257197: Add APNG support (also wants to share the animation code) (this one provides also most of the patch for this one...) * bug 192790: make imgContainerGIF::BuildCompositeMask useless (requires changes to gfx/image, and patch seems to be incomplete and only tested for XP_WIN). Doing the 'move animation from ImgContainerGIF to imgContainer, and remove the imgContainerGIF class' first, before doing APNG is cleaner. After this step then APNG can be separately reviewed and checked in.
heh. note bug 77497
OS: Windows XP → All
About bug 77497: it does not give an actual reason for the split except for 'overload'. However, because animated GIF is heavily part of the Internet, and even used in the mozilla chromes/skins, the imgContainerGIF is always present together with imgContainer. So the split actually duplicated the imgContainer code with no additional benefit. Furthermore, now that APNG also wants to use the animated version, sharing the code seems now a better thing. Also other improvements did reduce the overall codesize of imgContainerGIF: * Bug 148634 - combine imgContainerGIF::ZeroMaskArea & OneMaskArea * Bug 181125 remove nsIEnumerator imgIContainer::enumerate() method * Convert imgContainerGIF to use nsCOMArray instead of nsSupportsArray. Bug 224621 * Bug 148637 Clean-up imgContainerGIF. r=biesi sr=tor * Bug 181829 remove imgIDecoderObserver from imgContainerGIF * Bug 178643: Remove uses of NS_INIT_ISUPPORTS, since it is no longer needed. So the split was good for this cleanup and optimisation. But now it can be merged back into imgContainer.
You talk about code size changes, but what about memory usage changes? As I stated in Bug 257197, wouldn't it be better to create a imgContainerFramed, and have imgContainerGIF and imgContainerAPNG (if needed) inherit from it? I'm just thinking of all the wasted bytes for non multiframed images (jpegs, bmps, etc) Thanks for reminding me about bug 192790, I forgot I had a patch for that!
The memory usage could be solved by either your imgContainerFrame (or -Animated), or by moving all member variables only for the animated stuff into a separately allocated structure, only allocated when needed?
I think this bug can be closed now, the work has been done in Bug 257197
Mark 'fixed' as it is covered by bug 257197. (Note, it is not a duplicate, as bug 257197 is about APNG implementation, which also happens to do this imgContainer merge.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.