822506 - Clean up Image's many ways of accessing frames: CopyFrame(), ExtractFrame(), GetFrame(), GetImageContainer(), and GetCurrentFrameRect()

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

There are too many ways to get access to an Image's frames. None of these are accessible to addons, so it shouldn't be too painful to clean up this interface a bit.

- GetFrame gets you a reference to the frame that the underlying image uses (except for SVG where it's still a copy). This is used in various places for rendering, and since at least for raster images this doesn't copy, this seems like one to keep.

- CopyFrame gives you a copy of the frame. It isn't clear to me that we actually need to copy the frame in every place where this is currently used, but we probably want to tackle that issue in a separate bug. If GetFrame never did any copying, I'd say we should just let callers do the copying themselves if they need a copy, but unfortunately sometimes GetFrame will copy, and we don't want to copy twice. It seems like for now this method could be replaced by an aForceCopy argument to GetFrame.

- ExtractFrame is basically CopyFrame + clipping, but it creates a new imgIContainer to hold the result. This is probably best handled by not actually doing any copying and just returning a wrapper version of the underlying image that draws with the appropriate clipping, especially given that the primary user of ExtractFrame is border-image which will end up using the entire image. I have this already implemented in my media fragments code, but it isn't posted yet.

- GetCurrentFrameRect is basically GetFrame()->GetRect(), but this isn't quite right because there are subtleties with frames of animated images that don't cover the entire image, and as mentioned above GetFrame() always copies for vector images. The only caller of this method is imgStatusTracker. I have a hunch this one is best replaced by Images sending this information in a notification and letting imgStatusTracker keep track of it, especially if we want to handle sending partial rects to OnFrameUpdate.

- GetImageContainer() also seems like it might be a bit redundant, but I don't understand its behavior well enough yet to suggest what to do with it.

Comment 1

[Seth Fowler [:seth] [:s2h]]

ExtractFrame will get cleaned up in bug 826093.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Here's an update on progress here:

Bug 836155 renamed GetCurrentFrameRect to FrameRect. Same analysis still applies. I believe Joe's OMTD work in bug 716140 handles this as suggested in comment 0, so it will end up being removed.

ExtractFrame is still slated for the chopping block in bug 826093.

I checked who still calls GetFrame and CopyFrame. Note that I am only checking for callers on an OS X build; it's very likely that there may be other callers in platform-specific GUI code on other operating systems.

GetFrame has the following callers outside of imagelib:
- nsLayoutUtils::SurfaceFromElement
- nsSVGFEImageElement::Filter

These appear to be performance-critical functions, and their uses of GetFrame do not always copy. (At least, not in a way that would allow them to be replaced by CopyFrame or Draw.) It looks like GetFrame is worth keeping to support this code.

CopyFrame has the following callers outside of imagelib:
- nsClipboard::PasteboardDictFromTransferable
- nsMenuItemIconX::OnStopFrame
- nsCocoaUtils::CreateNSImageFromImageContainer

I don't think these callers need anything that couldn't be just as easily provided by an aForceCopy argument to GetFrame. I'll file a bug soon to add this.

Also, CopyFrame appears to be used to implement imgTools::GetFirstImageFrame in a blatantly incorrect manner. (Doesn't actually grab the first frame of the image.) It seriously boggles my mind that that has gone undetected so far. I'll file a bug about that too.

Comment 3

[Seth Fowler [:seth] [:s2h]]

The process of removing CopyFrame (bug 846132) and ExtractFrame (bug 826093) is now complete. GetCurrentFrameRect() was removed, IIRC, as part of bug 716140. This leaves GetFrame, GetImageContainer, and Draw. I'd still like to eliminate GetFrame, but it seems nontrivial at this point. I'm going to consider this bug fixed; I'll open a new one if it turns out that it's possible to remove GetFrame at some point in the future.