The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in mozilla7

Status

()

Core
SVG
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: jwatt, Assigned: Robert Longson)

Tracking

Trunk
mozilla7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound], URL)

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

6 years ago
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

Comment 2

6 years ago
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.

Comment 4

6 years ago
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

6 years ago
Created attachment 502209 [details]
Crashing case

Comment 6

6 years ago
Created attachment 502210 [details]
Not crashing case (with animation element removed)

Comment 7

6 years ago
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....
(Assignee)

Comment 8

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

Comment 9

6 years ago
You're right, sorry, I copied my attachments and comments there...
(Assignee)

Updated

6 years ago
Attachment #502209 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #502210 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #502211 - Attachment is obsolete: true
Version: unspecified → Trunk
(Assignee)

Updated

6 years ago
(Assignee)

Updated

6 years ago
Assignee: jwatt → longsonr
(Assignee)

Comment 10

6 years ago
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.
(Reporter)

Comment 11

6 years ago
Sounds great, thanks Robert. :)
(Assignee)

Comment 12

6 years ago
Created attachment 541303 [details] [diff] [review]
patch
(Assignee)

Comment 13

6 years ago
Sent to Try: http://hg.mozilla.org/try/rev/38fff45aba13
(Assignee)

Comment 14

6 years ago
http://hg.mozilla.org/try/pushloghtml?changeset=38fff45aba13
(Assignee)

Comment 15

6 years ago
Created attachment 541425 [details] [diff] [review]
add missing files
Attachment #541303 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #541425 - Flags: review?(jwatt)
(Assignee)

Comment 16

6 years ago
Better once the missing files were included: http://tbpl.mozilla.org/?tree=Try&rev=920fb044cb37
(Assignee)

Updated

6 years ago
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+
(Assignee)

Comment 18

6 years ago
Created attachment 543263 [details] [diff] [review]
hg changeset patch with review comments addressed
Attachment #541425 - Attachment is obsolete: true
Attachment #541425 - Flags: review?(jwatt)
(Assignee)

Comment 19

6 years ago
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)
(Assignee)

Comment 22

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

Feel free to land if Try is happy.
(Assignee)

Comment 23

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
(Assignee)

Updated

6 years ago
Blocks: 608161

Updated

6 years ago
Depends on: 669025
(Assignee)

Updated

6 years ago
Flags: in-testsuite+
Depends on: 669155

Comment 26

6 years ago
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.
(Assignee)

Comment 27

6 years ago
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

6 years ago
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
Depends on: 692506
You need to log in before you can comment on or make changes to this bug.