This will be a generalization of the "widget callback" interface we have now. The two initial clients, scroll indicators and overscroll effects, are blocked by this.

Whups, s/affect/effect/ locally.

Comment on attachment 653221 [details] [diff] [review] part 2: Subclass EffectRenderer for GL layers Review of attachment 653221 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/opengl/ContainerLayerOGL.cpp @@ +167,5 @@ > const gfx3DMatrix& transform = aContainer->GetEffectiveTransform(); > + // FIXME/bug XXXXXX: optimize the case where we only have one child > + // layer to not create an intermediate fbo for composition effects > + bool needsFramebuffer = (aContainer->UseIntermediateSurface() || > + effects & LAYERS_EFFECT_COMPOSITION); We need to use exactly UseIntermediateSurface() since that was used in DefaultComputeEffectiveTransform to determine the effective transforms on our children. I think you need to move this extra check inside UseIntermediateSurface(), and ensure that ComputeEffectiveTransforms() is called anytime that the result of this might change.

Comment on attachment 653221 [details] [diff] [review] part 2: Subclass EffectRenderer for GL layers Right-o. Will update when we settle on an API.

Comment on attachment 653220 [details] [diff] [review] part 1: Add an interface for applying various to ContainerLayer content during composition Review of attachment 653220 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/Layers.h @@ +1163,5 @@ > + virtual uint32_t GetEffects(const TimeStamp& aNow) = 0; > + > + /** > + * The actual callback for applying effects is only defined per > + * layer backend, since it' entirely backend-specific. Each backend it's Does it actually make sense to have a cross-backend EffectRenderer class at all? Maybe it would make more sense to have entirely separate interfaces for different kinds of backends?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6) > Comment on attachment 653220 [details] [diff] [review] > Does it actually make sense to have a cross-backend EffectRenderer class at > all? I think so, for two reasons - there's some shared logic around dealing with effects in layers itself, e.g. comment 4 - the two initial consumers of this API will have a large amount of platform-independent code to compute effect parameters along with a bit of platform-specific code for the draw calls. Having a shared base class makes it simpler to organize the shared platform-neutral code.

Comment on attachment 653220 [details] [diff] [review] part 1: Add an interface for applying various to ContainerLayer content during composition Review of attachment 653220 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/Layers.h @@ +1148,5 @@ > +/** > + * Effect renderers are used to apply composition-time effects to > + * layer subtrees. > + */ > +class THEBES_API EffectRenderer : public RefCounted<EffectRenderer> { Why does this need to be refcounted? Couldn't we just manually manager this? @@ +1208,5 @@ > + * > + * Update the effect renderer for this subtree. |aRenderer| can be > + * nullptr to cancel effects. > + */ > + void SetEffectRenderer(EffectRenderer* aRenderer) Is the caller allowed to use a single EffectRenderer for several different layers?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8) > Comment on attachment 653220 [details] [diff] [review] > part 1: Add an interface for applying various to ContainerLayer content > during composition > > Review of attachment 653220 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/Layers.h > @@ +1148,5 @@ > > +/** > > + * Effect renderers are used to apply composition-time effects to > > + * layer subtrees. > > + */ > > +class THEBES_API EffectRenderer : public RefCounted<EffectRenderer> { > > Why does this need to be refcounted? Couldn't we just manually manager this? > The first consumers of the API will need to access to AsyncPanZoomController, which is itself threadsafe refcounted. I had thought we would keep the EffectRenderer set for the lifetime of the ContainerLayer, but now that I think of it, that would be tricky. I'd be OK with an API more like RefLayer::ConnectReferent/DetachReferent if you'd prefer. > @@ +1208,5 @@ > > + * > > + * Update the effect renderer for this subtree. |aRenderer| can be > > + * nullptr to cancel effects. > > + */ > > + void SetEffectRenderer(EffectRenderer* aRenderer) > > Is the caller allowed to use a single EffectRenderer for several different > layers? I don't have a use case yet but I don't see why not. I guess that would change the API though, so I can specify that it's discouraged here if you'd like.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #9) > The first consumers of the API will need to access to > AsyncPanZoomController, which is itself threadsafe refcounted. That doesn't require EffectRenderer to be refcounted though. If the AsyncPanZoomController needs a strong reference back to the EffectRenderer, I can see that would make refcounting desirable for EffectRenderer. Is that the issue? > > @@ +1208,5 @@ > > > + * > > > + * Update the effect renderer for this subtree. |aRenderer| can be > > > + * nullptr to cancel effects. > > > + */ > > > + void SetEffectRenderer(EffectRenderer* aRenderer) > > > > Is the caller allowed to use a single EffectRenderer for several different > > layers? > > I don't have a use case yet but I don't see why not. I guess that would > change the API though, so I can specify that it's discouraged here if you'd > like. It's either allowed or not. If it's allowed, EffectRenderer needs to be refcounted. If it's not, we need to ensure the callers respect that. Just decide which way you're going to go :-).

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #9) > If the AsyncPanZoomController needs a strong reference back to the > EffectRenderer, I can see that would make refcounting desirable for > EffectRenderer. Is that the issue? > I think want to implement this by asking the AsyncPanZoomController for an EffectRenderer, if it has one, during AutoResolveRefLayers, and then transiently setting the EffectRenderer on that ContainerLayer. So EffectRenderer doesn't need to be refcounted. > > > @@ +1208,5 @@ > > > > + * > > > > + * Update the effect renderer for this subtree. |aRenderer| can be > > > > + * nullptr to cancel effects. > > > > + */ > > > > + void SetEffectRenderer(EffectRenderer* aRenderer) > > > > > > Is the caller allowed to use a single EffectRenderer for several different > > > layers? > > > > I don't have a use case yet but I don't see why not. I guess that would > > change the API though, so I can specify that it's discouraged here if you'd > > like. > > It's either allowed or not. If it's allowed, EffectRenderer needs to be > refcounted. If it's not, we need to ensure the callers respect that. Just > decide which way you're going to go :-). Oh, I thought you might have had something in mind. Definitely, simpler is better, single user only.

This patch builds on the previous two. It fixes up the computation of transforms so that it works properly when a composition effect is applied to a container layer. However, we need a way to update the transform when the layer effect comes in and out of effect; that's easy if you know the parent's transform, but we need to cache that. That's the next step.

This updated patch only allows UNDERLAY and OVERLAY. I left the FIXME comment about taking the timestamp. Can you propose a definite way to fix it today or should I file a follow-up bug? Do I understand correctly that that is about having a single unified place in the compositor where we take composition timestamps?

Thanks for picking this up, Benoit :).

Comment on attachment 692066 [details] [diff] [review] Allow overlay and underlay effects, but don't allow overriding the whole composition >diff --git a/gfx/layers/Layers.h b/gfx/layers/Layers.h >+/** >+ * Effect renderers are used to apply composition-time effects to >+ * layer subtrees. >+ */ >+class THEBES_API EffectRenderer : public RefCounted<EffectRenderer> { Did we end up needing to refcount this? >+ /** >+ * CONSTRUCTION PHASE ONLY >+ * >+ * Update the effect renderer for this subtree. |aRenderer| can be >+ * nullptr to cancel effects. >+ */ >+ void SetEffectRenderer(EffectRenderer* aRenderer) Need to address roc's comment here: document that for now effect renderers can only be used by at most one container at a time. Looks good, thanks for updating. (Of course this is a grossly inbred review ;). )

(In reply to Chris Jones [:cjones] [:warhammer] from comment #15) > Need to address roc's comment here: document that for now effect > renderers can only be used by at most one container at a time. Since we made EffectRenderer refcounted in the end, I think it's OK to say that an EffectRendered can be used by any number of ContainerLayers at the same time.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #15) > Comment on attachment 692066 [details] [diff] [review] > Allow overlay and underlay effects, but don't allow overriding the whole > composition > > >diff --git a/gfx/layers/Layers.h b/gfx/layers/Layers.h > > >+/** > >+ * Effect renderers are used to apply composition-time effects to > >+ * layer subtrees. > >+ */ > >+class THEBES_API EffectRenderer : public RefCounted<EffectRenderer> { > > Did we end up needing to refcount this? I didn't question that, but at least I don't have a need for it to be refcounted. My current patch on bug 775469 has the APZC holding a reference to the EffectRenderer, but that is backwards now that I think about it: it is the EffectRenderer that needs to call methods on the APZC, therefore it is the EffectRenderer that should hold a reference to the APZC, not the other way around. Given that _I_ don't have a specific need for EffectRenderer to be refcounted, would you like to remove the reference-counting on on it?

I would feel slightly more comfortable keeping it refcounted.

Agreed.

(Oops, don't need to track this separately since it's a means to other ends.)

unbitrotted