Last Comment Bug 757347 - Share mask layers
: Share mask layers
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics: Layers (show other bugs)
: 15 Branch
: All All
: -- enhancement (vote)
: mozilla16
Assigned To: Nick Cameron [:nrc]
:
:
Mentors:
Depends on: 770299 773626 786817
Blocks: 757388 790124 1073219
  Show dependency treegraph
 
Reported: 2012-05-22 00:51 PDT by Nick Cameron [:nrc]
Modified: 2014-09-25 14:58 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (7.67 KB, patch)
2012-05-22 03:34 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
patch 1: cache for mask images (10.04 KB, patch)
2012-06-05 19:07 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Splinter Review
patch 2: user data (2.56 KB, patch)
2012-06-05 19:08 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Splinter Review
patch 3: changes to FrameLayerBuilder (12.74 KB, patch)
2012-06-05 19:09 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Splinter Review
patch 1: cache for mask images (10.04 KB, patch)
2012-06-05 22:17 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Splinter Review
patch 1: cache for mask images (9.99 KB, patch)
2012-06-05 22:18 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Splinter Review
patch 1: cache for mask images (9.45 KB, patch)
2012-06-05 23:06 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Splinter Review
patch 1: cache for mask images (9.28 KB, patch)
2012-06-07 15:23 PDT, Nick Cameron [:nrc]
roc: review+
Details | Diff | Splinter Review
patch 3: changes to FrameLayerBuilder (12.58 KB, patch)
2012-06-07 15:23 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Splinter Review
patch 4: add shutdown hook and remove MaskImageFormat() (6.55 KB, patch)
2012-06-07 15:24 PDT, Nick Cameron [:nrc]
roc: review+
Details | Diff | Splinter Review
patch 5: image format via gfxPlatform (5.67 KB, patch)
2012-06-13 19:39 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Splinter Review
patch 3: changes to FrameLayerBuilder (12.26 KB, patch)
2012-06-13 20:28 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Splinter Review
patch 4: add shutdown hook and remove MaskImageFormat() (6.65 KB, patch)
2012-06-13 22:52 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Splinter Review
patch 1: cache for mask images (9.17 KB, patch)
2012-06-15 03:21 PDT, Nick Cameron [:nrc]
ncameron: review+
Details | Diff | Splinter Review
patch 2: user data (1.87 KB, patch)
2012-06-15 03:21 PDT, Nick Cameron [:nrc]
roc: review+
Details | Diff | Splinter Review
patch 3: changes to FrameLayerBuilder (12.76 KB, patch)
2012-06-15 03:23 PDT, Nick Cameron [:nrc]
roc: review+
Details | Diff | Splinter Review
patch 4: add shutdown hook and remove MaskImageFormat() (6.43 KB, patch)
2012-06-15 03:23 PDT, Nick Cameron [:nrc]
roc: review+
Details | Diff | Splinter Review
patch 5: get image format from the retained layer manager (6.22 KB, patch)
2012-06-15 03:27 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Splinter Review
patch 3: changes to FrameLayerBuilder (12.66 KB, patch)
2012-06-15 17:00 PDT, Nick Cameron [:nrc]
ncameron: review+
Details | Diff | Splinter Review
patch 5: image formats part 1 (9.31 KB, patch)
2012-06-24 14:54 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Splinter Review
patch 6 image formats part 2 - dx9 (6.06 KB, patch)
2012-06-24 14:55 PDT, Nick Cameron [:nrc]
roc: review+
Details | Diff | Splinter Review
patch 7: image formats part 3 - OGL & OMTC (10.81 KB, patch)
2012-06-24 14:58 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Splinter Review
patch 7: image formats part 3 - OGL & OMTC (10.81 KB, patch)
2012-06-24 15:01 PDT, Nick Cameron [:nrc]
roc: review+
Details | Diff | Splinter Review
patch 5: image formats part 1 (13.60 KB, patch)
2012-06-24 20:29 PDT, Nick Cameron [:nrc]
no flags Details | Diff | Splinter Review
patch 5: image formats part 1 (15.04 KB, patch)
2012-06-24 22:07 PDT, Nick Cameron [:nrc]
roc: review+
Details | Diff | Splinter Review
patch 5: image formats part 1 (15.06 KB, patch)
2012-06-24 22:20 PDT, Nick Cameron [:nrc]
ncameron: review+
Details | Diff | Splinter Review
patch 8: assertions about the validity of alpha images (6.04 KB, patch)
2012-06-24 22:52 PDT, Nick Cameron [:nrc]
roc: review+
Details | Diff | Splinter Review

Description Nick Cameron [:nrc] 2012-05-22 00:51:51 PDT
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.
Comment 1 Nick Cameron [:nrc] 2012-05-22 01:02:02 PDT
Or (probably easier) share the mask surfaces rather than the layers
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-22 01:02:29 PDT
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.
Comment 3 Nick Cameron [:nrc] 2012-05-22 03:25:48 PDT
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).
Comment 4 Andreas Gal :gal 2012-05-22 03:34:37 PDT
Created attachment 625958 [details] [diff] [review]
patch
Comment 5 Andreas Gal :gal 2012-05-22 03:35:58 PDT
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 6 Nick Cameron [:nrc] 2012-06-05 19:07:30 PDT
Created attachment 630409 [details] [diff] [review]
patch 1: cache for mask images
Comment 7 Nick Cameron [:nrc] 2012-06-05 19:08:15 PDT
Created attachment 630410 [details] [diff] [review]
patch 2: user data
Comment 8 Nick Cameron [:nrc] 2012-06-05 19:09:23 PDT
Created attachment 630411 [details] [diff] [review]
patch 3: changes to FrameLayerBuilder
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-05 20:29:59 PDT
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>
Comment 10 Nick Cameron [:nrc] 2012-06-05 22:17:09 PDT
Created attachment 630445 [details] [diff] [review]
patch 1: cache for mask images
Comment 11 Nick Cameron [:nrc] 2012-06-05 22:18:04 PDT
Created attachment 630446 [details] [diff] [review]
patch 1: cache for mask images

3rd time lucky
Comment 12 Nick Cameron [:nrc] 2012-06-05 23:06:18 PDT
Created attachment 630462 [details] [diff] [review]
patch 1: cache for mask images

OK, maybe forth time lucky
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-06 04:32:33 PDT
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
Comment 14 Nick Cameron [:nrc] 2012-06-06 15:21:33 PDT
(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.
Comment 15 Nick Cameron [:nrc] 2012-06-06 16:08:54 PDT
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)
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-06 17:19:35 PDT
(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.
Comment 17 Nick Cameron [:nrc] 2012-06-06 17:43:05 PDT
> > > 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.
Comment 18 Nick Cameron [:nrc] 2012-06-06 20:47:35 PDT
And try push for OS X: https://tbpl.mozilla.org/?tree=Try&rev=5a41f831fa1b
Comment 19 Nick Cameron [:nrc] 2012-06-07 15:23:17 PDT
Created attachment 631174 [details] [diff] [review]
patch 1: cache for mask images
Comment 20 Nick Cameron [:nrc] 2012-06-07 15:23:57 PDT
Created attachment 631175 [details] [diff] [review]
patch 3: changes to FrameLayerBuilder
Comment 21 Nick Cameron [:nrc] 2012-06-07 15:24:47 PDT
Created attachment 631176 [details] [diff] [review]
patch 4: add shutdown hook and remove MaskImageFormat()
Comment 22 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-07 15:49:53 PDT
(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?
Comment 23 Nick Cameron [:nrc] 2012-06-07 15:55:26 PDT
(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 24 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-12 09:29:38 PDT
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 25 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-12 09:31:39 PDT
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 26 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-12 09:42:01 PDT
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()
Comment 27 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-12 10:04:51 PDT
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.
Comment 28 Nick Cameron [:nrc] 2012-06-13 19:39:01 PDT
Created attachment 632996 [details] [diff] [review]
patch 5: image format via gfxPlatform
Comment 29 Nick Cameron [:nrc] 2012-06-13 19:58:24 PDT
(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?
Comment 30 Nick Cameron [:nrc] 2012-06-13 20:27:13 PDT
(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()
Comment 31 Nick Cameron [:nrc] 2012-06-13 20:28:00 PDT
Created attachment 633002 [details] [diff] [review]
patch 3: changes to FrameLayerBuilder
Comment 32 Nick Cameron [:nrc] 2012-06-13 20:46:45 PDT
(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?
Comment 33 Nick Cameron [:nrc] 2012-06-13 22:52:28 PDT
Created attachment 633033 [details] [diff] [review]
patch 4: add shutdown hook and remove MaskImageFormat()

added a delete to the shutdown hook to tidy up the global cache, as requested in comment for patch 3
Comment 34 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-14 00:48:59 PDT
(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 35 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-14 00:54:45 PDT
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 36 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-14 00:56:08 PDT
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 37 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-14 01:01:11 PDT
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.
Comment 38 Nick Cameron [:nrc] 2012-06-14 03:11:34 PDT
(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)
Comment 39 Nick Cameron [:nrc] 2012-06-14 03:13:33 PDT
(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.
Comment 40 Nick Cameron [:nrc] 2012-06-15 03:21:13 PDT
Created attachment 633454 [details] [diff] [review]
patch 1: cache for mask images

Made roc's changes, carrying r=roc
Comment 41 Nick Cameron [:nrc] 2012-06-15 03:21:59 PDT
Created attachment 633456 [details] [diff] [review]
patch 2: user data
Comment 42 Nick Cameron [:nrc] 2012-06-15 03:23:01 PDT
Created attachment 633457 [details] [diff] [review]
patch 3: changes to FrameLayerBuilder
Comment 43 Nick Cameron [:nrc] 2012-06-15 03:23:45 PDT
Created attachment 633458 [details] [diff] [review]
patch 4: add shutdown hook and remove MaskImageFormat()
Comment 44 Nick Cameron [:nrc] 2012-06-15 03:27:48 PDT
Created attachment 633459 [details] [diff] [review]
patch 5: get image format from the retained layer manager

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 45 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-15 09:44:43 PDT
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 46 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-15 09:58:13 PDT
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?
Comment 47 Nick Cameron [:nrc] 2012-06-15 17:00:29 PDT
Created attachment 633724 [details] [diff] [review]
patch 3: changes to FrameLayerBuilder

changed a comment, carrying r=roc
Comment 48 Nick Cameron [:nrc] 2012-06-15 17:04:28 PDT
(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?
Comment 49 Nick Cameron [:nrc] 2012-06-24 14:54:55 PDT
Created attachment 636189 [details] [diff] [review]
patch 5: image formats part 1
Comment 50 Nick Cameron [:nrc] 2012-06-24 14:55:38 PDT
Created attachment 636190 [details] [diff] [review]
patch 6 image formats part 2 - dx9
Comment 51 Nick Cameron [:nrc] 2012-06-24 14:58:01 PDT
Created attachment 636191 [details] [diff] [review]
patch 7: image formats part 3 - OGL & OMTC

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.
Comment 53 Nick Cameron [:nrc] 2012-06-24 15:01:03 PDT
Created attachment 636193 [details] [diff] [review]
patch 7: image formats part 3 - OGL & OMTC
Comment 54 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-24 15:54:00 PDT
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 55 Nick Cameron [:nrc] 2012-06-24 20:29:14 PDT
Created attachment 636221 [details] [diff] [review]
patch 5: image formats part 1
Comment 56 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-24 20:37:32 PDT
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.
Comment 57 Nick Cameron [:nrc] 2012-06-24 20:47:21 PDT
(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 58 Nick Cameron [:nrc] 2012-06-24 22:07:01 PDT
Created attachment 636230 [details] [diff] [review]
patch 5: image formats part 1
Comment 59 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-24 22:08:48 PDT
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()
Comment 60 Nick Cameron [:nrc] 2012-06-24 22:20:18 PDT
Created attachment 636231 [details] [diff] [review]
patch 5: image formats part 1

addressed comments, carrying r=roc
Comment 61 Nick Cameron [:nrc] 2012-06-24 22:52:32 PDT
Created attachment 636235 [details] [diff] [review]
patch 8: assertions about the validity of alpha images
Comment 63 Nick Cameron [:nrc] 2012-06-25 19:46:47 PDT
second go: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=9a1934aec83c

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