In bug 927892 I'm adding SVG filter support for canvas drawing. If you set ctx.filter = "url(#someFilter)", draw something using that filter, and then make changes to any values in #someFilter, the next draw operation should be respecting those changes. So the canvas rendering context needs to be notified when something changes in the filter. Traditional filtering does this by storing an nsSVGFilterProperty on the filtered frame, which will invalidate the frame at the right times. But a canvas doesn't necessarily have a frame. This bugs deals with generalizing nsSVGFilterProperty and friends so that they can be used without a frame. The end result will be that we have an abstract class "nsSVGFilterChainObserver" that will call its implementations' DoUpdate method when something changes, and canvas will provide such an implementation. The existing nsSVGFilterProperty will be another subclass of nsSVGFilterChainObserver and its DoUpdate implementation will invalidate the frame.

It's not needed, nobody calls GetFilters().

Use the same order as in nsSVGEffects.h: nsSVGRenderingObserver, nsSVGIDRenderingObserver, nsSVGFilterFrame, nsSVGFilterProperty, rest

This introduces another class called nsSVGRenderingObserverProperty into the class hierarchy. I've also moved the mFramePresShell->IsDestroying() frame validity check into a new class called nsSVGFrameReferenceFromProperty because I'm going to use for the implementation of nsSVGFilterProperty later.

This part needs to be reviewed carefully because I don't really know what I'm doing. When the canvas rendering context has a strong reference to its subclass of nsSVGFilterChainObserver and references an SVG filter, I think we have the following cycle: canvas element -> canvasRenderingContext2D (CanvasRenderingContext2D) -> mFilterChainObserver (CanvasFilterChainObserver) -> nsSVGFilterChainObserver::mReferences[0] (nsSVGFilterReference) -> nsSVGIDRenderingObserver::mElement (nsReferencedElement) -> nsReferencedElement::mElement (this is the SVG filter element) -> ?? (document maybe?) -> canvas element? This patch at least fixes the leaks that I was seeing.

(In reply to Markus Stange [:mstange] from comment #3) > Created attachment 8484120 [details] [diff] [review] > part 3: Reorder stuff in nsSVGEffects.cpp > > Use the same order as in nsSVGEffects.h: > nsSVGRenderingObserver, nsSVGIDRenderingObserver, nsSVGFilterFrame, > nsSVGFilterProperty, rest err, nsSVGRenderingObserver, nsSVGIDRenderingObserver, nsSVGFilterReference, nsSVGFilterProperty, rest

Seems to me we only need to these changes because you cache in the CanvasRenderingContext2D the current filter description. So for a sanity check: are you sure you need to cache that? Have you done any experiments that show a significant performance benefit to such caching?

Good point! For some reason I never considered not caching the filter description. I haven't done any performance testing to see how much it gains us.

The 3 patches I r+ed are good to land anyway. But please check whether we need to cache the description.

In this test case, if I re-build the FilterDescription on every draw call, that rebuilding takes up 13% of every call. This is with a software color matrix applied on a 100x100 surface. The cost of FilterDescription building is largely due to memory allocation / deallocation from the FilterPrimitiveDescription attribute hashtables, and due to URL string encoding conversion (?) in nsReferencedElement.

Another advantage of storing an nsSVGFilterChainObserver on the context occurred to me: It automatically takes care of keeping external resource documents alive. That's something we need regardless of whether the FilterDescription is cached, right?

Yeah OK.

Comment on attachment 8484133 [details] [diff] [review] part 7: Make nsSVGFilterChainObserver participate in cycle collection Review of attachment 8484133 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/svg/nsSVGEffects.cpp @@ +242,5 @@ > +NS_IMPL_CYCLE_COLLECTION_UNLINK_END > + > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsSVGFilterReference) > + tmp->mElement.Traverse(&cb); > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END I think you can use NS_IMPL_CYCLE_COLLECTION here.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16) > ::: layout/svg/nsSVGEffects.cpp > @@ +242,5 @@ > > +NS_IMPL_CYCLE_COLLECTION_UNLINK_END > > + > > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsSVGFilterReference) > > + tmp->mElement.Traverse(&cb); > > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END > > I think you can use NS_IMPL_CYCLE_COLLECTION here. This doesn't work at the moment, I filed bug 1064842 about it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/56a6f50b440d https://hg.mozilla.org/integration/mozilla-inbound/rev/33fb32e39f00 https://hg.mozilla.org/integration/mozilla-inbound/rev/81f4f8bf7444 https://hg.mozilla.org/integration/mozilla-inbound/rev/36e4f8ea9cca https://hg.mozilla.org/integration/mozilla-inbound/rev/883f9410c408 https://hg.mozilla.org/integration/mozilla-inbound/rev/eba08f974e74 https://hg.mozilla.org/integration/mozilla-inbound/rev/6c0b0d329a51

https://hg.mozilla.org/mozilla-central/rev/56a6f50b440d https://hg.mozilla.org/mozilla-central/rev/33fb32e39f00 https://hg.mozilla.org/mozilla-central/rev/81f4f8bf7444 https://hg.mozilla.org/mozilla-central/rev/36e4f8ea9cca https://hg.mozilla.org/mozilla-central/rev/883f9410c408 https://hg.mozilla.org/mozilla-central/rev/eba08f974e74 https://hg.mozilla.org/mozilla-central/rev/6c0b0d329a51