Closed Bug 716439 (GPU-clipping-rounded) Opened 13 years ago Closed 13 years ago

Implement clipping to rectangles with rounded corners on the GPU

Categories

(Core :: Graphics, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: nrc, Assigned: nrc)

References

Details

(Keywords: css3)

Attachments

(26 files, 72 obsolete files)

(deleted), text/html
Details
(deleted), text/html
Details
(deleted), patch
nrc
: review+
Details | Diff | Splinter Review
(deleted), image/png
Details
(deleted), patch
nrc
: review+
Details | Diff | Splinter Review
(deleted), patch
roc
: review+
Details | Diff | Splinter Review
(deleted), patch
roc
: review+
Details | Diff | Splinter Review
(deleted), patch
nrc
: review+
Details | Diff | Splinter Review
(deleted), patch
nrc
: review+
Details | Diff | Splinter Review
(deleted), patch
bas.schouten
: review+
Details | Diff | Splinter Review
(deleted), patch
bas.schouten
: review+
Details | Diff | Splinter Review
(deleted), patch
bas.schouten
: review+
Details | Diff | Splinter Review
(deleted), patch
nrc
: review+
Details | Diff | Splinter Review
(deleted), patch
nrc
: review+
Details | Diff | Splinter Review
(deleted), patch
nrc
: review+
Details | Diff | Splinter Review
(deleted), patch
nrc
: review+
Details | Diff | Splinter Review
(deleted), patch
BenWa
: review+
Details | Diff | Splinter Review
(deleted), patch
BenWa
: review+
Details | Diff | Splinter Review
(deleted), patch
nrc
: review+
Details | Diff | Splinter Review
(deleted), patch
roc
: review+
Details | Diff | Splinter Review
(deleted), patch
BenWa
: review+
Details | Diff | Splinter Review
(deleted), patch
BenWa
: review+
Details | Diff | Splinter Review
(deleted), patch
BenWa
: review+
Details | Diff | Splinter Review
(deleted), patch
BenWa
: review+
Details | Diff | Splinter Review
(deleted), patch
nrc
: review+
Details | Diff | Splinter Review
(deleted), patch
BenWa
: review+
Details | Diff | Splinter Review
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?
Depends on: 718521
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.
Attachment #590015 - Attachment is obsolete: true
Attachment #590015 - Flags: review?(roc)
Attached patch Using a mask layer with the software backend (obsolete) (deleted) — Splinter Review
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.
Attachment #593681 - Flags: feedback?
Attachment #593681 - Flags: feedback? → ui-review?(roc)
Attachment #593681 - Flags: ui-review?(roc) → review?(roc)
Attachment #593681 - Flags: review?(bas.schouten)
Attached patch Mask layers w/ software backend (obsolete) (deleted) — Splinter Review
Added masking for color layers; did a little refactoring. Still TODO: shadowable layer masking and the effective mask optimisation.
Attachment #593681 - Attachment is obsolete: true
Attachment #593681 - Flags: review?(roc)
Attachment #593681 - Flags: review?(bas.schouten)
Attachment #593964 - Flags: review?(roc)
Attachment #593964 - Flags: review?(bas.schouten)
Attached patch store a mask layer in a layer (obsolete) (deleted) — Splinter Review
I've split the large patch into multiple smaller ones, uploading them in order...
Attachment #593964 - Attachment is obsolete: true
Attachment #593964 - Flags: review?(roc)
Attachment #593964 - Flags: review?(bas.schouten)
Attachment #594807 - Flags: review?(roc)
Attached patch user data for mask layers (obsolete) (deleted) — Splinter Review
Attachment #594808 - Flags: review?(roc)
Attached patch recycling for mask layers (obsolete) (deleted) — Splinter Review
Attachment #594809 - Flags: review?(roc)
Attachment #594810 - Flags: review?(roc)
Attached patch frontend for mask layers (obsolete) (deleted) — Splinter Review
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.
Attachment #594813 - Flags: review?(roc)
Attached patch mask layers in the software backend (obsolete) (deleted) — Splinter Review
Attachment #594815 - Flags: review?(roc)
Attached patch member/getter/setter - bugfix (obsolete) (deleted) — Splinter Review
Attachment #594807 - Attachment is obsolete: true
Attachment #594807 - Flags: review?(roc)
Attachment #595194 - Flags: review?(roc)
Attached patch user data for mask layers - bugfix (obsolete) (deleted) — Splinter Review
Attachment #594808 - Attachment is obsolete: true
Attachment #594808 - Flags: review?(roc)
Attachment #595195 - Flags: review?(roc)
Attachment #595194 - Attachment description: bugfix → member/getter/setter - bugfix
Attached patch building mask layers - bugfix (obsolete) (deleted) — Splinter Review
Attachment #594813 - Attachment is obsolete: true
Attachment #594813 - Flags: review?(roc)
Attachment #595196 - Flags: review?(roc)
Attachment #595196 - Attachment description: building mask layers → building mask layers - bugfix
Attachment #595196 - Attachment is patch: true
Attached patch mask layers - basic backend - bugfix (obsolete) (deleted) — Splinter Review
Attachment #594815 - Attachment is obsolete: true
Attachment #594815 - Flags: review?(roc)
Attachment #595197 - Flags: review?(roc)
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.
Attached patch optimise by pushing down mask layers (obsolete) (deleted) — Splinter Review
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.
Attachment #595279 - Flags: review?(roc)
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.
Attachment #595194 - Flags: review?(roc) → review+
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?
Attached patch member/getter/setter - updated (obsolete) (deleted) — Splinter Review
Attachment #595194 - Attachment is obsolete: true
Attachment #597648 - Flags: review?(roc)
Attached patch user data for mask layers - updated (obsolete) (deleted) — Splinter Review
Attachment #595195 - Attachment is obsolete: true
Attachment #595195 - Flags: review?(roc)
Attachment #597649 - Flags: review?(roc)
The above two patches are the first two in the queue, in order (sorry forgot to number them).
Attachment #597653 - Flags: review?(bas.schouten)
Attachment #597654 - Flags: review?(bas.schouten)
Attachment #597655 - Flags: review?(bas.schouten)
Attached patch mask layers patch 5 (building mask layers) (obsolete) (deleted) — Splinter Review
To help the DX10 patches make sense, I've uploaded the most up to date patch for building mask layers.
Attachment #595196 - Attachment is obsolete: true
Attachment #595196 - Flags: review?(roc)
Attachment #597656 - Attachment is patch: true
Attachment #595279 - Attachment is obsolete: true
Attachment #595279 - Flags: review?(roc)
Attachment #597653 - Attachment description: dx10 patch 1 → dx10 patch 1 (loading a mask layer as a texture in DX10)
Attachment #597654 - Attachment description: dx10 patch 2 → dx10 patch 2 (changes to C++ code to use shaders for masking)
Attachment #597655 - Attachment description: dx10 patch 3 → dx10 patch 3 (changes to shader code to use GPU masking)
Depends on: 727681
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.
Attachment #597648 - Flags: review?(roc) → review+
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.
Attachment #597653 - Attachment is obsolete: true
Attachment #597653 - Flags: review?(bas.schouten)
Attachment #598094 - Flags: review?(bas.schouten)
Attachment #597654 - Attachment is obsolete: true
Attachment #597654 - Flags: review?(bas.schouten)
Attachment #598095 - Flags: review?(bas.schouten)
Minor changes to the DX10 patches - polishing only.
Attachment #597655 - Attachment is obsolete: true
Attachment #597655 - Flags: review?(bas.schouten)
Attachment #598096 - Flags: review?(bas.schouten)
Attached file testcase (deleted) —
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).
Attached patch dx9 patch 1 (loading shaders and textures) (obsolete) (deleted) — Splinter Review
Attachment #598692 - Flags: review?(bas.schouten)
Attachment #598694 - Flags: review?(bas.schouten)
Attached patch dx9 patch 3 (the actual shaders) (obsolete) (deleted) — Splinter Review
Attachment #598695 - Flags: review?(bas.schouten)
Attached patch building masks patch 3 (recycling) (obsolete) (deleted) — Splinter Review
Attachment #594809 - Attachment is obsolete: true
Attachment #594809 - Flags: review?(roc)
Attachment #598697 - Flags: review?(roc)
Attached patch building masks patch 2 (user data) (obsolete) (deleted) — Splinter Review
Attachment #598698 - Flags: review?(roc)
Attachment #597649 - Attachment is obsolete: true
Attachment #597649 - Flags: review?(roc)
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.
Attachment #598698 - Flags: review?(roc) → review+
Attachment #595197 - Attachment is obsolete: true
Attachment #595197 - Flags: review?(roc)
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.
Attachment #598698 - Attachment is obsolete: true
Attachment #598722 - Flags: review+
Attached patch building masks patch 3 (recycling) - updated (obsolete) (deleted) — Splinter Review
Attachment #598697 - Attachment is obsolete: true
Attachment #598697 - Flags: review?(roc)
Attachment #598723 - Flags: review?(roc)
Attachment #597656 - Attachment is obsolete: true
Attachment #598724 - Flags: review?(roc)
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.
Attachment #598723 - Flags: review?(roc) → review+
Attached patch building masks patch 3 (recycling) - updated (obsolete) (deleted) — Splinter Review
Attachment #598723 - Attachment is obsolete: true
Attachment #598741 - Flags: review+
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.
Attachment #598724 - Attachment is obsolete: true
Attachment #598724 - Flags: review?(roc)
Attachment #599645 - Flags: review?(roc)
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.
Attached patch dx10 patch 4 (selecting a shader) (obsolete) (deleted) — Splinter Review
Attachment #599650 - Flags: review?(bas.schouten)
(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.
Attached patch dx10 patch 4 (selecting a shader) (obsolete) (deleted) — Splinter Review
Now with comment, sorry, jet-lagged.
Attachment #599650 - Attachment is obsolete: true
Attachment #599650 - Flags: review?(bas.schouten)
Attachment #599664 - Flags: review?(bas.schouten)
Attached patch dx10 patch 3 (the actual shaders) (obsolete) (deleted) — Splinter Review
Attachment #598096 - Attachment is obsolete: true
Attachment #598096 - Flags: review?(bas.schouten)
Attachment #599713 - Flags: review?(bas.schouten)
Attached patch dx9 patch 3 (the actual shaders) (obsolete) (deleted) — Splinter Review
Attachment #598695 - Attachment is obsolete: true
Attachment #598695 - Flags: review?(bas.schouten)
Attachment #599714 - Flags: review?(bas.schouten)
Attachment #599713 - Attachment description: dx9 patch 3 (the actual shaders) → dx10 patch 3 (the actual shaders)
(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.
Attachment #599645 - Attachment is obsolete: true
Attachment #599645 - Flags: review?(roc)
Attachment #600658 - Flags: review?(roc)
Attached patch mask layers patch 5 - BasicLayers backend (obsolete) (deleted) — Splinter Review
Attachment #600659 - Flags: review?(roc)
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.
Depends on: 731777
Depends on: 733894
Attachment #598094 - Attachment is obsolete: true
Attachment #598094 - Flags: review?(bas.schouten)
Attachment #604324 - Flags: review?(bas.schouten)
Attachment #598095 - Attachment is obsolete: true
Attachment #598095 - Flags: review?(bas.schouten)
Attachment #604325 - Flags: review?(bas.schouten)
Attachment #599713 - Attachment is obsolete: true
Attachment #599713 - Flags: review?(bas.schouten)
Attachment #604326 - Flags: review?(bas.schouten)
Attached patch dx10 patch 4 (selecting a shader) (obsolete) (deleted) — Splinter Review
Replaces some of the functionality in DX10 patch 2, gets rid of some of the big nested if statements
Attachment #599664 - Attachment is obsolete: true
Attachment #599664 - Flags: review?(bas.schouten)
Attachment #604327 - Flags: review?(bas.schouten)
Attached patch dx9 patch 1 (loading shaders and textures) (obsolete) (deleted) — Splinter Review
Attachment #598692 - Attachment is obsolete: true
Attachment #598692 - Flags: review?(bas.schouten)
Attachment #604328 - Flags: review?(bas.schouten)
Attachment #598694 - Attachment is obsolete: true
Attachment #598694 - Flags: review?(bas.schouten)
Attachment #604329 - Flags: review?(bas.schouten)
Attached patch dx9 patch 3 (the actual shaders) (obsolete) (deleted) — Splinter Review
Attachment #599714 - Attachment is obsolete: true
Attachment #599714 - Flags: review?(bas.schouten)
Attachment #604330 - Flags: review?(bas.schouten)
Attached patch dx9 patch 1 (loading shaders and textures) (obsolete) (deleted) — Splinter Review
Attachment #604328 - Attachment is obsolete: true
Attachment #604328 - Flags: review?(bas.schouten)
Attachment #604333 - Flags: review?(bas.schouten)
Attached patch building masks patch 1 (member/getter/setter) (obsolete) (deleted) — Splinter Review
Attachment #597648 - Attachment is obsolete: true
Attachment #604336 - Flags: review?(roc)
Attachment #600658 - Attachment is obsolete: true
Attachment #600658 - Flags: review?(roc)
Attachment #604337 - Flags: review?(roc)
Attached patch mask layers patch 5 - BasicLayers backend (obsolete) (deleted) — Splinter Review
Attachment #600659 - Attachment is obsolete: true
Attachment #600659 - Flags: review?(roc)
Attachment #604338 - Flags: review?(roc)
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
Attachment #604338 - Flags: review?(roc) → review+
(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.
Attached patch building masks patch 1 (member/getter/setter) (obsolete) (deleted) — Splinter Review
Attachment #604336 - Attachment is obsolete: true
Attachment #604336 - Flags: review?(roc)
Attachment #606419 - Flags: review?(roc)
Attachment #604337 - Attachment is obsolete: true
Attachment #604337 - Flags: review?(roc)
Attachment #606420 - Flags: review?(roc)
Attached patch mask layers patch 5 (basic layers backend) (obsolete) (deleted) — Splinter Review
Attachment #604338 - Attachment is obsolete: true
Attachment #606421 - Flags: review+
Attachment #606420 - Attachment is obsolete: true
Attachment #606420 - Flags: review?(roc)
Attachment #606449 - Flags: review?(roc)
Attached patch OGL patch 1 (the shaders) (obsolete) (deleted) — Splinter Review
Attachment #606450 - Flags: review?(bas.schouten)
Attached patch OGL patch 2 (managing the shaders) (obsolete) (deleted) — Splinter Review
Attachment #606451 - Flags: review?(bas.schouten)
Attached patch OGL patch 3 (Loading the mask layer) (obsolete) (deleted) — Splinter Review
Attachment #606452 - Flags: review?(bas.schouten)
Attached patch OGL patch 4 (enable mask layers) (obsolete) (deleted) — Splinter Review
Attachment #606453 - Flags: review?(bas.schouten)
> > 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.
Attachment #606449 - Attachment is obsolete: true
Attachment #606449 - Flags: review?(roc)
Attachment #607003 - Flags: review+
Attachment #606421 - Attachment description: mask layers patch 5 (building mask layers) → mask layers patch 5 (basic layers backend)
Attached patch OGL patch 2 (managing the shaders) (obsolete) (deleted) — Splinter Review
Attachment #606451 - Attachment is obsolete: true
Attachment #606451 - Flags: review?(bas.schouten)
Attachment #607855 - Flags: review?(bgirard)
Attached patch dx9 patch 3 (the actual shaders) (obsolete) (deleted) — Splinter Review
Attachment #604330 - Attachment is obsolete: true
Attachment #604330 - Flags: review?(bas.schouten)
Attachment #607856 - Flags: review?(bas.schouten)
Attached patch OGL patch 4 (enable mask layers) (obsolete) (deleted) — Splinter Review
Attachment #606453 - Attachment is obsolete: true
Attachment #606453 - Flags: review?(bas.schouten)
Attachment #607857 - Flags: review?(bgirard)
Attached patch last patch: shadow layer support (obsolete) (deleted) — Splinter Review
Attachment #607858 - Flags: review?(bgirard)
Attached patch last patch: shadow layer support (obsolete) (deleted) — Splinter Review
Attachment #607858 - Attachment is obsolete: true
Attachment #607858 - Flags: review?(bgirard)
Attachment #607861 - Flags: review?
Attachment #607861 - Flags: review? → review?(bgirard)
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.
Attached image Complex clip example (deleted) —
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.
Attached patch OGL patch 5 (changes to building) (obsolete) (deleted) — Splinter Review
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).
Attachment #608134 - Flags: review?(bgirard)
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.
Attachment #607855 - Flags: review?(bgirard) → review-
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!
Attachment #607857 - Flags: review?(bgirard) → review+
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.
Attachment #608134 - Flags: review?(bgirard) → review+
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*.
Attachment #604324 - Flags: review?(bas.schouten) → review+
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.
Attachment #604325 - Attachment is obsolete: true
Attachment #604325 - Flags: review?(bas.schouten)
Attachment #609139 - Flags: review?(bas.schouten)
Attached patch dx10 patch 3 (the actual shaders) (obsolete) (deleted) — Splinter Review
Attachment #604326 - Attachment is obsolete: true
Attachment #604326 - Flags: review?(bas.schouten)
Attachment #609140 - Flags: review?(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.
Attachment #604333 - Flags: review?(bas.schouten) → review+
Attachment #604329 - Flags: review?(bas.schouten) → review+
Attachment #607856 - Flags: review?(bas.schouten) → review+
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
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!
@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 :(
This will help whether D2D is turned on or not.
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
Rebased, no changes. Carrying the r=roc
Attachment #594810 - Attachment is obsolete: true
Attachment #619722 - Flags: review+
Changed to recycle mask layers for different layer types separately
Attachment #598741 - Attachment is obsolete: true
Attachment #619724 - Flags: review?
Small change: setter takes a Layer* as arg, instead of alread_addRefed<Layer>
Attachment #606419 - Attachment is obsolete: true
Attachment #619726 - Flags: review?(roc)
Attachment #619724 - Flags: review? → review?(roc)
rebased, carrying r=roc
Attachment #606421 - Attachment is obsolete: true
Attachment #619731 - Flags: review+
Addressed Bas's comments and rebased; carrying r=Bas
Attachment #604324 - Attachment is obsolete: true
Attachment #619733 - Flags: review+
rebased and addressed comments, awaiting re-review
Attachment #619734 - Flags: review?(bas.schouten)
Attachment #619734 - Attachment description: dx9 patch 2 (using the new shaders in each layer) → dx10 patch 2 (using the new shaders in each layer)
Attachment #609139 - Attachment is obsolete: true
Attachment #609139 - Flags: review?(bas.schouten)
Rebased and addressed review comments, awating re-review
Attachment #609140 - Attachment is obsolete: true
Attachment #609140 - Flags: review?(bas.schouten)
Attachment #619736 - Flags: review?(bas.schouten)
rebased, awaiting review (this patch hasn't been reviewed yet)
Attachment #604327 - Attachment is obsolete: true
Attachment #604327 - Flags: review?(bas.schouten)
Attachment #619738 - Flags: review?(bas.schouten)
rebased and addressed comments, carrying r=Bas
Attachment #604333 - Attachment is obsolete: true
Attachment #619740 - Flags: review+
rebased and addressed comments, carrying r=Bas
Attachment #604329 - Attachment is obsolete: true
Attachment #619741 - Flags: review+
rebased and addressed comments, carrying r=Bas
Attachment #607856 - Attachment is obsolete: true
Attachment #619742 - Flags: review+
rebased and fixed a small - failing to initialise a field; carrying r=roc
Attachment #607003 - Attachment is obsolete: true
Attachment #619744 - Flags: review+
Attached patch OGL patch 1 (the shaders) (obsolete) (deleted) — Splinter Review
awaiting review
Attachment #606450 - Attachment is obsolete: true
Attachment #606450 - Flags: review?(bas.schouten)
Attachment #619747 - Flags: review?(bgirard)
Addressed comments and rebased, awating re-review
Attachment #607855 - Attachment is obsolete: true
Attachment #619748 - Flags: review?(bgirard)
Attached patch OGL patch 3 (Loading the mask layer) (obsolete) (deleted) — Splinter Review
Attachment #606452 - Attachment is obsolete: true
Attachment #606452 - Flags: review?(bas.schouten)
Attachment #619749 - Flags: review?(bgirard)
Minor change (from a comment on a different patch): use flags rather than bools for the different kinds of mask. Also rebased.
Attachment #607857 - Attachment is obsolete: true
Attachment #619752 - Flags: review?(bgirard)
Rebased and changed to reflect other patches (macro-processed code), carrying r=BenWa
Attachment #608134 - Attachment is obsolete: true
Attachment #619753 - Flags: review+
Attached patch support shadow layers (obsolete) (deleted) — Splinter Review
Lots of changes, and never reviewed
Attachment #607861 - Attachment is obsolete: true
Attachment #607861 - Flags: review?(bgirard)
Attachment #619755 - Flags: review?(bgirard)
Attached patch changes to tests (deleted) — Splinter Review
Attachment #619756 - Flags: review?(roc)
Attachment #619763 - Flags: review?(bgirard)
Attached patch support shadow layers (obsolete) (deleted) — Splinter Review
Attachment #619755 - Attachment is obsolete: true
Attachment #619755 - Flags: review?(bgirard)
Attachment #619774 - Flags: review?(bgirard)
Attached patch support shadow layers (deleted) — Splinter Review
Attachment #619774 - Attachment is obsolete: true
Attachment #619774 - Flags: review?(bgirard)
Attachment #619776 - Flags: review?(bgirard)
Attachment #619763 - Flags: review?(bgirard) → review+
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.
Attachment #619747 - Flags: review?(bgirard)
(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.
Attached patch OGL patch 1 (the shaders) (deleted) — Splinter Review
Added comments as requested by BenWa
Attachment #619747 - Attachment is obsolete: true
Attachment #619821 - Flags: review?(bgirard)
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
Testing https://bugzilla.mozilla.org/show_bug.cgi?id=702485 14eeb79a91ca = low drop rate 9dbe02adbed1 = low drop rate ce953c7f7a59 = high drop rate
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.
Attachment #619821 - Flags: review?(bgirard) → review+
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.
Attachment #619748 - Flags: review?(bgirard) → review-
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
Attachment #619749 - Flags: review?(bgirard) → review-
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 on attachment 619752 [details] [diff] [review] OGL patch 4 (enable mask layers) Review of attachment 619752 [details] [diff] [review]: ----------------------------------------------------------------- Looks good!
Attachment #619752 - Flags: review?(bgirard) → review+
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 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?
Attachment #619776 - Flags: review?(bgirard) → review+
Attachment #619736 - Flags: review?(bas.schouten) → review+
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.
Attachment #619738 - Flags: review?(bas.schouten) → review+
Attachment #619734 - Flags: review?(bas.schouten) → review+
(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.
(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.
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.
Attached patch OGL patch 6: on-demand compilation of shaders (obsolete) (deleted) — Splinter Review
Attachment #620188 - Flags: review?(bgirard)
Attachment #619749 - Attachment is obsolete: true
Attachment #620202 - Flags: review?(bgirard)
> ::: 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
(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).
Attached patch misc. small changes for reviewers (obsolete) (deleted) — Splinter Review
Attachment #620208 - Flags: review?(roc)
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
(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 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".
Attachment #620208 - Flags: review?(roc) → review+
Attachment #620202 - Flags: review?(bgirard) → review+
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?
Attachment #620188 - Flags: review?(bgirard) → review-
Changed a text string, carrying r=roc
Attachment #620472 - Flags: review+
Attachment #620208 - Attachment is obsolete: true
(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.
Try run with lazy compilation of shaders: https://tbpl.mozilla.org/?tree=Try&rev=eadd3dcc9680
Attachment #620188 - Attachment is obsolete: true
Attachment #620579 - Flags: review?(bgirard)
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.
Attachment #620579 - Flags: review?(bgirard) → review+
Attachment #619748 - Flags: review- → review+
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Blocks: 628366
Depends on: 754488
Blocks: 755078
Depends on: 769949
Depends on: 786817
Depends on: 787695
Depends on: 790124
Depends on: 815593
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: