If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED

Status

()

Core
SVG
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: birtles, Assigned: dholbert)

Tracking

Trunk
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

8 years ago
Created attachment 404760 [details]
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.
(Reporter)

Comment 1

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

Comment 2

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

Comment 3

8 years ago
Created attachment 407847 [details] [diff] [review]
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 }
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #407847 - Flags: superreview?(roc)
Attachment #407847 - Flags: review?(birtles)
Attachment #407847 - Flags: superreview?(roc) → superreview+
(Reporter)

Updated

8 years ago
Attachment #407847 - Flags: review?(birtles) → review+
(Assignee)

Comment 4

8 years ago
Created attachment 407861 [details] [diff] [review]
same fix, with tests

Same patch, plus some simple reftests for <set>.  Carrying forward sr+
Attachment #407847 - Attachment is obsolete: true
Attachment #407861 - Flags: superreview+
Attachment #407861 - Flags: review?(birtles)
(Reporter)

Comment 5

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

Carrying forward r+
Attachment #407861 - Flags: review?(birtles) → review+
(Assignee)

Comment 6

8 years ago
http://hg.mozilla.org/mozilla-central/rev/1a60b95e6537
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED

Comment 7

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

Comment 8

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

Comment 9

8 years ago
(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)
I think I have the very latest XCode on my 10.5 Mac.
(whatever the gcc version is)

Anyway, I'll retest on Sunday.
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

8 years ago
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.
Removing 'inline' from the methods helps.
(Assignee)

Comment 14

8 years ago
Created attachment 408282 [details] [diff] [review]
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.
Attachment #408282 - Flags: review?(roc)
(Assignee)

Comment 15

8 years ago
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.
Attachment #408282 - Flags: review?(roc) → review?(Olli.Pettay)
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.
Attachment #408282 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 17

8 years ago
Created attachment 408285 [details] [diff] [review]
followup v2 (with nit fixed)

Thanks smaug! Here's the patch with that nit fixed -- carrying forward r+.
Attachment #408282 - Attachment is obsolete: true
Attachment #408285 - Flags: review+
(Assignee)

Comment 18

8 years ago
followup v2 pushed:
http://hg.mozilla.org/mozilla-central/rev/8e2bef3d55e8
You need to log in before you can comment on or make changes to this bug.