Where multiple layers have the same masks, it would be nice if they could share the masks, rather than have to redraw the mask layer. This would save time in drawing and memory. Especially on pages with many small, identical masks (e.g., Twitter). Ownership might be an issue, we assume a layer owns its mask layer at the moment. It would be nice to share mask layers between objects with similar masks, e.g., the same mask, but at a different location relative to the masked layer. This would be a lot more work because it would change the spatial relationship between a layer and its mask.

Or (probably easier) share the mask surfaces rather than the layers

Sharing the mask Images, instead of the mask itself, would be easy in terms of the ownership model, since Images are refcounted. If we record the mask images in FrameLayerBuilder during a particular layer transaction, we can't really lose.

We should check the shadow layers stuff - if we are sharing the mask images, we don't want to copy them to the compositor process multiple times. I think this will require some work. If we share the mask layer, then this ought to come out in the wash, but it would cause lots of other problems (transforms, in particular).

Completely untested. Share ImageContainer for mask layers if possible. Masks are rare, and expensive to draw, so walking the linear list of layers should be acceptable here.

Comment on attachment 630409 [details] [diff] [review] patch 1: cache for mask images Review of attachment 630409 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/Makefile.in @@ +62,5 @@ > $(NULL) > > CPPSRCS = \ > FrameLayerBuilder.cpp \ > + MaskLayerImageCache.cpp \ Fix indent to match the rest ::: layout/base/MaskLayerImageCache.h @@ +20,5 @@ > + * count for the image container (which does not include the key itself), the > + * count is mostly maintained by the mask layer user data (which also keeps a > + * reference to the key). When the key's reference count is zero, the cache > + * may remove the entry, which deletes the key (the key is 'new'ed by the client), > + * which (implicitly) deletes the image container. How about this? * The cache stores MaskLayerImageEntries indexed by MaskLayerImageKeys. * Each MaskLayerImageEntry owns a heap-allocated MaskLayerImageKey * (heap-allocated so that a mask layer's userdata can keep a pointer to the * key for its image in spite of the hashtable moving its entries around). * The key consists of the rounded rects used to create the mask, * an nsRefPtr to the ImageContainer containing the image, and a count * of the number of layers currently using this ImageContainer. * When the key's layer count is zero, the cache * may remove the entry, which deletes the key object. Usually it's best to mention concrete type names wherever possible. It's also important to be clear that although keys contain a reference count, they are not reference-counted themselves in the normal way. @@ +156,5 @@ > + return mRoundedClipRects == aOther.mRoundedClipRects; > + } > + > + mutable nsRefPtr<mozilla::layers::ImageContainer> mContainer; > + mutable PRInt32 mRefCount; Why all the "mutable"? Which methods need to be const that make these need to be mutable? Instead of mRefCount, which could be confusing, I'd call this mLayerCount and be explicit that this is the number of mask layers currently using mContainer as their image container. @@ +172,5 @@ > + return hash; > + } > + > + mutable bool mHashIsInitialized; > + mutable PLDHashNumber mHash; Are you sure caching these is worthwhile? I'm skeptical. Especially because nsTHashtable/pldhash already caches the hash value in entries!!! @@ +237,5 @@ > + return KeyEquals(aOther.mKey); > + } > + > + // An owning reference > + KeyTypePointer mKey; could be nsAutoPtr<MaskLayerImageKey>

3rd time lucky

OK, maybe forth time lucky

Comment on attachment 630462 [details] [diff] [review] patch 1: cache for mask images Review of attachment 630462 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/MaskLayerImageCache.h @@ +27,5 @@ > +class MaskLayerImageCache > +{ > +public: > + > + MaskLayerImageCache() Unnecessary blank line @@ +33,5 @@ > + mMaskImageContainers.Init(); > + } > + > + /** > + * Representation of a rounded rectangle in pixel coordinates, in device pixel coordinates @@ +35,5 @@ > + > + /** > + * Representation of a rounded rectangle in pixel coordinates, in > + * contrast to FrameLayerBuilder::Clip::RoundedRect, which uses device > + * units. RoundedRect uses appunits. @@ +37,5 @@ > + * Representation of a rounded rectangle in pixel coordinates, in > + * contrast to FrameLayerBuilder::Clip::RoundedRect, which uses device > + * units. > + * In particular, our internal representation uses a gfxRect, rather than > + * an nsRect, so this class is easier to use with transforms. Actually you use an nsIntRect. A gfxRect would make more sense to me. @@ +41,5 @@ > + * an nsRect, so this class is easier to use with transforms. > + */ > + struct PixelRoundedRect > + { > + PixelRoundedRect(FrameLayerBuilder::Clip::RoundedRect aRRect, PRInt32 A2D) make aRRect a const reference. Pass nsPresContext* instead of A2D @@ +45,5 @@ > + PixelRoundedRect(FrameLayerBuilder::Clip::RoundedRect aRRect, PRInt32 A2D) > + : mRect(aRRect.mRect.x/A2D, > + aRRect.mRect.y/A2D, > + aRRect.mRect.width/A2D, > + aRRect.mRect.height/A2D) Use aPresContext->AppUnitsToGfxUnits @@ +48,5 @@ > + aRRect.mRect.width/A2D, > + aRRect.mRect.height/A2D) > + { > + NS_FOR_CSS_HALF_CORNERS(corner) { > + mRadii[corner] = aRRect.mRadii[corner] / A2D; Use AppUnitsToGfxUnits here too @@ +63,5 @@ > + > + mRect.x = mRect.x * aTransform.xx + aTransform.x0; > + mRect.y = mRect.y * aTransform.yy + aTransform.y0; > + mRect.width *= aTransform.xx; > + mRect.height *= aTransform.yy; Just call gfxMatrix::Transform(const gfxRect&)? @@ +65,5 @@ > + mRect.y = mRect.y * aTransform.yy + aTransform.y0; > + mRect.width *= aTransform.xx; > + mRect.height *= aTransform.yy; > + > + for (int i = 0; i < 4; i+=2) { i += 2 @@ +67,5 @@ > + mRect.height *= aTransform.yy; > + > + for (int i = 0; i < 4; i+=2) { > + mRadii[i] *= aTransform.xx; > + mRadii[i+1] *= aTransform.yy; i + 1 You're not transforming all the radii! use i < ArrayLength(mRadii) @@ +93,5 @@ > + { > + PLDHashNumber hash = HashGeneric(mRect.x, > + mRect.y, > + mRect.width, > + mRect.height); I don't think this is a very good hash function. It seems to me that it hashes gfxFloat by implicitly converting it to a uint32_t, which will truncate. Maybe just hash the mRect memory using HashKnownLength? @@ +104,5 @@ > + } > + > + nsIntRect mRect; > + // Indices into mRadii are the NS_CORNER_* constants in nsStyleConsts.h > + PRInt32 mRadii[8]; Seems to me mRect should be a gfxRect and the mRadii should be float or gfxFloat. @@ +110,5 @@ > + > + /** > + * A key to identify cached image containers. > + * The const-ness of this class is with respect to its use as a key into a > + * hashtable, so anything not used to create the hash is mutable. I suspect using const this way is overkill here. If you don't use const, things could be simpler. @@ +112,5 @@ > + * A key to identify cached image containers. > + * The const-ness of this class is with respect to its use as a key into a > + * hashtable, so anything not used to create the hash is mutable. > + * The mLayerCount is a ref count for the image container, and is maintained by > + * MaskLayerUserData, which keeps a reference to the key. There will usually be It's confusing to say that mLayerCount is a "ref count for the image container". The ImageContainer already has its own refcount. Just say the invariant directly: mLayerCount is the number of mask layers using this ImageContainer. @@ +114,5 @@ > + * hashtable, so anything not used to create the hash is mutable. > + * The mLayerCount is a ref count for the image container, and is maintained by > + * MaskLayerUserData, which keeps a reference to the key. There will usually be > + * ref count + 1 references to a key object (the +1 being from the hashtable > + * entry), but this invariant may be temporarily broken. say "pointers to a key object" @@ +119,5 @@ > + */ > + class MaskLayerImageKey > + { > + public: > + MaskLayerImageKey(nsTArray<PixelRoundedRect> aRoundedClipRects) const TArray& @@ +129,5 @@ > + { > + if (mLayerCount < 0) { > + mLayerCount = 1; > + return; > + } This is weird. I don't think we should allow negative values for mLayerCount. Can't we have Sweep simply delete all entries with mLayerCount 0, and never allow negative mLayerCount? @@ +151,5 @@ > + { > + return mRoundedClipRects == aOther.mRoundedClipRects; > + } > + > + mutable nsRefPtr<mozilla::layers::ImageContainer> mContainer; Hmm, can we put mContainer in the MaskLayerImageEntry? I'm guessing mask layers don't need to access it from here. Then it won't need to be mutable. @@ +161,5 @@ > + // Find an image container for aKey, returns nsnull if there is no suitable > + // cached image. If there is an image, then it returns the key for that > + // image, which includes a refernce to the image container itself. > + // The returned key will not be aKey > + const MaskLayerImageKey* FindImageFor(const MaskLayerImageKey* aKey); const MaskLayerImageKey& aKey You could return an ImageContainer* and have a MaskLayerImageKey** out parameter

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13) > Comment on attachment 630462 [details] [diff] [review] > patch 1: cache for mask images > > Review of attachment 630462 [details] [diff] [review]: > ----------------------------------------------------------------- > > @@ +37,5 @@ > > + * Representation of a rounded rectangle in pixel coordinates, in > > + * contrast to FrameLayerBuilder::Clip::RoundedRect, which uses device > > + * units. > > + * In particular, our internal representation uses a gfxRect, rather than > > + * an nsRect, so this class is easier to use with transforms. > > Actually you use an nsIntRect. A gfxRect would make more sense to me. > > @@ +63,5 @@ > > + > > + mRect.x = mRect.x * aTransform.xx + aTransform.x0; > > + mRect.y = mRect.y * aTransform.yy + aTransform.y0; > > + mRect.width *= aTransform.xx; > > + mRect.height *= aTransform.yy; > > Just call gfxMatrix::Transform(const gfxRect&)? > > @@ +93,5 @@ > > + { > > + PLDHashNumber hash = HashGeneric(mRect.x, > > + mRect.y, > > + mRect.width, > > + mRect.height); > > I don't think this is a very good hash function. It seems to me that it > hashes gfxFloat by implicitly converting it to a uint32_t, which will > truncate. > > Maybe just hash the mRect memory using HashKnownLength? > > @@ +104,5 @@ > > + } > > + > > + nsIntRect mRect; > > + // Indices into mRadii are the NS_CORNER_* constants in nsStyleConsts.h > > + PRInt32 mRadii[8]; > > Seems to me mRect should be a gfxRect and the mRadii should be float or > gfxFloat. > For all the above my idea was to use ints in this representation of the rounded rect, then the hash function can be good and fast, but thinking about it, this loses precision for the radii and the transform should be done pre-conversion to int, so maybe just using floats is better. > @@ +110,5 @@ > > + > > + /** > > + * A key to identify cached image containers. > > + * The const-ness of this class is with respect to its use as a key into a > > + * hashtable, so anything not used to create the hash is mutable. > > I suspect using const this way is overkill here. If you don't use const, > things could be simpler. > The trouble is that the hashtable needs a const pointer, so I keep anything that affects the hash const. > > @@ +129,5 @@ > > + { > > + if (mLayerCount < 0) { > > + mLayerCount = 1; > > + return; > > + } > > This is weird. > > I don't think we should allow negative values for mLayerCount. > > Can't we have Sweep simply delete all entries with mLayerCount 0, and never > allow negative mLayerCount? > We can do this, but by keeping the mask layers around for an extra cycle we get some temporal caching, which seems to get used quite often, it seems like a small price in weirdness for a decent saving. > @@ +151,5 @@ > > + { > > + return mRoundedClipRects == aOther.mRoundedClipRects; > > + } > > + > > + mutable nsRefPtr<mozilla::layers::ImageContainer> mContainer; > > Hmm, can we put mContainer in the MaskLayerImageEntry? I'm guessing mask > layers don't need to access it from here. Then it won't need to be mutable. > I wanted to keep the reference with the ref count, keeping them in separate places feels wrong, and the ref count has to be in the key because that is the non-moving part. > @@ +161,5 @@ > > + // Find an image container for aKey, returns nsnull if there is no suitable > > + // cached image. If there is an image, then it returns the key for that > > + // image, which includes a refernce to the image container itself. > > + // The returned key will not be aKey > > + const MaskLayerImageKey* FindImageFor(const MaskLayerImageKey* aKey); > > const MaskLayerImageKey& aKey > > You could return an ImageContainer* and have a MaskLayerImageKey** out > parameter I did something similar before, but it means that FindImageFor has to delete aKey if aKey is in the hashtable, and that seemed pretty yuck. I guess the caller could do some dance to keep track, but that didn't seem very nice either.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=db33e7c2d883 (OS X is broken, I think I have fixed the problem already, doing another push, otherwise all good)

(In reply to Nick Cameron [:nrc] from comment #14) > For all the above my idea was to use ints in this representation of the > rounded rect, then the hash function can be good and fast, but thinking > about it, this loses precision for the radii and the transform should be > done pre-conversion to int, so maybe just using floats is better. Please do :-) > The trouble is that the hashtable needs a const pointer, so I keep anything > that affects the hash const. OK > > Can't we have Sweep simply delete all entries with mLayerCount 0, and never > > allow negative mLayerCount? > > > We can do this, but by keeping the mask layers around for an extra cycle we > get some temporal caching, which seems to get used quite often, it seems > like a small price in weirdness for a decent saving. Let's do the simple thing first. If we need something more elaborate, let's add it as a separate patch. I don't understand what this extra temporal caching buys us though. Can you give a detailed example of when this extra cycle matters? > > Hmm, can we put mContainer in the MaskLayerImageEntry? I'm guessing mask > > layers don't need to access it from here. Then it won't need to be mutable. > > I wanted to keep the reference with the ref count, keeping them in separate > places feels wrong, and the ref count has to be in the key because that is > the non-moving part. I think it should be in the entry. That also makes the key smaller to construct for the lookup. Really, the ImageContainer is not part of the key and the only reason you're putting the refcount in the key is as an optimization so we can easily adjust the refcount from the layer.

> > > Can't we have Sweep simply delete all entries with mLayerCount 0, and never > > > allow negative mLayerCount? > > > > > We can do this, but by keeping the mask layers around for an extra cycle we > > get some temporal caching, which seems to get used quite often, it seems > > like a small price in weirdness for a decent saving. > > Let's do the simple thing first. If we need something more elaborate, let's > add it as a separate patch. > > I don't understand what this extra temporal caching buys us though. Can you > give a detailed example of when this extra cycle matters? It seems that in some cases only the transform for a layer changes (for e.g., I think, when you start playing a video, I think this is because the layer tree is re-organised or something, so the transform is relative to a new container) which means we can't re-use the mask layer, but if we have a cache of the image, we can re-use that. I don't know how much actual time this saves us, but it seems to happen fairly often.

And try push for OS X: https://tbpl.mozilla.org/?tree=Try&rev=5a41f831fa1b

(In reply to Nick Cameron [:nrc] from comment #17) > It seems that in some cases only the transform for a layer changes (for > e.g., I think, when you start playing a video, I think this is because the > layer tree is re-organised or something, so the transform is relative to a > new container) which means we can't re-use the mask layer, but if we have a > cache of the image, we can re-use that. I don't know how much actual time > this saves us, but it seems to happen fairly often. I still don't get it. If the image was used in the previous tree, and it's used again in the new tree (somewhere else), the reference count may go to zero during the construction of the new tree but it will be at least one again after we've finished constructing the new tree. So if we sweep out zero-refcount entries after constructing the new tree, we should be fine. Right?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away June 9-19) from comment #22) > (In reply to Nick Cameron [:nrc] from comment #17) > > It seems that in some cases only the transform for a layer changes (for > > e.g., I think, when you start playing a video, I think this is because the > > layer tree is re-organised or something, so the transform is relative to a > > new container) which means we can't re-use the mask layer, but if we have a > > cache of the image, we can re-use that. I don't know how much actual time > > this saves us, but it seems to happen fairly often. > > I still don't get it. > > If the image was used in the previous tree, and it's used again in the new > tree (somewhere else), the reference count may go to zero during the > construction of the new tree but it will be at least one again after we've > finished constructing the new tree. So if we sweep out zero-refcount entries > after constructing the new tree, we should be fine. Right? At the moment I Sweep at the end of a transaction, I guess I had assumed the previous layer tree is dead then, I call after RemoveThebesItemsForLayerSubtree(root); But I could move the sweep call earlier and then we ought to be fine. I removed the -1 ref count code anyway.

Comment on attachment 631174 [details] [diff] [review] patch 1: cache for mask images Review of attachment 631174 [details] [diff] [review]: ----------------------------------------------------------------- r+ if you do all those. ::: layout/base/MaskLayerImageCache.h @@ +88,5 @@ > + // Create a hash for this object. > + PLDHashNumber Hash() const > + { > + PLDHashNumber hash = HashBytes(&mRect.x, 4*sizeof(gfxFloat)); > + AddToHash(hash, HashBytes(mRadii, 8*sizeof(gfxFloat))); Why not just HashBytes(this, sizeof(*this))? @@ +153,5 @@ > + // Add an image container with a key to the cache > + // The image container used will be set as the container in aKey and aKey > + // itself will be linked from this cache > + void PutImage(const MaskLayerImageKey* aKey, > + mozilla::layers::ImageContainer* aContainer); Add typedef mozilla::layers::ImageContainer ImageContainer; at the top of the class so we don't have to prefix here. @@ +157,5 @@ > + mozilla::layers::ImageContainer* aContainer); > + > + // Sweep the cache for old image containers that can be deleted > + // Keeps entries alive for one generation after they are otherwise not required > + // to give some measure of temporal caching Is this comment still true? I'd just remove it.

Comment on attachment 630410 [details] [diff] [review] patch 2: user data Review of attachment 630410 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/FrameLayerBuilder.cpp @@ +521,5 @@ > + mScaleX == aOther.mScaleX && > + mScaleY == aOther.mScaleY; > + } > + > + const MaskLayerImageCache::MaskLayerImageKey* mImageKey; Can we use nsRefPtr here? @@ +526,4 @@ > // properties of the mask layer; the mask layer may be re-used if these > // remain unchanged. > nsTArray<FrameLayerBuilder::Clip::RoundedRect> mRoundedClipRects; > + float mScaleX, mScaleY; document what these mean

Comment on attachment 631175 [details] [diff] [review] patch 3: changes to FrameLayerBuilder Review of attachment 631175 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/FrameLayerBuilder.cpp @@ +60,5 @@ > > namespace { > > +// a global cache of image containers used for mask layers > +static MaskLayerImageCache gMaskLayerImageCache; We can't add global constructors and destructors. This will need to be a pointer to a heap-allocated object that we create on demand and destroy via nsLayoutStatics::Shutdown. @@ +930,5 @@ > mRecycledMaskImageLayers.Remove(aLayer); > + // XXX if we use clip on mask layers, null it out here > + > + // we might change the image container for the recycled mask layer, so we > + // need to decrement it's entry's ref count, we'll increment it, if we reuse its @@ +2856,5 @@ > + void > + ContainerState::SetupMaskLayer(Layer *aLayer, const FrameLayerBuilder::Clip& aClip, > + PRUint32 aRoundedRectClipCount) > + { > + #ifdef MOZ_ENABLE_MASK_LAYERS I thought this #ifdef was gone @@ +2886,5 @@ > > + // calculate a more precise bounding rect > + const PRInt32 A2D = mContainerFrame->PresContext()->AppUnitsPerDevPixel(); > + boundingRect = CalculateBounds(newData.mRoundedClipRects, A2D); > + boundingRect.ScaleRoundIn(mParameters.mXScale, mParameters.mYScale); Shouldn't boundingRect be a gfxRect here? @@ +2894,5 @@ > nsIntSize surfaceSize(NS_MIN<PRInt32>(boundingRect.Width(), maxSize), > NS_MIN<PRInt32>(boundingRect.Height(), maxSize)); > > + // maskTransform is applied to the clip when it is painted into the mask (as a > + // component of imageTransform), and it's inverse used when the mask is used for its @@ +2910,4 @@ > > + // copy and transform the rounded rects > + nsTArray<MaskLayerImageCache::PixelRoundedRect> roundedRects; > + roundedRects.SetCapacity(newData.mRoundedClipRects.Length()); This SetCapacity is really unnecessary, just remove it @@ +2919,4 @@ > } > > + // check to see if we can reuse a mask image > + const MaskLayerImageCache::MaskLayerImageKey* key = nsAutoPtr @@ +2928,4 @@ > > + if (container) { > + // track the returned key for the mask image > + delete key; Don't need this delete with nsAutoPtr @@ +2947,4 @@ > > + // useful for debugging, make masked areas semi-opaque > + //context->SetColor(gfxRGBA(0, 0, 0, 0.3)); > + //context->Paint(); remove commented-out code @@ +2964,5 @@ > + data.mSize = surfaceSize; > + static_cast<CairoImage*>(image.get())->SetData(data); > + container->SetCurrentImage(image); > + > + gMaskLayerImageCache.PutImage(key, container); key.forget()

One more thing: I think we should have a gfxPlatform method to choose the mask surface format, so that when we know all GPU layer backends are disabled we can keep choosing A8 format for higher performance.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away June 9-19) from comment #24) > > ::: layout/base/MaskLayerImageCache.h > @@ +88,5 @@ > > + // Create a hash for this object. > > + PLDHashNumber Hash() const > > + { > > + PLDHashNumber hash = HashBytes(&mRect.x, 4*sizeof(gfxFloat)); > > + AddToHash(hash, HashBytes(mRadii, 8*sizeof(gfxFloat))); > > Why not just HashBytes(this, sizeof(*this))? > I was worried somebody might one day add a virtual method to gfxRect and I'd end up hasing the vtblptr too, or someone adds a field to this struct, which is less likely. Should I worry about these things or change to hashing this?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away June 9-19) from comment #26) > Comment on attachment 631175 [details] [diff] [review] > patch 3: changes to FrameLayerBuilder > > Review of attachment 631175 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/base/FrameLayerBuilder.cpp > @@ +60,5 @@ > > > > namespace { > > > > +// a global cache of image containers used for mask layers > > +static MaskLayerImageCache gMaskLayerImageCache; > > We can't add global constructors and destructors. This will need to be a > pointer to a heap-allocated object that we create on demand and destroy via > nsLayoutStatics::Shutdown. > I fixed this in this patch, but do the tidying up in the next patch because I already have shutdown stuff there > @@ +2964,5 @@ > > + data.mSize = surfaceSize; > > + static_cast<CairoImage*>(image.get())->SetData(data); > > + container->SetCurrentImage(image); > > + > > + gMaskLayerImageCache.PutImage(key, container); > > key.forget() I think this should still be just key, and the next (and final) use of key is key.forget()

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away June 9-19) from comment #25) > Comment on attachment 630410 [details] [diff] [review] > patch 2: user data > > Review of attachment 630410 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/base/FrameLayerBuilder.cpp > @@ +521,5 @@ > > + mScaleX == aOther.mScaleX && > > + mScaleY == aOther.mScaleY; > > + } > > + > > + const MaskLayerImageCache::MaskLayerImageKey* mImageKey; > > Can we use nsRefPtr here? > I think we can, but thought I'd run it by you first: we make MaskLayerImageKey a ref counted class, each user data and the entry hold owning refs (I think this will be OK with the entry being memmoved around, the key won't get AddRefed/released), when we sweep, we check for any keys with refcount == 1 and remove their entries, which will give ref count == 0, and they will die. This makes for a slightly less intuitive ref graph, but less tricky code in the user data. Do you think this sounds right?

added a delete to the shutdown hook to tidy up the global cache, as requested in comment for patch 3

(In reply to Nick Cameron [:nrc] from comment #32) > > ::: layout/base/FrameLayerBuilder.cpp > > @@ +521,5 @@ > > > + mScaleX == aOther.mScaleX && > > > + mScaleY == aOther.mScaleY; > > > + } > > > + > > > + const MaskLayerImageCache::MaskLayerImageKey* mImageKey; > > > > Can we use nsRefPtr here? > > I think we can, but thought I'd run it by you first: we make > MaskLayerImageKey a ref counted class, each user data and the entry hold > owning refs (I think this will be OK with the entry being memmoved around, > the key won't get AddRefed/released), when we sweep, we check for any keys > with refcount == 1 and remove their entries, which will give ref count == 0, > and they will die. This makes for a slightly less intuitive ref graph, but > less tricky code in the user data. Do you think this sounds right? I thought we could just use nsRefPtr there without changing anything else. It just means we get automatic calls to AddRef/Release. Nothing in nsRefPtr forces the object to be deallocated by the last Release. (PS, I just found out that you're on the FOOL 2012 committee!)

Comment on attachment 633002 [details] [diff] [review] patch 3: changes to FrameLayerBuilder Review of attachment 633002 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/FrameLayerBuilder.cpp @@ +2892,5 @@ > + > + // calculate a more precise bounding rect > + const PRInt32 A2D = mContainerFrame->PresContext()->AppUnitsPerDevPixel(); > + gfxRect boundingRect = CalculateBounds(newData.mRoundedClipRects, A2D); > + boundingRect.ScaleRoundIn(mParameters.mXScale, mParameters.mYScale); Why are you rounding here? That seems wrong. Can we not round? @@ +2924,5 @@ > + } > + > + // check to see if we can reuse a mask image > + nsAutoPtr<const MaskLayerImageCache::MaskLayerImageKey> key = > + new MaskLayerImageCache::MaskLayerImageKey(roundedRects); Actually, it's confusing to have nsAutoPtr and nsRefPtr both used for the same class. How about we just do everything manually here so we don't have that confusion. Sorry.

Comment on attachment 633033 [details] [diff] [review] patch 4: add shutdown hook and remove MaskImageFormat() Review of attachment 633033 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/FrameLayerBuilder.cpp @@ +600,5 @@ > +/* static */ void > +FrameLayerBuilder::Shutdown() > +{ > + if (gMaskLayerImageCache) { > + gMaskLayerImageCache->Sweep(); Why do we need to sweep? Shouldn't destroying the cache free everything? @@ +601,5 @@ > +FrameLayerBuilder::Shutdown() > +{ > + if (gMaskLayerImageCache) { > + gMaskLayerImageCache->Sweep(); > + delete gMaskLayerImageCache; set gMaskImageLayerCache to nsnull as well.

Comment on attachment 632996 [details] [diff] [review] patch 5: image format via gfxPlatform Review of attachment 632996 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxPlatform.cpp @@ +1490,5 @@ > + nsCOMPtr<nsIXULRuntime> xr = do_GetService("@mozilla.org/xre/runtime;1"); > + if (xr) { > + xr->GetInSafeMode(&safeMode); > + } > + disabled = disabled || safeMode; Don't duplicate code! Factor the stuff you need out of nsBaseWidget::GetShouldAccelerate. Maybe nsBaseWidget::IsLayerAccelerationDisabled.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away June 9-19) from comment #34) > (In reply to Nick Cameron [:nrc] from comment #32) > > > ::: layout/base/FrameLayerBuilder.cpp > > > @@ +521,5 @@ > > > > + mScaleX == aOther.mScaleX && > > > > + mScaleY == aOther.mScaleY; > > > > + } > > > > + > > > > + const MaskLayerImageCache::MaskLayerImageKey* mImageKey; > > > > > > Can we use nsRefPtr here? > > > > I think we can, but thought I'd run it by you first: we make > > MaskLayerImageKey a ref counted class, each user data and the entry hold > > owning refs (I think this will be OK with the entry being memmoved around, > > the key won't get AddRefed/released), when we sweep, we check for any keys > > with refcount == 1 and remove their entries, which will give ref count == 0, > > and they will die. This makes for a slightly less intuitive ref graph, but > > less tricky code in the user data. Do you think this sounds right? > > I thought we could just use nsRefPtr there without changing anything else. > It just means we get automatic calls to AddRef/Release. Nothing in nsRefPtr > forces the object to be deallocated by the last Release. > > (PS, I just found out that you're on the FOOL 2012 committee!) The AddReF/Release are my methods for the mask count, not the standard ref count ones, can I still just use nsRefPtr without implementing anything else? (Yes, it is quite exciting, I like FOOL a lot, and hopefully should be a lot less work than ECOOP)

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away June 9-19) from comment #36) > Comment on attachment 633033 [details] [diff] [review] > patch 4: add shutdown hook and remove MaskImageFormat() > > Review of attachment 633033 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/base/FrameLayerBuilder.cpp > @@ +600,5 @@ > > +/* static */ void > > +FrameLayerBuilder::Shutdown() > > +{ > > + if (gMaskLayerImageCache) { > > + gMaskLayerImageCache->Sweep(); > > Why do we need to sweep? Shouldn't destroying the cache free everything? Yes, I guess we don't have to. I was getting errors before because the DX9 textures were being freed before the cache was cleared, but if we destroy it here rather than later, then I don't need to sweep.

Made roc's changes, carrying r=roc

After a discussion with mattwoodrow on irc, I couldn't work out a nice way to get the widget, and I worry it might change anyway. Matt convinced me that looking at the retained layer manager was enough to get a consistent answer as to which image format to use. But sometimes it will be null, this should be rare enough that it is not important to cache the mask layers, so I only use the cache if there is a retained layer manager. No bugs so far from local testing.

Comment on attachment 633457 [details] [diff] [review] patch 3: changes to FrameLayerBuilder Review of attachment 633457 [details] [diff] [review]: ----------------------------------------------------------------- Fix the comment. ::: layout/base/FrameLayerBuilder.cpp @@ +2864,5 @@ > + // component of imageTransform), and its inverse used when the mask is used for > + // masking. > + // It consists of a translation to line the mask up with the masked layer's > + // visible region and a scale if the target texture is smaller than necessary > + // for the mask This is a bit confusing. The translation is not "to line the mask up with the masked layer's visible region", is it? Or else I don't understand what that means. We translate so the top-left of the bounding-rect for the mask is at 0,0 in the surface, that's all. This doesn't depend on the masked layer's visible region at all.

Comment on attachment 633459 [details] [diff] [review] patch 5: get image format from the retained layer manager Review of attachment 633459 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/FrameLayerBuilder.cpp @@ +2895,3 @@ > > + // only try to use the image cache if we have a retaining layer manager > + // otherwise we can't be sure that the image format will be correct I'm not convinced this works. I think in some cases we can have a retaining layer manager that's Basic even though other widgets can have accelerated layer managers. Would it be a problem to simply support A8 CairoSurfaceImages in all layer backends?

changed a comment, carrying r=roc

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away June 9-19) from comment #46) > Comment on attachment 633459 [details] [diff] [review] > patch 5: get image format from the retained layer manager > > Review of attachment 633459 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/base/FrameLayerBuilder.cpp > @@ +2895,3 @@ > > > > + // only try to use the image cache if we have a retaining layer manager > > + // otherwise we can't be sure that the image format will be correct > > I'm not convinced this works. I think in some cases we can have a retaining > layer manager that's Basic even though other widgets can have accelerated > layer managers. > GAH! > Would it be a problem to simply support A8 CairoSurfaceImages in all layer > backends? Depends what you mean by problem. I expect that the hardware backends ought to be able to work with A8 textures, but they currently don't. At least one seems to have argb32 baked in to our code. iirc one of the backend's shaders don't work with a8, but I can't remember which one or why. So, it is probably a fair bit of work to do this, but probably possible. I guess it is the ideal solution. Otherwise, a static method in nsBaseWidget?

Turns out using a gfxImageSurface everywhere was a bad idea - it caused lots of 'fuzz' errors, and we can do better (as in use graphics memory) on the DX backends, and maybe even basic if it is using d2d.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=139906e3285e https://tbpl.mozilla.org/?tree=Try&rev=d67fd7dcb0d5

Comment on attachment 636189 [details] [diff] [review] patch 5: image formats part 1 Review of attachment 636189 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/opengl/LayerManagerOGL.cpp @@ +433,5 @@ > + // we will have to copy the surface via a gfxImageSurface anyway, so we may as well > + // just return a gfxImageSurface > + nsRefPtr<gfxImageSurface> imageSurface = surface->GetAsImageSurface(); > + if (!imageSurface) { > + surface = new gfxImageSurface(aSize, gfxASurface::ImageFormatA8); What if we get the ability to accelerate drawing into something that's not an image surface and lives in VRAM? Also, on X for example we actually create an X surface and throw it away. We should avoid that. I suggest adding a method to GLContext which creates a surface that is optimized for use in UploadSurfaceToTexture, using platform rasterization (matching gfxPlatform::CreateOffscreenSurface) if possible, and call that here.

Comment on attachment 636221 [details] [diff] [review] patch 5: image formats part 1 Review of attachment 636221 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxPlatformMac.h @@ +34,5 @@ > + virtual already_AddRefed<gfxASurface> > + CreateOffscreenImageSurface(const gfxIntSize& aSize, > + gfxASurface::gfxContentType aContentType) > + { > + return CreateOffscreenSurface(aSize, aContentType); Add an NS_ASSERTION that GetAsImageSurface succeeds. ::: gfx/thebes/gfxWindowsPlatform.h @@ +107,5 @@ > + virtual already_AddRefed<gfxASurface> > + CreateOffscreenImageSurface(const gfxIntSize& aSize, > + gfxASurface::gfxContentType aContentType) > + { > + return CreateOffscreenSurface(aSize, aContentType); This isn't right. When D2D is enabled, CreateOffscreenSurface could return a D2D surface. You should check mRenderMode like CreateOffscreenSurface does.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #56) > Comment on attachment 636221 [details] [diff] [review] > patch 5: image formats part 1 > > Review of attachment 636221 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/thebes/gfxWindowsPlatform.h > @@ +107,5 @@ > > + virtual already_AddRefed<gfxASurface> > > + CreateOffscreenImageSurface(const gfxIntSize& aSize, > > + gfxASurface::gfxContentType aContentType) > > + { > > + return CreateOffscreenSurface(aSize, aContentType); > > This isn't right. When D2D is enabled, CreateOffscreenSurface could return a > D2D surface. You should check mRenderMode like CreateOffscreenSurface does. But CreateOptimalMaskSurface for the DX backends does not call CreateOffscreenImageSurface, because it can always do better without a gfxImageSurface-compatible surface. This method should return a gfxImageSurface (default behaviour) if CreateOffscreenSurface would return a D2D surface, because the D2D surface cannot be converted to a gfxImageSurface.

Comment on attachment 636230 [details] [diff] [review] patch 5: image formats part 1 Review of attachment 636230 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxPlatformMac.h @@ +39,5 @@ > +#ifdef DEBUG > + nsRefPtr<gfxImageSurface> imageSurface = surface->GetAsImageSurface(); > + NS_ASSERTION(imageSurface, "Surface cannot be converted to a gfxImageSurface"); > +#endif > + return surface; surface.forget() ::: gfx/thebes/gfxWindowsPlatform.cpp @@ +695,5 @@ > +#ifdef DEBUG > + nsRefPtr<gfxImageSurface> imageSurface = surface->GetAsImageSurface(); > + NS_ASSERTION(imageSurface, "Surface cannot be converted to a gfxImageSurface"); > +#endif > + return surface; surface.forget()

addressed comments, carrying r=roc

second go: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=9a1934aec83c

https://hg.mozilla.org/mozilla-central/rev/271b085a0100 https://hg.mozilla.org/mozilla-central/rev/01f8597b6724 https://hg.mozilla.org/mozilla-central/rev/6e2583de79b4 https://hg.mozilla.org/mozilla-central/rev/1a6a30f467a5 https://hg.mozilla.org/mozilla-central/rev/673264ebb9ea https://hg.mozilla.org/mozilla-central/rev/8e0f729afc4d https://hg.mozilla.org/mozilla-central/rev/edb911d3aeaa https://hg.mozilla.org/mozilla-central/rev/24469179b562 https://hg.mozilla.org/mozilla-central/rev/57077263e3c5 https://hg.mozilla.org/mozilla-central/rev/9a1934aec83c