463939 - The area covered by a filtered element is not repainted if the element is moved

Status: RESOLVED FIXED

(Core :: SVG, defect, P2)

Description

[clyde6]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1b2pre) Gecko/20081109 Minefield/3.1b2pre
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1b2pre) Gecko/20081109 Minefield/3.1b2pre

If a filtered SVG element is animated, the dirty area beneath its previous location is not refreshed.  Present in latest trunk, verified in WinXP and Linux.  See the attached test case for more details.

Reproducible: Always

Steps to Reproduce:
1. create svg element
2. apply filter to element
3. animate it with javascript using transforms
Actual Results:  
The animation leaves a trail of artifacts that remain until the element underneath is refreshed.

Expected Results:  
No artifacts.

Comment 1

[clyde6]

Attachment: Testcase showing artifacts

Comment 2

[(mostly gone) XtC4UaLL [:xtc4uall]]

i confirm the issue for Minefield/3.1b2pre ID:20081109032233 on XP.
the issue does not appear in Daniel Holbert's SMIL tryserver build (mentioned https://bugzilla.mozilla.org/show_bug.cgi?id=216462#c122)
so i presume either he fixed the issue and it's not landed yet in MC or some fixed SVG bug (http://tinyurl.com/5grbm2) regressed this.

Comment 3

[Daniel Holbert [:dholbert]]

This looks very similar to bug 465996, which has a patch.  However, that patch only changes the way we draw SVG circles, so it almost certainly wont fix *this* bug's testcase.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to comment #2)
> the issue does not appear in Daniel Holbert's SMIL tryserver build (mentioned
> https://bugzilla.mozilla.org/show_bug.cgi?id=216462#c122)
> so i presume either he fixed the issue and it's not landed yet in MC or some
> fixed SVG bug (http://tinyurl.com/5grbm2) regressed this.

The SMIL patch does not fix this issue -- I just tested an up-to-date build.  This bug probably just regressed in the time since the tryserver build you mentioned.

Comment 5

[Robert Longson [:longsonr]]

This is unconnected to bug 465996 as it does not use extreme scaling, nor does it use SMIL.

The issue is that FindFilterInvalidation returns a smaller rect than the covered area and when the shape moves it doesn't invalidate enough.

Comment 6

[Jonathan Watt [:jwatt]]

From my debugging so far I'm strongly suspicious that the underlying issue here is the same as (or strongly related to) bug 457486. More to come after I run some errands...

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: reduced testcase

Comment 8

[Jonathan Watt [:jwatt]]

The problem seems to be that nsSVGOuterSVGFrame::UpdateAndInvalidateCoveredRegion uses nsSVGUtils::FindFilterInvalidation incorrectly. The current call that passes in aFrame and oldRegion seems to expect to have the filters on aFrame's parent chain applied to oldRegion to find the area that would be covered if oldRegion was the area being filtered. However, what FindFilterInvalidation really does is find the area covered by the filtered frame and reduces it to the intersection with oldRegion. Unfortunately, since we're under AttributeChanged at this point, the area covered by the frame is the area covered by the frame in its *new* position, and as a result, nothing outside the frame's new covered area is invalidated.

Comment 9

[Robert Longson [:longsonr]]

Do we need to go back to caching the filterRect so that we know the old rect during AttributeChanged?

Comment 10

[Robert Longson [:longsonr]]

bug 461131 removed filterRect as it was no longer used. Rather than storing GetFilterBBox in the rect I think we need to have it contain GetInvalidationBBox and then use the filterRect in FindFilterInvalidation rather than calling GetInvalidationBBox directly.

Comment 11

[Jonathan Watt [:jwatt]]

Attachment: patch - not right (the covered region and bbox are in different coordinate systems)

Actually, now that I understand the code a little better, I thing this is simply a case of using the wrong function. We want to be able to pretend the filter covers a region that it doesn't currently cover, and that's what GetFilterBBox provides.

Comment 12

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

That isn't the right fix, as we discussed.

The key issue here is what should be included in an SVG frame's CoveredRegion. Right now it includes the area painted by the element, *ignoring* any filter set on the element. I think probably it should include the effects of filters set on the element, but continue excluding the effects of filters on ancestors.

Comment 13

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

I wrote up a statement of the current situation and what I think needs to be done. Feel free to edit!
https://wiki.mozilla.org/SVG:CoveredRegions

Comment 15

[Martin]

Attachment: Reftests for problems related to updating filters

Had some time to look at this, but don't think I can add much understanding to what's here already.

This is basically a regression from bug 445297 - though behaviour has changed in various related areas in the interim.

Results of tests with Mozilla -r 970e86f6eb56 [1], -r 3f5016ee13e8 [2], and tip -r e2d57aab6d20 [3], also Opera 9.63 and Batik 1.7 - presumably there exists a webkit browser with reasonable svg support, but I don't seem to have one. Patch does not include fudging I did to get the tests usable across all five.

 1 | 2 | 3 | O | B |
   |   |   | p | a |
 M | M | M | e | t |
 o | o | o | r | i |
 z | z | z | a | k |
---+---+---+---+---+----------------------------
 P | P | P | P | P | rect_shrink
 P |   |   | P | P | rect_shrink_filter_direct
 P | P | P | P |   | rect_shrink_filter_use_dims
 P | P | P | P |   | rect_shrink_filter_use_all
 P |   |   | P | P | rect_move_filter_direct
 P | P | P | P |   | rect_move_filter_use_dims
 P | P | P |   |   | rect_move_filter_use_all
 P |   |   | P |   | rect_filter_primitive_shrink
 P |   |   | P |   | rect_filter_primitive_move
 P |   |   | P |   | rect_filter_shrink
 P |   |   | P |   | rect_filter_move
   |   | P | P |   | whole_filter_shrink
   |   | P | P |   | whole_filter_move

The patch also includes the test from bug 445297 comment 4 by roc in reftest form, as that's re-regressed since.

Comment 16

[Jonathan Watt [:jwatt]]

Attachment: wip patch using approach in comment 12

This is what I currently have based on the approach in comment 12. I've been so busy trying to understand the intricacies of invalidation for all sorts of different types of changes that I only just realized this approach doesn't help when a non-"leaf" in being filtered and one of its "leaf" children moves. For example, take the 'reduced testcase' and move the 'filter' attribute up to a parent <g>. The fundamental problem is that GetInvalidationBBox clips to GetBBox() (+20% expansion) of the frame at its *new* position, so skipping the GetInvalidationBBox call in nsSVGUtils::FindFilterInvalidation for "leafs" only helps in there are no filters on the containers. If there are filters on the containers then we'll essentially clip to their new bbox as FindFilterInvalidation walks up the tree. I need to get some sleep before I can figure out what to do about this though.

Comment 17

[Jonathan Watt [:jwatt]]

Sorry, I thought I'd removed all my non-essential comments for that diff. Anyway, probably good to get eyes on them.

Comment 18

[Jonathan Watt [:jwatt]]

Apparently my explanation in comment 16 of why things sometimes still don't work even after changing the cached covered regions on the leafs to include filters on the leafs was unclear. For the record I'll try again. :-) It's very important to make a distinction between "bbox" and "covered region" in the text that follows. The former is a very specific term in SVG and is a rect in some user space, the latter is a rect essentially in app units in nearest outer <svg> viewport space. (Our code is a bit confused and uses the term "bbox" incorrectly, but I'll fix that in another bug.)

So... FindFilterInvalidation walks up the parent chain, and if it encounters a filter on a container it calls GetInvalidationBBox. In the nsSVGFilterInstance that's created we need to calculate the filter region and filter primitive subregions. These regions are typically derived from the bbox returned by calling GetBBox on the filtered element. Unfortunately, even thought we're trying to invalidate the *old* area, the value that GetBBox returns at this point is the *new* bbox since all this is happening under AttributeChanged (note that's passed tense). It doesn't matter that we're calling GetBBox on a container, because the value returned from GetBBox is then simply the union of all the bboxes of its graphic element children, and all of them return their new bbox.

So...essentially what we get is a filter graph of the filtered container element at its *new* position, and we're trying to use that to invalidate the *old* covered region. This doesn't work because under GetInvalidationBBox the filter primitives' mResultBoundingBox members are clipped to the filter region and their respective primitive filter primitive subregions, and the respective mResultChangeBox rects are in turn clipped to the mResultBoundingBox rects. Since the filter region and filter primitive subregions are for the element in the wrong (its new) position, we end up clipping the old covered region we passed into FindFilterInvalidation by the new filtered position. This is of course quite useless for finding the *old* area the filtered element used to cover.

Comment 19

[Jonathan Watt [:jwatt]]

So the conclusion roc and I have come to is that for the purposes of 1.9.1 we should simply invalidate the entire area of the nearest viewport element whenever we need to invalidate a filtered element. This is not a great solution, but we should put correctness above performance.

Comment 20

[Jonathan Watt [:jwatt]]

Attachment: patch using approach in comment 19

Here's a patch that invalidates the entire nearest viewport. I'm working on a rather more involved patch that should enable us to not do that though, so I'm not sure if I want review an this yet.

Comment 21

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

+  nsMargin bp = GetUsedBorderAndPadding();
+  ApplySkipSides(bp);
+  rect.MoveBy(nsPoint(bp.left, bp.top));

As noted in the other bug, this change isn't quite right.

+  /**
+   * Gets the nearest nsSVGInnerSVGFrame or nsSVGOuterSVGFrame frame. If 
+   * aFrame is of type nsGkAtoms::svgOuterSVGFrame, returns nsnull.
+   */
+  static nsSVGDisplayContainerFrame* GetNearestSVGSVGFrame(nsIFrame *aFrame);

This name sucks. I know you borrowed it. Can we call it GetNearestSVGViewport instead? Document that aFrame must be eSVG.

+      //rect = filter->GetInvalidationBBox(aFrame, rect);

Just take this out.

+        NS_ERROR("Need to handle outer-<svg> with overflow:visible below");

We can handle it. Just use GetOverflowRect here:
+        nsRect r = static_cast<nsSVGOuterSVGFrame*>(viewportFrame)->GetContentRect();
+        r.MoveTo(0,0);
+        return r;

Personally, I think it's not worth handling the inner <svg> case. We should just call GetOuterSVGFrame and be done so we don't need all this extra code.

Comment 22

[Jonathan Watt [:jwatt]]

Attachment: patch

(In reply to comment #21)
> As noted in the other bug, this change isn't quite right.

Yeah, sorry. Missed that bit when I took the other changes out. I've removed it in this patch.

> Can we call it GetNearestSVGViewport
> instead? Document that aFrame must be eSVG.

Done.

> Just take this out.

I'd rather leave it in so I can figure out how the filter graph stuff was intended to work and where that function is/was used. It just makes things easier.

> Just use GetOverflowRect

Done.

> Personally, I think it's not worth handling the inner <svg> case. We should
> just call GetOuterSVGFrame and be done so we don't need all this extra code.

I'd like some sort of workaround for people if they are finding this really hurts in some use cases when wrapping filtered elements with an <svg> could help.

Comment 23

[Jonathan Watt [:jwatt]]

I'm off to catch my flight. If someone could check this is for me that'd be great.

Comment 24

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

http://hg.mozilla.org/mozilla-central/rev/b0a3fc399eff

Comment 25

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

Also had some bustage fixes to remove extraneous semicolons:
http://hg.mozilla.org/mozilla-central/rev/4d32e821e751
http://hg.mozilla.org/mozilla-central/rev/864a2e460096

Comment 26

[Jonathan Watt [:jwatt]]

Sorry about that. :-( I could swear that I not only built that, but also ran the SVG reftests.

Comment 27

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

Jonathan, this patch needs some non-trivial backporting to the 1.9.1 branch, can you take care of it?

Comment 28

[Jonathan Watt [:jwatt]]

Attachment: patch for 1.9.1

Here's the patch, but I'm still waiting on the tree to look landable.

Comment 29

[Jonathan Watt [:jwatt]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f57e2a9fb994

Comment 30

[Aakash Desai [:aakashd]]

When using the latest nightlies on trunk and Shiretoko for the test case in comment #1...

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090526 Minefield/3.6a1pre ID:20090526044156 

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090528 Shiretoko/3.5pre ID:20090528045343

...my CPU utilization spikes to 100% everytime the box goes from bottom to the top of the large rectangle.

Does anyone else see this?