520710 - SVG SMIL: <set> uses calcMode="discrete"

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

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

Attachment: Test case to reproduce problem

I think our implementation of <set> needs review. With the attached test case it seems like we're applying calcMode="discrete" for <set> when we should just set the value. So at t=1s, the fill should be set at to green. Instead it seems like we're using two values: red (the base value, i.e. implied 'from' value), green and then applying discrete animation. Hence, it doesn't go green until t=2s. I added the 'x' animation to show when t=1s.

I could be wrong about this, but I don't have time to look into it at the moment. I had a quick look at nsSVGSetElement.cpp and I can't quite see what's going on. I suspect there are other issues there. e.g. Is it possible to set calcMode=linear on a <set> element and have it interpolate? Like I said, I haven't had time to look into it but I'm pretty sure the attached test case is wrong.

Comment 1

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

Sorry, I had a bit more of a look into it. Seems like we'll probably handle attempts to set the calcMode. But we are forcing calcMode="discrete" and I'm not sure that that's correct.

It's in content/smil/nsSMILSetAnimationFunction.cpp

Comment 2

[Daniel Holbert [:dholbert]]

You're right -- currently, <set to="val"> behaves just like <animate to="val" calcMode="discrete"> -- it sets the base value for the first half of the duration, when it shouldn't.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix: override "InterpolateResult" to just return the 'to' value

This fixes the problem by overriding InterpolateResult for <set> elements, such that it just always returns the 'to' value.

When we arrive in InterpolateResult, we've already verified that our animation is enabled and the current time is in its duration.  Given that those things are true, <set> should always just apply its "to" value.

I've included some basic sanity-checking at the top of nsSMILSetAnimationFunction::InterpolateResult (and cleaned up the corresponding sanity-checking in the nsSMILAnimationFunction::InterpolateResult).  But aside from that sanity-checking, all we really need to do is 
  { aResult = aValues[0]; // apply 'to' value }

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: same fix, with tests

Same patch, plus some simple reftests for <set>.  Carrying forward sr+

Comment 5

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

Comment on attachment 407861 [details] [diff] [review]
same fix, with tests

Carrying forward r+

Comment 6

[Daniel Holbert [:dholbert]]

http://hg.mozilla.org/mozilla-central/rev/1a60b95e6537

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

I don't how how or why, but this broke my debug build on OSX
Undefined symbols:
  "nsSMILAnimationFunction::IsToAnimation() const", referenced from:
      nsSMILSetAnimationFunction::InterpolateResult(nsTArray<nsSMILValue> const&, nsSMILValue&, nsSMILValue&)in nsSMILSetAnimationFunction.o
ld: symbol(s) not found

Comment 8

[Daniel Holbert [:dholbert]]

Weird! I'll see if I can reproduce on my mac machine at the office... given that this worked fine on the tinderboxen, though, I wonder if this is a quirk in the particular version of your compiler?

[Talked to smaug on IRC -- he's away from this OS X box until Sunday. If we don't see this failure anywhere else, maybe we can find out more from smaug's box after Sunday]

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to comment #8)
> Weird! I'll see if I can reproduce on my mac machine at the office

FWIW, I can't reproduce smaug's linker issue on my mac, with a fresh mozilla-central checkout. (running OS X 10.5.7, with gcc / g++ version 4.0.1 build 5465)

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

I think I have the very latest XCode on my 10.5 Mac.
(whatever the gcc version is)

Anyway, I'll retest on Sunday.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

I did a clobber build and the result is still
Undefined symbols:
  "nsSMILAnimationFunction::IsToAnimation() const", referenced from:
      nsSMILSetAnimationFunction::InterpolateResult(nsTArray<nsSMILValue> const&, nsSMILValue&, nsSMILValue&)in nsSMILSetAnimationFunction.o
ld: symbol(s) not found
collect2: ld returned 1 exit status

gcc version 4.0.1 (Apple Inc. build 5493)

Comment 12

[Robert Longson [:longsonr]]

If you remove the inline keyword from

inline PRBool
nsSMILAnimationFunction::IsToAnimation() const

in content/smil/nsSMILAnimationFunction.cpp

does it link then? inline seems a strange thing to have in a cpp file. Maybe the two inline methods should move to the header file.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Removing 'inline' from the methods helps.

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: followup: move inline methods IsToAnimation / IsAdditive to header

Followup patch to fix smaug's build issue.  This just moves two inline methods' definitions from the CPP file to the header file, as Robert Longson suggested above.

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 408282 [details] [diff] [review]
followup: move inline methods IsToAnimation / IsAdditive to header

/me redirects r= to smaug, since he's on IRC and can review this as well.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 408282 [details] [diff] [review]
followup: move inline methods IsToAnimation / IsAdditive to header

This seems to work here.

Nit, && should not start a line.

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: followup v2 (with nit fixed)

Thanks smaug! Here's the patch with that nit fixed -- carrying forward r+.

Comment 18

[Daniel Holbert [:dholbert]]

followup v2 pushed:
http://hg.mozilla.org/mozilla-central/rev/8e2bef3d55e8