1062832 - Make filter references and their update notification mechanisms usable for canvas filters

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Markus Stange [:mstange]]

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.

Comment 1

[Markus Stange [:mstange]]

Attachment: part 1: remove nsSVGFilterProperty::mFilters

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

Comment 2

[Markus Stange [:mstange]]

Attachment: part 2: Use nsRefPtr in the filter references array instead of manual refcounting

Comment 3

[Markus Stange [:mstange]]

Attachment: part 3: Reorder stuff in nsSVGEffects.cpp

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

Comment 4

[Markus Stange [:mstange]]

Attachment: part 4: Remove frame handling from nsSVGIDRenderingObserver and put it into nsSVGRenderingObserverProperty

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.

Comment 5

[Markus Stange [:mstange]]

Attachment: part 5: Move frame invalidation from nsSVGFilterReference to nsSVGFilterProperty

Comment 6

[Markus Stange [:mstange]]

Attachment: part 6: Create abstract class nsSVGFilterChainObserver for observers without frames

Comment 7

[Markus Stange [:mstange]]

Attachment: part 7: Make nsSVGFilterChainObserver participate in cycle collection

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.

Comment 8

[Markus Stange [:mstange]]

(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

Comment 9

[Markus Stange [:mstange]]

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

Comment 10

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

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?

Comment 11

[Markus Stange [:mstange]]

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.

Comment 12

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

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

Comment 13

[Markus Stange [:mstange]]

Attachment: performance test

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.

Comment 14

[Markus Stange [:mstange]]

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?

Comment 15

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

Yeah OK.

Comment 16

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

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.

Comment 17

[Markus Stange [:mstange]]

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

Comment 18

[Markus Stange [:mstange]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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