Closed Bug 738192 Opened 12 years ago Closed 12 years ago

Get rid of the horrendous "invalidate everything" hack in nsSVGUtils::FindFilterInvalidation

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: jwatt, Assigned: jwatt)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(1 file)

We should get rid of the horrendous "invalidate everything" hack in nsSVGUtils::FindFilterInvalidation that was introduced in bug 463939.
No longer blocks: 734082
Depends on: 734082
Assignee: nobody → jwatt
Depends on: 767647
Attached patch patchSplinter Review
Attachment #635998 - Flags: review?(longsonr)
Comment on attachment 635998 [details] [diff] [review]
patch


>diff --git a/layout/svg/base/src/nsSVGFilterFrame.cpp b/layout/svg/base/src/nsSVGFilterFrame.cpp
>--- a/layout/svg/base/src/nsSVGFilterFrame.cpp
>+++ b/layout/svg/base/src/nsSVGFilterFrame.cpp
>@@ -10,26 +10,31 @@
> #include "gfxASurface.h"
> #include "gfxUtils.h"
> #include "nsGkAtoms.h"
> #include "nsRenderingContext.h"
> #include "nsSVGEffects.h"
> #include "nsSVGFilterElement.h"
> #include "nsSVGFilterInstance.h"
> #include "nsSVGFilterPaintCallback.h"
>+#include "nsSVGIntegrationUtils.h"

This is supposed to be for SVG effects in html only. Move GetCSSPxToDevPxMatrix to nsSVGUtils instead.

> void
>-nsSVGUtils::InvalidateBounds(nsIFrame *aFrame, bool aDuringUpdate)
>+nsSVGUtils::InvalidateBounds(nsIFrame *aFrame, bool aDuringUpdate,
>+                             const nsRect *aBoundsSubArea, PRUint32 aFlags)
> {
...
>-  nsSVGOuterSVGFrame* outerSVGFrame = GetOuterSVGFrame(aFrame);
>-  NS_ASSERTION(outerSVGFrame, "no outer svg frame");
>-  if (outerSVGFrame) {
>-    nsISVGChildFrame *svgFrame = do_QueryFrame(aFrame);
>-    if (!svgFrame)
>+  nsRect rect;

Could we have a more informative variable name than rect please?

>+  if (aBoundsSubArea) {
>+    rect = *aBoundsSubArea;
>+  } else {
>+    rect = aFrame->GetVisualOverflowRect();
>+    // GetVisualOverflowRect() already includes filter effects and transforms,
>+    // so advance to our parent before the loop below:
>+    rect += aFrame->GetPosition();
>+    aFrame = aFrame->GetParent();
>+  }
>+
>+    if (aFrame->GetType() == nsGkAtoms::svgInnerSVGFrame &&
>+        aFrame->GetStyleDisplay()->IsScrollableOverflow()) {
>+      // Clip rect to the viewport established by this inner-<svg>:
>+      float x, y, width, height;
>+      static_cast<nsSVGSVGElement*>(aFrame->GetContent())->
>+        GetAnimatedLengthValues(&x, &y, &width, &height, nsnull);

Are we sure that width, height are > 0 here. I think we are so we should assert. If you
think we aren't then you should test and return as // nothing to invalidate so as not to
call RoundGfxRectToAppRect with either width or height <= 0

>+      nsRect viewportRect =
>+        nsLayoutUtils::RoundGfxRectToAppRect(gfxRect(0.0, 0.0, width, height),
>+                                             appUnitsPerCSSPx);

r=longsonr with comments addressed.
Attachment #635998 - Flags: review?(longsonr) → review+
Blocks: 767734
Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/5c26b1320e1a
Target Milestone: --- → mozilla16
I had a bit of trouble with layout/reftests/svg/dynamic-text-04.svg failing intermittently after that push, although it passed on Try on all machines. I changed it to use MozReftestInvalidate in fe96d0162dee, but that just turned it perma-orange. After temporarily disabling it in 2e40e269180e to give me a bit of time to investigate I discovered the problem.

As mentioned in bug 767647 comment 8, I killed off the MarkDirtyBitsOnDescendants function in nsSVGUtils.cpp, and reinstated it as a text-only thing in nsSVGTextFrame.cpp. When I did so I added some optimizations to it to make it return early if aFrame was already dirty, or to skip already dirty descendants (and their descendants). Turns out that was a bad idea, since for whatever reason it seems we can end up dirtying a text frame without dirtying its descendants, which then means we get a bad visual overflow rect when we update. This was causing the dynamic-text-04.svg failure.
So in my final follow-up checkin, 12e4d32ecd07, I put MarkDirtyBitsOnDescendants back to the way it used to be when it was in nsSVGUtils.cpp, and re-enabled dynamic-text-04.svg
What happened to my review comments? They don't seem to have been actioned.
The only review comment I didn't change the patch for was the request to move GetCSSPxToDevPxMatrix. I meant to comment on that part, but got distracted by the issues described in comment 4, sorry.

Regarding moving GetCSSPxToDevPxMatrix, nsSVGIntegrationUtils becomes "integration of SVG effects into display lists", rather than "integration of SVG effects into non-SVG". So in fact I think it makes more sense to keep GetCSSPxToDevPxMatrix there (or as much sense as anywhere). Besides that it would have meant working the changes through my patch queue, which is a pain I could do without right now for something that doesn't clearly need changed.

If you want to change it after my other bug 614738 work has landed, then we can do that.
Err, bug 614732, I mean.
> Regarding moving GetCSSPxToDevPxMatrix, nsSVGIntegrationUtils becomes
> "integration of SVG effects into display lists", rather than "integration of
> SVG effects into non-SVG". So in fact I think it makes more sense to keep
> GetCSSPxToDevPxMatrix there (or as much sense as anywhere). Besides that it
> would have meant working the changes through my patch queue, which is a pain
> I could do without right now for something that doesn't clearly need changed.

OK, I guess I buy that.

> 
> If you want to change it after my other bug 614738 work has landed, then we
> can do that.

No.
Depends on: 767996
Depends on: 768237
Blocks: 772152
Depends on: 779403
Depends on: 787837
Depends on: 807002
You need to log in before you can comment on or make changes to this bug.