436418 - (animateMotion) SVG SMIL: Implement "animateMotion"

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

The SMIL patch in its current state doesn't support 'animateMotion'.  The attached testcase is taken from the specified URL, at kevlindev.com.

Expected behavior: red square should move along the black semicircle path
Current behavior:  red square is motionless in upper-left corner

Testing using mozilla-central, with patch-queue from this HG repo:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/

(See Bug 216462, Comment #58 for more details.)

Comment 1

[Daniel Holbert [:dholbert]]

Assigning this bug to Eric Hedekar, per discussion on the mdt.svg newsgroup here:
http://groups.google.com/group/mozilla.dev.tech.svg/browse_thread/thread/ddaf1fc5863294fc

Thanks for taking this on, Eric!

Comment 2

[Eric Hedekar]

I've made some initial progress in understanding the parsing and creating an nsSVGAnimateMotionElement.cpp but I'm not sure exactly how to tackle the parsing of XY co-ordinate pairs for from/to attributes.  Any suggestions?

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to comment #2)
> I'm not sure exactly how to tackle the
> parsing of XY co-ordinate pairs for from/to attributes.  Any suggestions?

Well, <animateMotion> doesn't use from/to attributes at all, AFAIK -- it uses an arbitrary path. Maybe you were suggesting _emulating_ from/to attributes for an easier fit into the existing SMIL code, but I'm not sure that would end up being workable, because the motion-path could be arbitrarily complex.

I'd suggest storing an internal representation of the full path, sharing code (particularly parsing & data-structures) as much as possible with the SVG <path> element.

Then, I'd add a custom subclass of nsSMILAnimationFunction that is animate-motion-specific. In contrast to the current nsSMILAnimationFunction class, which stores from/to values and responds to "ComposeResult" by interpolating to a particular position between those values, you'd want your custom subclass to store the *path* and interpolate to a position along that *path*.

Now, as to where this subclass could fit in to the SMIL pipeline -- I'm not 100% sure what makes the most sense, but here are two ideas:
  (a) we could probably just put it in the array of compositors for both the "x"  attribute _and_ in the array for the "y" attribute.
  (b) we could create a special "motion" pseudo-attribute with its own array of compositors, for all <animateMotion>s targeting a particular element.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to comment #3)
> (In reply to comment #2)
> > I'm not sure exactly how to tackle the
> > parsing of XY co-ordinate pairs for from/to attributes.  Any suggestions?
> 
> Well, <animateMotion> doesn't use from/to attributes at all, AFAIK -- it uses
> an arbitrary path. 

Ah, sorry, I misremembered -- animateMotion *can* use from/to/by/values, but those are basically are just shorthand for very simple paths.  So, I still think it's best to use a path structure for the internal representation.

Comment 5

[Daniel Holbert [:dholbert]]

RE where to do the parsing -- "nsSMILParserUtils" (linked below) is currently the catch-all SMIL parsing class right now.  You'll probably find that the parsing code you need for animateMotion's "from/to/by/values" attributes is already there (at the least, you'll find some helper functions & boiler-plate code that you can use as a reference for your parsing needs).

http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILParserUtils.h
http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILParserUtils.cpp

(For the "path" attribute, though, you should try to share parsing code with the SVG <path> element, as noted in comment 3.)

Comment 6

[Eric Hedekar]

Thanks Daniel, that's kind of what we were planning for the path attributes (the sharing with SVG <path> code), but our initial plans were to try implementing the simpler from/to/by code first to get a better feel for the codebase.

Comment 7

[Eric Hedekar]

Attachment: a beginning patch

This initial patch requires some modification of the original svg file - in other words it is a woefully incomplete implementation.  It requires an attributeName attribute to direct what item is being animated (this should not be there according to the spec).  It also only looks at path elements that have a move to, followed by a single line to.  Furthermore, it only utilizes the X data from that path (even if the attributeName is animating the y attribute).  So to recap this is a woefully incomplete patch.

Here's a quick example of a modified SVG to see what this patch does:
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
    <rect y="100" width="10" height="10" fill="red">
        <animateMotion attributeName="x" path="M300 100 L600 500" dur="1s" repeatCount="indefinite" />
    </rect>
</svg>

Comment 8

[Robert Longson [:longsonr]]

Comment on attachment 371952 [details] [diff] [review]
a beginning patch

Just a couple of things in passing that don't really address any of your issues :-(

>diff -r 6dbc129ccd19 content/svg/content/src/Makefile.in
>--- a/content/svg/content/src/Makefile.in	Tue Jan 27 19:56:55 2009 -0800
>+++ b/content/svg/content/src/Makefile.in	Thu Apr 09 15:16:46 2009 -0700
>@@ -141,6 +141,7 @@
> ifdef MOZ_SMIL
> CPPSRCS += nsSVGAnimateElement.cpp \
>            nsSVGAnimateTransformElement.cpp \
>+	   nsSVGAnimateMotionElement.cpp \

I think you have a tab where you should have spaces.

>+ * Portions created by the Initial Developer are Copyright (C) 2005

2009

>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Brian Birtles <[e-mail]>
>+ *   Chris Double  <[e-mail]>
>+ *   Daniel Holbert <[e-mail]>

This should be you.

Comment 9

[Eric Hedekar]

I'll gladly fix those spacing issues.

As for the copyright, the new files in question are derrivatives of
http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGAnimateTransformElement.cpp
and
http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/svg/nsIDOMSVGAnimateTransformElement.idl
so I could see adding our (Dickson Wong and my) names to the Contributor(s) sections, but should the copyright date change? (I'm new to the ettiquite of MPL documents)

I also notice that some of the folder hierarchy has changed since I last pulled the source so that patch needs some adjustment to meet the latest folder layout.  (dom/public/idl/svg/nsIDOMSVGAnimateMotionElement.idl should be dom/interfaces/svg/nsIDOMSVGAnimateMotionElement.idl)

Comment 10

[Robert Longson [:longsonr]]

One content element always looks pretty much like another I suppose. The existing contributors have their names on the original documents so I'm sure they don't need to be on any more they haven't originated.

This line is pretty important to get right.

+ * The Initial Developer of the Original Code is Brian Birtles.

And should be yourselves or your organisation depending on who is the copyright holder.

The contributors section is less important and we've often left that blank.

Comment 12

[Daniel Holbert [:dholbert]]

Eric: Are you still actively working on this bug?  If not, I'd like to steal it from you sometime soon, and pick up where you left off. :)

Comment 13

[Eric Hedekar]

(In reply to comment #12)
> Eric: Are you still actively working on this bug?  If not, I'd like to steal it
> from you sometime soon, and pick up where you left off. :)

I'm not actively working on it, so feel free to 'steal it'.  I am however very open to working on it again, so if you need a hand or can't get to it I am willing to.  Ubuntu Studio is just currently taking up most of my FOSS time these days.

Comment 14

[Daniel Holbert [:dholbert]]

Ok -- stealing. Thanks!

Comment 15

[Daniel Holbert [:dholbert]]

Just as a status update here -- I've got a WIP stack of patches to implement this in my patch queue repo at:
   http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/
If anyone wants to test, just hg-clone that repo as mozilla-central/.hg/patches, then run "hg qselect animateMotion", and finally "hg qpush" up to smil_animateMotion_main .

Right now, that patch-stack supports <animateMotion> with the "from/by/to/values" attributes, and I'm currently working on supporting the path / mpath motion-specifications (in a separate patch). I'll post patches on this bug when I've got that, in the next few days.

Also: the WIP patch-stack doen't support the "keyPoints" attribute yet -- I want to getbug 557885 fixed before implementing that, since keyPoints will require specializations of code that bug 557885 needs to tweak.

(I'd been planning on posting my current WIP series of patches without path support, but I'm going to wait in the interests of reducing bug-attachment-spam, since it's 4 patches and I'll be rewriting some chunks of them as I add path support.)

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: Patch A: Laying the foundations (from Eric)

Posting my current patch-stack for this bug. Here's the first patch -- a pruned and un-bitrotted version of the beginning patch that Eric had posted (attachment 371952 [details] [diff] [review]).

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: Patch B: minor tweaks

and here's a patch with some minor tweaks to layer on top of Patch A:
 - Fix a few compilation issues (due to some DOM changes since Eric posted his patch)
 - Add Eric's name as a contributor where appropriate

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: Patch C: GenericValueParser

This patch abstracts out the logic of nsSMILParserUtils::ParseValues, allowing us to have customize the action we perform on each of the parsed-out entries in a SMIL animation's element's "values" attribute. (I take advantage of this in the main patch -- coming up -- to convert a "values" list into a string of SVG path commands.)

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: Patch D: Tests

And here's a patch with some reftests. (definitely need to add more of these)

Comment 20

[Daniel Holbert [:dholbert]]

Attachment: Patch E: main animateMotion patch

and here's the main patch for this.  This patch is essentially done, though it still has a few "XXXdholbert" comments that I need to address. (mostly code-removal and error-checking).

With all these patches applied, <animateMotion> should be fully supported -- including the use of "from/to/by/values/path" attributes and the "mpath" sub-element for specifying a path, and the "keyPoints" attribute for tweaking the progress across that path.

See e.g. these SVG 1.1 testsuite examples, which pass after the patches are applied:
http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-animate-elem-04-t.html
http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-animate-elem-05-t.html
http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-animate-elem-06-t.html
http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-animate-elem-07-t.html
http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-animate-elem-08-t.html

I'm kicking off a tryserver build with the patches applied, for anyone who's interested in testing.

Comment 21

[Daniel Holbert [:dholbert]]

TryServer build done:
https://build.mozilla.org/tryserver-builds/[e-mail]-try-7bb5c931b983/

Comment 22

[d]

(In reply to comment #21)
> TryServer build done:
> https://build.mozilla.org/tryserver-builds/[e-mail]-try-7bb5c931b983/

I can confirm that the listed testcases pass with this patch/build on Mac OS X.

Comment 23

[Robert Longson [:longsonr]]

> // XXXdholbert Are these all correct & necessary? (based on nsSVGStopElement.cpp)

Yes. They forward interface implementation to the base class.

Comment 24

[Jeff Schiller]

This is a nice start, Daniel!

I ran the SVG test suite animation tests that involved animateMotion.  I'm not an expert on this, just reporting results blindly:

1) The rotation on http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-animate-elem-08-t.html seems a little hurky-jerky

2) The right-bar does not move in the following tests:
http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-animate-elem-09-t.html

3) The browser hung for several seconds (thought it was going to crash) when I clicked on http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-animate-elem-11-t.html but then the test ran and passed

4) http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-animate-elem-24-t.html fails

5) http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-animate-elem-30-t.html fails

OSX Leopard.

Comment 25

[Daniel Holbert [:dholbert]]

Thanks for the notes, guys!
Jeff: To address the things you brought up -- (4) and (5) will be fixed in next version of patch E that I post -- I think I just need to reverse the order in which I apply |transform| matrix & |animateMotion| matrix.  Can't reproduce (3) -- might have been an unfortunately-timed garbage collection?  (2) should be fixed with the next version too -- known issue with calcMode="discrete".  I think (1) is probably a result of how we "flatten" the curve into a finite number of straight line-segments (a gfxFlattenedPath). Perhaps I can fix that by requesting more/tinier line-segments somehow? (That test's rotation is a bit jerky in Opera, too, FWIW, though not in exactly the same way.)

Comment 26

[Daniel Holbert [:dholbert]]

Attachment: Patch E v2: main animateMotion patch

Here's an updated version of Patch E. It's got a few XXXdholbert-comment fixes (including comment 23), and I've restructured/removed some chunks of the core interpolation/addition code to fix a few issues and remove unneeded code.

This version fixes the SVG-testsuite failures that Jeff brought up in comment 24. (in full-animate-elem-09-t.html, full-animate-elem-24-t.html, and full-animate-elem-30-t.html)

Comment 27

[Daniel Holbert [:dholbert]]

FWIW, I started a TryServer run with Patch E v2; builds will appear at:
https://build.mozilla.org/tryserver-builds/[e-mail]-try-3becd7896d43/

Comment 28

[Bill Gianopoulos [:WG9s]]

(In reply to comment #27)
> FWIW, I started a TryServer run with Patch E v2; builds will appear at:
> https://build.mozilla.org/tryserver-builds/[e-mail]-try-3becd7896d43/

Those tree tests now all pass for me under Linux, except that in full-animate-elem-30-t.html the horizontally moving blue object is not the correct color (obviously not an animate motion issue).  It is supposed to be the same blue as everything else.  Is there a bug someplace that covers this issue?

Comment 29

[Robert Longson [:longsonr]]

(In reply to comment #28)
> 
> Those tree tests now all pass for me under Linux, except that in
> full-animate-elem-30-t.html the horizontally moving blue object is not the
> correct color (obviously not an animate motion issue).  It is supposed to be
> the same blue as everything else.  Is there a bug someplace that covers this
> issue?

The rect has an animateColor animation - bug 436296

Comment 30

[Jeff Schiller]

Nice work Daniel!

Ship it! ;)

Comment 31

[Daniel Holbert [:dholbert]]

Attachment: Patch D v2: Tests

This updated tests patch adds two mochitests in content/smil/test -- one to set various path-specifiers on animateMotion & check against expected output CTM matrices, and one to just test various valid & invalid values and make sure the invalid values have no effect.

Comment 32

[Daniel Holbert [:dholbert]]

Attachment: Patch E v3: main animateMotion patch

...and here's an updated animateMotion patch, with some fixes to XXXdholbert comments & some fixes for minor issues that I ran across while writing the mochitests in the new tests patch.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 439202 [details] [diff] [review]
Patch C: GenericValueParser

+  nsTArray<nsSMILValue>& mValuesArray;
+  PRBool& mCanCache;

I'd prefer these to be real pointers so we know to watch the lifetime etc.

Comment 34

[Daniel Holbert [:dholbert]]

Attachment: Patch C1 v2: GenericValueParser

Per comment 33, I changed all the member vars in SMILValueParser from Foo& to Foo*. Attaching updated patch, w/ carried-forward r=roc.  Changes are here:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/9269021cc595

(Thanks for the quick turnaround on the reviews, roc!)

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> SVGMotionSMILType::CreateGFXMatrix

Just call it CreateMatrix

SVGMotionSMILAnimationFunction::GeneratePathString is a bit disturbing. Is it really necessary to generate a string and then parse it? Could we refactor some code and generate a gfxFlattenedPath instead? Maybe we could hang on to a gfxFlattenedPath and avoid creating an anonymous helper path element?

Do we have any tests for mpath? We'll want to test multiple mpath elements, mpaths in the wrong namespace, and elements with both 'path' attributes and mpath children. We should also be testing all the other override rules.

+    if (child->Tag() == nsGkAtoms::mpath) {
+      return static_cast<nsSVGMpathElement*>(child);
+    }

Need to check the namespace here too?

+ * @param [out] aResult The resulting parsed angle, in radian units, or
+ *                      FLT_MAX for "auto", or FLT_MIN for "auto-reverse.

That encoding is a bit too non-obvious for my taste. I think we should have a separate value to represent auto/auto-reverse/explicit.

If you inline ParseRotateVal into the caller, you could use atom comparisons instead of string comparisons which would be a little more efficient.

+  struct TranslParams {  // Simple translation

I think we can afford the characters to write TranslationParams :-)

+  struct PathPtParams {  // Point along a path

and PathPointParams :-)

If you want brevity, declare these at file scope so that you don't need to quality the names later.

+  PRBool foundDifference = PR_FALSE;
+  for (PRUint32 i = 0; i < length; ++i) {
+    if (leftArr[i] != rightArr[i]) {
+      foundDifference = PR_TRUE;
+      break;
+    }

Why not just return false when you find a difference and return true at the end of the loop?

+    aDistance = sqrt(dX*dX + dY*dY);

NS_hypot

Why are we only interpolating along a path? Can't we interpolate between translations as well?

Have we got tests for getting animateMotion and the 'transform' list transforms in the right order?

+  // XXXdholbert Perhaps this should be stored in element's property-table
+  // (like mapped attributes) so that it doesn't take up extra space on
+  // un-animated elements?

Probably, but it's OK here too.

I don't see anything to handle ContentChanged on the mpath's nsReferencedElement. I think you need to handle this, to handle cases where the referenced path changes (e.g. because of DOM changes in the referenced document). You'll need a test for this too.

You could probably split this patch up fairly cleanly by adding mpath support in its own patch, but if that's troublesome at this point, don't bother.

Comment 36

[Daniel Holbert [:dholbert]]

Attachment: Patch C2: Change GET_FLAG / SET_FLAG from macros to inline methods

This patch addresses an XXXdholbert comment from the main animateMotion patch -- it changes the SET_FLAG/GET_FLAG macros (defined in nsSMILAnimationFunction.cpp) into inline methods on nsSMILAnimationFunction, so that subclasses of nsSMILAnimationFunction can use them.

Comment 37

[Daniel Holbert [:dholbert]]

Attachment: Patch D v3: Tests

Comment 38

[Daniel Holbert [:dholbert]]

Attachment: Patch E v4: main animateMotion patch

Comment 39

[Daniel Holbert [:dholbert]]

Attachment: Patch H v1: mpath implementation

Comment 40

[Daniel Holbert [:dholbert]]

Thanks for the review -- responses below (with patch-queue changeset links, for the more self-contained single-changeset fixes).

(In reply to comment #35)
> Just call it CreateMatrix

Fixed:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/bda2883fb680

> SVGMotionSMILAnimationFunction::GeneratePathString is a bit disturbing. Is it
> really necessary to generate a string and then parse it? Could we refactor some
> code and generate a gfxFlattenedPath instead? Maybe we could hang on to a
> gfxFlattenedPath and avoid creating an anonymous helper path element?

Yes, good point -- given from/by/to/values, we should just go straight to a gfxFlattenedPath -- no need to use a <path> element.  Fixed.

> Do we have any tests for mpath? We'll want to test multiple mpath elements,
> mpaths in the wrong namespace, and elements with both 'path' attributes and
> mpath children. We should also be testing all the other override rules.

oops -- I had written some mpath tests, but I forgot to include them.  I've enhanced & included them now, in the tests patch.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/diff/1514539aa642/smil_animateMotion_tests

> +    if (child->Tag() == nsGkAtoms::mpath) {
> +      return static_cast<nsSVGMpathElement*>(child);
> +    }
> 
> Need to check the namespace here too?

Fixed:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/8d2706bcb382

> + * @param [out] aResult The resulting parsed angle, in radian units, or
> + *                      FLT_MAX for "auto", or FLT_MIN for "auto-reverse.
> 
> That encoding is a bit too non-obvious for my taste. I think we should have a
> separate value to represent auto/auto-reverse/explicit.

Fixed to use an enum "RotateType".

> If you inline ParseRotateVal into the caller, you could use atom comparisons
> instead of string comparisons which would be a little more efficient.

Actually, it was silly to be re-parsing "rotate" every sample, anyway -- I've made this more efficient by only parsing it at attribute-setting-time instead, and storing the result on the animationFunction for use at sample-time.

So, given that we'll only be doing the comparison once now, I think it's now more efficient to use string comparisons instead of atoms, to avoid whatever overhead is involved in converting an arbitrary string into an atom.  I'm happy to change to use atoms though, if you think I should.

> I think we can afford the characters to write TranslationParams :-)
[...]
> and PathPointParams :-)

Ok. :) I fixed the typenames (moved 'em to file scope, too, as you suggested):
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/2e994a832bd8
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/9247830f3095

I'm still using the abbreviations in variable names (MotionSegment:mU.mTranslParams & MotionSegment:mU.mPathPtParams), for brevity -- let me know if you'd like me to change those, too.

> +  PRBool foundDifference = PR_FALSE;
> Why not just return false when you find a difference and return true at the end
> of the loop?

Good point. :) Fixed:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/e173eaeeea9e

> +    aDistance = sqrt(dX*dX + dY*dY);
> 
> NS_hypot

Ah, handy! Fixed:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/f8d4edf44342

> Why are we only interpolating along a path? Can't we interpolate between
> translations as well?

No -- we'll never try to interpolate between Translation-flavored MotionSegments.

Note that with <animateMotion>, I convert from/to/by/values *all* to use a gfxFlattenedPath, so even though they're conceptually "translations", they're *handled* as paths. (this makes the keyPoints implementation much simpler)

The *only* place where we get a Translation-flavored MotionSegment is as the Add() outparam, when we're accumulating repeated animations. (Accumulation takes us off our path, and hence needs a generic Translation).  The result of that Add() operation doesn't get used as an input for interpolation -- rather, *Add* takes the interpolation output as one if *its* inputs.  See e.g. nsSMILAnimationFunction::AccumulateResult (which gets called right after InterpolateResult).

(Note that in OTHER nsISMILType subclasses, Add() *is* used to compute inputs to interpolation, when we're doing "from-by" animation.  But that's not true for animateMotion, because I override nsSMILAnimationFunction::GetValues -- which is where that Add() call would originate from -- and I generate a path instead of doing any addition for that case.)

> Have we got tests for getting animateMotion and the 'transform' list transforms
> in the right order?

Yes.  The reftest animateMotion-rotate-1.svg has a chunk where we have transform="rotate(90)" on an element, and we apply an animateMotion to that element (with an additional rotation).  (I had to change that test to make it pass again after reversing the matrix order in comment 25 -- that was one of the changes in v2 of my tests patch, though I didn't mention it in comment 31).

> +  // XXXdholbert Perhaps this should be stored in element's property-table
[...]
> Probably, but it's OK here too.

Ok.  I'll leave a one-line XXX comment there, since that's still a change we might want to make in the future.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/32da74dc30dc

> I don't see anything to handle ContentChanged on the mpath's
> nsReferencedElement. I think you need to handle this, to handle cases where the
> referenced path changes (e.g. because of DOM changes in the referenced
> document). You'll need a test for this too.

Ah, good catch! That did cross my mind, but I (incorrectly) dismissed it, because I was thinking "it'll just get updated in the next sample anyway".  But of course that could have problems if we tweaked the mpath-target in an animation where we're not sampling (or optimizing away recomposing when it looks like nothing's changed) due to frozenness / pauses / etc.  (And even without that, we do try to keep up-to-date with DOM changes between samples, and we should do that here too.)

That's fixed (along with a few other mpath issues I ran across) in the separated <mpath> patch.

> You could probably split this patch up fairly cleanly by adding mpath support
> in its own patch, but if that's troublesome at this point, don't bother.

Did that -- sorry, I should have done that up front to make the review easier.  (In my initial work, I actually did have it it split up that way, but then I had additional non-mpath tweaks & refactoring layered on top of that, and it was easier to fold everything together than to splice around the intermediate mpath patch.)

Comment 41

[Daniel Holbert [:dholbert]]

TryServer build using the updated patches:
http://build.mozilla.org/tryserver-builds/[e-mail]-try-a4785461f8d8

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> I'm still using the abbreviations in variable names
> (MotionSegment:mU.mTranslParams & MotionSegment:mU.mPathPtParams), for brevity
> -- let me know if you'd like me to change those, too.

Please do, but you don't have to repost a new patch just for that.

Comment 43

[Daniel Holbert [:dholbert]]

(In reply to comment #42)
> Please do, but you don't have to repost a new patch just for that.
Done in patch-queue:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/1ba2c5b44a04

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 441116 [details] [diff] [review]
Patch C2: Change GET_FLAG / SET_FLAG from macros to inline methods

Honestly, I'd rather just have five boolean getters and setters

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  } else if (HasAttr(nsGkAtoms::to) || HasAttr(nsGkAtoms::by)) {
+

Lose blank line

Can you explain why we need an anonymous path element for the 'path' attribute? Couldn't we bypass that and parse the path directly?

Comment 46

[Daniel Holbert [:dholbert]]

(In reply to comment #44)
> (From update of attachment 441116 [details] [diff] [review])
> Honestly, I'd rather just have five boolean getters and setters

Just to be clear -- I understand this to mean you'd like to switch to using a group of GetXXXErrorFlag() / SetXXXErrorFlag(PRBool aNewValue) methods -- is that correct?  And they'd still all be backed by the same underlying mErrorFlags variable? (so we can check "if (mErrorFlags)" in the core nsSMILAnimationFunction logic, and skip sampling if we've got any errors)

I think that makes sense, I just want to make sure I understand the suggested change before I modify the patch.

(In reply to comment #45)
> Lose blank line

Done in patch-queue.

> Can you explain why we need an anonymous path element for the 'path' attribute?
> Couldn't we bypass that and parse the path directly?

I'm using the anonymous path element to take advantage of its bundle of existing functionality -- namely:
 (a) Parsing a path description string into some [object].
 (b) Querying that [object] for a gfxFlattenedPath
 (c) Querying that [object] for the list of segment-endpoints defined by the path description string (via nsSVGPathElement::CreatePathSegList())

nsSVGPathElement works reasonably well as the [object] above, but alternately, we could indeed use the various objects that nsSVGPathElement uses internally.  In particular:
 - We can get (a) with a nsSVGPathDataParserToInternal & a nsSVGPathList
 - We can use the resulting nsSVGPathList to get (b)
 
 - However for (c), we'll need to re-parse, using a nsSVGPathDataParserToDOM to generate a nsCOMArray<nsIDOMSVGPathSeg>.  (my patch currently does this part via a call to via nsSVGPathElement::CreatePathSegList())

The third point above is needed because the nsSVGPathList object (for (a)/(b)) may get additional "sub-segments" -- e.g. some curves in the path description can get converted into a *series* of sub-curves in the nsSVGPathList.  So if we iterate through a nsSVGPathList to find our relevant points, we may get some incorrect intermediate points.  nsSVGPathDataParserToDOM doesn't have this problem.

I'll look into using these structures explicitly, rather than using a nsSVGPathElement to access them.

Comment 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #46)
> Just to be clear -- I understand this to mean you'd like to switch to using a
> group of GetXXXErrorFlag() / SetXXXErrorFlag(PRBool aNewValue) methods -- is
> that correct?

Yes

> And they'd still all be backed by the same underlying
> mErrorFlags variable? (so we can check "if (mErrorFlags)" in the core
> nsSMILAnimationFunction logic, and skip sampling if we've got any errors)

Yes

> > Can you explain why we need an anonymous path element for the 'path' attribute?
> > Couldn't we bypass that and parse the path directly?
> 
> I'm using the anonymous path element to take advantage of its bundle of
> existing functionality -- namely:
>  (a) Parsing a path description string into some [object].
>  (b) Querying that [object] for a gfxFlattenedPath
>  (c) Querying that [object] for the list of segment-endpoints defined by the
> path description string (via nsSVGPathElement::CreatePathSegList())
> 
> nsSVGPathElement works reasonably well as the [object] above, but alternately,
> we could indeed use the various objects that nsSVGPathElement uses internally. 
> In particular:
>  - We can get (a) with a nsSVGPathDataParserToInternal & a nsSVGPathList
>  - We can use the resulting nsSVGPathList to get (b)
> 
>  - However for (c), we'll need to re-parse, using a nsSVGPathDataParserToDOM to
> generate a nsCOMArray<nsIDOMSVGPathSeg>.  (my patch currently does this part
> via a call to via nsSVGPathElement::CreatePathSegList())
> 
> The third point above is needed because the nsSVGPathList object (for (a)/(b))
> may get additional "sub-segments" -- e.g. some curves in the path description
> can get converted into a *series* of sub-curves in the nsSVGPathList.  So if we
> iterate through a nsSVGPathList to find our relevant points, we may get some
> incorrect intermediate points.  nsSVGPathDataParserToDOM doesn't have this
> problem.
> 
> I'll look into using these structures explicitly, rather than using a
> nsSVGPathElement to access them.

I don't want it to end up more complex. Maybe you could refactor the guts of nsSVGPathElement into an object that can then be reused here?

Comment 48

[Daniel Holbert [:dholbert]]

Attachment: Patch C2 v2: Change GET_FLAG / SET_FLAG from macros to inline SetXXXErrorFlag(val) methods

> (In reply to comment #46)
> > Just to be clear -- I understand this to mean you'd like to switch to using a
> > group of GetXXXErrorFlag() / SetXXXErrorFlag(PRBool aNewValue) methods -- is
> > that correct?
> 
> Yes

Ok, did that here. (I actually completely removed the "Get" version of the method, because it's never used -- instead, we only ever test the whole |mErrorFlags| variable at once.)

Comment 49

[Daniel Holbert [:dholbert]]

Attachment: Patch C3: Add "TreatSingleValueAsStatic" boolean method

This is one more "tweak the underlying nsSMILAnimationFunction logic" patch that allows me to fix a bug that I noticed in my previous patches, where single-point animateMotion elements weren't affecting the presentation value when they should have been. (For example, animateMotion with path="M12,34", which should be treated as a static "move" command.)

Basically, the existing nsSMILAnimationFunction logic already knows that when our parsed nsSMILValue list has a length of 1, we might be trying to just set that single value (skipping interpolation). With the <animate> node, that only happens when we have a |values| attribute, and that's what's currently hard-coded into this check in nsSMILAnimationFunction.  But <set> actually *always* wants that behavior, and <animateMotion> needs to be able to have that behavior when we don't have values but we do have a |path| attr or an <mpath> child.

So, this patch lets subclasses specialize the check for "Should we treat single-value values list as a static value?". (The animateMotion-specific bits of it will be in the upcoming revised "main" patch, and tests are included in the upcoming revised test patch.)

Note that this change simplifies the nsSMILSetAnimationFunction implementation a bit, too. (I verified that it passes reftests & mochitests.)

Comment 50

[Daniel Holbert [:dholbert]]

Attachment: Patch D v4: Tests

This updated tests patch moves some "todo" single-value paths to now be marked as valid & working (in smilAnimateMotionValueLists.js  / test_smilAnimateMotionInvalidValues.xhtml).  It also adds some single-value paths & values-lists for more rigorous testing in db_smilAnimateMotion.js / test_smilAnimateMotion.xhtml.

Comment 51

[Daniel Holbert [:dholbert]]

Attachment: Patch E v5: main animateMotion patch

Here's an updated main animateMotion patch.  This version removes the need for an anonymous helper path element, using the parsers & structures defined at the end of comment 46.  RE roc's "I don't want it to end up more complex" note in comment 47 -- I think this actually ended up simpler than before (better decomposed, definitely).  I think that it'd be more complex and error-prone to refactor the nsSVGPathElement internals.

Other changes in this patch:
 - Adds custom "TreatSingleValueAsStatic" impl (the method added in Patch C3), to handle single-command motion paths
 - The patch now retains our gfxFlattenedPath + list of path vertices between samples (instead of the anonymous helper path), and I only update them when relevant attributes have changed. (I track whether we'll need to update them with an enum "PathSourceType" that tells us what attribute(s) we used to define our current gfxFlattenedPath, and hence what attributes could possibly affect our path definition.)

Comment 52

[Daniel Holbert [:dholbert]]

Attachment: Patch H v2: mpath implementation

... and here's the updated mpath patch, to fit into the fixes / refactoring from the updated main patch.

Comment 53

[Daniel Holbert [:dholbert]]

Comment on attachment 441756 [details] [diff] [review]
Patch E v5: main animateMotion patch

Actually, I think I can still subdivide the main patch a bit, into multiple patches, to make it  more manageable to review. I'll do that in the morning; canceling review request on that patch for now.

Comment 54

[Daniel Holbert [:dholbert]]

Attachment: Patch E v6: main animateMotion patch

I've spun off two more patches from the main patch:
 - support for the |path| attribute
 - support for the |keyPoints| attribute

So, here's the main patch again, with those chunks of code removed. (Note: since |path| has been spun off, this main patch only supports the use of from/by/to/values for controlling the motion.)

I'll post the spun-off patches in a sec.

Comment 55

[Daniel Holbert [:dholbert]]

Attachment: Patch F v1: keyPoints attr

This patch adds support for the |keyPoints| attribute.  (Note: I'm re-labeling the patches after the main one, so this one is now "Patch F", and the mpath patch will now be "Patch H")

Comment 56

[Daniel Holbert [:dholbert]]

Attachment: Patch G v1: path attr

This spun-off patch adds support for the |path| attribute.

Comment 57

[Daniel Holbert [:dholbert]]

Attachment: Patch H v3: mpath implementation

Comment 58

[Daniel Holbert [:dholbert]]

(That last version of the mpath patch -- v3 -- just has a bitrot fix, for a minor tweak I made in contextual code while spinning off the keyPoints & path patches.)

Comment 59

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 441742 [details] [diff] [review]
Patch C3: Add "TreatSingleValueAsStatic" boolean method

+    return (HasAttr(nsGkAtoms::values));

lose extra parens

Comment 60

[Robert O'Callahan (:roc) (email my personal email if necessary)]

-private:
   const nsIContent* mElement;

Shouldn't we just add getters here if you want access to the fields?

+    (void)isAffected; // Suppress GCC warning about using uninitialized variable

I think it would make more sense to just set isAffected to false here.

+      success = pathGenerator.MoveToAbsolute(NS_LITERAL_STRING("0 0"));

Can we add a method to pathGenerator for this? Parsing constant strings makes me itchy.

Instead of having ApplyRotate, it would be more efficient and probably no more complex to apply the rotation as we generate the values in GenerateValuesForPathAndPoints. It's worth avoiding multiple passes over an array.

+ if (aRotate.EqualsLiteral("auto")) {

Fix indent

+    // Parse as an atom for quicker comparisons vs auto & auto-rotate

This comment is incorrect.

+  mGfxContext.IdentityMatrix();

How could the matrix not be the identity?

+  // XXXdholbert Ideally we'd like to know if the delimeter we found was a
+  // comma (and if so, fail if we come across any more commas before the next
+  // value). Current behavior will accept mutliple commas in between values,
+  // and that's not technically spec-correct, but it's simpler and it matches
+  // our behavior elsewhere where we use strtok with SVG_COMMA_WSP_DELIM.

I actually think we should fix this (and add a test). Accepting incorrect values is bad for interop.

Comment 61

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 441865 [details] [diff] [review]
Patch F v1: keyPoints attr

+  double totalPathLen;
+  if (aIsKeyPoints) {
+    // If we're using "keyPoints" as our list of input distances, then we need
+    // to de-normalize from the [0,1] scale to the [0, totalPathLen] scale.
+    totalPathLen = aPath->GetLength();
+  }
   const PRUint32 numPoints = aPointDistances.Length();
   for (PRUint32 i = 0; i < numPoints; ++i) {
     double curDist = aPointDistances[i];
+    if (aIsKeyPoints) {
+      curDist *= totalPathLen;
+    }

Just set totalPathLen to 1.0 initially to avoid warning on uninitialized value. Then you don't need the second if.

Comment 62

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  mHrefTarget.Reset(aParent, targetURI);

What if aParent is null, does this work? Probably should test <mpath> as the root element...

Comment 63

[Daniel Holbert [:dholbert]]

Point-by-point review responses below -- in short, I fixed everything in my patch queue, except for the "multiple commas" issue, which I'll fix in a separate patch as noted below.

(In reply to comment #59)
> lose extra parens

Fixed.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/

(In reply to comment #60)
> -private:
>    const nsIContent* mElement;
> 
> Shouldn't we just add getters here if you want access to the fields?

Actually, I've changed the way I get that nsIContent pointer -- I don't like opening up this supposed-to-be-private cache struct, so I added a method on nsISMILAttr instead, since that also knows about the target element.  I think this is less sketchy than what I was doing before.

http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/a590c21fae74

> I think it would make more sense to just set isAffected to false here.

Fixed (as part of in changeset above).

> Can we add a method to pathGenerator for this? Parsing constant strings makes
> me itchy.

Good point. Fixed (as part of in changeset above) - added method MoveToOrigin.

> Instead of having ApplyRotate, it would be more efficient and probably no more
> complex to apply the rotation as we generate the values in
> GenerateValuesForPathAndPoints. It's worth avoiding multiple passes over an
> array.

Nice, that simplifies things a bit, too. Fixed:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/b419e55128d4

> Fix indent

Fixed (as part of changeset a590c21fae74, linked above)

> This comment is incorrect.

Yup, that's out of date -- I've now removed it.
(as part of changeset a590c21fae74, linked above)

> +  mGfxContext.IdentityMatrix();
> 
> How could the matrix not be the identity?

You're right, it's already the identity. (copy-paste fail) Fixed now.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/0e7a775606f5

> +  // XXXdholbert Ideally we'd like to know if the delimeter we found was a
> +  // comma (and if so, fail if we come across any more commas before the next
> +  // value). Current behavior will accept mutliple commas in between values,
> +  // and that's not technically spec-correct, but it's simpler and it matches
> +  // our behavior elsewhere where we use strtok with SVG_COMMA_WSP_DELIM.
> 
> I actually think we should fix this

Ok -- I'll fix this in a separate patch that will layer on top of everything else. (since a complete fix should touch more than just animateMotion)
 --> TODO

>  (and add a test)
(FWIW, I've actually already got some tests for this case, in the "gInvalidValuesTodo" and "gInvalidToByTodo" lists, in my tests patch.  The file test_smilAnimateMotionInvalidValues.xhtml uses todo()'s to indicate that these values currently are treated as valid & affect our CTM, even though they're technically invalid).

(In reply to comment #61)
> Just set totalPathLen to 1.0 initially to avoid warning on uninitialized value.
> Then you don't need the second if.

Done. I renamed the variable to "distanceMultiplier", too, now that it's not necessarily an actual path length.
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/32f3fd7f9072

(In reply to comment #62)
> +  mHrefTarget.Reset(aParent, targetURI);
> 
> What if aParent is null, does this work? 

Nope, it doesn't work -- if we manage to get there with a null parent, we do indeed crash inside of Reset, from calling GetCurrentDoc() on a null pointer.

Fixed to call Unlink() instead of Reset() in that case.  I added a crashtest to check this (verified to crash before the fix, and not crash after):
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/f0de30d763bc

Comment 64

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, those changes look good. r=me on the remaining patches.

Comment 65

[Daniel Holbert [:dholbert]]

(In reply to comment #63)
> Ok -- I'll fix this in a separate patch that will layer on top of everything
> else. (since a complete fix should touch more than just animateMotion)
>  --> TODO

Just to update on this -- I filed a separate bug on this (bug 562310), since it turned out to be a decent-sized patch (and it touches some non-animation-related code, for other attributes that use |comma-wsp| as a delimiter).

I've got a patch posted there, though as longsonr suggests, it might stand to benefit from integrating with and/or extending the existing "nsCommaSeparatedTokenizer" class.

roc, do you want me to hold off on landing the animateMotion patches (on this bug) until bug 562310 has a final fix?

I personally tend to lean towards getting animateMotion on trunk, for some testing & exposure, and then landing bug 562310 separately to fix up the comma-handling issues (in few days, once it's ready).  The lax comma-handling in the current animateMotion parsing is undesirable as you said, but it does match our existing behavior for other comma-wsp-delimited attributes, and it'll be fixed soon enough... :)

Comment 66

[Daniel Holbert [:dholbert]]

Comment on attachment 441864 [details] [diff] [review]
Patch E v6: main animateMotion patch

(setting r+ on main animateMotion patch, with fixes in patch queue, per comment 64)

Comment 67

[Daniel Holbert [:dholbert]]

Comment on attachment 441867 [details] [diff] [review]
Patch H v3: mpath implementation

(setting r+ on mpath patch, with fixes in patch queue, per comment 64)

Comment 68

[Daniel Holbert [:dholbert]]

(In reply to comment #65)
> and then landing bug 562310 separately to fix up the
> comma-handling issues (in few days, once it's ready).

(er s/in/within/)

Comment 69

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, I think you should land these patches now.

Comment 70

[Daniel Holbert [:dholbert]]

Landed!
http://hg.mozilla.org/mozilla-central/rev/06f995a12cbb
http://hg.mozilla.org/mozilla-central/rev/48cd33adb97a
http://hg.mozilla.org/mozilla-central/rev/82866f774437
http://hg.mozilla.org/mozilla-central/rev/a729bbd470af
http://hg.mozilla.org/mozilla-central/rev/f33f3bc8fd80
http://hg.mozilla.org/mozilla-central/rev/6b47c52ef03a
http://hg.mozilla.org/mozilla-central/rev/4daae068abb7
http://hg.mozilla.org/mozilla-central/rev/351afa5020f6
http://hg.mozilla.org/mozilla-central/rev/c49f111bb95b
http://hg.mozilla.org/mozilla-central/rev/2d5ccb713cd2

Comment 71

[Jonathan Watt [:jwatt]]

Woo! :-)