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.

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.

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.

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?

(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.)

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?

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.

(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.

Please do.

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.

Added masking for color layers; did a little refactoring. Still TODO: shadowable layer masking and the effective mask optimisation.

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

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.

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.

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 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 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 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 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?

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

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

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 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 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.

(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.

(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.

(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.

Minor changes to the DX10 patches - polishing only.

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 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 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 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 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 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 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 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.

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.

(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.

Now with comment, sorry, jet-lagged.

(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.

(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.

(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 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 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 //

(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?

(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.

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

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.

(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 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 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

(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.

> > 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.

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.

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.

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.

(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.

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 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.

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 on attachment 607857 [details] [diff] [review] OGL patch 4 (enable mask layers) Review of attachment 607857 [details] [diff] [review]: ----------------------------------------------------------------- Looks good!

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 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 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?

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

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.

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 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 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!

(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.

(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