Last Comment Bug 695610 - Move nsImageFrame::GetContainer into imgIContainer
: Move nsImageFrame::GetContainer into imgIContainer
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: unspecified
: x86 Mac OS X
-- normal (vote)
: mozilla11
Assigned To: Matt Woodrow (:mattwoodrow)
: Milan Sreckovic [:milan]
Depends on: 705580
Blocks: 700545
  Show dependency treegraph
Reported: 2011-10-18 21:58 PDT by Matt Woodrow (:mattwoodrow)
Modified: 2011-11-27 18:39 PST (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Move GetImageContainer into imgIContainer (17.55 KB, patch)
2011-10-18 21:58 PDT, Matt Woodrow (:mattwoodrow)
roc: feedback+
Details | Diff | Splinter Review
Move GetImageContainer into imgIContainer v2 (17.77 KB, patch)
2011-10-25 21:03 PDT, Matt Woodrow (:mattwoodrow)
joe: review+
Details | Diff | Splinter Review

Description User image Matt Woodrow (:mattwoodrow) 2011-10-18 21:58:05 PDT
Created attachment 567969 [details] [diff] [review]
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: 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 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-18 22:14:01 PDT
Comment on attachment 567969 [details] [diff] [review]
Move GetImageContainer into imgIContainer

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

Makes sense.
Comment 2 User image Matt Woodrow (:mattwoodrow) 2011-10-25 21:03:37 PDT
Created attachment 569604 [details] [diff] [review]
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 User image Joe Drew (not getting mail) 2011-11-03 15:36:24 PDT
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 User image Matt Woodrow (:mattwoodrow) 2011-11-08 19:16:02 PST
Comment 5 User image Marco Bonardo [::mak] 2011-11-09 05:32:38 PST

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

Note You need to log in before you can comment on or make changes to this bug.