570453 - The SMIL engine should not get default/zero values by calling <type>::Init(nsSMILValues)

Status: NEW

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILAnimationFunction.cpp#787

nsSMILAnimationFunction::GetValues contains the following code to handle 'by' animation:

      nsSMILValue effectiveFrom(by.mType);
      if (!from.IsNull())
        effectiveFrom = from;
      // Set values to 'from; from + by'
      result.AppendElement(effectiveFrom);
      nsSMILValue effectiveTo(effectiveFrom);
      if (!effectiveTo.IsNull() && NS_SUCCEEDED(effectiveTo.Add(by))) {
        result.AppendElement(effectiveTo);
      } else {
        // Using by-animation with non-additive type or bad base-value
        return NS_ERROR_FAILURE;
      }

The problem is that this depends on being able to Add() to the default vale for the nsSMILValue type in question. This is fine if nsSMILValue is a float type. The default is then zero I guess, and zero plus the 'by' value is the 'by' value. Fine. But in the case of length lists, I'm specifically disallowing Add of lists of different lengths. Since 'effectiveFrom' in that code is an empty list when we're dealing with length lists, 'by' animation like the following won't work:

  <text x="10 20 30">ABC
    <animate attributeName="x"
             calcMode="linear"
             begin="0s" dur="2s"
             by="10 10 10"
             fill="freeze"/>
  </text>

Why are we adding 'by' to "zero" anyway, rather than just doing a straight up append of the 'by' value to 'result'?

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to comment #0)
> Why are we adding 'by' to "zero" anyway, rather than just doing a straight up
> append of the 'by' value to 'result'?

Basically it's because so far we're not special-casing by-animation. We're using the same code to handle from-by animation and by-animation. But that's not to say it has to be that way.

For the case you mentioned I think we want GetValues to return two lists: (0 0 0), (10 10 10). The code in this class relies pretty heavily on that behaviour.

Some possibilities for achieving that might be:
i) Special handling here in GetValues to initialise the default value based on the by-value
ii) Special handling in the list code so that the first time a default (zero-element) list is added to something it is resized to match the dimensions of whatever it's being added to and filled with zeros.

I prefer (ii) if it's possible.

Applying that behaviour in the case where you're doing by-animation with the wrong number of elements as in:

  <text x="10 20 30">ABC
    <animate attributeName="x"
             calcMode="linear"
             begin="0s" dur="2s"
             by="10 10"
             fill="freeze"/>
  </text>

GetValues would end up returning (0 0), (10 10) but then the call to SandwichAdd (http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILAnimationFunction.cpp#289) would fail so we'd still end up disallowing adding lists of different lengths.

Also, I don't think it's a problem to rely on Add in this case since by definition by-animation is additive. It's just a matter of either initialising the from value appropriately or making Add more lenient for this particular case. Does that sound possible?

That said, dholbert may know of other situations where this behaviour is problematic with CSS.

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Actually, ignore that comment. I just realised it wasn't really addressing what you were asking except the first sentence.

Yep, we could just special-case by-animation and return <default>, (10 10 10) but we'll still need to ensure that Interpolate expands <default> to (0 0 0). Alternatively we get GetValues to init the 'from' value some how to (0 0 0).

I thought you were suggesting we just return one value from GetValues so I misunderstood. Sorry!

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to comment #0)
> Why are we adding 'by' to "zero" anyway, rather than just doing a straight up
> append of the 'by' value to 'result'?

I believe that was purely to simplify the logic -- to only have one variable that gets appended for the 'from' value, and one variable that gets appended for 'to' value.  (In the from-by case, we need the addition in order to generate the 'to' value, whereas in the pure-by case, the addition should be addition with 0 and should have no effect.)

A straight-up append would be fine too, though, in the pure-by case (i.e. when from.IsNull() is true).  That's probably better, because it would skip an unnecessary (and problematic in your case) Add() operation.

(As birtles says in comment 2, though, we'll still need to make sure that SVGLengthListSMILType::Interpolate correctly handles interpolation between <default> and (10,10,10) in order for this to work.  Looking at patch v5 on bug 515116, I think that will fail right now, due to different list-lengths.)

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Just looking at nsSMILAnimationFunction::InterpolateResult (http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILAnimationFunction.cpp#369) I notice that for some calc modes we end up using the first value in the values array without calling Interpolate (e.g. discrete animation). Therefore I think we'll also need to get SVGLengthListSMILType::SandwichAdd to be able to add <default> to (10 20 30).

Comment 5

[Daniel Holbert [:dholbert]]

See also bug 515116 comment 28 for the results of an IRC discussion between me & jwatt yesterday on handling length-lists, for both the <text> x & y attrs (which have default-value="0" and which are tricky since their list-values are absolute-glyph-positions) and for the dx & dy attrs (which have default-value="" and which are easier since its values are just offsets from the normal positions).