Closed
Bug 463939
Opened 16 years ago
Closed 16 years ago
The area covered by a filtered element is not repainted if the element is moved
Categories
(Core :: SVG, defect, P2)
Core
SVG
Tracking
()
RESOLVED
FIXED
People
(Reporter: clyde6, Assigned: jwatt)
References
Details
(Keywords: fixed1.9.1, testcase)
Attachments
(5 files, 3 obsolete files)
(deleted),
image/svg+xml
|
Details | |
(deleted),
image/svg+xml
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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 2•16 years ago
|
||
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•16 years ago
|
Flags: blocking1.9.1?
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Comment 3•16 years ago
|
||
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•16 years ago
|
||
(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•16 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.
Assignee | ||
Comment 6•16 years ago
|
||
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...
Assignee | ||
Comment 7•16 years ago
|
||
Assignee: nobody → jwatt
Assignee | ||
Updated•16 years ago
|
Summary: Animating a filtered SVG element leaves artifacts. → The area previously covered by any filtered element that has moved is not repainted
Assignee | ||
Comment 8•16 years ago
|
||
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•16 years ago
|
||
Do we need to go back to caching the filterRect so that we know the old rect during AttributeChanged?
Comment 10•16 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.
Assignee | ||
Comment 11•16 years ago
|
||
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
Attachment #361928 -
Flags: superreview?(roc)
Attachment #361928 -
Flags: superreview-
Attachment #361928 -
Flags: review?(roc)
Attachment #361928 -
Flags: review-
Comment 15•16 years ago
|
||
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.
Assignee | ||
Comment 16•16 years ago
|
||
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.
Assignee | ||
Comment 17•16 years ago
|
||
Sorry, I thought I'd removed all my non-essential comments for that diff. Anyway, probably good to get eyes on them.
Assignee | ||
Updated•16 years ago
|
Attachment #361928 -
Attachment description: patch → patch - not right (the covered region and bbox are in different coordinate systems)
Assignee | ||
Updated•16 years ago
|
Attachment #361928 -
Attachment is obsolete: true
Assignee | ||
Comment 18•16 years ago
|
||
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.
Assignee | ||
Comment 19•16 years ago
|
||
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.
Assignee | ||
Comment 20•16 years ago
|
||
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•16 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.
Assignee | ||
Comment 22•16 years ago
|
||
(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+
Assignee | ||
Comment 23•16 years ago
|
||
I'm off to catch my flight. If someone could check this is for me that'd be great.
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 16 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
Assignee | ||
Comment 26•16 years ago
|
||
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?
Assignee | ||
Comment 28•16 years ago
|
||
Here's the patch, but I'm still waiting on the tree to look landable.
Assignee | ||
Comment 29•16 years ago
|
||
Comment 30•16 years ago
|
||
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?
You need to log in
before you can comment on or make changes to this bug.
Description
•