445297 - Optimize filters so that changes to the source image don't have to repaint the entire filter

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

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

Attachment: testcase

The attached testcase rapidly toggles the color of a small rectangle inside a large filtered region. Currently we invalidate the entire filter area at every change, but in reality only a small subregion needs to be invalidated.

Comment 1

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

Attachment: fix

With a small extension to the filter analysis framework we can do the right thing here. Knowing what might have changed in the input, we can propagate a bounding box for what's changed forward through the filter graph to find a bounding box for the changes in the filter output.

On my branch, this sped up the blinkmark test above from 34393ms to 20284ms.

Comment 2

[Robert Longson [:longsonr]]

Comment on attachment 329647 [details] [diff] [review]
fix

nsSVGFilterInstance::ComputeFilterPrimitiveSubregion(PrimitiveInfo* aPrimitive)
> {
>   nsSVGFE* fE = aPrimitive->mFE;
> 
>   gfxRect defaultFilterSubregion(0,0,0,0);

Nit: Spaces after commas please.

> class nsSVGFilterProperty :
>   public nsSVGPropertyBase, public nsISVGFilterProperty {
> public:
>   nsSVGFilterProperty(nsIContent *aFilter, nsIFrame *aFilteredFrame);
>   virtual ~nsSVGFilterProperty() {
>     mFrame->RemoveStateBits(NS_STATE_SVG_FILTERED);
>   }
> 
>-  nsRect GetRect() { return mFilterRect; }
>   nsSVGFilterFrame *GetFilterFrame();
>   void UpdateRect();
> 
>   // nsISupports
>   NS_DECL_ISUPPORTS
> 
>   // nsIMutationObserver
>   NS_DECL_NSIMUTATIONOBSERVER_PARENTCHAINCHANGED
> 
>   // nsISVGFilterProperty
>   virtual void Invalidate() { DoUpdate(); }
> 
>@@ -198,49 +197,49 @@ private:
> };
> 
> NS_IMPL_ISUPPORTS_INHERITED1(nsSVGFilterProperty,
>                              nsSVGPropertyBase,
>                              nsISVGFilterProperty)
> 
> nsSVGFilterProperty::nsSVGFilterProperty(nsIContent *aFilter,
>                                          nsIFrame *aFilteredFrame)
>   : nsSVGPropertyBase(aFilter, aFilteredFrame, nsGkAtoms::filter)
> {
>   nsSVGFilterFrame *filterFrame = GetFilterFrame();
>   if (filterFrame)
>-    mFilterRect = filterFrame->GetInvalidationRegion(mFrame);
>+    mFilterRect = filterFrame->GetInvalidationRegion(mFrame, mFrame->GetRect());

The lines above are no longer required aren't they as nothing uses mFilterRect any more does it?

Can't mFilterRect disappear altogether?

> 
>   mFrame->AddStateBits(NS_STATE_SVG_FILTERED);
> }
> 
> void
> nsSVGFilterProperty::UpdateRect()
> {
>   nsSVGFilterFrame *filter = GetFilterFrame();
>   if (filter)
>-    mFilterRect = filter->GetInvalidationRegion(mFrame);
>+    mFilterRect = filter->GetInvalidationRegion(mFrame, mFrame->GetRect());
> }

This entire method can go can't it?

> 
> void
> nsSVGFilterProperty::DoUpdate()
> {
>   nsSVGOuterSVGFrame *outerSVGFrame = nsSVGUtils::GetOuterSVGFrame(mFrame);
>   if (outerSVGFrame) {
>     outerSVGFrame->InvalidateCoveredRegion(mFrame);
>     UpdateRect();
>     outerSVGFrame->InvalidateCoveredRegion(mFrame);
>   }
> }

UpdateRect no longer does anything useful so don't you need to change this? Otherwise you're just Invalidating the same covered region twice aren't you?

>-    // Invalidate the area we used to cover
>-    outerSVGFrame->InvalidateCoveredRegion(frame);
>-
>-    aSVGFrame->UpdateCoveredRegion();
>-
>-    // Invalidate the area we now cover
>-    outerSVGFrame->InvalidateCoveredRegion(frame);
>-
>-    NotifyAncestorsOfFilterRegionChange(frame);
>+    PRBool changed = outerSVGFrame->UpdateAndInvalidateCoveredRegion(frame);
>+    if (changed) {
>+      NotifyAncestorsOfFilterRegionChange(frame);
>+    }

Does the testcase in bug 421584 still work?

Also worth checking are testcases in bug 331762 and bug 340908. These tend to trigger infinite loops on 100% CPU if there are logic errors.

I would like another look at this post these comments.

Comment 3

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

You've actually found a bug. The intent of the two InvalidateCoveredRegion calls in DoUpdate is that the first one will invalidate the area painted by the old filter, but as things stand, it will invalidate the area painted by the new filter so if the filter area shrinks then we won't invalidate enough.

Comment 4

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

Attachment: testcase

This testcase shows the bug. Unfortunately it's not automatable yet.

Comment 5

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

Attachment: fix v2

This fixes the problem. I also found another bug which is we should call GetCoveredRegion instead of accessing mRect directly, since mRect is often not up to date.

The testcase in 421584 works. foreignObject with filter applied doesn't work and hasn't for a while, that's the subject of bug 445079.

Comment 6

[Robert Longson [:longsonr]]

Comment on attachment 329707 [details] [diff] [review]
fix v2

This version makes much more sense :-)

> 
> void
> nsSVGFilterProperty::DoUpdate()
> {
>   nsSVGOuterSVGFrame *outerSVGFrame = nsSVGUtils::GetOuterSVGFrame(mFrame);
>   if (outerSVGFrame) {
>-    outerSVGFrame->InvalidateCoveredRegion(mFrame);
>+    outerSVGFrame->InvalidateRect(mFilterRect);
>     UpdateRect();
>-    outerSVGFrame->InvalidateCoveredRegion(mFrame);
>+    outerSVGFrame->InvalidateRect(mFilterRect);
>   }
> }

Can you optimise this by checking whether the old mFilterRect == the new mFilterRect and skipping the second invalidate as you do in nsSVGOuterSVGFrame::UpdateAndInvalidateCoveredRegion?

Comment 7

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

I could but it's not really worth it IMHO. UpdateAndInvalidateCoveredRegion is a bit more worthwhile to optimize since we can avoid an extra call to nsSVGUtils::FindFilterInvalidation, which is quite expensive when filters are around since it may call GetInvalidationRegion one or more times which sets up an nsSVGFilterInstance.

Comment 8

[Robert Longson [:longsonr]]

OK. I'm happy with it as is then.

Comment 9

[Mats Palmgren (inactive)]

Comment on attachment 329707 [details] [diff] [review]
fix v2

sr=mats

None of the ComputeChangeBBox implementations uses the aSourceBBoxes
parameter - I assume you foresee it will be used.

> content/svg/content/src/nsSVGFilters.h
> // Given the bounding-boxes of the inputs, and bounding boxes for the

s/bounding-boxes/bounding boxes/

> layout/svg/base/src/nsSVGUtils.cpp
> nsISVGChildFrame *svg;
> CallQueryInterface(mFrame, &svg);
> mFilterRect = filter->GetInvalidationRegion(mFrame, svg->GetCoveredRegion());

nsSVGClipPathProperty::DoUpdate() and nsSVGMaskProperty::DoUpdate()
null-check the result of the same QI, can we remove those null-checks?
http://mxr.mozilla.org/mozilla-central/source/layout/svg/base/src/nsSVGUtils.cpp#295
http://mxr.mozilla.org/mozilla-central/source/layout/svg/base/src/nsSVGUtils.cpp#352

Comment 10

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

(In reply to comment #9)
> None of the ComputeChangeBBox implementations uses the aSourceBBoxes
> parameter - I assume you foresee it will be used.

I think it could be used by feDisplacementMap in the future. But actually I'll take it out for now, we shouldn't land code that has no immediate use.

> > content/svg/content/src/nsSVGFilters.h
> > // Given the bounding-boxes of the inputs, and bounding boxes for the
> 
> s/bounding-boxes/bounding boxes/

OK.

> > layout/svg/base/src/nsSVGUtils.cpp
> > nsISVGChildFrame *svg;
> > CallQueryInterface(mFrame, &svg);
> > mFilterRect = filter->GetInvalidationRegion(mFrame, svg->GetCoveredRegion());
> 
> nsSVGClipPathProperty::DoUpdate() and nsSVGMaskProperty::DoUpdate()
> null-check the result of the same QI, can we remove those null-checks?
> http://mxr.mozilla.org/mozilla-central/source/layout/svg/base/src/nsSVGUtils.cpp#295
> http://mxr.mozilla.org/mozilla-central/source/layout/svg/base/src/nsSVGUtils.cpp#352

We could, but all this code changes later in my branch and the QI actually disappears, so I think we shouldn't bother.

Comment 11

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

Pushed 3f5016ee13e8