617623 - Add support for SMIL animation of <number-optional-number> attributes

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

We're missing support for SMIL animation of <number-optional-number> attributes. Specifically:

  'filterRes' attribute on <filter>
  http://www.w3.org/TR/SVG11/filters.html#FilterElementFilterResAttribute

  'order' attribute on <feConvolveMatrix>
  http://www.w3.org/TR/SVG11/filters.html#feConvolveMatrixElementOrderAttribute

  'kernelUnitLength' on <feConvolveMatrix>, <feDiffuseLighting> and <feSpecularLighting>
  http://www.w3.org/TR/SVG11/filters.html#feConvolveMatrixElementKernelUnitLengthAttribute
  http://www.w3.org/TR/SVG11/filters.html#feDiffuseLightingKernelUnitLengthAttribute
  http://www.w3.org/TR/SVG11/filters.html#feSpecularLightingKernelUnitLengthAttribute

  'stdDeviation' on <feGaussianBlur>
  http://www.w3.org/TR/SVG11/filters.html#feGaussianBlurStdDeviationAttribute

  'radius' on <feMorphology>
  http://www.w3.org/TR/SVG11/filters.html#feMorphologyRadiusAttribute

  'baseFrequency' on <feTurbulence>
  http://www.w3.org/TR/SVG11/filters.html#feTurbulenceBaseFrequencyAttribute

Note that 'filterRes' and 'order' are really <integer-optional-integer>, even though no one went to the trouble to add that type to the spec.

These attributes are split across two ECMAScript SVGAnimatedNumber properties or, in the case of 'filterRes' and 'order', two SVGAnimatedInteger properties. For example, the 'filterRes' attribute corresponds to the SVGAnimatedInteger properties 'filterResX' and 'filterResY'.

Comment 1

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

OK, let's do this.

Comment 2

[Marek Raida]

It may be, or may be not is related to this bug, but:

http://srufaculty.sru.edu/david.dailey/svg/texture/spout6.svg
Crashes in nightly FFX even with SMIL svg.smil.enabled = false
Works in FFX 3.6, although there is no SMIL animation.
Reason: probably attempt to animate baseFrequency filter attribute (but when SMIL is disabled, crashing should disappear, or not?).

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to comment #2)
> It may be, or may be not is related to this bug, but:
> http://srufaculty.sru.edu/david.dailey/svg/texture/spout6.svg

Not related, I think. That url has a lot of e.g.
	<feImage xlink:href="#QQ" result="M" />
	<feImage xlink:href="#CP" result="CP" />
which would triggers bug 613899 and crash. That's probably all that's going on.

Comment 4

[Marek Raida]

I wouldn't say so, or that animation must be in some relation there...
I modified original file by commenting out the animation only, and tried to open it 20 times, and it never crashed for me, not a once (spout6ok.svg).
But the original file (spout6ko.svg) crashed for me every time, immediately after document load (tried another 20 times.)
This test done on Win7 I tried it couple of times on WinXP as well and it behaves the very same...

Comment 5

[Marek Raida]

Attachment: Crashing case

Comment 6

[Marek Raida]

Attachment: Not crashing case (with animation element removed)

Comment 7

[Marek Raida]

Attachment: Not crashing case with removed feImage and animate element still there

But probably the animation's relation is really weak, as you are saying, Daniel, because this case does not crashes. However, some relation there statistically maybe is....

Comment 8

[Robert Longson [:longsonr]]

Marek, your testcase is related to bug 613899 and not this bug.

Comment 9

[Marek Raida]

You're right, sorry, I copied my attachments and comments there...

Comment 10

[Robert Longson [:longsonr]]

I have this implemented in my patch queue, no tests yet though.

I've also fixed the HasAttr tests in the filter code so that it all animates as expected.

Comment 11

[Jonathan Watt [:jwatt]]

Sounds great, thanks Robert. :)

Comment 12

[Robert Longson [:longsonr]]

Attachment: patch

Comment 13

[Robert Longson [:longsonr]]

Sent to Try: http://hg.mozilla.org/try/rev/38fff45aba13

Comment 14

[Robert Longson [:longsonr]]

http://hg.mozilla.org/try/pushloghtml?changeset=38fff45aba13

Comment 15

[Robert Longson [:longsonr]]

Attachment: add missing files

Comment 16

[Robert Longson [:longsonr]]

Better once the missing files were included: http://tbpl.mozilla.org/?tree=Try&rev=920fb044cb37

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 541425 [details] [diff] [review]
add missing files

Generally looks great! My comments are mostly nits.

>+++ b/content/svg/content/src/SVGIntegerPairSMILType.cpp
>+void
>+SVGIntegerPairSMILType::Destroy(nsSMILValue& aValue) const
>+{
>+  NS_PRECONDITION(aValue.mType == this, "Unexpected SMIL value");
>+  aValue.mU.mPtr = nsnull;
>+  aValue.mType = &nsSMILNullType::sSingleton;

Nit: We shouldn't be setting mU.mPtr here, since this class doesn't use mU.mPtr for data storage. Instead, we should set the mIntPair vars directly to 0.

This applies to the same spot in SVGNumberPairSMILType, too.

(Kind of silly, but good for consistency.  Note that SVGOrientSMILType has this same issue -- I just noticed that -- but I think all the other nsISMILType impls are correct.  Bug 668296 will make this cleaner, I hope.)

>+SVGIntegerPairSMILType::ComputeDistance(const nsSMILValue& aFrom,
[...]
>+  double delta[2];
[...]
>+  aDistance = sqrt(delta[0] * delta[0] + delta[1] * delta[1]);

Use NS_Hypot here.

This applies to the same spot in SVGNumberPairSMILType, too.

>+nsresult
>+SVGIntegerPairSMILType::Interpolate(const nsSMILValue& aStartVal,
[...]
>+  NS_PRECONDITION(aResult.mType == this, "Unexpected result type");
>+  

Nix the whitespace on that first empty line in Interpolate.

>+  aResult.mU.mIntPair[0] =
>+    aStartVal.mU.mIntPair[0] +
>+    (aEndVal.mU.mIntPair[0] - aStartVal.mU.mIntPair[0]) * aUnitDistance;

So, this does double-to-int conversion, which will implicitly floor(). In contrast, SMILIntegerType::Interpolate rounds to the nearest integer.  I think we should match that behavior here, for consistency.  (I think we should use NS_lround() for the rounding -- I filed bug 668315 to make that change to SMILIntegerType, too.)

>+++ b/content/svg/content/src/SVGNumberPairSMILType.cpp
>+namespace mozilla {
>+  
>+/*static*/ SVGNumberPairSMILType SVGNumberPairSMILType::sSingleton;

Fix whitespace on that blank line.

>diff --git a/content/svg/content/src/nsSVGBoolean.cpp b/content/svg/content/src/nsSVGBoolean.cpp

The nsSVGBoolean changes look good, but they seem unrelated to the rest of this patch.  Perhaps these should get their own bug?  (not a big deal I guess)

>+++ b/content/svg/content/src/nsSVGElement.h
>+    IntegerPairAttributesInfo(nsSVGIntegerPair *aIntegerPairs,
>+                              IntegerPairInfo *aIntegerPairInfo,
>+                              PRUint32 aIntegerPairCount) :
>+      mIntegerPairs(aIntegerPairs), mIntegerPairInfo(aIntegerPairInfo), mIntegerPairCount(aIntegerPairCount)

Nit: Wrap mIntegerPairCount to the next line, so that this line's not so long.

>@@ -4204,39 +4209,38 @@ nsSVGFEConvolveMatrixElement::Filter(nsS
>   PRUint16 edgeMode = mEnumAttributes[EDGEMODE].GetAnimValue();
>   PRBool preserveAlpha = mBooleanAttributes[PRESERVEALPHA].GetAnimValue();
> 
>   float bias = 0;
>-  if (HasAttr(kNameSpaceID_None, nsGkAtoms::bias)) {
>+  if (mNumberAttributes[BIAS].IsExplicitlySet()) {
>     bias = mNumberAttributes[BIAS].GetAnimValue();
>   }

Why do we care about IsExplicitlySet() here?  I think bias defaults to 0 if it's not explicitly set, so I think we drop that check and just say:
  float bias = mNumberAttributes[BIAS].GetAnimValue();

>+++ b/content/svg/content/src/nsSVGInteger.cpp
>+nsresult
>+nsSVGInteger::SetBaseValueString(const nsAString &aValueAsString,
[...]
>+  mIsBaseSet = PR_TRUE;
>+  mBaseVal = mAnimVal = value;

The mAnimVal assignment here needs to be inside an "if (!mIsAnimated)" clause, right?

>+++ b/content/svg/content/src/nsSVGIntegerPair.cpp
>+ParseIntegerOptionalInteger(const nsAString& aValue,
>+                            PRInt32 aValues[2])
>+{
>+  NS_ConvertUTF16toUTF8 value(aValue);
>+  const char *str = value.get();
>+
>+  if (NS_IsAsciiWhitespace(*str))
>+    return NS_ERROR_FAILURE;
[...]
>+    while (NS_IsAsciiWhitespace(*rest)) {
>+      ++rest;
>+    }

I think this wants to use IsSVGWhitespace(), not NS_IsAsciiWhitespace.

(This also applies to ParseNumberOptionalNumber.)

Also -- we probably should be using nsCharSeparatedTokenizerTemplate<IsSVGWhitespace> for tokenization in both of these parsing functions, rather than having separate implementations of a whitespace-and/or-comma-delimited-list parser in each function.  We don't necessarily need to make that change now, but it'd be great if you could file a followup for it, at least. (assuming you agree)

BTW, I think the current custom tokenization in these methods actually has a minor bug -- right now, it doesn't skip over whitespace between the comma and the number, when really it should.  In most cases, that won't matter, since strtol / PR_strtod start out by skipping over whitespace (I think), but I suspect they might define "whitespace" differently from how SVG does...

>+  if (str == rest) {
>+    //first value was illformed
[...]
>+      //second value was illformed or there was trailing content

Nit: Add a space after "//" in both these comments. (within ParseNumberOptionalNumber as well as ParseNumberOptionalNumber)

>+void
>+nsSVGIntegerPair::GetBaseValueString(nsAString & aValueAsString)

Nit: "&" floating between spaces -- bump it to the left or to the right. :)

>+nsSVGIntegerPair::SetBaseValue(PRInt32 aValue, PairIndex aIndex,
>+                               nsSVGElement *aSVGElement,
>+                               PRBool aDoSetAttr)
>+{
>+  mBaseVal[aIndex == eFirst ? 0 : 1] = aValue;
>+  mIsBaseSet = PR_TRUE;
>+  if (!mIsAnimated) {
>+    mAnimVal[aIndex == eFirst ? 0 : 1] = aValue;
>+  }

Since we need to use the array index twice here, perhaps it'd be better to stick it in a local variable, rather than re-figuring-it-out the second time?

e.g.:
    PRUint32 i = (aIndex == eFirst ? 0 : 1);
    mBaseVal[i] = ...
    [...]
      mAnimVal[i] = ...

That seems cleaner & less fragile to me, but I'm fine with the current code too.

(nsSVGNumberPair::SetBaseValue has the same pattern)

>+nsresult
>+nsSVGIntegerPair::ToDOMAnimatedInteger(nsIDOMSVGAnimatedInteger **aResult,
>+                                       PairIndex aIndex,
>+                                       nsSVGElement *aSVGElement)
>+{
>+  *aResult = new DOMAnimatedIntegerPair(this, aIndex, aSVGElement);
>+  if (!*aResult)
>+    return NS_ERROR_OUT_OF_MEMORY;

infallible new --> no need for null-check, hooray! :)

This applies to nsSVGNumberPair::ToDOMAnimatedNumber, too.

>+++ b/content/svg/content/src/nsSVGIntegerPair.h

>+public:
>+  enum PairIndex {
>+        eFirst,
>+        eSecond};

Nit: add a newline before the closing "}".

This also applies to nsSVGNumberPair.h's PairIndex decl.

>+    NS_IMETHOD SetBaseVal(PRInt32 aValue)
>+      {
>+        NS_ENSURE_FINITE(aValue, NS_ERROR_ILLEGAL_VALUE);

We don't need that NS_ENSURE_FINITE here -- aValue is an integer, so it's finite. :)

r=dholbert with the above.

Comment 18

[Robert Longson [:longsonr]]

Attachment: hg changeset patch with review comments addressed

Comment 19

[Robert Longson [:longsonr]]

I'll file a followup for the parsing tokeniser change.

Comment 20

[Daniel Holbert [:dholbert]]

Cool -- so this is ready to land?  If so, I can land it for you tomorrow.

(If you don't mind pushing it to tryserver in the meantime, I'd feel safer about landing it. :) )

Comment 21

[Daniel Holbert [:dholbert]]

(I know you tryserver'd it already; just want to be sure the review changes didn't break anything)

Comment 22

[Robert Longson [:longsonr]]

http://tbpl.mozilla.org/?tree=Try&rev=b093bbd4d0a5 

Feel free to land if Try is happy.

Comment 23

[Robert Longson [:longsonr]]

Try server failures seem to be unrelated to this patch (fixed on trunk by 18f22e82fffa)

Comment 24

[Daniel Holbert [:dholbert]]

Comment on attachment 543263 [details] [diff] [review]
hg changeset patch with review comments addressed

>+++ b/content/svg/content/src/nsSVGNumberPair.cpp
>+static nsresult
>+ParseNumberOptionalNumber(const nsAString& aValue,
>+                          float aValues[2])
>+{
[...]
>+    if (*rest != '\0' || !NS_FloatIsFinite(y)) {
>+      //s econd value was illformed or there was trailing content

Fixed "//s econd" --> "// second" & landed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/6ef387126d0f

Comment 25

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/6ef387126d0f

Comment 26

[Vlad [QA]]

Hi. 
I have run the test from the URL and it's working fine on:
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0

It's enough to verify this? 
I've tried the same test on a older version of Firefox: Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101208 Firefox/4.0b8pre and it also works fine.

Is there another test?

thanks.

Comment 27

[Robert Longson [:longsonr]]

In Firefox 4 the URL should animate incorrectly. It looks like the surface is compressing/expanding in the x direction only.

In Firefox 7 it should animate in the x and y simultaneously so that it looks like the whole surface is getting bigger/smaller.

Comment 28

[Vlad [QA]]

Setting resolution to Verified Fixed on Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0 beta 2