If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED

Status

()

Core
SVG
P2
normal
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: clyde6, Assigned: jwatt)

Tracking

({fixed1.9.1, testcase})

Trunk
fixed1.9.1, testcase
Points:
---
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

9 years ago
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.
(Reporter)

Updated

9 years ago
Version: unspecified → Trunk
(Reporter)

Comment 1

9 years ago
Created attachment 347196 [details]
Testcase showing artifacts
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.
Status: UNCONFIRMED → NEW
Component: General → SVG
Ever confirmed: true
Keywords: testcase
Product: Firefox → Core
QA Contact: general → general

Updated

9 years ago
Flags: blocking1.9.1?
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
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.
(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

9 years ago
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.
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...
Created attachment 353362 [details]
reduced testcase
Assignee: nobody → jwatt
(Assignee)

Updated

9 years ago
Summary: Animating a filtered SVG element leaves artifacts. → The area previously covered by any filtered element that has moved is not repainted
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.
Summary: The area previously covered by any filtered element that has moved is not repainted → The area covered by a filtered element is not repainted if the element is moved

Comment 9

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

Comment 10

9 years ago
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.
Created attachment 361928 [details] [diff] [review]
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.
Attachment #361928 - Flags: superreview?(roc)
Attachment #361928 - Flags: review?(roc)
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.
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

Updated

9 years ago
Duplicate of this bug: 479722
Attachment #361928 - Flags: superreview?(roc)
Attachment #361928 - Flags: superreview-
Attachment #361928 - Flags: review?(roc)
Attachment #361928 - Flags: review-

Comment 15

9 years ago
Created attachment 370292 [details] [diff] [review]
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.
Created attachment 372534 [details] [diff] [review]
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.
Sorry, I thought I'd removed all my non-essential comments for that diff. Anyway, probably good to get eyes on them.
(Assignee)

Updated

9 years ago
Attachment #361928 - Attachment description: patch → patch - not right (the covered region and bbox are in different coordinate systems)
(Assignee)

Updated

9 years ago
Attachment #361928 - Attachment is obsolete: true
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.
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.
Created attachment 373859 [details] [diff] [review]
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.
Attachment #372534 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #373859 - Flags: review?(roc)
+  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.
Created attachment 374544 [details] [diff] [review]
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.
Attachment #373859 - Attachment is obsolete: true
Attachment #374544 - Flags: review?(roc)
Attachment #373859 - Flags: review?(roc)
Attachment #374544 - Flags: review?(roc) → review+
I'm off to catch my flight. If someone could check this is for me that'd be great.
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/b0a3fc399eff
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs 191 landing]
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
Sorry about that. :-( I could swear that I not only built that, but also ran the SVG reftests.
Jonathan, this patch needs some non-trivial backporting to the 1.9.1 branch, can you take care of it?
Created attachment 378582 [details] [diff] [review]
patch for 1.9.1

Here's the patch, but I'm still waiting on the tree to look landable.
Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f57e2a9fb994
Keywords: fixed1.9.1
Hardware: x86 → All
Whiteboard: [needs 191 landing]
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?

Updated

5 years ago
Depends on: 485361
You need to log in before you can comment on or make changes to this bug.