716439 - (GPU-clipping-rounded) Implement clipping to rectangles with rounded corners on the GPU

Status: RESOLVED FIXED

(Core :: Graphics, enhancement)

Description

[Nick Cameron [:nrc]]

Attachment: A video clipped to a rounded rectangle

Clipping (using css) of objects to a plain rectangle uses hardware acceleration. However, if an object is clipped to a rectangle with rounded corners (by setting the border-radius property), then a non-accelerated fallback path is used.

Clipping to a rectangle with rounded corners should be accelerated using the GPU.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Another case that would benefit from this is where we have a clip and a transformation. Currently we have to allocate a temporary surface to draw the clipped content before transforming into onto the destination.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Is that common? And I'm not sure it's the same as this bug. That shouldn't need new layers API, just some optimization(s) in the layers backends.

For this bug we definitely need new layers API. I was thinking of adding a SetClipRoundedRect method that takes a rect and a border-radii parameter.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Fairly common, yes. To the extent that we have existing optimizations that detect if the transform will preserve axis aligned rectangles and skip the temporary in that case. Apparently mobile is especially affected by allocating temporary surfaces.

It shouldn't require any new API, no, but the I think the implementation would be the same. Either way, we're requiring the backend to be able to clip to arbitrary shapes. Do we also want to handle svg-mask through this at some point?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> It shouldn't require any new API, no, but the I think the implementation
> would be the same. Either way, we're requiring the backend to be able to
> clip to arbitrary shapes.

Actually I was thinking for this we would handle rounded rects specifically. Then instead of generating a temporary mask texture we can compute the mask value in the shader. We may wish to restrict this support to cases where the corners are all the same shape, and defer the most general case to another API that clips to a mask image. (That API's going to be harder to figure out since we may need to come up with a plan for SVG masks that can reference other layers, e.g. if the SVG mask itself contains content that's accelerated, like a video.)

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

One question I don't know the answer to is how easy it would be to combine multiple shader options. E.g. can we write a "rounded-mask" shader and compose it cleanly with the existing set of shaders for rendering various layer types? Or are we stuck with using one shader at a time, giving us a choice between temporary surfaces and a combinatorial explosion of shaders?

Comment 6

[Nick Cameron [:nrc]]

Attachment: change the callback for drawing Thebes layers to be per-layer, rather than per-transaction

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we should probably eliminate the callback data parameter to EndTransaction. It doesn't make sense to pass the same callback data to different callback functions. Layer clients can set any necessary data as user data on the layer itself, I think.

Comment 8

[Nick Cameron [:nrc]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> I think we should probably eliminate the callback data parameter to
> EndTransaction. It doesn't make sense to pass the same callback data to
> different callback functions. Layer clients can set any necessary data as
> user data on the layer itself, I think.

I thought that it might (for whatever reason) be useful in the future to pass in parameters which were per-run rather than per-layer.

Currently, this is only used in nsWebBrowser::HandleEvent, where the background colour is passed in as a parameter, but in this case it could be saved as user data on the layer.

If you think not, I'll go ahead and whip out the callback data too.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Please do.

Comment 10

[Nick Cameron [:nrc]]

Attachment: Using a mask layer with the software backend

First draft of the frontend for creating mask layers for rounded-rect clipping and the Cairo/BasicLayers backend. Doesn't touch Shadowable layers yet, nor does it do some of the optimisations discussed. Will break rounded-rect clipping for the other backends.

Would be good to get some early feedback on the code and ideas.

Comment 11

[Nick Cameron [:nrc]]

Attachment: Mask layers w/ software backend

Added masking for color layers; did a little refactoring.

Still TODO: shadowable layer masking and the effective mask optimisation.

Comment 12

[Nick Cameron [:nrc]]

Attachment: store a mask layer in a layer

I've split the large patch into multiple smaller ones, uploading them in order...

Comment 13

[Nick Cameron [:nrc]]

Attachment: user data for mask layers

Comment 14

[Nick Cameron [:nrc]]

Attachment: recycling for mask layers

Comment 15

[Nick Cameron [:nrc]]

Attachment: remove code to enable mask layers for rounded rects

Comment 16

[Nick Cameron [:nrc]]

Attachment: frontend for mask layers

Building mask layers for the various types of layer and attaching them to the layer. There is a bug in here that I am working on - produces tearing during scrolling, so I assume the mask layers are not quite pixel-perfect yet; but, uploading now so the next patch will work.

Comment 17

[Nick Cameron [:nrc]]

Attachment: mask layers in the software backend

Comment 18

[Nick Cameron [:nrc]]

Attachment: member/getter/setter - bugfix

Comment 19

[Nick Cameron [:nrc]]

Attachment: user data for mask layers - bugfix

Comment 20

[Nick Cameron [:nrc]]

Attachment: building mask layers - bugfix

Comment 21

[Nick Cameron [:nrc]]

Attachment: mask layers - basic backend - bugfix

Comment 22

[Nick Cameron [:nrc]]

Fixed a bug with multiple tranforms. Still todo: bug - white lines while scrolling; push layers down in stack optimisation; masking for shadowable layers; mochitest for scrolling redraw; hardware backends.

Comment 23

[Nick Cameron [:nrc]]

Attachment: optimise by pushing down mask layers

If a container has a mask layer and a single child, then the mask layer will be pushed down to the child (unless there are 3D transforms on the container or child). Only applies to the software backend.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 595194 [details] [diff] [review]
member/getter/setter - bugfix

Review of attachment 595194 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/Layers.h
@@ +983,5 @@
>    PRUint32 mContentFlags;
>    bool mUseClipRect;
>    bool mUseTileSourceRect;
>    bool mIsFixedPosition;
> +  nsRefPtr<Layer> mMaskLayer;

Move this up next to other pointer-valued fields so that we don't lose space to alignment padding.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 595195 [details] [diff] [review]
user data for mask layers - bugfix

Review of attachment 595195 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/Layers.h
@@ +1371,5 @@
> +{
> +  MaskLayerUserData(const gfxMatrix& aTransform, PRUint8* aUserDataKey) :
> +    mTransform(aTransform), mUserDataKey(aUserDataKey) {}
> +
> +  gfxMatrix mTransform;

This should just be the layer's transform, so you don't need user data here.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 595196 [details] [diff] [review]
building mask layers - bugfix

Review of attachment 595196 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/Layers.h
@@ +1103,5 @@
> +   * The number of rounded rect clips the items of this layer have in common
> +   * with each other.
> +   * -1 if no items in the layer; must be >=0 by the time the layer is drawn.
> +   */
> +  PRInt32 mCommonClipCount;

Why is this in Layers.h? Isn't this something that only FrameLayerBuilder cares about, that's not used by backends? In that case it should go in userdata defined and maintained by FrameLayerBuilder.

::: layout/base/FrameLayerBuilder.cpp
@@ +373,5 @@
>      return mThebesLayerDataStack.IsEmpty() ? nsnull
>          : mThebesLayerDataStack[mThebesLayerDataStack.Length() - 1].get();
>    }
>  
> +  //Build a ThebesLayer representing the clipping region. Will return NULL if

You really should explain exactly what this builds.

@@ +374,5 @@
>          : mThebesLayerDataStack[mThebesLayerDataStack.Length() - 1].get();
>    }
>  
> +  //Build a ThebesLayer representing the clipping region. Will return NULL if
> +  //there is no clipping specified or a mask layer cannot be built.

Use /* comments for this many lines:
 /**
  * Build a ThebesLayer...
  * there is no ...

@@ +381,5 @@
> +  //If aIncludePlainRect is true, then mClipRect will be incuded in the result.
> +  already_AddRefed<Layer> BuildMaskLayer(Layer *aLayer,
> +                                         const FrameLayerBuilder::Clip& aClip,
> +                                         nsPresContext* aPresContext,
> +                                         const gfxMatrix* aClipTransform = 0,

This should be const gfxMatrix&. A temp gfxMatrix() should do for the one case where we need to use the identity.

@@ +382,5 @@
> +  already_AddRefed<Layer> BuildMaskLayer(Layer *aLayer,
> +                                         const FrameLayerBuilder::Clip& aClip,
> +                                         nsPresContext* aPresContext,
> +                                         const gfxMatrix* aClipTransform = 0,
> +                                         PRInt32 aCount = 0,

What's aCount for?

@@ +383,5 @@
> +                                         const FrameLayerBuilder::Clip& aClip,
> +                                         nsPresContext* aPresContext,
> +                                         const gfxMatrix* aClipTransform = 0,
> +                                         PRInt32 aCount = 0,
> +                                         bool aIncludePlainRect = false);

Don't add this :-)

@@ +2512,5 @@
>  }
>  
> +already_AddRefed<Layer>
> +ContainerState::BuildMaskLayer
> +  (Layer *aLayer, const FrameLayerBuilder::Clip& aClip,

Put as many parameters as will fit on the same line as BuildMaskLayer

@@ +2516,5 @@
> +  (Layer *aLayer, const FrameLayerBuilder::Clip& aClip,
> +   nsPresContext* aPresContext, const gfxMatrix* aClipTransform,
> +   PRInt32 aCount, bool aIncludePlainRect) 
> +{
> +  //don't build an unnecessary mask

Put a space after //

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 595197 [details] [diff] [review]
mask layers - basic backend - bugfix

Review of attachment 595197 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ThebesLayerBuffer.cpp
@@ +103,5 @@
>  
>    pattern->SetMatrix(transform);
>    aTarget->SetPattern(pattern);
> +  aTarget->Save();
> +  aTarget->Clip();

We don't want to save/restore and clip if there's no mask and 1.0 opacity. Those operations are expensive.

::: gfx/layers/ThebesLayerBuffer.h
@@ +175,5 @@
> +                          gfxASurface* aMask,
> +                          const gfxMatrix* aMaskTransform);
> +  void DrawBufferWithRotation(gfxContext* aTarget, float aOpacity,
> +                              gfxASurface* aMask = NULL,
> +                              const gfxMatrix* aMaskTransform = NULL);

Document how the mask parameters are used.

::: gfx/layers/basic/BasicLayers.cpp
@@ +116,5 @@
>     * set up to account for all the properties of the layer (transform,
>     * opacity, etc).
>     */
> +  virtual void Paint(gfxContext* aContext, gfxASurface* aMask,
> +                     const gfxMatrix& aMaskTransform) {}

should this be a pointer so it can be null if there's no mask?

@@ +126,5 @@
>     * effective visible region (snapped or unsnapped, it doesn't matter).
>     */
>    virtual void PaintThebes(gfxContext* aContext,
> +                           gfxASurface* aMask,
> +                           const gfxMatrix& aMaskTransform,

ditto?

@@ +173,5 @@
> +   * Return a surface for this layer. Will use an existing surface, if
> +   * possible, or may create a temporary surface.
> +   * Implement this method for any layers that might be used as a mask.
> +   */
> +  virtual already_AddRefed<gfxASurface> GetAsSurface() { return NULL; }

Document what this does if the layer has a transform. Does it return null then, or what? Are there other conditions under which it returns null?

@@ +1982,5 @@
>      groupTarget = aTarget;
>    }
>  
> +  //extract a mask and transform for the mask
> +  nsRefPtr<gfxASurface> maskSurface = NULL;

nsnull instead of NULL, everywhere

@@ +1983,5 @@
>    }
>  
> +  //extract a mask and transform for the mask
> +  nsRefPtr<gfxASurface> maskSurface = NULL;
> +  gfxMatrix maskTransform; // = savedMatrix;

Make this a const gfxMatrix*?

@@ +2801,4 @@
>      return;
>    }
>  
> +  //TODO[nrc] masking

what needs to be done here?

Comment 28

[Nick Cameron [:nrc]]

Attachment: member/getter/setter - updated

Comment 29

[Nick Cameron [:nrc]]

Attachment: user data for mask layers - updated

Comment 30

[Nick Cameron [:nrc]]

The above two patches are the first two in the queue, in order (sorry forgot to number them).

Comment 31

[Nick Cameron [:nrc]]

Attachment: dx10 patch 1 (loading a mask layer as a texture in DX10)

Comment 32

[Nick Cameron [:nrc]]

Attachment: dx10 patch 2 (changes to C++ code to use shaders for masking)

Comment 33

[Nick Cameron [:nrc]]

Attachment: dx10 patch 3 (changes to shader code to use GPU masking)

Comment 34

[Nick Cameron [:nrc]]

Attachment: mask layers patch 5 (building mask layers)

To help the DX10 patches make sense, I've uploaded the most up to date patch for building mask layers.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 597648 [details] [diff] [review]
member/getter/setter - updated

Review of attachment 597648 [details] [diff] [review]:
-----------------------------------------------------------------

Your commit messages need a bug number and r= marker. See the commit messages on mozilla-central.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 597649 [details] [diff] [review]
user data for mask layers - updated

Review of attachment 597649 [details] [diff] [review]:
-----------------------------------------------------------------

Probably should merge the FrameLayerBuilder part of this patch into the patch that uses the user data.

::: gfx/layers/Layers.h
@@ +799,5 @@
>    /**
> +   * This can be used anytime.
> +   */
> +  void ClearUserData()
> +  { mUserData.Clear(); }

What's this for?

This is probably not a good API to have. We want clients to be able to store their user data without interference. Removing someone else's user data is bad.

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 594809 [details] [diff] [review]
recycling for mask layers

Review of attachment 594809 [details] [diff] [review]:
-----------------------------------------------------------------

I actually think we should recycle mask layers to a separate list. That will simplify your user-data management and make mask layers more likely to be successfully recycled.

Comment 38

[Nick Cameron [:nrc]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #36)
> ::: gfx/layers/Layers.h
> @@ +799,5 @@
> >    /**
> > +   * This can be used anytime.
> > +   */
> > +  void ClearUserData()
> > +  { mUserData.Clear(); }
> 
> What's this for?
> 

When I recycle image layers I recycle them into whatever layer they used to be, e.g., back to ImageLayers, this requires changing the user data type, e.g., from MaskLayerUserData to ImageLayerUserData (and vice versa when I create a mask layer). SetUserData/RemoveUserData (which also allows you to remove someone else's user data) only allows you to change/remove user data with the same type, whereas ClearUserData clears the type too.

Comment 39

[Nick Cameron [:nrc]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #37)
> Comment on attachment 594809 [details] [diff] [review]
> recycling for mask layers
> 
> Review of attachment 594809 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I actually think we should recycle mask layers to a separate list. That will
> simplify your user-data management and make mask layers more likely to be
> successfully recycled.

But once we allow different types of layers to be used as mask layers, then we will need to double the number of lists - recycled image layers, recycled thebes layers, recycled image mask layers, recycled thebes mask layers, etc. If that is acceptable, then yes, it would make things much easier.

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Nick Cameron from comment #39)
> But once we allow different types of layers to be used as mask layers, then
> we will need to double the number of lists - recycled image layers, recycled
> thebes layers, recycled image mask layers, recycled thebes mask layers, etc.
> If that is acceptable, then yes, it would make things much easier.

I think that's fine. The number of lists might be proportional to the number of allocation sites but it won't get worse than that.

Comment 41

[Nick Cameron [:nrc]]

Attachment: dx10 patch 1 (loading a mask layer as a texture in DX10)

Comment 42

[Nick Cameron [:nrc]]

Attachment: dx10 patch 2 (changes to C++ code to use shaders for masking)

Comment 43

[Nick Cameron [:nrc]]

Attachment: dx10 patch 3 (changes to shader code to use GPU masking)

Minor changes to the DX10 patches - polishing only.

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: testcase

This testcase is much smoother for me with no border-radius (hovering the yellow div) than with the border-radius ... even though the FPS counter at the bottom right of the test shows same value in both cases.

With this bug fixed, the testcase should be just as smooth with or without border-radius (and as smooth as a trunk build in both cases).

Comment 45

[Nick Cameron [:nrc]]

Attachment: dx9 patch 1 (loading shaders and textures)

Comment 46

[Nick Cameron [:nrc]]

Attachment: dx9 patch 2 (using the new shaders in each layer)

Comment 47

[Nick Cameron [:nrc]]

Attachment: dx9 patch 3 (the actual shaders)

Comment 48

[Nick Cameron [:nrc]]

Attachment: building masks patch 3 (recycling)

Comment 49

[Nick Cameron [:nrc]]

Attachment: building masks patch 2 (user data)

Comment 50

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 598698 [details] [diff] [review]
building masks patch 2 (user data)

Review of attachment 598698 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that

::: layout/base/FrameLayerBuilder.cpp
@@ +434,5 @@
> +{
> +  MaskLayerUserData(PRUint8* aUserDataKey) :
> +    mUserDataKey(aUserDataKey) {}
> +
> +  PRUint8* mUserDataKey;

As we discussed, we don't need this here (yet), and maybe never at all, so let's just take it out.

Comment 51

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 598697 [details] [diff] [review]
building masks patch 3 (recycling)

Review of attachment 598697 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/FrameLayerBuilder.cpp
@@ +838,5 @@
> +      return nsnull;
> +    layer->SetUserData(&gMaskLayerUserData,
> +                       new MaskLayerUserData(&gImageLayerUserData));
> +  }
> +  return layer.forget();

Might be a good idea to look into doing something clever with member pointers and/or templates to share recycling code. But future work.

@@ +1677,5 @@
>         layer = layer->GetNextSibling()) {
> +    RecycleLayer(layer);
> +
> +    if (Layer* maskLayer = layer->GetMaskLayer()) {
> +      RecycleLayer(maskLayer);

We shouldn't really share RecycleLayer between these two callsites. That just creates extra work, when we already know here that this is a mask --- RecycleLayer has to check it for all the non-mask types anyway. Probably ditch RecycleLayer and just write the possible options here inline.

@@ +1678,5 @@
> +    RecycleLayer(layer);
> +
> +    if (Layer* maskLayer = layer->GetMaskLayer()) {
> +      RecycleLayer(maskLayer);
> +      layer->SetMaskLayer(nsnull);

This doesn't really belong here since CollectOldLayers doesn't modify the layer tree otherwise. This should move somewhere else, probably to the patch where we set mask layers.

Comment 52

[Nick Cameron [:nrc]]

Attachment: building masks patch 2 (user data) - updated

Comment 53

[Nick Cameron [:nrc]]

Attachment: building masks patch 3 (recycling) - updated

Comment 54

[Nick Cameron [:nrc]]

Attachment: building mask layers patch 4 (building the mask layer) - updated

Comment 55

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 598723 [details] [diff] [review]
building masks patch 3 (recycling) - updated

Review of attachment 598723 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that

::: layout/base/FrameLayerBuilder.cpp
@@ +1678,5 @@
>        NS_ASSERTION(layer->AsThebesLayer(), "Wrong layer type");
>        mRecycledThebesLayers.AppendElement(static_cast<ThebesLayer*>(layer));
>      }
> +#ifdef DEBUG
> +    else if (layer->HasUserData(&gMaskLayerUserData)) {

This could just be NS_ASSERTION(!layer->HasUserData(&gMaskLayerUserData)) right at the start.

Comment 56

[Nick Cameron [:nrc]]

Attachment: building masks patch 3 (recycling) - updated

Comment 57

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 598724 [details] [diff] [review]
building mask layers patch 4 (building the mask layer) - updated

Review of attachment 598724 [details] [diff] [review]:
-----------------------------------------------------------------

The Layers.cpp/.h changes belong in a different patch (but look fine).

::: layout/base/FrameLayerBuilder.cpp
@@ +301,2 @@
>       */
>      FrameLayerBuilder::Clip mImageClip;

Rename this to something not image-specific, say mItemClip?

@@ +390,5 @@
> +   * only build a mask layer for the first aCount rounded rects in aClip
> +   */
> +  already_AddRefed<Layer> BuildMaskLayer(Layer *aLayer,
> +                                         const FrameLayerBuilder::Clip& aClip,
> +                                         nsPresContext* aPresContext,

Don't need to pass aPresContext, use mContainerFrame->PresContext() instead

@@ +391,5 @@
> +   */
> +  already_AddRefed<Layer> BuildMaskLayer(Layer *aLayer,
> +                                         const FrameLayerBuilder::Clip& aClip,
> +                                         nsPresContext* aPresContext,
> +                                         PRInt32 aCount = 0);

aClipRoundedRectCount?

A more logical default would be aClip.mRoundedClipRects.Length(). Or if we should use a special value, use PR_INT32_MAX.

@@ +443,5 @@
>     */
>    float mXScale, mYScale;
>    /**
> +    * The number of rounded rect clips the items of this layer have in common
> +    * with each other.

Explain that, more precisely, the first mCommonClipCount rounded-rect clips of the items in the layer are all identical.

@@ +1095,5 @@
> +
> +      //use a mask layer for rounded rect clipping
> +      if (!data->mImageClip.mRoundedClipRects.IsEmpty()) {
> +        imageLayer->SetMaskLayer(BuildMaskLayer(imageLayer, data->mImageClip,
> +                                 mContainerFrame->PresContext()));

Seems like BuildMaskLayer should be called SetupMaskLayer and actually do the SetMaskLayer itself (if necessary).

Also seems like the mRoundedRects.IsEmpty() check should be folded into SetupMaskLayer.

@@ +1150,5 @@
> +    //try to build mask layers for color or Thebes layers
> +    if (layer->GetType() == Layer::TYPE_COLOR &&
> +        !data->mImageClip.mRoundedClipRects.IsEmpty()) {
> +      layer->SetMaskLayer(BuildMaskLayer(layer, data->mImageClip,
> +                          mContainerFrame->PresContext()));

Move this further up to where we set up colorLayer?

@@ +1152,5 @@
> +        !data->mImageClip.mRoundedClipRects.IsEmpty()) {
> +      layer->SetMaskLayer(BuildMaskLayer(layer, data->mImageClip,
> +                          mContainerFrame->PresContext()));
> +    } else if (layer->GetType() == Layer::TYPE_THEBES &&
> +               !layer->GetVisibleRegion().IsEmpty()) {

Why test the visible region here?

@@ +1155,5 @@
> +    } else if (layer->GetType() == Layer::TYPE_THEBES &&
> +               !layer->GetVisibleRegion().IsEmpty()) {
> +      ThebesLayer* thebesLayer = static_cast<ThebesLayer*>(layer.get());
> +
> +      //TODO[nrc] ???Sometimes used again below, is it worth pulling up a scope?

No.

@@ +1560,5 @@
>        }
>        RestrictVisibleRegionForLayer(ownLayer, itemVisibleRect);
> +
> +      //rounded rectangle clipping using mask layers
> +      //(must be done after visible rect is set on layer)

Space after //

@@ +1562,5 @@
> +
> +      //rounded rectangle clipping using mask layers
> +      //(must be done after visible rect is set on layer)
> +      if (!aClip.mRoundedClipRects.IsEmpty() &&
> +          aClip.IsRectClippedByRoundedCorner(item->GetVisibleRect())) {

We can't rely on drawing being restricted to GetVisibleRect.

Use itemContent here instead.

@@ +1677,5 @@
> +    //check to see if the new item has rounded rect clips in common with
> +    //other items in the layer
> +    ThebesDisplayItemLayerUserData* userData =
> +      static_cast<ThebesDisplayItemLayerUserData*>
> +        (aLayer->GetUserData(&gThebesDisplayItemLayerUserData));

How about having a helper function for this GetUserData and cast?

@@ +1697,5 @@
> +        userData->mCommonClipCount = 0;
> +      }
> +    } else if (userData->mCommonClipCount < 0) { //first item in the layer
> +      userData->mCommonClipCount = aClip.mRoundedClipRects.Length();
> +    } 

Actually instead of having mCommonClipCount, what if we added a method to Clip, say Clip::RestrictToCommonRoundedRects(const Clip& aOther) which simply discards rounded rects starting at the first which is different in 'this' and aOther?

::: layout/base/FrameLayerBuilder.h
@@ +380,5 @@
> +    // apply the first aCount rounded rects to aContext
> +    // if aCount == 0, apply all the rounded rects
> +    // if aCount < 0, apply the rects from aCount to the end of the list
> +    void ApplyRoundedRectsTo(gfxContext* aContext, PRInt32 A2DPRInt32,
> +                             PRInt32 aCount) const;

How about calling this aSkipRoundedRectsCount and making it positive. Making it negative is confusing to me.

Comment 58

[Bas Schouten (:bas.schouten)]

Comment on attachment 598094 [details] [diff] [review]
dx10 patch 1 (loading a mask layer as a texture in DX10)

Review of attachment 598094 [details] [diff] [review]:
-----------------------------------------------------------------

Sadly for this particular patch a sad amount conflicts with one of the patches on bug 651192. I'll look at this later.

Comment 59

[Bas Schouten (:bas.schouten)]

Comment on attachment 598095 [details] [diff] [review]
dx10 patch 2 (changes to C++ code to use shaders for masking)

Review of attachment 598095 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/d3d10/CanvasLayerD3D10.cpp
@@ +263,5 @@
>    SetEffectTransformAndOpacity();
>  
>    ID3D10EffectTechnique *technique;
>  
> +  if (LoadMaskTexture()) {

I'm wondering if we could do static string concatenation  here to make this code look a little bit less complex. But I can live with it. I'm thinking we should probably add a method on the LayerManager to GetTechnique(alpha, filter, mask, premul) or something like that.

Comment 60

[Bas Schouten (:bas.schouten)]

Comment on attachment 598096 [details] [diff] [review]
dx10 patch 3 (changes to shader code to use GPU masking)

Review of attachment 598096 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/d3d10/LayerManagerD3D10.fx
@@ +94,5 @@
> +struct VS_MASK_OUTPUT {
> +  float4 vPosition : SV_Position;
> +  float2 vTexCoords : TEXCOORD0;
> +  float3 vMaskCoords : TEXCOORD1;
> +};

We should probably have a separate VS_MASK_OUTPUT_3D (with the float3 texcoords), it probably doesn't matter too much, but it won't add that much code. It could be that the interpolators will be able to do the 8 floats with 2 units, rather than 3.

@@ +196,5 @@
> +  //calculate the position on the mask texture
> +  position.xyz /= position.w;
> +  outp.vMaskCoords.x = (position.x - vMaskQuad.x) / vMaskQuad.z;
> +  outp.vMaskCoords.y = (position.y - vMaskQuad.y) / vMaskQuad.w;
> +  //we use the w to do non-perspective correct interpolation

Explain this in more detail or include a link for future reference.

@@ +226,5 @@
> +{
> +  float2 maskCoords = aVertex.vMaskCoords.xy;
> +  float mask = tMask.Sample(LayerTextureSamplerLinear, maskCoords).a;
> +  return tRGB.Sample(LayerTextureSamplerPoint, aVertex.vTexCoords) * fLayerOpacity * mask;
> +}

We should combine these and pass the sampler as a parameter, see how SampleRadialGradient does this for wrapping modes: http://mxr.mozilla.org/mozilla-central/source/gfx/2d/ShadersD2D.fx

@@ +566,5 @@
> +technique10 RenderComponentAlphaLayerMask
> +{
> +	Pass P0
> +	{
> +	    SetRasterizerState( LayerRast );

nit: Tabs

@@ +585,5 @@
> +        SetVertexShader( CompileShader( vs_4_0_level_9_3, LayerQuadMaskVS() ) );
> +        SetGeometryShader( NULL );
> +        SetPixelShader( CompileShader( ps_4_0_level_9_3, SolidColorShaderMask() ) );
> +    }
> +}
\ No newline at end of file

I'm wondering out loud here, if we could leave out SetGeometryShader (for all the other shader techniques as well), it should save us some significant overhead per-layer.

Comment 61

[Nick Cameron [:nrc]]

Attachment: building mask layers patch 4 (building the mask layer) - updated

Comment 62

[Nick Cameron [:nrc]]

Anything without comments is done in the new patch

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #57)
> @@ +1150,5 @@
> > +    //try to build mask layers for color or Thebes layers
> > +    if (layer->GetType() == Layer::TYPE_COLOR &&
> > +        !data->mImageClip.mRoundedClipRects.IsEmpty()) {
> > +      layer->SetMaskLayer(BuildMaskLayer(layer, data->mImageClip,
> > +                          mContainerFrame->PresContext()));
> 
> Move this further up to where we set up colorLayer?
>
I need to call BuildMaskLayer after the visible region has been calculated, which is after where the ColorLayer is setup 

> @@ +1152,5 @@
> > +        !data->mImageClip.mRoundedClipRects.IsEmpty()) {
> > +      layer->SetMaskLayer(BuildMaskLayer(layer, data->mImageClip,
> > +                          mContainerFrame->PresContext()));
> > +    } else if (layer->GetType() == Layer::TYPE_THEBES &&
> > +               !layer->GetVisibleRegion().IsEmpty()) {
> 
> Why test the visible region here?
> 
Because ThebesLayers which have been optimised into color/image layers will still be around and we don't want to build mask layers for them.

> @@ +1697,5 @@
> > +        userData->mCommonClipCount = 0;
> > +      }
> > +    } else if (userData->mCommonClipCount < 0) { //first item in the layer
> > +      userData->mCommonClipCount = aClip.mRoundedClipRects.Length();
> > +    } 
> 
> Actually instead of having mCommonClipCount, what if we added a method to
> Clip, say Clip::RestrictToCommonRoundedRects(const Clip& aOther) which
> simply discards rounded rects starting at the first which is different in
> 'this' and aOther?
> 
I thought this wouldn't work, but on second thoughts, maybe it will, I'll have a go...

> ::: layout/base/FrameLayerBuilder.h
> @@ +380,5 @@
> > +    // apply the first aCount rounded rects to aContext
> > +    // if aCount == 0, apply all the rounded rects
> > +    // if aCount < 0, apply the rects from aCount to the end of the list
> > +    void ApplyRoundedRectsTo(gfxContext* aContext, PRInt32 A2DPRInt32,
> > +                             PRInt32 aCount) const;
> 
> How about calling this aSkipRoundedRectsCount and making it positive. Making
> it negative is confusing to me.

It can be positive or negative - if it is positive it we draw rounded rects 0..aCount, if it is negative we draw aCount.. This is useful because when making the mask layer we want to do the former, and when drawing the items to the ThebesLayer we want to do the latter. However, if the above change works, then this ought be be simplifiable.

Comment 63

[Nick Cameron [:nrc]]

Attachment: dx10 patch 4 (selecting a shader)

Comment 64

[Nick Cameron [:nrc]]

(In reply to Bas Schouten (:bas) from comment #59)
> Comment on attachment 598095 [details] [diff] [review]
> dx10 patch 2 (changes to C++ code to use shaders for masking)
> 
> Review of attachment 598095 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/d3d10/CanvasLayerD3D10.cpp
> @@ +263,5 @@
> >    SetEffectTransformAndOpacity();
> >  
> >    ID3D10EffectTechnique *technique;
> >  
> > +  if (LoadMaskTexture()) {
> 
> I'm wondering if we could do static string concatenation  here to make this
> code look a little bit less complex. But I can live with it. I'm thinking we
> should probably add a method on the LayerManager to GetTechnique(alpha,
> filter, mask, premul) or something like that.

I was thinking a similar thing, but using flags instead of string concatenation, but wasn't sure if it was worthwhile. See dx10 patch 4 and let me know what you think.

ps: I'm in Europe and working intermittently, so comments/updated patches might be a little bit slow, sorry.

Comment 65

[Nick Cameron [:nrc]]

Attachment: dx10 patch 4 (selecting a shader)

Now with comment, sorry, jet-lagged.

Comment 66

[Nick Cameron [:nrc]]

Attachment: dx10 patch 3 (the actual shaders)

Comment 67

[Nick Cameron [:nrc]]

Attachment: dx9 patch 3 (the actual shaders)

Comment 68

[Nick Cameron [:nrc]]

(In reply to Bas Schouten (:bas) from comment #60)
> Comment on attachment 598096 [details] [diff] [review]
> dx10 patch 3 (changes to shader code to use GPU masking)
> 
> Review of attachment 598096 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 
> I'm wondering out loud here, if we could leave out SetGeometryShader (for
> all the other shader techniques as well), it should save us some significant
> overhead per-layer.

Hmm, not sure, I couldn't find any info online about whether we can omit the SetGeometryShader call. It does compile and the compiled shader doesn't have a default call or anything. Why will it save significant overhead, is it doing more than just saving 0 to a register?

All other changes are made in the new patch.

Comment 69

[Nick Cameron [:nrc]]

(In reply to Nick Cameron from comment #62)

> 
> > @@ +1697,5 @@
> > > +        userData->mCommonClipCount = 0;
> > > +      }
> > > +    } else if (userData->mCommonClipCount < 0) { //first item in the layer
> > > +      userData->mCommonClipCount = aClip.mRoundedClipRects.Length();
> > > +    } 
> > 
> > Actually instead of having mCommonClipCount, what if we added a method to
> > Clip, say Clip::RestrictToCommonRoundedRects(const Clip& aOther) which
> > simply discards rounded rects starting at the first which is different in
> > 'this' and aOther?
> > 
> I thought this wouldn't work, but on second thoughts, maybe it will, I'll
> have a go...

I've had a think about this and I think it will work, but it would mean having another Clip object per thebeslayer and we would need to find the length of the rounded rects array when we draw each item in the Thebes layer (although this would be fast because the list will always be short). So, I am not convinced that it is worth it for the simplification. If you (roc) still think it's a worthwhile change, it will be easy to do.

Comment 70

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Nick Cameron from comment #62)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February
> 21 to March 17) from comment #57)
> > @@ +1152,5 @@
> > > +        !data->mImageClip.mRoundedClipRects.IsEmpty()) {
> > > +      layer->SetMaskLayer(BuildMaskLayer(layer, data->mImageClip,
> > > +                          mContainerFrame->PresContext()));
> > > +    } else if (layer->GetType() == Layer::TYPE_THEBES &&
> > > +               !layer->GetVisibleRegion().IsEmpty()) {
> > 
> > Why test the visible region here?
> > 
> Because ThebesLayers which have been optimised into color/image layers will
> still be around and we don't want to build mask layers for them.

But this path is only taken when layer->GetType() != Layer::TYPE_COLOR.

> > ::: layout/base/FrameLayerBuilder.h
> > @@ +380,5 @@
> > > +    // apply the first aCount rounded rects to aContext
> > > +    // if aCount == 0, apply all the rounded rects
> > > +    // if aCount < 0, apply the rects from aCount to the end of the list
> > > +    void ApplyRoundedRectsTo(gfxContext* aContext, PRInt32 A2DPRInt32,
> > > +                             PRInt32 aCount) const;
> > 
> > How about calling this aSkipRoundedRectsCount and making it positive. Making
> > it negative is confusing to me.
> 
> It can be positive or negative - if it is positive it we draw rounded rects
> 0..aCount, if it is negative we draw aCount.. This is useful because when
> making the mask layer we want to do the former, and when drawing the items
> to the ThebesLayer we want to do the latter. However, if the above change
> works, then this ought be be simplifiable.

Instead of overloading a single parameter, pass separate start and end values to apply the rects in that range.

Comment 71

[Nick Cameron [:nrc]]

Attachment: building mask layers patch 4 (building the mask layer) - updated

Comment 72

[Nick Cameron [:nrc]]

Attachment: mask layers patch 5 - BasicLayers backend

Comment 73

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 600658 [details] [diff] [review]
building mask layers patch 4 (building the mask layer) - updated

Review of attachment 600658 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/FrameLayerBuilder.cpp
@@ +441,5 @@
> +    * The first mCommonClipCount rounded rectangle clips are identical for
> +    * all items in the layer.
> +    * -1 if there are no items in the layer; must be >=0 when the layer is drawn.
> +    */
> +  PRInt32 mCommonClipCount;

Can we put this in ThebesLayerItemsEntry instead? ThebesDisplayItemLayerUserData sticks around between paints, but ThebesLayerItemsEntry doesn't. Also it seems we have ThebesLayerItemsEntry available already where we use mCommonClipCount so we can reduce the need to get the userdata.

@@ +507,5 @@
> +  * Helper functions for getting user data and casting it to the correct type.
> +  * aLayer is the layer where the user data is stored.
> +  */
> +MaskLayerUserData* GetMaskLayerUserData(Layer* aLayer)
> +{ return static_cast<MaskLayerUserData*>(aLayer->GetUserData(&gMaskLayerUserData)); }

break into three lines

@@ +1145,5 @@
>      nsIntRegion rgn = data->mVisibleRegion;
>      rgn.MoveBy(-nsIntPoint(PRInt32(transform.x0), PRInt32(transform.y0)));
>      layer->SetVisibleRegion(rgn);
> +
> +    //try to build mask layers for color or Thebes layers

space after //

@@ +1147,5 @@
>      layer->SetVisibleRegion(rgn);
> +
> +    //try to build mask layers for color or Thebes layers
> +    if (layer->GetType() == Layer::TYPE_COLOR) {
> +      SetupMaskLayer(layer, data->mItemClip);

Don't we need to take this path for image layers as well?

How about checking "if (layer == data->mLayer) { ... do stuff with data->mLayer ... } else { SetupMaskLayer(layer, data->mItemClip); }"?

@@ +1666,5 @@
> +    ThebesDisplayItemLayerUserData* userData =
> +      GetThebesDisplayItemLayerUserData(aLayer);
> +    NS_ASSERTION(userData, "Should have user data.");
> +
> +    if (userData->mCommonClipCount > 0) {

How about making this >= 0, then we don't need another "if" after the else?

@@ +1680,5 @@
> +        }
> +        userData->mCommonClipCount = clipCount;
> +      } else {
> +        userData->mCommonClipCount = 0;
> +      }

I'm not sure why you set mCommonClipCount directly to 0 when rrLength < mCommonClipCount.

I think we can simplify this a bit, to something like this:
  mCommonClipCount = NS_MIN(mCommonClipCount, aClip.mRoundedClipRects.Length());
  while (mCommonClipCount > 0 &&
         firstClip.mRoundedClipRects[mCommonClipCount - 1] != aClip.mRoundedClipRects[mCommonClipCount - 1]) {
    --mCommonClipCount;
  }

@@ +1683,5 @@
> +        userData->mCommonClipCount = 0;
> +      }
> +    } else if (userData->mCommonClipCount < 0) {
> +      // first item in the layer
> +      userData->mCommonClipCount = aClip.mRoundedClipRects.Length();

This code can be factored out into a helper function, perhaps on ThebesLayerItemsEntry if we move mCommonClipCount there.

@@ +2408,5 @@
> +                                             PRInt32 A2D,
> +                                             PRInt32 aBegin, PRInt32 aEnd) const
> +{
> +  NS_ASSERTION(aBegin > 0, "Start index must be greater than 0.");
> +  aEnd = MinInt(aEnd, mRoundedClipRects.Length());

Just use NS_MIN<PRInt32>. MinInt is weird.

@@ +2548,5 @@
> +    aLayer->SetMaskLayer(nsnull);
> +    return;
> +  }
> +
> +  gfx3DMatrix layerTransform = aLayer->GetTransform();

const gfx3DMatrix&

@@ +2554,5 @@
> +
> +  // check if we can re-use the mask layer
> +  nsRefPtr<ImageLayer> maskLayer = CreateOrRecycleMaskImageLayer();
> +  MaskLayerUserData* userData = GetMaskLayerUserData(maskLayer);
> +  if (userData->mRoundedClipRects == aClip.mRoundedClipRects &&

Shouldn't this check involve aRoundedClipRectCount?

@@ +2562,5 @@
> +    return;
> +  }
> +
> +  // A layer with a 2D transform gets the mask in that layer's coord space
> +  gfxMatrix clipTransform;

maskTransform is probably a better name

@@ +2566,5 @@
> +  gfxMatrix clipTransform;
> +  bool is2D = layerTransform.CanDraw2D(&clipTransform);
> +  if (is2D) { 
> +    // we only use clipTransform in 2D
> +    clipTransform.Invert();

Add a comment explaining that if it's not invertible it doesn't matter what we do because nothing will render.

@@ +2567,5 @@
> +  bool is2D = layerTransform.CanDraw2D(&clipTransform);
> +  if (is2D) { 
> +    // we only use clipTransform in 2D
> +    clipTransform.Invert();
> +    clipTransform.Scale(mParameters.mXScale, mParameters.mYScale);

I actually don't understand why we're doing this clipTransform. Why don't we just always do the !is2D path?

And so, why does the layerTransform even matter and need to be checked?

@@ +2572,5 @@
> +  }
> +
> +  // if 3D, we can't transform the mask to aLayer's space, so we keep it in aLayer's
> +  // parent's space and must adjust the bounds of the mask to cope.
> +  if (!is2D) {

Use "else" after the last if

@@ +2591,5 @@
> +
> +  // fallback to an image surface if we couldn't get an optimal one
> +  if (!surface || surface->CairoStatus()) {
> +    surface = new gfxImageSurface(boundingRect.Size(),
> +                                  aLayer->Manager()->MaskImageFormat());

Is this path needed?

I don't see why it is. If the above surface creation fails, we should just assert (nonfatally) and then return no mask.

@@ +2601,5 @@
> +  NS_ASSERTION(surface, "Could not create surface for mask layer.");
> +  nsRefPtr<gfxContext> context = new gfxContext(surface);
> +
> +  gfxMatrix visRgnTranslation;
> +  visRgnTranslation.Translate(boundingRect.TopLeft()*-1);

You can just write "-boundingRect.TopLeft()".

@@ +2615,5 @@
> +  PRInt32 A2D = mContainerFrame->PresContext()->AppUnitsPerDevPixel();
> +  aClip.ApplyRoundedRectsTo(context, A2D, 0, aRoundedRectClipCount);
> +
> +  // paint through the clipping rects with alpha to create the mask
> +  context->NewPath();

This NewPath isn't needed since we just created the context.

@@ +2618,5 @@
> +  // paint through the clipping rects with alpha to create the mask
> +  context->NewPath();
> +  context->SetColor(gfxRGBA(0, 0, 0, 1));
> +  context->Paint();
> +  context->SetMatrix(gfxMatrix());

This SetMatrix isn't needed since we don't do anything with the context after this.

@@ +2633,5 @@
> +  static_cast<CairoImage*>(image.get())->SetData(data);
> +  container->SetCurrentImage(image);
> +
> +  maskLayer->SetContainer(container);
> +  maskLayer->SetTransform(gfx3DMatrix::From2D(visRgnTranslation.Invert()));

Instead of doing a full Invert, just construct the matrix that translates by boundingRect.TopLeft().

Call SetVisibleRegion on maskLayer too.

Comment 74

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 600659 [details] [diff] [review]
mask layers patch 5 - BasicLayers backend

Review of attachment 600659 [details] [diff] [review]:
-----------------------------------------------------------------

We do need to make this work with shadow layers (can be a separate patch, and possibly a separate bug). You probably should talk to BenWa/cjones on #gfx about that.

::: gfx/layers/basic/BasicLayers.cpp
@@ +213,5 @@
>  static void ContainerRemoveChild(Layer* aChild, Container* aContainer);
>  
> +// Paint the current source to a context using a mask, if present
> +void PaintWithMask(gfxContext* aContext, float aOpacity,
> +                   Layer* aMaskLayer)

Could we have a FillWithMask version of this that does the save/clip/restore dance if we actually have an aMaskLayer? That would save code in callers that only want to fill the current path, and is a bit less risky because it wouldn't always convert fills to clip/paints (the latter might be slower on some backends).

@@ +229,5 @@
> +        aContext->PushGroup(gfxASurface::CONTENT_COLOR_ALPHA);
> +        aContext->Paint(aOpacity);
> +        aContext->PopGroupToSource();
> +      }
> +      aContext->Mask(maskSurface, maskTransform.GetTranslation());

What if it's not just a translation?

If we're going to require the mask transform to be a translation, then we need to document it in Layers.h and add an assertion here and in SetMaskLayer.

@@ +1141,5 @@
>    BasicLayerManager* BasicManager()
>    {
>      return static_cast<BasicLayerManager*>(mManager);
>    }
> +  void UpdateSurface(Layer* aMaskLayer, gfxASurface* aDestSurface = nsnull);

Should not take an aMaskLayer.

@@ +1191,5 @@
>  
>    if (!mGLContext && aDestSurface) {
>      nsRefPtr<gfxContext> tmpCtx = new gfxContext(aDestSurface);
>      tmpCtx->SetOperator(gfxContext::OPERATOR_SOURCE);
> +    BasicCanvasLayer::PaintWithOpacity(tmpCtx, 1.0f, aMaskLayer);

This path is only taken when we are using shadow layers, but in that case we won't want to apply aMaskLayer here, we'll want to pass aMaskLayer over to the shadow layer compositor. So don't use aMaskLayer here, and UpdateSurface shouldn't take aMaskLayer as a parameter.

@@ +1910,5 @@
>  {
>    const nsIntRect* clipRect = aLayer->GetEffectiveClipRect();
>    const gfx3DMatrix& effectiveTransform = aLayer->GetEffectiveTransform();
> +  //aLayer might not be a container layer, but if so we take care not to use
> +  //the container variable

space after //

Comment 75

[Nick Cameron [:nrc]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #73)
> Comment on attachment 600658 [details] [diff] [review]
> building mask layers patch 4 (building the mask layer) - updated
> 
> Review of attachment 600658 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/FrameLayerBuilder.cpp
> @@ +441,5 @@
> > +    * The first mCommonClipCount rounded rectangle clips are identical for
> > +    * all items in the layer.
> > +    * -1 if there are no items in the layer; must be >=0 when the layer is drawn.
> > +    */
> > +  PRInt32 mCommonClipCount;
> 
> Can we put this in ThebesLayerItemsEntry instead?
> ThebesDisplayItemLayerUserData sticks around between paints, but
> ThebesLayerItemsEntry doesn't. Also it seems we have ThebesLayerItemsEntry
> available already where we use mCommonClipCount so we can reduce the need to
> get the userdata.
> 
I couldn't find a way to get the ThebesLayerItemsEntry in PopThebesLayerData, I could pass it in, but that would also require changing where ThebesLayerItemsEntry is defined, because it is not visible in ContainerState at the moment. Shall I do this, or leave it as is?
> @@ +1147,5 @@
> >      layer->SetVisibleRegion(rgn);
> > +
> > +    //try to build mask layers for color or Thebes layers
> > +    if (layer->GetType() == Layer::TYPE_COLOR) {
> > +      SetupMaskLayer(layer, data->mItemClip);
> 
> Don't we need to take this path for image layers as well?
> 
> How about checking "if (layer == data->mLayer) { ... do stuff with
> data->mLayer ... } else { SetupMaskLayer(layer, data->mItemClip); }"?
>
Image layers are dealt with further up.

It seemed more logical dealing with mask layers wherever the existing code deals with visible regions, but changing as you suggest (with Thebes background colour) would mean only pulling out the thebes user data once and combines the image/color layer paths, so is probably worthwhile.
> @@ +2567,5 @@
> > +  bool is2D = layerTransform.CanDraw2D(&clipTransform);
> > +  if (is2D) { 
> > +    // we only use clipTransform in 2D
> > +    clipTransform.Invert();
> > +    clipTransform.Scale(mParameters.mXScale, mParameters.mYScale);
> 
> I actually don't understand why we're doing this clipTransform. Why don't we
> just always do the !is2D path?
> 
> And so, why does the layerTransform even matter and need to be checked?
> 
We must have the mask in the same coord space in which it will be used, for Basic layers, at least (unless we use another temp surface, or there's something I've missed in Cairo for transforming masks), and the backends use different transforms for 2d and 3d, so we need to use different transforms here.
> @@ +2591,5 @@
> > +
> > +  // fallback to an image surface if we couldn't get an optimal one
> > +  if (!surface || surface->CairoStatus()) {
> > +    surface = new gfxImageSurface(boundingRect.Size(),
> > +                                  aLayer->Manager()->MaskImageFormat());
> 
> Is this path needed?
> 
> I don't see why it is. If the above surface creation fails, we should just
> assert (nonfatally) and then return no mask.
>
Having this fallback path was Bas's suggestion (unless I misunderstood), if we don't set a mask, then we won't get rounded corners, so it seems better to have slower but correct rendering, we don't really have the option of returning to the non-mask clipping path at this point. 
> @@ +2633,5 @@
> > +  static_cast<CairoImage*>(image.get())->SetData(data);
> > +  container->SetCurrentImage(image);
> > +
> > +  maskLayer->SetContainer(container);
> > +  maskLayer->SetTransform(gfx3DMatrix::From2D(visRgnTranslation.Invert()));
> 
> Instead of doing a full Invert, just construct the matrix that translates by
> boundingRect.TopLeft().
> 
The invert is optimised for matrices that are just translations anyway, so I figured the performance hit is not significant (an extra non-virtual function call vs creating another matrix) and using invert made the code clearer. Do you still prefer me to change it?

Comment 76

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Nick Cameron from comment #75)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February
> 21 to March 17) from comment #73)
> > Can we put this in ThebesLayerItemsEntry instead?
> > ThebesDisplayItemLayerUserData sticks around between paints, but
> > ThebesLayerItemsEntry doesn't. Also it seems we have ThebesLayerItemsEntry
> > available already where we use mCommonClipCount so we can reduce the need to
> > get the userdata.
> > 
> I couldn't find a way to get the ThebesLayerItemsEntry in
> PopThebesLayerData, I could pass it in, but that would also require changing
> where ThebesLayerItemsEntry is defined, because it is not visible in
> ContainerState at the moment. Shall I do this, or leave it as is?

Sure, make it visible.

> > @@ +1147,5 @@
> > >      layer->SetVisibleRegion(rgn);
> > > +
> > > +    //try to build mask layers for color or Thebes layers
> > > +    if (layer->GetType() == Layer::TYPE_COLOR) {
> > > +      SetupMaskLayer(layer, data->mItemClip);
> > 
> > Don't we need to take this path for image layers as well?
> > 
> > How about checking "if (layer == data->mLayer) { ... do stuff with
> > data->mLayer ... } else { SetupMaskLayer(layer, data->mItemClip); }"?
> >
> Image layers are dealt with further up.
> 
> It seemed more logical dealing with mask layers wherever the existing code
> deals with visible regions, but changing as you suggest (with Thebes
> background colour) would mean only pulling out the thebes user data once and
> combines the image/color layer paths, so is probably worthwhile.

Yes :-).

> > @@ +2591,5 @@
> > > +
> > > +  // fallback to an image surface if we couldn't get an optimal one
> > > +  if (!surface || surface->CairoStatus()) {
> > > +    surface = new gfxImageSurface(boundingRect.Size(),
> > > +                                  aLayer->Manager()->MaskImageFormat());
> > 
> > Is this path needed?
> > 
> > I don't see why it is. If the above surface creation fails, we should just
> > assert (nonfatally) and then return no mask.
> >
> Having this fallback path was Bas's suggestion (unless I misunderstood), if
> we don't set a mask, then we won't get rounded corners, so it seems better
> to have slower but correct rendering, we don't really have the option of
> returning to the non-mask clipping path at this point. 

I don't think we should be worrying about rounded corner rendering in OOM situations. We should limit our aspirations to not crashing :-).

> > Instead of doing a full Invert, just construct the matrix that translates by
> > boundingRect.TopLeft().
> > 
> The invert is optimised for matrices that are just translations anyway, so I
> figured the performance hit is not significant (an extra non-virtual
> function call vs creating another matrix) and using invert made the code
> clearer. Do you still prefer me to change it?

No, you're right.

Comment 77

[Nick Cameron [:nrc]]

Attachment: dx10 patch 1 (loading a mask layer as a texture in DX10)

Comment 78

[Nick Cameron [:nrc]]

Attachment: dx10 patch 2 (changes to C++ code to use shaders for masking)

Comment 79

[Nick Cameron [:nrc]]

Attachment: dx10 patch 3 (changes to shader code to use GPU masking)

Comment 80

[Nick Cameron [:nrc]]

Attachment: dx10 patch 4 (selecting a shader)

Replaces some of the functionality in DX10 patch 2, gets rid of some of the big nested if statements

Comment 81

[Nick Cameron [:nrc]]

Attachment: dx9 patch 1 (loading shaders and textures)

Comment 82

[Nick Cameron [:nrc]]

Attachment: dx9 patch 2 (using the new shaders in each layer)

Comment 83

[Nick Cameron [:nrc]]

Attachment: dx9 patch 3 (the actual shaders)

Comment 84

[Nick Cameron [:nrc]]

Attachment: dx9 patch 1 (loading shaders and textures)

Comment 85

[Nick Cameron [:nrc]]

Attachment: building masks patch 1 (member/getter/setter)

Comment 86

[Nick Cameron [:nrc]]

Attachment: building mask layers patch 4 (building the mask layer) - updated

Comment 87

[Nick Cameron [:nrc]]

Attachment: mask layers patch 5 - BasicLayers backend

Comment 88

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 604336 [details] [diff] [review]
building masks patch 1 (member/getter/setter)

Review of attachment 604336 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/Layers.h
@@ +689,5 @@
> +   * this layer's parent's transform and the mask layer's transform, but not
> +   * this layer's. That is, the mask layer is specified relative to this layer's
> +   * position in it's parent layer's coord space.
> +   * The mask layer's transform should be the 2D translation used to move the
> +   * mask over the masked layer's visible region.

Just say "Currently, only 2D translations are supported for the mask layer transform."

@@ +889,5 @@
> +    
> +  /**
> +   * computes the effective transform for a mask layer, if this layer has one
> +   */
> +  void ComputeEffectiveTransformForMaskLayer(const gfx3DMatrix& aTransformToSurface);

The implementation of this method should be in the same patch that introduces this definition.

Comment 89

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #74)
> We do need to make this work with shadow layers (can be a separate patch,
> and possibly a separate bug). You probably should talk to BenWa/cjones on
> #gfx about that.

Actually it needs to be in this bug since we mustn't break B2G.

Comment 90

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 604337 [details] [diff] [review]
building mask layers patch 4 (building the mask layer) - updated

Review of attachment 604337 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/Layers.cpp
@@ +471,5 @@
>    idealTransform.ProjectTo2D();
>    mEffectiveTransform = SnapTransform(idealTransform, gfxRect(0, 0, 0, 0), &residual);
>  
>    bool useIntermediateSurface;
> +  if (GetMaskLayer()) {

This BasicLayers change belongs in another patch I think. Preferably each prefix of the patch queue should build.

::: layout/base/FrameLayerBuilder.cpp
@@ +1618,5 @@
> +      // rounded rectangle clipping using mask layers
> +      // (must be done after visible rect is set on layer)
> +      if (aClip.IsRectClippedByRoundedCorner(itemContent)) {
> +          SetupMaskLayer(ownLayer, aClip);
> +      }

Need to call SetMaskLayer(nsnull) in the else branch otherwise the old mask layer will stick around.

@@ +1649,5 @@
> +      // check to see if the new item has rounded rect clips in common with
> +      // other items in the layer
> +      ThebesLayerData* data = GetTopThebesLayerData();
> +      if (data) {
> +        data->UpdateCommonClipCount(aClip);

You can't use GetTopThebesLayerData here. You need to use the ThebesLayerData for the layer that we actually put the item into. I suggest changing FindThebesLayerFor to return the ThebesLayerData. Then we can avoid addreffing the ThebesLayer too.

@@ +2622,5 @@
>    mRoundedClipRects.Clear();
>  }
>  
> +template<class T> bool
> +ArrayRangeEquals(nsTArray<T> a1, nsTArray<T> a2, PRUint32 aEnd)

const& references, otherwise you're copying the entire array contents!!!

Document exactly what this method checks. It seems to me that it should check that a1 and a2 have at least aEnd elements and the first aEnd elements of each array are equal, but it actually doesn't check that?

@@ +2632,5 @@
> +  }
> +
> +  PRUint32 end = NS_MIN<PRUint32>(aEnd, a1.Length());
> +  for (PRUint32 i = 0; i < end; ++i) {
> +    if (a1[i] != a2[i]) return false;

two lines

Comment 91

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 604338 [details] [diff] [review]
mask layers patch 5 - BasicLayers backend

Review of attachment 604338 [details] [diff] [review]:
-----------------------------------------------------------------

Great!

::: gfx/layers/basic/BasicLayers.cpp
@@ +216,5 @@
> +/*
> + * Extract a mask surface for a mask layer
> + * Returns a surface for the mask layer if a mask layer is present and has a
> + * valid surface and transform; nsnull otherwise.
> + * The transform for the layer will be put in aMaskTranform

aMaskTransform

Comment 92

[Nick Cameron [:nrc]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #90)
> Comment on attachment 604337 [details] [diff] [review]
> building mask layers patch 4 (building the mask layer) - updated
> 
> Review of attachment 604337 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/Layers.cpp
> @@ +471,5 @@
> >    idealTransform.ProjectTo2D();
> >    mEffectiveTransform = SnapTransform(idealTransform, gfxRect(0, 0, 0, 0), &residual);
> >  
> >    bool useIntermediateSurface;
> > +  if (GetMaskLayer()) {
> 
> This BasicLayers change belongs in another patch I think. Preferably each
> prefix of the patch queue should build.
> 

all tranforming of mask layers should now be in this patch.

> ::: layout/base/FrameLayerBuilder.cpp
> @@ +1618,5 @@
> > +      // rounded rectangle clipping using mask layers
> > +      // (must be done after visible rect is set on layer)
> > +      if (aClip.IsRectClippedByRoundedCorner(itemContent)) {
> > +          SetupMaskLayer(ownLayer, aClip);
> > +      }
> 
> Need to call SetMaskLayer(nsnull) in the else branch otherwise the old mask
> layer will stick around.
> 
This is done when a layer is taken out of recycling, in the same place that the clip is erased.

> @@ +1649,5 @@
> > +      // check to see if the new item has rounded rect clips in common with
> > +      // other items in the layer
> > +      ThebesLayerData* data = GetTopThebesLayerData();
> > +      if (data) {
> > +        data->UpdateCommonClipCount(aClip);
> 
> You can't use GetTopThebesLayerData here. You need to use the
> ThebesLayerData for the layer that we actually put the item into. I suggest
> changing FindThebesLayerFor to return the ThebesLayerData. Then we can avoid
> addreffing the ThebesLayer too.
> 
I'm not 100% sure what you mean with your suggestion, we need the ThebesLayer in this part of the code too. The updated patch returns the ThebesLayerData as well (via a reference arg).

Everything else is done.

Comment 93

[Nick Cameron [:nrc]]

Attachment: building masks patch 1 (member/getter/setter)

Comment 94

[Nick Cameron [:nrc]]

Attachment: building mask layers patch 4 (building the mask layer) - updated

Comment 95

[Nick Cameron [:nrc]]

Attachment: mask layers patch 5 (basic layers backend)

Comment 96

[Nick Cameron [:nrc]]

Attachment: building mask layers patch 4 (building the mask layer) - updated

Comment 97

[Nick Cameron [:nrc]]

Attachment: OGL patch 1 (the shaders)

Comment 98

[Nick Cameron [:nrc]]

Attachment: OGL patch 2 (managing the shaders)

Comment 99

[Nick Cameron [:nrc]]

Attachment: OGL patch 3 (Loading the mask layer)

Comment 100

[Nick Cameron [:nrc]]

Attachment: OGL patch 4 (enable mask layers)

Comment 101

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> > Need to call SetMaskLayer(nsnull) in the else branch otherwise the old mask
> > layer will stick around.
> > 
> This is done when a layer is taken out of recycling, in the same place that
> the clip is erased.

Well then, you don't need to call SetMaskLayer(nsnull) on any path in SetupMaskLayer.

> > @@ +1649,5 @@
> > > +      // check to see if the new item has rounded rect clips in common with
> > > +      // other items in the layer
> > > +      ThebesLayerData* data = GetTopThebesLayerData();
> > > +      if (data) {
> > > +        data->UpdateCommonClipCount(aClip);
> > 
> > You can't use GetTopThebesLayerData here. You need to use the
> > ThebesLayerData for the layer that we actually put the item into. I suggest
> > changing FindThebesLayerFor to return the ThebesLayerData. Then we can avoid
> > addreffing the ThebesLayer too.
> > 
> I'm not 100% sure what you mean with your suggestion, we need the
> ThebesLayer in this part of the code too. The updated patch returns the
> ThebesLayerData as well (via a reference arg).

If you just return the ThebesLayerData*, then the ThebesLayer is available in ThebesLayerData::mLayer.

Comment 102

[Nick Cameron [:nrc]]

Attachment: building mask layers patch 4 (building the mask layer) - updated

Comment 103

[Nick Cameron [:nrc]]

Attachment: OGL patch 2 (managing the shaders)

Comment 104

[Nick Cameron [:nrc]]

Attachment: dx9 patch 3 (the actual shaders)

Comment 105

[Nick Cameron [:nrc]]

Attachment: OGL patch 4 (enable mask layers)

Comment 106

[Nick Cameron [:nrc]]

Attachment: last patch: shadow layer support

Comment 107

[Nick Cameron [:nrc]]

Attachment: last patch: shadow layer support

Comment 108

[Benoit Girard (:BenWa)]

These are big changes and significantly increase the complexity of the engine (i.e. reduce maintainability):
- What do other browser use
- How do these changes affect performance with/without rounded corner. I would be interested in a quick analysis on Mobile as well?


Is it possible to use the Alpha Mask of the back buffer? This lets us do arbitrary clipping. I've used this technique before to draw a black to transparent white gradient with a complex clip:

// (1) Clear the alpha mask (or have a precondition that it's always left clear)
glDisable(SGL.GL_BLEND);
glColorMask(false, false, false, true);
// code to fill bounding rect of visible region with r=g=b=*,a=1

// (2) Draw your clip to the alpha channel
glEnable(SGL.GL_BLEND);
glBlendFunc(SGL.GL_SRC_ALPHA, SGL.GL_ONE);
// code to draw your clip, this can be any drawable shape or an image mask.

// (3) Return to drawing to your RGB channel, leave alpha untouched, enable blending with alpha channel
glColorMask(true, true, true, false);
GL.glBlendFunc(SGL.GL_DST_ALPHA, SGL.GL_ONE_MINUS_DST_ALPHA);
// code to draw something that 'clipped' to the shape in (2)

// (4) Reset your state
GL.glBlendFunc(SGL.GL_SRC_ALPHA, SGL.GL_ONE_MINUS_SRC_ALPHA);

This is adapted from one of my project, code may not work exactly but should be very close.

The reason I'm suggesting using blend function is because it keeps clipping independent to shaders. It would be nice to see the performance.

Comment 109

[Benoit Girard (:BenWa)]

Attachment: Complex clip example

Here's an example of a complex clip. The shadow is drawn by drawing the character image with a skew to the back buffer's alpha channel. Then a gradient rectangle is drawn using the back buffer's alpha channel clip to the skew character image.

Comment 110

[Benoit Girard (:BenWa)]

Actually the suggestion above needs to draw with intermediate surfaces to retain the alpha value of the destination so I'm not convinced it's better. I think we definitely need to understand how these shader changes will affect mobile.

Comment 111

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Benoit Girard (:BenWa) from comment #108)
> These are big changes and significantly increase the complexity of the
> engine (i.e. reduce maintainability):

Anything specific you're concerned about?

> - What do other browser use

Chrome seems to simply not support rounded corners on accelerated elements (canvas, video). That's a bug, obviously. IE9 works but we don't know what it does.

> - How do these changes affect performance with/without rounded corner.

Putting any one of the Microsoft 2D canvas performance demos in an IFRAME with rounded corners significantly reduces perceived frame rate (with D2D). (Interestingly, the frame rate reported by the demo script hardly changes.) These patches fix that.

We have bugs about overflow:auto elements in rounded-rect clips scrolling very slowly. (E.g. bug 623615, which we worked around by removing the clipping.) These bugs happen because we can't retain layers for the scrolled content currently. These patches fix that.

> I would be interested in a quick analysis on Mobile as well?

Recently there was a B2G issue where someone added rounded corners to a WebGL canvas and performance evaporated since we started reading back the WebGL data so we could clip it to rounded borders in software. These patches should fix that.

> The reason I'm suggesting using blend function is because it keeps clipping
> independent to shaders. It would be nice to see the performance.

What's your concern? The cost of switching shaders? Note that the shaders-with-mask are only used when there actually is a mask for the layer, and we only set masks for layers when there is "active" content clipped that needs to be clipped by a non-rectangular clip, e.g. a canvas or actively scrolling content inside a rounded-corner element. In such situations, we already generate a mask and composite with it --- but it happens on the CPU, we lose the ability to retain layers, and we have to read back any clipped contents that are on the GPU. I seriously doubt that doing this in GL is going to be a performance loss whichever way we do it, on any platform with working GL.

If you think the code could be structured better, we should definitely look into that. Keep in mind that pretty soon we want to be working on accelerated SVG filters, which will require significantly more demanding shader management, including dynamically generated shaders. Keeping the set of shaders to a small fixed number is not going to be an option.

Comment 112

[Nick Cameron [:nrc]]

Attachment: OGL patch 5 (changes to building)

Change to run genshaders.py manually and keep the generated header in the tree. genshaders.sh generates the header file (to be similar to the DirectX backends).

Comment 113

[Benoit Girard (:BenWa)]

Comment on attachment 607855 [details] [diff] [review]
OGL patch 2 (managing the shaders)

Review of attachment 607855 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just a few nits to improve readability and extendability.

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +196,4 @@
>    }
>    mPrograms.AppendElement(p);
> +
> +  if (aType < Copy2DProgramType) {

We should decide if a shader gets a mask/3d version in the shader declaration rather then here. This is harder to find, understand and could easily be missed while adding a new shader.

@@ +1077,4 @@
>  void
>  LayerManagerOGL::SetLayerProgramProjectionMatrix(const gfx3DMatrix& aMatrix)
>  {
> +  for (unsigned int i = 0; i < Copy2DProgramType; ++i) {

I don't like checking for 'Copy2DProgramType' to exclude Copy2DProgramType/Copy2DRectProgramType. The natural way to add more shaders is to add them to the end of the ShaderProgramType enum but this check here make it tricky.

::: gfx/layers/opengl/LayerManagerOGL.h
@@ +180,5 @@
>      }
>      mGLContext->MakeCurrent(aForce);
>    }
>  
> +  ShaderProgramOGL* GetBasicLayerProgram(bool aOpaque, bool aIsRGB, bool aMask)

enum instead of bool

@@ +199,4 @@
>    }
>  
> +  ShaderProgramOGL* GetProgram(gl::ShaderProgramType aType,
> +                               bool aMask = false, bool a3D = false) {

enum instead of bool

@@ +406,5 @@
> +   * Must be the same length as mPrograms, use nsnull if there is no
> +   * masked/3D masked version of the program.
> +   */
> +  nsTArray<ShaderProgramOGL*> mMaskedPrograms;
> +  nsTArray<ShaderProgramOGL*> mMasked3DPrograms;

Perhaps we should use a struct instead of an associative array.

::: gfx/layers/opengl/LayerManagerOGLProgram.cpp
@@ +31,3 @@
>  
>  /* static */ ProgramProfileOGL
> +ProgramProfileOGL::GetProfileFor(gl::ShaderProgramType aType,

Please use dual value enum instead of bool.

@@ +35,3 @@
>  {
> +  NS_ASSERTION(aType >= 0 &&
> +               aType < (aMask ? gl::Copy2DProgramType : gl::NumProgramTypes) &&

If we decide not to make shaders at and after Copy2DProgramType this will need to be changed.

@@ +37,5 @@
> +               aType < (aMask ? gl::Copy2DProgramType : gl::NumProgramTypes) &&
> +               (!a3D ||
> +               aType == gl::RGBARectLayerProgramType ||
> +               aType == gl::RGBALayerProgramType),
> +               "Invalid program type.");

This logic is very hard to read. Perhaps we should expand it into each case statement. It will make this check easier to read.

Comment 114

[Benoit Girard (:BenWa)]

Also the above patch is adding shaders load on startup that rarely get used. We should either make them lazy loaded or measure the compile time to make sure it wont regress startup.

Comment 115

[Benoit Girard (:BenWa)]

Comment on attachment 607857 [details] [diff] [review]
OGL patch 4 (enable mask layers)

Review of attachment 607857 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Comment 116

[Benoit Girard (:BenWa)]

Comment on attachment 608134 [details] [diff] [review]
OGL patch 5 (changes to building)

Review of attachment 608134 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/LayerManagerOGLShaders.h
@@ +1,4 @@
> +/* AUTOMATICALLY GENERATED from LayerManagerOGLShaders.txt*/
> +/* DO NOT EDIT! */
> +
> +const char sLayerVS[] = "/* sLayerVS */\n\

It would be nice to make these static. We don't have include guards in this header and they should only get included once. Making them static will make it so we generate less bloat:

http://glandium.org/blog/?p=2361

If you don't fix it here let me know and I'll write that patch.

Comment 117

[Bas Schouten (:bas.schouten)]

Comment on attachment 604324 [details] [diff] [review]
dx10 patch 1 (loading a mask layer as a texture in DX10)

Review of attachment 604324 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this looks alright. We rely on the backend data to keep the ShaderResourceView around though, as the functions don't return an already_AddRefed, we should document this.

::: gfx/layers/d3d10/ImageLayerD3D10.cpp
@@ +123,5 @@
> + * component.
> + */
> +ID3D10ShaderResourceView* 
> +GetImageSRView(Image* aImage, ID3D10Device* aDevice, 
> +               gfxIntSize* aSize, bool& aHasAlpha)

Please make this a member, so you don't have to pass aDevice and use gfxIntSize & or bool*.

Comment 118

[Bas Schouten (:bas.schouten)]

Comment on attachment 604325 [details] [diff] [review]
dx10 patch 2 (changes to C++ code to use shaders for masking)

Review of attachment 604325 [details] [diff] [review]:
-----------------------------------------------------------------

This actually looks like part of the DX9 patch queue, correct?

Comment 119

[Nick Cameron [:nrc]]

Attachment: dx10 patch 2 (changes to C++ code to use shaders for masking)

Now it should be the dx10 version; sorry about having the wrong version - too many patches with similar names.

Comment 120

[Nick Cameron [:nrc]]

Attachment: dx10 patch 3 (the actual shaders)

Comment 121

[Bas Schouten (:bas.schouten)]

Comment on attachment 604333 [details] [diff] [review]
dx9 patch 1 (loading shaders and textures)

Review of attachment 604333 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/d3d9/ImageLayerD3D9.cpp
@@ +318,5 @@
> +  * alpha component, false otherwise.
> +  */
> +IDirect3DTexture9* 
> +GetTexture(Image *aImage, IDirect3DDevice9* aDevice,
> +           gfxIntSize* aSize, bool& aHasAlpha)

Similar to D3D10 version comments, lets make it a private member so we don't need to pass device, and use either gfxIntSize& or bool*

::: gfx/layers/d3d9/LayerManagerD3D9.h
@@ +302,5 @@
>    }
>  
> +  /*
> +   * Returns a texture containing the contents of this
> +   * layer. Will try to return an existing texture if possible, or a temporary

If this -can- return a temporary texture it needs to return already_AddRefed to keep it alive while returning. I don't see this happening in your current implementation though so maybe you can just change the documentation.

Comment 122

[Bas Schouten (:bas.schouten)]

Right now there's a dependency from part 3 on part 1. If you check these in as separate changesets, please re-order so that each changeset will compile(and hopefully run) properly by itself.

Comment 123

[Bas Schouten (:bas.schouten)]

Comment on attachment 609139 [details] [diff] [review]
dx10 patch 2 (changes to C++ code to use shaders for masking)

Review of attachment 609139 [details] [diff] [review]:
-----------------------------------------------------------------

The general code here is good, but I think it makes a lot of stuff more unreadable.

::: gfx/layers/d3d10/CanvasLayerD3D10.cpp
@@ +264,5 @@
>  
>    ID3D10EffectTechnique *technique;
>  
> +  if (LoadMaskTexture()) {
> +    if (mDataIsPremultiplied) {

Can you try and think of a way we can clean this up a little? Possibly by using a flag system and doing something like we do in D3D9? Some string concatenation would be nice but we'd have to be sure it wasn't actually concatenating strings at runtime.

Comment 124

[Bas Schouten (:bas.schouten)]

Comment on attachment 609140 [details] [diff] [review]
dx10 patch 3 (the actual shaders)

Review of attachment 609140 [details] [diff] [review]:
-----------------------------------------------------------------

In general, .fx files are quite a powerful shader description format. We should try and share more code here and make things a little more readable, see suggestions.

::: gfx/layers/d3d10/LayerManagerD3D10.fx
@@ +153,5 @@
> +  // Xout = x + Xin * width
> +  // Yout = y + Yin * height
> +  float2 size = vLayerQuad.zw;
> +  position.x = vLayerQuad.x + aVertex.vPosition.x * size.x;
> +  position.y = vLayerQuad.y + aVertex.vPosition.y * size.y;

Several functions could share this bit of code, if possibly try moving it into a separate function and sharing it.

@@ +257,5 @@
> +
> +  color.r = yuv.g * 1.164 + yuv.r * 1.596;
> +  color.g = yuv.g * 1.164 - 0.813 * yuv.r - 0.391 * yuv.b;
> +  color.b = yuv.g * 1.164 + yuv.b * 2.018;
> +  color.a = 1.0f;

Try sharing stuff here with the normal YCbCr shaders as well!

Comment 125

[Nick Cameron [:nrc]]

(In reply to Bas Schouten (:bas) from comment #122)
> Right now there's a dependency from part 3 on part 1. If you check these in
> as separate changesets, please re-order so that each changeset will
> compile(and hopefully run) properly by itself.

I've tried my best to make each patch compile when applied in order and to eliminate dependencies like this, but there are one or two still around, worse there are some patches but give incorrect results without later ones :-( It's just got too difficult to get it 100% correct with so many interconnected patches, sorry. Hopefully I will do better next time now I know what to expect.

Also, I will have to apply all the patches at once because once I go down the mask layer patch in the build patch, rounded rects are broken until mask layers are implemented on that backend.

Comment 126

[Nick Cameron [:nrc]]

(In reply to Bas Schouten (:bas) from comment #123)
> Comment on attachment 609139 [details] [diff] [review]
> dx10 patch 2 (changes to C++ code to use shaders for masking)
> 
> Review of attachment 609139 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The general code here is good, but I think it makes a lot of stuff more
> unreadable.
> 
> ::: gfx/layers/d3d10/CanvasLayerD3D10.cpp
> @@ +264,5 @@
> >  
> >    ID3D10EffectTechnique *technique;
> >  
> > +  if (LoadMaskTexture()) {
> > +    if (mDataIsPremultiplied) {
> 
> Can you try and think of a way we can clean this up a little? Possibly by
> using a flag system and doing something like we do in D3D9? Some string
> concatenation would be nice but we'd have to be sure it wasn't actually
> concatenating strings at runtime.

I've attempted to do this in dx10 patch 4

Comment 127

[Nick Cameron [:nrc]]

Benoit and Bas, I've made all the other changes you've suggested. I'll rebase and test and upload soon. Thank you for the comments!

Comment 128

[CruNcher]

@Robert 
Their is some connection between Direct2D Performance and Frame Drops and i fear that having the fix for the Addon Manager means activating the Direct2D part which would result in the Multimedia Scenario i explained here in this bug to be heavily affected negatively :(
https://bugzilla.mozilla.org/show_bug.cgi?id=702485
don't force 1 for the other :(

Comment 129

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This will help whether D2D is turned on or not.

Comment 130

[CruNcher]

Adapter DescriptionIntel(R) HD GraphicsVendor ID0x8086Device ID0x0102Adapter RAMUnknownAdapter Driversigdumd64 igd10umd64 igd10umd64 igdumd32 igd10umd32 igd10umd32Driver Version8.15.10.2696Driver Date3-19-2012Direct2D EnabledfalseDirectWrite Enabledfalse (6.1.7601.17776)ClearType ParametersClearType parameters not foundWebGL RendererGoogle Inc. -- ANGLE (Intel(R) HD Graphics) -- OpenGL ES 2.0 (ANGLE 1.0.0.963)GPU Accelerated Windows1/1 Direct3D 9

btw this happens if you copy the support information (a small feature request would be to make a copy this to clipboard button which copies it correctly formated so it can be easily cut and pasted ;) )

Adapter Description Intel(R) HD Graphics
Vendor ID0 x8086
Device ID0 x0102
Adapter RAM Unknown
Adapter Drivers igdumd64 igd10umd64 igd10umd64 igdumd32 igd10umd32 igd10umd32
Driver Version 8.15.10.2696
Driver Date 3-19-2012 
Direct2D Enabled false
DirectWrite Enabled false (6.1.7601.17776) 
ClearType Parameters ClearType parameters not found
WebGL Renderer Google Inc. -- ANGLE (Intel(R) HD Graphics) -- OpenGL ES 2.0 (ANGLE 1.0.0.963)
GPU Accelerated Windows1/1 Direct3D 9

Comment 131

[Nick Cameron [:nrc]]

Try run:

https://tbpl.mozilla.org/?tree=Try&rev=234abbef8624

Comment 132

[Nick Cameron [:nrc]]

Attachment: patch 1: remove code to enable mask layers for rounded rects

Rebased, no changes. Carrying the r=roc

Comment 133

[Nick Cameron [:nrc]]

Attachment: building masks patch 3 (recycling)

Changed to recycle mask layers for different layer types separately

Comment 134

[Nick Cameron [:nrc]]

Attachment: building masks patch 1 (member/getter/setter)

Small change: setter takes a Layer* as arg, instead of alread_addRefed<Layer>

Comment 135

[Nick Cameron [:nrc]]

Attachment: mask layers patch 5 - BasicLayers backend

rebased, carrying r=roc

Comment 136

[Nick Cameron [:nrc]]

Attachment: dx10 patch 1 (loading a mask layer as a texture in DX10)

Addressed Bas's comments and rebased; carrying r=Bas

Comment 137

[Nick Cameron [:nrc]]

Attachment: dx10 patch 2 (using the new shaders in each layer)

rebased and addressed comments, awaiting re-review

Comment 138

[Nick Cameron [:nrc]]

Attachment: dx10 patch 3 (the actual shaders)

Rebased and addressed review comments, awating re-review

Comment 139

[Nick Cameron [:nrc]]

Attachment: dx10 patch 4 (selecting a shader)

rebased, awaiting review (this patch hasn't been reviewed yet)

Comment 140

[Nick Cameron [:nrc]]

Attachment: dx9 patch 1 (loading shaders and textures)

rebased and addressed comments, carrying r=Bas

Comment 141

[Nick Cameron [:nrc]]

Attachment: dx9 patch 2 (using the new shaders in each layer)

rebased and addressed comments, carrying r=Bas

Comment 142

[Nick Cameron [:nrc]]

Attachment: dx9 patch 3 (the actual shaders)

rebased and addressed comments, carrying r=Bas

Comment 143

[Nick Cameron [:nrc]]

Attachment: building mask layers patch 4 (building the mask layer)

rebased and fixed a small - failing to initialise a field; carrying r=roc

Comment 144

[Nick Cameron [:nrc]]

Attachment: OGL patch 1 (the shaders)

awaiting review

Comment 145

[Nick Cameron [:nrc]]

Attachment: OGL patch 2 (managing the shaders)

Addressed comments and rebased, awating re-review

Comment 146

[Nick Cameron [:nrc]]

Attachment: OGL patch 3 (Loading the mask layer)

Comment 147

[Nick Cameron [:nrc]]

Attachment: OGL patch 4 (enable mask layers)

Minor change (from a comment on a different patch): use flags rather than bools for the different kinds of mask. Also rebased.

Comment 148

[Nick Cameron [:nrc]]

Attachment: OGL patch 5 (changes to building)

Rebased and changed to reflect other patches (macro-processed code), carrying r=BenWa

Comment 149

[Nick Cameron [:nrc]]

Attachment: support shadow layers

Lots of changes, and never reviewed

Comment 150

[Nick Cameron [:nrc]]

Attachment: changes to tests

Comment 151

[Nick Cameron [:nrc]]

Attachment: OGL patch 1.5: change to glContext

Comment 152

[Nick Cameron [:nrc]]

Attachment: support shadow layers

Comment 153

[Nick Cameron [:nrc]]

Attachment: support shadow layers

Comment 154

[Benoit Girard (:BenWa)]

Comment on attachment 619747 [details] [diff] [review]
OGL patch 1 (the shaders)

Review of attachment 619747 [details] [diff] [review]:
-----------------------------------------------------------------

Here's some initial feedback. I'm going to hold of reviewing the shaders themselves until I can compare with the generated shaders.

::: gfx/layers/opengl/LayerManagerOGLShaders.txt
@@ +62,5 @@
> +// @shader may have a single parameter with multiple values using
> +// <param:val1,val2> an empty value is allowed. The shader is expanded for
> +// each value, using <param> in the body of the shader will be expanded to
> +// the current value, as will the declaration of the parameter. The name of
> +// defines may include <...> and this should work as expected

I don't see any examples of <...> and I'm not sure what is expected. Can you clarify that comment?

@@ +84,5 @@
>  varying vec2 vTexCoord;
>  #endif
> +@end
> +
> +@define VERTEX_SHADER_HEADER<Mask>

Changes to this file make it a lot more complex getting an idea of what shaders you're generating. Can you add a comment to this file explaining that this file gets compiled to 'objdir/gfx/layers/LayerManagerOGLShaders.h' which can be checked to see the results of the compilation.

It would make the review process easier if you could attach that sample so that I can compare the generated vs. the source. I'm not a fan of the template-ish changes here since you're asking someone to learn a new simple grammar to save a few repetition. I'm not sure how others feel about this.

Comment 155

[Nick Cameron [:nrc]]

(In reply to Benoit Girard (:BenWa) from comment #154)
> Comment on attachment 619747 [details] [diff] [review]
> OGL patch 1 (the shaders)
> 
> Review of attachment 619747 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Here's some initial feedback. I'm going to hold of reviewing the shaders
> themselves until I can compare with the generated shaders.
> 
> ::: gfx/layers/opengl/LayerManagerOGLShaders.txt
> @@ +62,5 @@
> > +// @shader may have a single parameter with multiple values using
> > +// <param:val1,val2> an empty value is allowed. The shader is expanded for
> > +// each value, using <param> in the body of the shader will be expanded to
> > +// the current value, as will the declaration of the parameter. The name of
> > +// defines may include <...> and this should work as expected
> 
> I don't see any examples of <...> and I'm not sure what is expected. Can you
> clarify that comment?
> 

I can try to clarify it yes, <...> appear in, e.g., lines 95->114

> @@ +84,5 @@
> >  varying vec2 vTexCoord;
> >  #endif
> > +@end
> > +
> > +@define VERTEX_SHADER_HEADER<Mask>
> 
> Changes to this file make it a lot more complex getting an idea of what
> shaders you're generating. Can you add a comment to this file explaining
> that this file gets compiled to 'objdir/gfx/layers/LayerManagerOGLShaders.h'
> which can be checked to see the results of the compilation.

Shall do
> 
> It would make the review process easier if you could attach that sample so
> that I can compare the generated vs. the source. I'm not a fan of the
> template-ish changes here since you're asking someone to learn a new simple
> grammar to save a few repetition. I'm not sure how others feel about this.

The compiled shaders are in OGL patch 5 (which you've looked at already, comment 116). We had a bit of a discussion on IRC and I think this got the thumbs up. There is also an alternative just using the C pre-processor that jgilbert did, but it needs a little bit of work, if we prefer that approach we can swap it in later, but I didn't want to delay this stuff for that, I'll file a bug at some point soon to decide between the various options.

Comment 156

[Nick Cameron [:nrc]]

Attachment: OGL patch 1 (the shaders)

Added comments as requested by BenWa

Comment 157

[CruNcher]

Some results testing with https://developer.mozilla.org/media/uploads/demos/p/a/paulrouget/8bfba7f0b6c62d877a2b82dd5e10931e/hacksmozillaorg-achi_1334270447_demo_package/HWACCEL/ on Intel HD 2000 

14eeb79a91ca = only 2 images of the wheel get rendered so 60+ fps
9dbe02adbed1 = 29 fps
ce953c7f7a59 = Rendering is ok 60+ FPS

Comment 158

[CruNcher]

Testing https://bugzilla.mozilla.org/show_bug.cgi?id=702485

14eeb79a91ca = low drop rate
9dbe02adbed1 = low drop rate
ce953c7f7a59 = high drop rate

Comment 159

[Benoit Girard (:BenWa)]

Comment on attachment 619821 [details] [diff] [review]
OGL patch 1 (the shaders)

Review of attachment 619821 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/LayerManagerOGLShaders.txt
@@ +86,5 @@
> +// string name = "Name2";
> +// @end
> +//
> +// This will be straightaway compiled to two constant strings named
> +// ShaderName1 and ShaderName2.

Nice, this example is very helpful.

Comment 160

[Benoit Girard (:BenWa)]

Comment on attachment 619748 [details] [diff] [review]
OGL patch 2 (managing the shaders)

Review of attachment 619748 [details] [diff] [review]:
-----------------------------------------------------------------

Unless we have a compelling case for landing this now I'd rather move shader loading on demand.

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +184,5 @@
>    return context.forget();
>  }
>  
>  bool
> +LayerManagerOGL::InitAndAddPrograms(ShaderProgramType aType)

I did a quick test and we take 13ms to compile 11 shaders on OSX and I imagine this is worse on mobile. Let's move this on demand now and we'll go from a startup regression to a startup improvement. I imagine the difference is even greater on mobile.

Comment 161

[Benoit Girard (:BenWa)]

Comment on attachment 619749 [details] [diff] [review]
OGL patch 3 (Loading the mask layer)

Review of attachment 619749 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/ImageLayerOGL.cpp
@@ +592,5 @@
> +}
> +
> +
> +// shares a lot of code with RenderLayer, but it doesn't seem to be possible
> +// to factor it out into a helper method

Nit: This comment doesn't document the method, maybe move it inside.

::: gfx/layers/opengl/LayerManagerOGL.h
@@ +535,5 @@
> +   * aSize will contain the size of the image.
> +   */
> +  virtual bool LoadAsTexture(GLuint aTextureUnit, gfxIntSize* aSize)
> +  {
> +    return false;

If you don't expect this to be called during normal execution we should have a debug assertion.

::: gfx/layers/opengl/LayerManagerOGLProgram.cpp
@@ +334,5 @@
> +
> +bool
> +ShaderProgramOGL::LoadMask(Layer* aMaskLayer)
> +{
> +  if (aMaskLayer) {

If there a good reason to have a null check here? Normally I think it would be safe to leave that up to the caller.

@@ +345,5 @@
> +    SetUniform(mProfile.LookupUniformLocation("uMaskTexture"),
> +               (GLint)(mProfile.mTextureCount - 1));
> +
> +    gfxMatrix maskTransform;
> +    bool maskIs2D = aMaskLayer->GetEffectiveTransform().CanDraw2D(&maskTransform);

Nit: is2DMask or isMask2D

@@ +346,5 @@
> +               (GLint)(mProfile.mTextureCount - 1));
> +
> +    gfxMatrix maskTransform;
> +    bool maskIs2D = aMaskLayer->GetEffectiveTransform().CanDraw2D(&maskTransform);
> +    NS_ASSERTION(maskIs2D, "How did we end up with a 3D transform here?!");

Is not clear that LoadMask is for 2D transforms only. I can't see from the context of this patch where that is checked.

::: gfx/layers/opengl/LayerManagerOGLProgram.h
@@ +210,5 @@
>    GLint GetTexCoordMultiplierUniformLocation() {
>      return mTexCoordMultiplierUniformLocation;
>    }
>  
> +  bool LoadMask(Layer* aLayer);

This could use a bit of documentation. From what I understand you're loading the layer as a texture mask in the next free texture unit.

::: layout/base/FrameLayerBuilder.cpp
@@ +2763,5 @@
>    gfxMatrix scale;
>    scale.Scale(mParameters.mXScale, mParameters.mYScale);
>    context->Multiply(scale);
>    
> +  // useful for debugging, make masked areas semi-opaque

neat

Comment 162

[Benoit Girard (:BenWa)]

Comment on attachment 619749 [details] [diff] [review]
OGL patch 3 (Loading the mask layer)

Review of attachment 619749 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/LayerManagerOGLProgram.cpp
@@ +334,5 @@
> +
> +bool
> +ShaderProgramOGL::LoadMask(Layer* aMaskLayer)
> +{
> +  if (aMaskLayer) {

Nvm, I see why in part 4. Nit: I think I personally like this better to reduce indentation:
if (!aMaskLayer)
  return false;

Comment 163

[Benoit Girard (:BenWa)]

Comment on attachment 619752 [details] [diff] [review]
OGL patch 4 (enable mask layers)

Review of attachment 619752 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Comment 164

[Bas Schouten (:bas.schouten)]

Comment on attachment 619734 [details] [diff] [review]
dx10 patch 2 (using the new shaders in each layer)

Review of attachment 619734 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/d3d10/CanvasLayerD3D10.cpp
@@ +272,5 @@
> +        if (mFilter == gfxPattern::FILTER_NEAREST) {
> +          technique = effect()->GetTechniqueByName("RenderRGBLayerPremulPointMask");
> +        } else {
> +          technique = effect()->GetTechniqueByName("RenderRGBLayerPremulMask");
> +        }

I thought we agreed to do something clever with masks here like we're doing in D3D9?

::: gfx/layers/d3d10/ImageLayerD3D10.cpp
@@ +216,5 @@
> +        if (mFilter == gfxPattern::FILTER_NEAREST) {
> +          technique = effect()->GetTechniqueByName("RenderRGBALayerPremulPointMask");
> +        } else {
> +          technique = effect()->GetTechniqueByName("RenderRGBALayerPremulMask");
> +        }

Indent's off..

And also, flags would make this nicer too!

Comment 165

[Benoit Girard (:BenWa)]

Comment on attachment 619776 [details] [diff] [review]
support shadow layers

Review of attachment 619776 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent!

::: gfx/layers/basic/BasicLayers.cpp
@@ +2726,5 @@
>  
> +void
> +BasicShadowableColorLayer::Paint(gfxContext* aContext, Layer* aMaskLayer)
> +{
> +  // TODO[nrc] move this back inside the if, and clear the context some other way

Just wondering: Do you plan on fixing this before landing and/or soon?

Comment 166

[Bas Schouten (:bas.schouten)]

Comment on attachment 619738 [details] [diff] [review]
dx10 patch 4 (selecting a shader)

Review of attachment 619738 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, you rolled it in here, good :)

::: gfx/layers/d3d10/LayerManagerD3D10.cpp
@@ +886,5 @@
> +    return effect()->GetTechniqueByName("RenderYCbCrLayer");
> +  default:
> +    NS_ERROR("Invalid shader.");
> +    return nsnull;
> +  }

I wonder how efficient GetTechniqueByName is, please add a comment to that extent to we make sure to check at some point and use a more efficient method if need be.

Comment 167

[Nick Cameron [:nrc]]

(In reply to CruNcher from comment #158)
> Testing https://bugzilla.mozilla.org/show_bug.cgi?id=702485
> 
> 14eeb79a91ca = low drop rate
> 9dbe02adbed1 = low drop rate
> ce953c7f7a59 = high drop rate

None of those try runs are for this bug. They are testing canvas patches on various other bugs.

Comment 168

[Nick Cameron [:nrc]]

(In reply to Benoit Girard (:BenWa) from comment #160)
> Comment on attachment 619748 [details] [diff] [review]
> OGL patch 2 (managing the shaders)
> 
> Review of attachment 619748 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Unless we have a compelling case for landing this now I'd rather move shader
> loading on demand.
> 
> ::: gfx/layers/opengl/LayerManagerOGL.cpp
> @@ +184,5 @@
> >    return context.forget();
> >  }
> >  
> >  bool
> > +LayerManagerOGL::InitAndAddPrograms(ShaderProgramType aType)
> 
> I did a quick test and we take 13ms to compile 11 shaders on OSX and I
> imagine this is worse on mobile. Let's move this on demand now and we'll go
> from a startup regression to a startup improvement. I imagine the difference
> is even greater on mobile.

My testing on MacOS gives 15ms before my patches (I guess that's the same 11 shaders) and 27ms with my patches, i.e., nearly double, or my patches add 12ms in shader compilation time to startup.

Comment 169

[CruNcher]

Yep i gonna test it on some of those also like the Addon Manager overhead or the ajive testcases heavy gpu frequency fluctuations http://img713.imageshack.us/img713/6710/firefoxd2d.png , i just wanted to correct a mistake i did when looking @ your last try build which also forced D3D9 and so also fixed the drop rate which of course was nonsense it doesn't fix this D2D performance overhead issue.(In reply to Nick Cameron [:nrc] from comment #167)
> (In reply to CruNcher from comment #158)
> > Testing https://bugzilla.mozilla.org/show_bug.cgi?id=702485
> > 
> > 14eeb79a91ca = low drop rate
> > 9dbe02adbed1 = low drop rate
> > ce953c7f7a59 = high drop rate
> 
> None of those try runs are for this bug. They are testing canvas patches on
> various other bugs.

Yep i gonna test it on some of those also like the Addon Manager overhead or the ajive testcases heavy gpu frequency fluctuations http://img713.imageshack.us/img713/6710/firefoxd2d.png , i just wanted to correct a mistake i did when looking @ your last try build which also forced D3D9 and so also fixed the drop rate which of course was nonsense it doesn't fix this D2D performance overhead issue.

Comment 170

[Nick Cameron [:nrc]]

Attachment: OGL patch 6: on-demand compilation of shaders

Comment 171

[Nick Cameron [:nrc]]

Attachment: OGL patch 3 (Loading the mask layer)

Comment 172

[Nick Cameron [:nrc]]

> ::: gfx/layers/opengl/LayerManagerOGLProgram.cpp
> @@ +334,5 @@
> > +
> > +bool
> > +ShaderProgramOGL::LoadMask(Layer* aMaskLayer)
> > +{
> > +  if (aMaskLayer) {
> 
> If there a good reason to have a null check here? Normally I think it would
> be safe to leave that up to the caller.
> 

The pattern is to call LoadMask(GetMaskLayer()), which may be null to indicate no mask, I figured this was better than always forcing the client to check for a mask layer (easy to forget).

> @@ +346,5 @@
> > +               (GLint)(mProfile.mTextureCount - 1));
> > +
> > +    gfxMatrix maskTransform;
> > +    bool maskIs2D = aMaskLayer->GetEffectiveTransform().CanDraw2D(&maskTransform);
> > +    NS_ASSERTION(maskIs2D, "How did we end up with a 3D transform here?!");
> 
> Is not clear that LoadMask is for 2D transforms only. I can't see from the
> context of this patch where that is checked.
> 
The way ComputeEffectiveTransform works, we should never get a 3D transform here, and we make sure where we create the mask layer that it's transform is 2D. This is documented (briefly) on Layers::SetMaskLayer. I've added a few words to the documentation of this method about it too. Let me know if you think we need more.

Everything else is done in the updated patch

Comment 173

[Nick Cameron [:nrc]]

(In reply to Benoit Girard (:BenWa) from comment #165)
> ::: gfx/layers/basic/BasicLayers.cpp
> @@ +2726,5 @@
> >  
> > +void
> > +BasicShadowableColorLayer::Paint(gfxContext* aContext, Layer* aMaskLayer)
> > +{
> > +  // TODO[nrc] move this back inside the if, and clear the context some other way
> 
> Just wondering: Do you plan on fixing this before landing and/or soon?

No, not at all for now, I have removed the TODO.

The reasoning is that, although this is a bit of a waste, it is fairly cheap, because it is only a colour layer. Figuring out why it is necessary is hard and will probably be very time consuming, and it is no worse than before the patch (i.e., we did this all the time before).

Comment 174

[Nick Cameron [:nrc]]

Attachment: misc. small changes for reviewers

Comment 175

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 620208 [details] [diff] [review]
misc. small changes for reviewers

Review of attachment 620208 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/basic/BasicImplData.h
@@ +108,5 @@
> +  virtual already_AddRefed<gfxASurface> GetAsSurface()
> +  {
> +    NS_WARNING("GetAsSurface called without being overriden");
> +    return nsnull;
> +  }

Can't this be declared pure virtual so the compiler enforces that it's overridden?

::: gfx/layers/d3d10/LayerManagerD3D10.h
@@ +307,1 @@
>      return nsnull;

Ditto

::: gfx/layers/d3d9/LayerManagerD3D9.h
@@ +318,1 @@
>      return nsnull;

Ditto

Comment 176

[Nick Cameron [:nrc]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #175)
> Comment on attachment 620208 [details] [diff] [review]
> misc. small changes for reviewers
> 
> Review of attachment 620208 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/basic/BasicImplData.h
> @@ +108,5 @@
> > +  virtual already_AddRefed<gfxASurface> GetAsSurface()
> > +  {
> > +    NS_WARNING("GetAsSurface called without being overriden");
> > +    return nsnull;
> > +  }
> 
> Can't this be declared pure virtual so the compiler enforces that it's
> overridden?

I wanted to avoid writing the same thing above for each flavour of layer class, when at the moment only image layers need non-trivial implementations

Comment 177

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 620208 [details] [diff] [review]
misc. small changes for reviewers

Review of attachment 620208 [details] [diff] [review]:
-----------------------------------------------------------------

Change the warnings to say "Not yet implemented".

Comment 178

[Benoit Girard (:BenWa)]

Comment on attachment 620188 [details] [diff] [review]
OGL patch 6: on-demand compilation of shaders

Review of attachment 620188 [details] [diff] [review]:
-----------------------------------------------------------------

Yay for on-demands compilation, just a few things to fix first.

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +225,3 @@
>    }
>  
> +  // initialise a common shader to check that we can actually compile a shader

That's a good idea, but also this reminds me that I didn't check if the first shader to compile had a larger cost. In any case it's an improvement over central.

@@ +1087,5 @@
>  
>  void
>  LayerManagerOGL::SetLayerProgramProjectionMatrix(const gfx3DMatrix& aMatrix)
>  {
> +  mProjectionMatrix = aMatrix;

I really don't like this line. It's not clear that aMatrix isn't cleared before mProjectionMatrix is null-ed later and even if it is, this is very fragile.

::: gfx/layers/opengl/LayerManagerOGLProgram.h
@@ +322,5 @@
> +  /*
> +   * the OpenGL id of the program 
> +   * mProgram > 0 is a valid id
> +   * mProgram == 0 => program has not been initialised
> +   * mProgram < 0 => there was an error initialising the program

mProgram is an unsigned int so 'mProgram < 0' doesn't make much sense. Maybe we want an enum?

Comment 179

[Nick Cameron [:nrc]]

Attachment: misc. small changes for reviewers

Changed a text string, carrying r=roc

Comment 180

[Nick Cameron [:nrc]]

(In reply to Benoit Girard (:BenWa) from comment #178)
> Comment on attachment 620188 [details] [diff] [review]
> OGL patch 6: on-demand compilation of shaders
> 
> Review of attachment 620188 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yay for on-demands compilation, just a few things to fix first.
> 
> ::: gfx/layers/opengl/LayerManagerOGL.cpp
> @@ +225,3 @@
> >    }
> >  
> > +  // initialise a common shader to check that we can actually compile a shader
> 
> That's a good idea, but also this reminds me that I didn't check if the
> first shader to compile had a larger cost. In any case it's an improvement
> over central.
> 
I picked a shader that must get compiled in order to render anything at all, so it is getting compiled at startup one way or another, so the cost is kind of irrelevant (unless we are willing to initially composite with software to reduce startup time or something, but that sounds like a lot of work).

> @@ +1087,5 @@
> >  
> >  void
> >  LayerManagerOGL::SetLayerProgramProjectionMatrix(const gfx3DMatrix& aMatrix)
> >  {
> > +  mProjectionMatrix = aMatrix;
> 
> I really don't like this line. It's not clear that aMatrix isn't cleared
> before mProjectionMatrix is null-ed later and even if it is, this is very
> fragile.
> 

mProjectionMatrix is not nulled later, only the shader program's local pointer to it, mProjectionMatrix is an object, not a reference or pointer, so it is a copy of aMatrix, so it doesn't matter what happens to aMatrix. Is it still fragile, or is that OK?

> ::: gfx/layers/opengl/LayerManagerOGLProgram.h
> @@ +322,5 @@
> > +  /*
> > +   * the OpenGL id of the program 
> > +   * mProgram > 0 is a valid id
> > +   * mProgram == 0 => program has not been initialised
> > +   * mProgram < 0 => there was an error initialising the program
> 
> mProgram is an unsigned int so 'mProgram < 0' doesn't make much sense. Maybe
> we want an enum?

doh! Yes, enum.

Comment 181

[Nick Cameron [:nrc]]

Try run with lazy compilation of shaders: https://tbpl.mozilla.org/?tree=Try&rev=eadd3dcc9680

Comment 182

[Nick Cameron [:nrc]]

Attachment: OGL patch 6: on-demand compilation of shaders

Comment 183

[Benoit Girard (:BenWa)]

Comment on attachment 620579 [details] [diff] [review]
OGL patch 6: on-demand compilation of shaders

Review of attachment 620579 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good :)

::: gfx/layers/opengl/LayerManagerOGLProgram.cpp
@@ +197,5 @@
>  
>  bool
>  ShaderProgramOGL::Initialize()
>  {
> +  NS_ASSERTION( mProgramState == STATE_NEW, "Shader program has already been initialised");

Spacing nit, not that it really matters.

Comment 184

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3443acc097c
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bb9fcabf53c
https://hg.mozilla.org/integration/mozilla-inbound/rev/a346891ea6db
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ca533ac7ddd
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7b8deeb0cc4
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3dace261181
https://hg.mozilla.org/integration/mozilla-inbound/rev/563f352362d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/30336485e3c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/16f39df9be8b
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2eefdb6f9d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/82ad8aacc669
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc33273a36cc
https://hg.mozilla.org/integration/mozilla-inbound/rev/01ac1ce7d926
https://hg.mozilla.org/integration/mozilla-inbound/rev/e286076d78b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/76d50f302206
https://hg.mozilla.org/integration/mozilla-inbound/rev/10263ce3532a
https://hg.mozilla.org/integration/mozilla-inbound/rev/f248836433c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/01b9dc47a6c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/44708c07084e
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b27def0885e
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c6adccfeb41
https://hg.mozilla.org/integration/mozilla-inbound/rev/77bf50b33a05

Comment 185

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f3443acc097c
https://hg.mozilla.org/mozilla-central/rev/8bb9fcabf53c
https://hg.mozilla.org/mozilla-central/rev/a346891ea6db
https://hg.mozilla.org/mozilla-central/rev/3ca533ac7ddd
https://hg.mozilla.org/mozilla-central/rev/f7b8deeb0cc4
https://hg.mozilla.org/mozilla-central/rev/c3dace261181
https://hg.mozilla.org/mozilla-central/rev/563f352362d7
https://hg.mozilla.org/mozilla-central/rev/30336485e3c9
https://hg.mozilla.org/mozilla-central/rev/16f39df9be8b
https://hg.mozilla.org/mozilla-central/rev/d2eefdb6f9d4
https://hg.mozilla.org/mozilla-central/rev/82ad8aacc669
https://hg.mozilla.org/mozilla-central/rev/bc33273a36cc
https://hg.mozilla.org/mozilla-central/rev/01ac1ce7d926
https://hg.mozilla.org/mozilla-central/rev/e286076d78b7
https://hg.mozilla.org/mozilla-central/rev/76d50f302206
https://hg.mozilla.org/mozilla-central/rev/10263ce3532a
https://hg.mozilla.org/mozilla-central/rev/f248836433c5
https://hg.mozilla.org/mozilla-central/rev/01b9dc47a6c4
https://hg.mozilla.org/mozilla-central/rev/44708c07084e
https://hg.mozilla.org/mozilla-central/rev/0b27def0885e
https://hg.mozilla.org/mozilla-central/rev/0c6adccfeb41
https://hg.mozilla.org/mozilla-central/rev/77bf50b33a05

(d2eefdb6f9d4 landed with bug 716438 instead in the commit message, have commented in that bug redirecting this way)

Comment 186

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Removed now unused variable:

https://hg.mozilla.org/mozilla-central/rev/16405bf04a10