615658 - SMIL animation of some filter attributes don't invalidate correctly

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Marek Raida]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101130 Firefox/4.0b8pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101130 Firefox/4.0b8pre

If animating some filter attributes, like pointsAtY, pointsAtX... and in document is no other SMIL animation, rendering is not repainted until at least some user operation, like click, does not occur.

Reproducible: Always

Steps to Reproduce:
1. Open file from attachment or from URL
2. Image seems static, but when you start clicking on it
3. you can see, that animation is really running, because rendering changes
Actual Results:  
looks like static image

Expected Results:  
you should see animation

If you add some other animation to the document, which forces redraw, then it "starts" to be working... (in second attachment)

Comment 1

[Marek Raida]

Attachment: Reduced testcase

Comment 2

[Marek Raida]

Attachment: Modified testcase with another animation, which "works"

Comment 3

[Daniel Holbert [:dholbert]]

Confirmed on today's nightly:
Mozilla/5.0 (X11; Linux x86_64; rv:2.0b8pre) Gecko/20101130 Firefox/4.0b8pre

Comment 4

[Robert Longson [:longsonr]]

Attachment: patch with reftests

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 494797 [details] [diff] [review]
patch with reftests

The nsSVGFilters.cpp tweaks look good.  I don't entirely understand the nsCSSFrameConstructor.cpp tweak, though:

>diff --git a/layout/base/nsCSSFrameConstructor.cpp b/layout/base/nsCSSFrameConstructor.cpp
>--- a/layout/base/nsCSSFrameConstructor.cpp
>+++ b/layout/base/nsCSSFrameConstructor.cpp
>@@ -4878,27 +4878,27 @@ nsCSSFrameConstructor::FindSVGData(nsICo
>     SIMPLE_SVG_CREATE(feComposite, NS_NewSVGLeafFrame),
>-    SIMPLE_SVG_CREATE(feComponentTransfer, NS_NewSVGLeafFrame),
>+    SIMPLE_SVG_CREATE(feComponentTransfer, NS_NewSVGContainerFrame),
>     SIMPLE_SVG_CREATE(feConvolveMatrix, NS_NewSVGLeafFrame),
>-    SIMPLE_SVG_CREATE(feDiffuseLighting, NS_NewSVGLeafFrame),
>+    SIMPLE_SVG_CREATE(feDiffuseLighting, NS_NewSVGContainerFrame),
>     SIMPLE_SVG_CREATE(feDisplacementMap, NS_NewSVGLeafFrame),
[...]
>     SIMPLE_SVG_CREATE(feOffset, NS_NewSVGLeafFrame), 
>-    SIMPLE_SVG_CREATE(feSpecularLighting, NS_NewSVGLeafFrame),
>+    SIMPLE_SVG_CREATE(feSpecularLighting, NS_NewSVGContainerFrame),

What's the significance of this change?

Comment 6

[Jonathan Watt [:jwatt]]

Note how I'm handling invalidation for filter animation in bug 589439 without creating new frames.

Comment 7

[Robert Longson [:longsonr]]

(In reply to comment #5)

> What's the significance of this change?

These filters can have child elements. Without the parents being containers, the children don't get frames.

Comment 8

[Jonathan Watt [:jwatt]]

I'm not saying my approach is "right" or "better" BTW, just that it's worth looking at.

Comment 9

[Robert Longson [:longsonr]]

(In reply to comment #6)
> Note how I'm handling invalidation for filter animation in bug 589439 without
> creating new frames.

And you think that's better? In this case it could work as the children don't have any css properties. What do you think Daniel.

Comment 10

[Robert Longson [:longsonr]]

If we're going to go down your route Jonathan we should properly suppress the frames in nsCSSFrameConstructor and not pretend to create them when in any working markup we actually won't

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to comment #6)
> Note how I'm handling invalidation for filter animation in bug 589439 without
> creating new frames.

In particular:
 
+void
+nsSVGComponentTransferFunctionElement::DidAnimateNumberList(PRUint8 aAttrEnum)
+{
+  // We don't have a frame, so use our parent's
+  nsCOMPtr<nsIDOMSVGFEComponentTransferElement> compTrans =
+    do_QueryInterface(GetParent());
+  if (compTrans) {
+    // nsSVGLeafFrame doesn't implement AttributeChanged.
+    nsIFrame* frame = static_cast<nsSVGFE*>(GetParent())->GetPrimaryFrame();
+    if (frame) {
+      nsSVGEffects::InvalidateRenderingObservers(frame);
+    }
+  }

(In reply to comment #9)
> And you think that's better? In this case it could work as the children don't
> have any css properties. What do you think Daniel.

It strikes me as more conservative -- it's probably better to use existing frames rather than creating extra frames that we (apparently?) don't otherwise need. I'd defer to you guys & roc on this :) but my initial leaning is to prefer jwatt's method.

(If we go with that, though, we should probably *assert* that we don't have a frame -- that way, if we ever add a frame for the child elements for some other reason, we'll know to update this code to use our own frame.)

Also, Robert: could you add a reftest for feSpecularLighting as well? (You've got tests for the other two tweaked elements from Comment 5's quoted chunk, which look great -- we might as well have test coverage for that third one, too, though.)

Comment 12

[Jonathan Watt [:jwatt]]

I have no strong preference, but it would be nice not to allocate memory for frames if we don't need them.

(In reply to comment #10)
> If we're going to go down your route Jonathan we should properly suppress the
> frames in nsCSSFrameConstructor and not pretend to create them when in any
> working markup we actually won't

It would be nice if the code wasn't misleading, agreed.

(In reply to comment #11)
> (If we go with that, though, we should probably *assert* that we don't have a
> frame -- that way, if we ever add a frame for the child elements for some other
> reason, we'll know to update this code to use our own frame.)

Actually I was going to suggest that we use a kind of mixture of Robert's and my approach. Specifically to have DidAnimateAttr look up the element parent chain for the first element for which GetPrimaryFrame returns something, rather than just calling GetPrimaryFrame once. (It should probably assert that it doesn't hit an nsSVGFilterElement.)

Comment 13

[Jonathan Watt [:jwatt]]

Comment on attachment 494797 [details] [diff] [review]
patch with reftests

> protected:
>   virtual NumberAttributesInfo GetNumberInfo();
>+  virtual void DidAnimateNumber(PRUint8 aAttrEnum);
>   virtual EnumAttributesInfo GetEnumInfo();
>+  virtual void DidAnimateEnum(PRUint8 aAttrEnum);
> 

Could we not have:

> protected:
>   virtual NumberAttributesInfo GetNumberInfo();
>+  virtual void DidAnimateNumber(PRUint8 aAttrEnum) {
>+    DidAnimateAttr(this);
>+  }
>   virtual EnumAttributesInfo GetEnumInfo();
>+  virtual void DidAnimateEnum(PRUint8 aAttrEnum) {
>+    DidAnimateAttr(this);
>+  }
> 

I know since these are virtual we don't get the benefits of inlining of the code, but since the implementation is only a single line it might as well be here, which saves some searching up and down the file to check on implementations when examining the code.

Comment 14

[Jonathan Watt [:jwatt]]

dholbert points out that with such a loop the invalidation could be wrong in the face of display:none. So rather than a loop in DidAnimateAttr, how about passing the parent to DidAnimateAttr.

nsSVGComponentTransferFunctionElement::DidAnimateEnum(PRUint8 aAttrEnum)
{
  if (Parent()) {
    DidAnimateAttr(Parent());
  }
}

Comment 15

[Jonathan Watt [:jwatt]]

I believe checking the parent isn't null is necessary since the SMIL code can hold onto element's via cached nsSMILValues, right dholbert?

Comment 16

[Daniel Holbert [:dholbert]]

Good question -- the null-parent check would be unnecessary.  

In a given SMIL sample, there's no way that we can evaluate animations on an element that lacks a parent (and trigger DidAnimateEnum() calls on it).  As soon as an element is detached from the tree, any <animate> children will be immediately unregistered (& won't participate in the next SMIL sample), and any animations that use xlink:href to target its ID will not find it in the next sample.

> since the SMIL code can hold onto element's via cached nsSMILValues

That's true (via some types of nsSMILValues that have owning references, and also via nsSMILTargetIdentifiers).  However....
 (a) the point above stands (those elements can't be targeted by animations)
...and also...
 (b) those nsSMILValues and nsSMILTargetIdentifiers are only used for equality-checking
 (c) those nsSMILValues and nsSMILTargetIdentifiers (and their references) will be dropped in the first sample after their target is removed from the tree.

So, to sum up: in e.g. nsSVGComponentTransferFunctionElement::DidAnimateEnum -- we should assert that we don't have a frame, and either assert or comment that we assume we *do* have a parent element.

Comment 17

[Robert Longson [:longsonr]]

Attachment: address review comments

Comment 18

[Jonathan Watt [:jwatt]]

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

You can use GetFlattenedTreeParent instead of nsSVGUtils::GetElementParent if you change DidAnimateAttr's argument to nsIContent. (At some point nsSVGUtils::GetElementParent should die, or at least be changed to use GetFlattenedTreeParent.)

Comment 19

[Robert Longson [:longsonr]]

I'll leave that for bug 484966 if that's OK.

Comment 20

[Jonathan Watt [:jwatt]]

Seems like you might as well use GetFlattenedTreeParent in the code you're already adding, but either way...

Comment 21

[Jonathan Watt [:jwatt]]

Pushed http://hg.mozilla.org/mozilla-central/rev/f227a6e64b3f

Comment 22

[Jonathan Watt [:jwatt]]

And http://hg.mozilla.org/mozilla-central/rev/17bc5bbcb0c6

since somehow 'hg import' didn't add the new test files!

Comment 23

[Jonathan Watt [:jwatt]]

Actually, that wasn't 'hg import's fault - this was one of the patches I pushed that wasn't created using 'hg export', so I had to apply it manually but forgot I then needed to manually add new files.