Closed Bug 617623 Opened 9 years ago Closed 8 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla7

People

(Reporter: jwatt, Assigned: longsonr)

References

()

Details

(Whiteboard: [inbound])

Attachments

(1 file, 5 obsolete files)

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'.
OK, let's do this.
Assignee: nobody → jwatt
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?).
(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.
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...
Attached image Crashing case (obsolete) —
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....
Marek, your testcase is related to bug 613899 and not this bug.
You're right, sorry, I copied my attachments and comments there...
Attachment #502209 - Attachment is obsolete: true
Attachment #502210 - Attachment is obsolete: true
Attachment #502211 - Attachment is obsolete: true
Version: unspecified → Trunk
Assignee: jwatt → 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.
Sounds great, thanks Robert. :)
Attached patch patch (obsolete) — Splinter Review
Attached patch add missing files (obsolete) — Splinter Review
Attachment #541303 - Attachment is obsolete: true
Attachment #541425 - Flags: review?(jwatt)
Better once the missing files were included: http://tbpl.mozilla.org/?tree=Try&rev=920fb044cb37
Attachment #541425 - Flags: review?(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.
Attachment #541425 - Flags: review?(dholbert) → review+
Attachment #541425 - Attachment is obsolete: true
Attachment #541425 - Flags: review?(jwatt)
I'll file a followup for the parsing tokeniser change.
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. :) )
(I know you tryserver'd it already; just want to be sure the review changes didn't break anything)
http://tbpl.mozilla.org/?tree=Try&rev=b093bbd4d0a5 

Feel free to land if Try is happy.
Try server failures seem to be unrelated to this patch (fixed on trunk by 18f22e82fffa)
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
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/6ef387126d0f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Blocks: 608161
Depends on: 669025
Flags: in-testsuite+
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.
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.
Setting resolution to Verified Fixed on Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0 beta 2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.