Move nsImageFrame::GetContainer into imgIContainer

RESOLVED FIXED in mozilla11

Status

()

Core
ImageLib
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Tracking

(Blocks: 1 bug)

unspecified
mozilla11
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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: 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.
Attachment #567969 - Flags: feedback?
(Assignee)

Updated

6 years ago
Attachment #567969 - Flags: feedback? → feedback?(joe)
(Assignee)

Updated

6 years ago
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+
(Assignee)

Comment 2

6 years ago
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.
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+
(Assignee)

Updated

6 years ago
Blocks: 700545
(Assignee)

Comment 4

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/20c6e85f8a59
Assignee: nobody → matt.woodrow
Whiteboard: [inbound]
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.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla11

Updated

6 years ago
Depends on: 705580
You need to log in before you can comment on or make changes to this bug.