Looking aroung in imgContainerGIF.cpp, I saw that there are two ways to set image data in gfxImageFrame(->mImage): * SetImageData with an allocated and filled array of bytes * GetImageData to get pointer to image buffer itself and directly set the bytes. The first needs an allocated buffer, and memcpy's the data from this allocated buffer to the image buffer. So, in the image decoders (such as GIF), instead of using SetImageData(..., allocated array of bytes, ...), we could use GetImageData to get the pointer to the image buffer itself, and directly poke the image data into the image buffer in gfxImageFrame->mImage. This saves allocation of this passed array (mostly for each scanline), and the memcpy in SetImageData (as we are directly poking it). The image buffer pointer (from GetImageData) could kept around during decoding, preventing the need to call S/GetImageData for each decoded row. Note: GetImageData requires the use of LockImagePixels, which is a NOP for all platforms as long as the image is not (yet) optimized, which is true during decoding... So: * start decoding: create imageFrame (width*heigth) * lockimagepixels * buffer = imageFrame->GetImageData * set decoded image data into buffer * done decoding * unlockimagepixels * optimize image (when not animated) In summary, this saves a memcpy for all of the image data (normally 3 bytes * width * height), and allocating of a rowbuffer for each decoded image.

Note that we have a long-standing issue with GetImageData -- it violates COM ownership guidelines and crashes when used from JS, since it doesn't copy...

The proposed optimization is for the image decoders which are very closely related to the imageframe itself. Note that the imgContainerGIF allready using GetImageData. May be we need to shield GetImageData to being called from outside the imagelibrary???

There's only two locations in imgContainerGIF that are using GetImageData, BlackenFrame and CopyFrameImage. Both of these functions are actually image frame related and are technically supposed to be in nsImageXXX.cpp/gfxImageFrame.cpp, (or possibly, nsImageImpl.cpp, which doesn't exist yet - see Bug 182658). I hinted at this in Bug 195223, but unfortunately had to leave the mozilla project before creating a new bug to address the issue. Using GetImageData to modify the image directly has always been a bad idea. It causes some platforms to create a new memory area and ump the image to in on each call (usually because the image is optimized by the OS).

We should avoid using GetImageData. While it provides a small optimization in the decoder, it removes the opportunity for any platform to optimize the data as it comes in. As Paper said, calling Lock() can cause platforms to do way worse memory allocation than allocating a scanline.

Valid concerns, but 'It causes some platforms to create a new memory area and ump the image to in on each call (usually because the image is optimized by the OS)' only happens after optimization which is triggered by SetMutable(PR_FALSE). Looking at the current gfxImageFrame implementations, for all of the platforms, I saw that the platform specific image optimization is only done after 'SetMutable(PR_FALSE)' (so that the image is optimized, and the original bits are removed). So, for all platforms, as long as the image is not optimized 'LockImageData' is a NOP. Note that also SetImageData does a 'LockImageData', sets the data in the image buffer, and then 'UnlockImageData'. This will have a miserable impact if the image is allready optimized (but then it is set 'Mutable(FALSE)', so it should not be changed at all) So, as long as the image is not complete (and not set 'Mutable(FALSE)'), the image data itself is (and should be) available for poking. May be we need to be more explicit in the gfxImageFrame interface: as long as the image in creation, imagedata can be Set and Get safely. As soon as the image is set 'Mutable(FALSE)' (triggering optimization), SetImageData should be disallowed (it is unmutable anyway), and GetImageData should either be disallowed or disapproved (because of the image reconstruction from the system optimized data)... Note, I have a working patch, and especially for the larger animated GIF images it does speed up things (in total about 400MB is transferred from decoder to the image frames for an 4MB animated GIF). Also doing SetImageData for each row of an image has a lot of overhead, compared to getting the pointer to the image data once, and directly setting the image bits. In summary, I would propose to, as long as the image is in creation, writing to the image using GetImageData(pointer) is allowed, but after Mutable(FALSE), it is disallowed/disapproved (it is even questionable, if we want to expose image data at all) Or, may be we need to use 'SetMutable' more to indicate when image data is avalaible for modification. SetMutable(TRUE) ensures that platform independent image data is available for reading and writing, and SetMutable(FALSE) enables to de-allocate the platform independent image data (when an optimized image is available). It is upto the platform specific implementations to determine when and how to maintain its own platform optimized image. This would clear up the confusion around Lock/Unlock and Set/GetImageData. This will also be of benefit for other apps and JS when they want to play with the imagedata itself.

The idea is that calling gfxImageFrame should be the only way to manipulate the image. If a function is poking at the image data, it should be calling gfxImageFrame to do so. If the function is poking at the image data because gfxImageFrame does not have a proper function or the function is slow, then a gfxImageFrame function should be added or modified to do the poking. Your changes sound like they would cut out future optimizations in the nsImageXXX classes. It is foreseeable that one day a nsImageXXX class will be able to take over gfxImageFrame::SetImageData and draw to an optimized image. Switching everything (or almost everything) to GetImageData and poking is the opposite of this goal (as well as breaking the rules of gfxImageFrame being the only one to touch its image data). Also, having many locations poking at the data causes quite a bit of code duplication. For any poking that is over multiple rows, the same code to calculate row padding must be applied. It's not a matter of one simple memcpy for platforms that pad each row with bytes. You'd need a "if nopadding then big memcpy, else for (each row) memcpy a row", and you'd need to duplicate that code in all those places (the same code in each image decoder, for example). Again, as I stated a paragraph below, it would be better to just create a function in gfxImageFrame that handled modifying multiple rows of image data at once. Note sometimes my envisonment of Image/GFX differs from others in this project, so what I say isn't necessarily golden. :)

Indeed, gfxImageFrame must remain the interface to images. Note that current setup where decoders use SetImageData which uses Lock/UnlockImagePixels can get into (performance) trouble if indeed a platform version of nsImage decide to destroy the pixel buffer before completion. So, having an agreement when / how long the pixel buffer must be kept make sense anyway. My proposal is to specify that the pixel buffer must be kept until the decoder has completed the image. nsImage may do any optimization, as long as the pixel buffer is not destroyed until this completion. The completion could be signalled by SetMutable(bool). After completion, SetImageData should then be the official way to set new data in the image, hiding nsImage internal optimized image handling. So, as long as the image is in construction, it makes sense to build up and keep the complete image buffer in (allmost) platform independent format. The platform dep. code can create the platform optimized image as soon as its wants/needs to. The platform dep. code also wants to free the original data, but is only allowed to do so after the completion of the construction. So, what I propose is to allow the use of 'GetImageData' to get the pointer to the image data, only during image creation. nsImage is still allowed to do optimization, but required to keep the original image data until 'SetMutable(bool)' is called (signalling that no more changes will be done). When bool is PR_TRUE, SetImageData is still allowed, when bool is PR_FALSE SetImageData is not allowed. In both cases however, GetImageData is allowed, but not garanteed to be fast (as the pixels may need to be reconstructed...) Users requiring original data (such as XPrint) can ensure that the original image data is provided (regenerated or still around) for getting image data for processing into other formats, using LockImagePixels and UnlockImagePixels. SetImageData keeps its existing function, also using LockImagePixels and UnlockImagePixels as it is currently doing. Note, that GetImageData is allready used in a number of location to get the original data: nsWindowsShellService: WriteBitmap (note: no Lock/Unlock used!) (only reads data) nsSVGImageFrame: ConvertFrame (note: no Lock/Unlock used!) (only reads data) nsSVGLibartBitmapDefault: GetBits (note: no Lock/Unlock used!) nsCanvasRenderingContext2D: DrawImage (Lock/Unlock) (note: ifdef 0) nsWindowsHooks: WriteBitmap (note: no Lock/Unlock used!) (only reads data) In summary, During image creation, as long as 'SetMutable(bool)' is not called, nsImage should be required (and allready does so) to keep the pixel buffer. After SetMutable the pixel buffer may be destroyed at will by nsImage. This allows decoders to directly set the pixels, but still use the gfxImageFrame interface. It also prevents problems in current situation where optimization may destroy the pixel buffer while still the decoder is active, so that the from SetImageData called LockImagePixel will do the nasty image reconstruction and allocation stuff...

I still don't like using GetImageData() here for that. If I had my way no one would ever use it and I'd probably remove it from the tree. I'm not opposed to adding new Set() apis they can speed things up, but I don't want to return the buffer in the way you describe.

Another way would be to indeed move those GetImagedata users (BlackenFrame, CopyImage) to gfxImageFrame, removing the need to directly access image data from imgContainerGIF. Also the mapping from palette image data (8bit) into 24bit can be moved to gfxImageFrame (which also allows other optimisations later). This would also remove the need for the 24bit row buffer in the gif decoder. It is then up to gfxImageFrame (or its 'childs' nsImage*) to decide how to handle 8bit data...

I have working patch, building on the patches for: bug 274391 and bug 280508 (as they reduce the complexity and codesize for the gif decoder overall). This patch moves 'BlackenFrame' and 'BlackenFrameRect' to gfxImageFrame, as well as 'CopyFrameTo' (may be we want to reverse this one into 'CopyFrameFrom' (to change the 'this' object instead of the object passed as argument?) BuildCompositeMask can be left in gifdecoder, until bug 192790 is fixed (moving this one into the platform image handling, as for some platforms this is automatically done (such as Mac)). By passing a 8bit based image row, and setting the image palette from the decoder, we can leave the decision to keep it at 8bits to the ImageFrame code (and/or the platform dependent image handling). Most importantly, this eliminates the need to: * allocate of row buffers (one for image data, one for alpha data) * copy of image data (24bits+alpha) for each row from decoder to imageframe * do row duplication by getting image locks etc, because now it is a directly memcpy in the image data of image frame itself. * BlackenFrameRect: allocation and copying of row buffer: image is directly blackened in its image data... * the need to request interface of the image itself (nsIImage) to call 'imageUpdated', gfxImageFrame can this much easier. So, in all, this improves the performance of large gif and animated gif decoding and image construction. Note, SetBackgroundColor and GetBackgroundColor can be removed because it is not used at all. After this change, the use of GetImageData can be further reduced to only 'readonly' access for example printing or for copying the image data to the clipboard.

Bug 331298 together with Cairo do directly writes images data to the image buffer of Cairo, so proposing to dup this one to bug 331298 *** This bug has been marked as a duplicate of 331298 ***