421584 - Filter on DOM-animated element incorrectly masks drawing region.

Status: RESOLVED FIXED

(Core :: SVG, defect, P1)

Description

[Gavin Kistner]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3

When the feColorMatrix filter is applied in the attached file, the moving hand of the dial is masked to some rectangular region near it originally. As the JS moves the hand (changes its transform), you can see the clipping.

The JS turns the filter on and off (by hacking the type attribute of the <style> element that has that filter in it); you can see how the hand renders with this filter not applied.

Reproducible: Always

Steps to Reproduce:
1. Preview attached file

(Uncertain how general this case is. Does it apply only to this filter? To any filter applied through CSS? To any filter at all, when JS changes the transform attribute dynamically? Only when the rotate() component of the transform is changed? Only when the element is in a group that has been cloned?)
2. Notice the hand being drawn clipped to different regions each time the filter becomes active again.

Actual Results:  
The hand would move smoothly, in greyscale, over the clock.

Expected Results:  
The hand is masked to expanded bounding boxes each time the filter is enabled.

JavaScript clones the #dial-template group in the <defs>; it is that clone that is controlled. (Unsure if this is a relevant detail or not.)

Seen on FF3.0b3 on both Windows XP and OS X.

I'm listing the severity as "Major" because incorrect rendering is worse than no rendering for this feature.

Comment 1

[Gavin Kistner]

Attachment: Animated Hand on a Clock, with feColorMatrix filter.

Comment 2

[Jonathan Watt [:jwatt]]

It seems the covered region for filtered elements isn't updated when they move.

Comment 3

[Robert Longson [:longsonr]]

I think the key thing is this structure

<g filter="yy">
   <g transform="xx">
   ...
   </g>
</g>

You update the child g transform and it notifies all its children but the filter is on the parent and it is not notified. It seems that we need to change transform notification so that it does some kind of filter region update notification from the outer svg frame. We should be able to use the NotifyChildrenOfSVGChange infrastructure with a new flag to do that.

Comment 4

[Robert Longson [:longsonr]]

Attachment: patch

Comment 5

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

Why are we just doing this for "transform"? Why not anything that can change child geometry, such as "x"?

Comment 6

[Robert Longson [:longsonr]]

Because nsSVGGraphicElement::GetLocalTransformMatrix only depends on transforms, so therefore so does mCanvasTM and the cached value of that is what we're using to determine the region.

Comment 7

[Robert Longson [:longsonr]]

But nsSVGFilterFrame::GetInvalidationRegion also needs to call GetBBox which is dependent on positioning so you are right, anything that affects the bounding box should cause a call to the new function.

Comment 8

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

Can we just detect invalidation of the child and use that to trigger recalculation of the invalidation region for the filter?

Comment 9

[Robert Longson [:longsonr]]

Attachment: address review comments

(In reply to comment #8)
> Can we just detect invalidation of the child and use that to trigger
> recalculation of the invalidation region for the filter?
> 

Good idea, that would seem to be more robust.

Comment 10

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

NotifyParentsOfFilterRegionChange should be NotifyAncestorsOfFilterRegionChange.

It looks like nsSVGContainerFrame propagates UpdateCoveredRegion over the whole frame subtree ... it seems wasteful to call NotifyAncestorsOfFilterRegionChange on every single frame in that subtree. Would it work if NotifyAncestorsOfFilterRegionChange was called by the code that calls UpdateCoveredRegion on the path object, glyphframe or foreignObject frame?

Comment 11

[Robert Longson [:longsonr]]

Attachment: address review comments

(In reply to comment #10)
> NotifyParentsOfFilterRegionChange should be
> NotifyAncestorsOfFilterRegionChange.

Done.

> 
> It looks like nsSVGContainerFrame propagates UpdateCoveredRegion over the whole
> frame subtree ... it seems wasteful to call NotifyAncestorsOfFilterRegionChange
> on every single frame in that subtree. Would it work if
> NotifyAncestorsOfFilterRegionChange was called by the code that calls
> UpdateCoveredRegion on the path object, glyphframe or foreignObject frame?
> 

The only caller of the child frame UpdateCoveredRegion is nsSVGMarkerFrame so I doubt it's significant. Still it is possible to do it the way you suggest so here you go...

Comment 12

[Robert Longson [:longsonr]]

Please ignore the nsSVGGradientFrame.cpp change that sneaked in from another bug. I won't be committing that change as part of this bug.

Comment 13

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

+  if (!dirtyRect.IsEmpty()) {

Why are you adding this? if this needs to be added anywhere it should be pushed down into the implementation of InvalidateRect. Although it's not clear to me this optimization actually matters.

     nsRect filterRect;
     filterRect = nsSVGUtils::FindFilterInvalidation(this);
     if (!filterRect.IsEmpty()) {
       outerSVGFrame->InvalidateRect(filterRect);
     } else {
       outerSVGFrame->InvalidateRect(mRect);
     }
+    nsSVGUtils::NotifyAncestorsOfFilterRegionChange(this);

Why don't we share this code with foreignobject?

Comment 14

[Jonathan Watt [:jwatt]]

We should certainly take a fix for this. Marking blocking1.9+.

Comment 15

[Robert Longson [:longsonr]]

Attachment: address review comments

Review comments addressed hopefully, although changing the foreignObject code meant that I had to test foreignObject which uncovered other existing bugs and made this patch bigger.

Let me try to head off some questions...

I've reverted the change in bug 401112 to FindFilterInvalidation so that it now returns an empty rect if there is no filter. I moved the filter vs covered region logic in nsSVGOuterSVGFrame::InvalidateCoveredRegion. This means that the invalidation of the entire foreignObject is now made explicit as per the desire in bug 418063 comment 3.

I've pulled the reflow = TRUE; line in  nsSVGForeignObjectFrame::NotifySVGChanged because filters on foreignObject go into an infinite loop invalidating their covered region and reflowing. This seems to be an existing bug. bug 340908 has a testcase showing this. 

I've added the suppressInvalidation argument to nsSVGForeignObject::UpdateGraphic because filters on foreignObject crash due to infinite recursion calling NotifySVGChanged and NotifyAncestorsOfFilterRegionChange. bug 331762 has a testcase for this.

N.B. bug 421043 contains a patch, part of which tried to address the issues above.

I've checked that the testcases in bug 401112 and bug 423071 still pass.

Comment 16

[Damon Sicore (:damons)]

Does this patch require a beta cycle?  Must it block beta 5?  Not saying we won't take the patch, but P1 means it blocks the beta.

Comment 17

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

No, but we'll land it for beta5 anyway with any luck.

Comment 18

[Damon Sicore (:damons)]

Moving to P2.  Again, these are still blockers, so let's land when we can.  Just gotta tighten down the beta 5 blocking list here.

Comment 19

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

Comment on attachment 309982 [details] [diff] [review]
address review comments

+  void UpdateGraphic(PRBool suppressInvalidation = PR_FALSE);

Don't use default parameters, they're not valuable here

Comment 20

[Jonathan Watt [:jwatt]]

Comment on attachment 309982 [details] [diff] [review]
address review comments

I'm pretty sure we don't want to skip the UpdateCoveredRegion() and mDirtyRegion.SetEmpty() calls in nsSVGForeignObjectFrame::UpdateGraphic when suppressInvalidation is true. (And can you make that aSuppressInvalidation?)


>+nsSVGUtils::NotifyAncestorsOfFilterRegionChange(nsIFrame *aFrame)
>+{
>+  aFrame = aFrame->GetParent();

We want to invalidate any filter property on aFrame as well as its ancestors, no? In that case we don't want the line above, and maybe a better name is simply NotifyFiltersOfRegionChange.

>+  while (aFrame) {
>+    if (aFrame->GetStateBits() & NS_STATE_IS_OUTER_SVG)
>+      break;

Could just return instead of break.


>-    // Perhaps unexpectedly, we reflow if our CTM changes. This is because
>-    // glyph metrics do not necessarily scale uniformly with change in scale
>-    // and, as a result, CTM changes may require text to break at different
>-    // points.
>-    // XXX roc says we shouldn't do this. See bug 381285 comment 20.
>-    reflow = PR_TRUE;

Please change that to:

  // In an ideal world we would reflow when our CTM changes. This is because
  // glyph metrics do not necessarily scale uniformly with change in scale
  // and, as a result, CTM changes may require text to break at different
  // points. The problem would be how to keep performance acceptable when
  // e.g. the transform of an ancestor is animated.
  // We also seem to get some sort of infinite loop post bug 421584 if we
  // reflow.


r=me with the above fixed/changed.


This patch would seem to make us do more walking up the tree than we should have to. I guess that can be improved later.

The name InvalidateCoveredRegion seems a little misleading since it invalidates the filtered region, but I guess we generally need to fix up all these functions to get our naming story straight.


(In reply to comment #15)
> I've pulled the reflow = TRUE; line in 
> nsSVGForeignObjectFrame::NotifySVGChanged because filters on foreignObject go
> into an infinite loop invalidating their covered region and reflowing. This
> seems to be an existing bug. bug 340908 has a testcase showing this. 

We should check that in as a crashtest.

> I've added the suppressInvalidation argument to
> nsSVGForeignObject::UpdateGraphic because filters on foreignObject crash due to
> infinite recursion calling NotifySVGChanged and
> NotifyAncestorsOfFilterRegionChange. bug 331762 has a testcase for this.

And that one.

I'd be interested in having a better understanding of what causes both these problems, but I don't have the cycles to think it through right now.

Comment 21

[Jonathan Watt [:jwatt]]

I think the invalidation logic is more fragile than I'd like it to be, so I wouldn't be comfortable with this going in without a beta cycle. Setting priority back to P1.

Comment 22

[Jonathan Watt [:jwatt]]

Attachment: patch with jwatt's comments addressed

To try and help get this in on time, here's the patch updated to TIP with my comments addressed. I'm not sure what roc meant about default parameters not being valuable. We'd have to add PR_TRUE to all the other calls for no good reason. Perhaps he missed that. Anyway, it's not a biggie, and it shouldn't block this from landing if necessary.

Comment 23

[Robert Longson [:longsonr]]

(In reply to comment #20)
> (From update of attachment 309982 [details] [diff] [review])
> I'm pretty sure we don't want to skip the UpdateCoveredRegion() and
> mDirtyRegion.SetEmpty() calls in nsSVGForeignObjectFrame::UpdateGraphic when
> suppressInvalidation is true. (And can you make that aSuppressInvalidation?)

All the other UpdateGraphic implementations work that way e.g. nsSVGPathGeometryFrame::UpdateGraphic, nsSVGGlyphFrame::UpdateGraphic. If it's not necessary in those then it shouldn't be necessary here either.

The implementation in your latest patch is wrong and will cause infinite loops. We must not call InvalidateXXX if suppressInvalidation is true.

> 
> 
> >+nsSVGUtils::NotifyAncestorsOfFilterRegionChange(nsIFrame *aFrame)
> >+{
> >+  aFrame = aFrame->GetParent();
> 
> We want to invalidate any filter property on aFrame as well as its ancestors,
> no? In that case we don't want the line above, and maybe a better name is
> simply NotifyFiltersOfRegionChange.

If there is a filter on aFrame, it will be invalidated by whatever is causing the update. You will invalidate twice if you do aFrame here and there is a filter on it.

> 
> >+  while (aFrame) {
> >+    if (aFrame->GetStateBits() & NS_STATE_IS_OUTER_SVG)
> >+      break;
> 
> Could just return instead of break.

Fine.

> 
> 
> >-    // Perhaps unexpectedly, we reflow if our CTM changes. This is because
> >-    // glyph metrics do not necessarily scale uniformly with change in scale
> >-    // and, as a result, CTM changes may require text to break at different
> >-    // points.
> >-    // XXX roc says we shouldn't do this. See bug 381285 comment 20.
> >-    reflow = PR_TRUE;
> 
> Please change that to:
> 
>   // In an ideal world we would reflow when our CTM changes. This is because
>   // glyph metrics do not necessarily scale uniformly with change in scale
>   // and, as a result, CTM changes may require text to break at different
>   // points. The problem would be how to keep performance acceptable when
>   // e.g. the transform of an ancestor is animated.
>   // We also seem to get some sort of infinite loop post bug 421584 if we
>   // reflow.
> 
> 
> r=me with the above fixed/changed.

Fine.

> 
> 
> This patch would seem to make us do more walking up the tree than we should
> have to. I guess that can be improved later.
> 
> The name InvalidateCoveredRegion seems a little misleading since it invalidates
> the filtered region, but I guess we generally need to fix up all these
> functions to get our naming story straight.

It invalidates the covered region in the normal (no filters case) :-)

I'll attach an updated patch.

Comment 24

[Robert Longson [:longsonr]]

Attachment: updated patch

This addresses roc's no default argument comment and 2 of jwatt's comments.

UpdateGraphic is left unchanged from previous patches although I am more than happy to raise another bug to look into all the implementations of UpdateGraphic in all objects.

Additionally NotifyAncestors does not notify the current frame.

Comment 25

[Jonathan Watt [:jwatt]]

(In reply to comment #23)
> All the other UpdateGraphic implementations work that way e.g.
> nsSVGPathGeometryFrame::UpdateGraphic, nsSVGGlyphFrame::UpdateGraphic. If it's
> not necessary in those then it shouldn't be necessary here either.

Of the three other UpdateGraphic methods, only nsSVGPathGeometryFrame has a suppressInvalidation argument. Besides that, while the names are similar, I'm not sure the semantics of the methods are the same.

> The implementation in your latest patch is wrong and will cause infinite loops.
> We must not call InvalidateXXX if suppressInvalidation is true.

It would be helpful if you could describe that loop. (Since InvalidateCoveredRegion doesn't do any notifying, I can only assume that it's the NotifyFilteringRegionChange call that would cause the problem.)

Please be sure to check in all these infinite loop testcases as crashtests.

> If there is a filter on aFrame, it will be invalidated by whatever is causing
> the update. You will invalidate twice if you do aFrame here and there is a
> filter on it.

Okay, cool.

Comment 26

[Robert Longson [:longsonr]]

Attachment: address review comments

(In reply to comment #25)

> It would be helpful if you could describe that loop. (Since
> InvalidateCoveredRegion doesn't do any notifying, I can only assume that it's
> the NotifyFilteringRegionChange call that would cause the problem.)

OK, you got me there bang to rights. I'll come quietly. 

I made this change before I realised it was necessary to remove the reflow = PR_TRUE line in nsSVGForeignObjectFrame::NotifySVGChanged to remove a different cause of infinite loops. 

In fact, the removal of reflow = PR_TRUE is actually sufficient to fix all the infinite loops I know of so the change to UpdateGraphic is unnecessary. I've now reverted it.

The basic cause was the NotifySVGChanged calls in nsSVGFilterFrame.cpp causing the filter to invalidate which meant it redrew causing the filter to be recalcualted causing another call to NotifySVGChanged.

> 
> Please be sure to check in all these infinite loop testcases as crashtests.
> 

Will do, although possibly after b5.

Comment 27

[Jonathan Watt [:jwatt]]

Comment on attachment 310436 [details] [diff] [review]
address review comments

(In reply to comment #26)
> I'll come quietly. 

Heh.

> In fact, the removal of reflow = PR_TRUE is actually sufficient to fix all the
> infinite loops I know of so the change to UpdateGraphic is unnecessary. I've
> now reverted it.

Excellent.

> The basic cause was the NotifySVGChanged calls in nsSVGFilterFrame.cpp causing
> the filter to invalidate which meant it redrew causing the filter to be
> recalcualted causing another call to NotifySVGChanged.

Thanks, that's very helpful.

> Will do, although possibly after b5.

Sure. As long as these testcases are in place for when I try to rip all our notification stuff apart and put it back together again post 1.9. :-)

r=me. Let's get approval for this asap.

Comment 28

[Jonathan Watt [:jwatt]]

Schrep, did you see comment 21? If you're going to reduce the priority, an explanatory comment would be appreciated.

Comment 29

[Mike Schroepfer]

JWatt - sorry stupidly did miss comment 21.   Resetting to previous state.

Comment 30

[Jonathan Watt [:jwatt]]

No problem. It should be academic anyway since I'm going to commit this for Robert just as soon as the tree turns green. ;-)

Comment 31

[Jonathan Watt [:jwatt]]

Checked in.