DrawToImage exists in platform gfx only for GIF compositing, but for various reasons most platforms ended up doing this by hand. Given this and that we'd like to get the GIF decoder self contained, it seems reasonable to move this functionality to imgContainerGIF, were the rest of the GIF animation/compositing logic lives. This patch transplants the gtk varient into imgContainerGIF, removes a now redundant compositing function in imgContainerGIF and all the platform DrawToImage implementations.

I should mention that this patch is currently untested on win32 and os2, the two platforms that need bottom-up image layout.

Aren't you removing the Win and Mac optimizations for DrawToImage by doing this? I always thought the reason for duplicate DrawToImage code in all the different platforms was due to no one making OS specific optimizations, rather than because the OS didn't have any built in image to image functions.

MacOS is the only tier-1 platform that could implement DrawToImage the way it was defined (perhaps unsuprisingly the need for that function was added by a MacOS programmer). Win32 accelerated the opaque case, but I don't think we'll be loosing much there. The reason I said I'd like to get this out of platform gfx is the belief that it will simplify being able to use animated GIFs with SVG in the future.

How far along is SVG, and has it been tested to see if it performs better on Win32? The reason I'm against this bug is because the main reason for the slowness in animated gifs is because the frames are not optimized. Before my "vacation" from Mozilla, I was working on getting the image bit crunching code in imgContainerGIF into nsImage, so that it can be optimized per platform. Once an image is Optimized in Win32, it no longer has bits, just a handle.. so by moving the code to imgContainerGIF, you kill any possibility of optimized GIF frames. My argument is moot if SVG is just around the corner and an excellent performer.

SVG is still some time in the future. No, this patch hasn't even been compiled on win32, yet alone speed tested. :) Do you have a handy benchmark for animated gifs? I'm not sure we want to go down the route of optimizing the frames of a GIF, especially on win32 with its restrictions on GDI handles. There a number of problems with speed and memory usage on large animated gifs, which would probably be better handled by not building overlay frames but just decompressing/compositing on the fly.

Comment on attachment 130066 [details] [diff] [review] remove DrawToImage Sorry, I can't review this. Mac, OS2 and Photon use native API calls in their DrawToImage, which probably offload the work to the graphics card. Windows can also do native calls to if it were set up better. By moving DrawToImage to a platform independent class, you kill any chance of allowing the graphics card to do the work for us.

The original part/patch is now very outdated, but this is still a very valid thing to do. The code now in nsImageThebes.cpp for DrawToImage can very easily be moved to imgContainer.cpp (where also ClearFrame and CopyFrameImage also do the image handling directly). This removes DrawToImage from nsIImage, nsImageThebes, and DrawTo from gfxIImageFrame.idl (Note, the Scale() may be not needed, as the rectangle w/h is always the same as the source w/h, as the Rect passed to is the Rect from the source frame)

Oops, posted the old diff which didn't compile...

Comment on attachment 270572 [details] [diff] [review] Post the right version! Is there a strong reason for using do_GetInterface (CopyFrameImage and DrawFrameTo)?

Yes, actually there is. gfxImageFrame.cpp doesn't implement a nsIImage interface, but provides a GetInterface which returns the nsIImage interface of its mImage member. We could extend the gfxIImageFrame interface to directly provide a 'GetImage' to get rid of this do_GetInterface to just get the image...

Comment on attachment 270572 [details] [diff] [review] Post the right version! +nsresult imgContainer::DrawFrameTo(gfxIImageFrame* aSrc, + gfxIImageFrame* aDst, + nsIntRect &aDstRect) should be: +nsresult imgContainer::DrawFrameTo(gfxIImageFrame *aSrc, + gfxIImageFrame *aDst, + nsIntRect &aDstRect)

argh, that should be: +nsresult imgContainer::DrawFrameTo(gfxIImageFrame *aSrc, + gfxIImageFrame *aDst, + nsIntRect& aDstRect)

Comment on attachment 276605 [details] [diff] [review] V4: Fix spacing for * and &, and use new GUID for the change idl. Who can do the checkin for me? Thanks in advance, Alfred

mozilla/gfx/idl/gfxIImageFrame.idl 1.16 mozilla/gfx/public/nsIImage.h 1.29 mozilla/gfx/src/shared/gfxImageFrame.cpp 1.39 mozilla/gfx/src/thebes/nsThebesImage.cpp 1.49 mozilla/gfx/src/thebes/nsThebesImage.h 1.15 mozilla/modules/libpr0n/src/imgContainer.cpp 1.51 mozilla/modules/libpr0n/src/imgContainer.h 1.26

Thanks! Green tree's, and current nightly (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007082618 Minefield/3.0a8pre) working fine.