Closed Bug 695610 Opened 8 years ago Closed 8 years ago

Move nsImageFrame::GetContainer into imgIContainer


(Core :: ImageLib, defect)

Not set





(Reporter: mattwoodrow, Assigned: mattwoodrow)


(Blocks 1 open bug)



(1 file, 1 obsolete file)

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.
Attachment #567969 - Flags: feedback?
Attachment #567969 - Flags: feedback? → feedback?(joe)
Attachment #567969 - Flags: feedback?(roc)
Comment on attachment 567969 [details] [diff] [review]
Move GetImageContainer into imgIContainer

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

Makes sense.
Attachment #567969 - Flags: feedback?(roc) → feedback+
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.
Attachment #567969 - Attachment is obsolete: true
Attachment #567969 - Flags: feedback?(joe)
Attachment #569604 - Flags: review?(joe)
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.
Attachment #569604 - Flags: review?(joe) → review+
Blocks: 700545
Assignee: nobody → matt.woodrow
Whiteboard: [inbound]

PS: you can avoid the [inbound] in the whiteboard if you wish, we don't use it anymore.
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla11
Depends on: 705580
You need to log in before you can comment on or make changes to this bug.