Last Comment Bug 617623 - Add support for SMIL animation of <number-optional-number> attributes
: Add support for SMIL animation of <number-optional-number> attributes
Status: VERIFIED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Robert Longson
:
Mentors:
http://svg.kvalitne.cz/adobe/3_anim_f...
Depends on: 669025 669155 692506
Blocks: 436276 608161
  Show dependency treegraph
 
Reported: 2010-12-08 10:06 PST by Jonathan Watt [:jwatt]
Modified: 2011-10-06 12:07 PDT (History)
8 users (show)
longsonr: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Crashing case (1.94 KB, image/svg+xml)
2011-01-08 02:07 PST, Marek Raida
no flags Details
Not crashing case (with animation element removed) (1.94 KB, image/svg+xml)
2011-01-08 02:08 PST, Marek Raida
no flags Details
Not crashing case with removed feImage and animate element still there (1.94 KB, image/svg+xml)
2011-01-08 02:20 PST, Marek Raida
no flags Details
patch (142.81 KB, patch)
2011-06-23 00:45 PDT, Robert Longson
no flags Details | Diff | Review
add missing files (155.40 KB, patch)
2011-06-23 10:46 PDT, Robert Longson
dholbert: review+
Details | Diff | Review
hg changeset patch with review comments addressed (122.88 KB, patch)
2011-06-30 14:33 PDT, Robert Longson
no flags Details | Diff | Review

Description Jonathan Watt [:jwatt] 2010-12-08 10:06:01 PST
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) (Exited; email my personal email if necessary) 2010-12-08 13:28:37 PST
OK, let's do this.
Comment 2 Marek Raida 2011-01-07 09:03:02 PST
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] 2011-01-07 09:33:33 PST
(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 2011-01-08 02:07:31 PST
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 2011-01-08 02:07:59 PST
Created attachment 502209 [details]
Crashing case
Comment 6 Marek Raida 2011-01-08 02:08:32 PST
Created attachment 502210 [details]
Not crashing case (with animation element removed)
Comment 7 Marek Raida 2011-01-08 02:20:47 PST
Created attachment 502211 [details]
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 2011-01-08 02:26:22 PST
Marek, your testcase is related to bug 613899 and not this bug.
Comment 9 Marek Raida 2011-01-08 04:14:33 PST
You're right, sorry, I copied my attachments and comments there...
Comment 10 Robert Longson 2011-06-20 13:21:32 PDT
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] 2011-06-20 14:45:59 PDT
Sounds great, thanks Robert. :)
Comment 12 Robert Longson 2011-06-23 00:45:35 PDT
Created attachment 541303 [details] [diff] [review]
patch
Comment 13 Robert Longson 2011-06-23 01:03:46 PDT
Sent to Try: http://hg.mozilla.org/try/rev/38fff45aba13
Comment 15 Robert Longson 2011-06-23 10:46:36 PDT
Created attachment 541425 [details] [diff] [review]
add missing files
Comment 16 Robert Longson 2011-06-23 13:20:15 PDT
Better once the missing files were included: http://tbpl.mozilla.org/?tree=Try&rev=920fb044cb37
Comment 17 Daniel Holbert [:dholbert] 2011-06-29 16:14:32 PDT
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 2011-06-30 14:33:21 PDT
Created attachment 543263 [details] [diff] [review]
hg changeset patch with review comments addressed
Comment 19 Robert Longson 2011-06-30 14:34:05 PDT
I'll file a followup for the parsing tokeniser change.
Comment 20 Daniel Holbert [:dholbert] 2011-06-30 18:34:45 PDT
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] 2011-06-30 18:35:28 PDT
(I know you tryserver'd it already; just want to be sure the review changes didn't break anything)
Comment 22 Robert Longson 2011-07-01 01:27:22 PDT
http://tbpl.mozilla.org/?tree=Try&rev=b093bbd4d0a5 

Feel free to land if Try is happy.
Comment 23 Robert Longson 2011-07-01 05:14:25 PDT
Try server failures seem to be unrelated to this patch (fixed on trunk by 18f22e82fffa)
Comment 24 Daniel Holbert [:dholbert] 2011-07-01 10:53:26 PDT
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] 2011-07-02 03:03:05 PDT
http://hg.mozilla.org/mozilla-central/rev/6ef387126d0f
Comment 26 Vlad [QA] 2011-08-24 07:23:21 PDT
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 2011-08-24 07:26:02 PDT
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] 2011-08-28 23:46:43 PDT
Setting resolution to Verified Fixed on Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0 beta 2

Note You need to log in before you can comment on or make changes to this bug.