695610 - Move nsImageFrame::GetContainer into imgIContainer

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: Move GetImageContainer into imgIContainer

Currently nsImageFrame has a method to return an ImageContainer for the current frame, which we use when putting nsDisplayImages into their own ImageLayers.

Since we can have multiple nsImageFrames for a single imgIContainer, I think this code (and the cached container) would make more sense as part of imgIContainer/RasterImage.

As an example, this testcase: http://www.tapper-ware.net/stable/web.dom.stresstest.transform/ uses the same image 20 times, and we end up using 20 identical textures currently.

This patch lets us share a single texture. Lots of win here.

Still needs a bit of work, I'm really not sure how to handle the CURRENT_FRAME stuff, Invalidating the cached container when the image changes etc. And comments would be nice.

We could also use this as a precursor to teaching imglib about textures, and if there are no other users of this image, freeing the system memory bitmap.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 567969 [details] [diff] [review]
Move GetImageContainer into imgIContainer

Review of attachment 567969 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Move GetImageContainer into imgIContainer v2

Are these the correct checks to delete the cached image container when the current frame data has changed?

I have a few other concerns, though these aren't going to be a change of behaviour from what we currently have.

Does it make sense to only have this method for the current frame? We currently only call the current code with FRAME_CURRENT, but if there's a use case for wanting this for other frames, we could cache the ImageContainer on the actual frames instead and pass a frame index parameter.

Should we be doing a sync decode here? The current code is doing this too (and I wrote it :)) but it feels incorrect. Maybe we should be returning nsnull if the image isn't decoded yet.

Comment 3

[Joe Drew (not getting mail)]

Comment on attachment 569604 [details] [diff] [review]
Move GetImageContainer into imgIContainer v2

Review of attachment 569604 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/public/imgIContainer.idl
@@ +180,5 @@
>                                    in PRUint32 aFlags);
>  
>    /**
> +   * Attempts to create an ImageContainer (and Image) containing the current
> +   * frame. Only valid for RASTER type images.

I don't see any reason why this should be specific to RasterImages. Right now we have no way of being notified on animated VectorImages, but if we added something to Image.h that got called by both VectorImage and RasterImage when they change, we'd be able to move this to Image.

I don't think that needs to happen before this patch lands though.

::: image/src/RasterImage.cpp
@@ +765,5 @@
> +  CairoImage::Data cairoData;
> +  nsRefPtr<gfxASurface> imageSurface;
> +  GetFrame(imgIContainer::FRAME_CURRENT,
> +           imgIContainer::FLAG_SYNC_DECODE,
> +           getter_AddRefs(imageSurface));

This can probably be on fewer lines.

@@ +1050,5 @@
>    imgFrame *frame = GetImgFrameNoDecode(aFrameNum);
>    NS_ABORT_IF_FALSE(frame, "Calling FrameUpdated on frame that doesn't exist!");
>  
>    frame->ImageUpdated(aUpdatedRect);
> +  mImageContainer = NULL;

Add a comment saying "The image has changed, so we need to invalidate our ImageContainer cache" or something like it.

@@ +1142,5 @@
>    // XXX - these should probably be combined when we fix animated image
>    // discarding with bug 500402.
>    mDecoded = true;
>    mHasBeenDecoded = true;
> +  mImageContainer = NULL;

This shouldn't be necessary, I think, since every time the image is changed, FrameUpdated is called.

@@ +1369,5 @@
>    // If we've been called before, ignore. Otherwise, flag that we have everything
>    if (mHasSourceData)
>      return NS_OK;
>    mHasSourceData = true;
> +  mImageContainer = NULL;

This one isn't necessary - SourceDataComplete() isn't necessarily related to decoded status.

::: image/src/RasterImage.h
@@ +510,5 @@
>    // How many times we've decoded this image.
>    // This is currently only used for statistics
>    PRInt32                        mDecodeCount;
>  
> +  nsRefPtr<mozilla::layers::ImageContainer> mImageContainer;

Add a comment saying that this is a cache for GetImageContainer. Also, OH MY GOODNESS is the name ImageContainer very confusing for someone used to imgIContainer.

Comment 4

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/20c6e85f8a59

Comment 5

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/20c6e85f8a59

PS: you can avoid the [inbound] in the whiteboard if you wish, we don't use it anymore.