Last Comment Bug 738192 - Get rid of the horrendous "invalidate everything" hack in nsSVGUtils::FindFilterInvalidation
: Get rid of the horrendous "invalidate everything" hack in nsSVGUtils::FindFil...
Status: RESOLVED FIXED
: perf
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Jonathan Watt [:jwatt]
:
Mentors:
Depends on: 734082 767647 767996 768237 779403 787837 807002
Blocks: 772152 767734
  Show dependency treegraph
 
Reported: 2012-03-22 03:55 PDT by Jonathan Watt [:jwatt]
Modified: 2012-10-30 10:45 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (20.45 KB, patch)
2012-06-22 18:34 PDT, Jonathan Watt [:jwatt]
longsonr: review+
Details | Diff | Splinter Review

Description Jonathan Watt [:jwatt] 2012-03-22 03:55:53 PDT
We should get rid of the horrendous "invalidate everything" hack in nsSVGUtils::FindFilterInvalidation that was introduced in bug 463939.
Comment 1 Jonathan Watt [:jwatt] 2012-06-22 18:34:19 PDT
Created attachment 635998 [details] [diff] [review]
patch
Comment 2 Robert Longson 2012-06-23 00:17:01 PDT
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.
Comment 3 Jonathan Watt [:jwatt] 2012-06-24 06:01:37 PDT
Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/5c26b1320e1a
Comment 4 Jonathan Watt [:jwatt] 2012-06-24 10:17:23 PDT
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.
Comment 5 Jonathan Watt [:jwatt] 2012-06-24 10:19:25 PDT
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
Comment 6 Robert Longson 2012-06-24 12:11:40 PDT
What happened to my review comments? They don't seem to have been actioned.
Comment 7 Jonathan Watt [:jwatt] 2012-06-24 12:20:00 PDT
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.
Comment 8 Jonathan Watt [:jwatt] 2012-06-24 12:21:27 PDT
Err, bug 614732, I mean.
Comment 9 Robert Longson 2012-06-24 14:47:11 PDT
> 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.

Note You need to log in before you can comment on or make changes to this bug.