280508 - Merge imgContainerGIF into imgContainer (save code and footprint)

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Alfred Kayser]

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.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

heh. note bug 77497

Comment 2

[Alfred Kayser]

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.

Comment 3

[ArronM (:paper)]

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!

Comment 4

[Alfred Kayser]

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?

Comment 5

[Andrew Smith]

I think this bug can be closed now, the work has been done in Bug 257197

Comment 6

[Alfred Kayser]

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.