Closed Bug 436418 (animateMotion) Opened 16 years ago Closed 14 years ago

SVG SMIL: Implement "animateMotion"

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug, )

Details

Attachments

(11 files, 13 obsolete files)

524 bytes, image/svg+xml
Details
12.52 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.47 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.06 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
9.12 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.23 KB, patch
roc
: review+
Details | Diff | Splinter Review
64.93 KB, patch
Details | Diff | Splinter Review
77.32 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
14.75 KB, patch
roc
: review+
Details | Diff | Splinter Review
10.27 KB, patch
roc
: review+
Details | Diff | Splinter Review
37.34 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
Attached image 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.)
Whiteboard: [hgurl:c0]
Version: Trunk → Other Branch
Depends on: 216462
Whiteboard: [hgurl:c0]
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!
Assignee: nobody → afterthebeep
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?
(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.
(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.
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.)
Version: Other Branch → Trunk
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.
Attached patch a beginning patch (obsolete) — Splinter Review
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 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 <birtles@gmail.com>
>+ *   Chris Double  <chris.double@double.co.nz>
>+ *   Daniel Holbert <dholbert@mozilla.com>

This should be you.
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)
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.
Flags: wanted-next+
Blocks: svg11tests
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. :)
(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.
Ok -- stealing. Thanks!
Assignee: afterthebeep → dholbert
Status: NEW → ASSIGNED
Alias: animateMotion
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.)
Depends on: 557885
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]).
Attachment #371952 - Attachment is obsolete: true
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
Attached patch Patch C: GenericValueParser (obsolete) — Splinter Review
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.)
Attached patch Patch D: Tests (obsolete) — Splinter Review
And here's a patch with some reftests. (definitely need to add more of these)
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.
(In reply to comment #21)
> TryServer build done:
> https://build.mozilla.org/tryserver-builds/dholbert@mozilla.com-try-7bb5c931b983/

I can confirm that the listed testcases pass with this patch/build on Mac OS X.
> // XXXdholbert Are these all correct & necessary? (based on nsSVGStopElement.cpp)

Yes. They forward interface implementation to the base class.
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.
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.)
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)
Attachment #439204 - Attachment is obsolete: true
FWIW, I started a TryServer run with Patch E v2; builds will appear at:
https://build.mozilla.org/tryserver-builds/dholbert@mozilla.com-try-3becd7896d43/
(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/dholbert@mozilla.com-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?
(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
Nice work Daniel!

Ship it! ;)
Attachment #439200 - Attachment is patch: true
Attachment #439200 - Attachment mime type: application/octet-stream → text/plain
Attached patch Patch D v2: Tests (obsolete) — Splinter Review
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.
Attachment #439203 - Attachment is obsolete: true
...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.
Attachment #439410 - Attachment is obsolete: true
Attachment #439200 - Flags: review?(roc)
Attachment #439201 - Flags: review?(roc)
Attachment #439202 - Flags: review?(roc)
Attachment #439838 - Flags: review?(roc)
Attachment #439839 - Flags: review?(roc)
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.
Attachment #439202 - Flags: review?(roc) → review+
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!)
Attachment #439202 - Attachment is obsolete: true
Attachment #439998 - Flags: review+
> 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.
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.
Attachment #441116 - Flags: review?(roc)
Attachment #439998 - Attachment description: Patch C v2: GenericValueParser → Patch C1 v2: GenericValueParser
Attached patch Patch D v3: Tests (obsolete) — Splinter Review
Attachment #439838 - Attachment is obsolete: true
Attachment #439839 - Attachment is obsolete: true
Attachment #441120 - Flags: review?(roc)
Attachment #439839 - Flags: review?(roc)
Attached patch Patch H v1: mpath implementation (obsolete) — Splinter Review
Attachment #441122 - Flags: review?(roc)
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.)
> 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.
(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 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
+  } 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?
(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.
(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?
> (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.)
Attachment #441116 - Attachment is obsolete: true
Attachment #441116 - Flags: review?(roc)
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.)
Attachment #441742 - Attachment description: Patch C3: Add "TreatSingleValueAsStatic" method → Patch C3: Add "TreatSingleValueAsStatic" boolean method
Attachment #441742 - Flags: review?(roc)
Attachment #441740 - Flags: review?(roc)
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.
Attachment #441118 - Attachment is obsolete: true
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.)
Attachment #441120 - Attachment is obsolete: true
Attachment #441756 - Flags: review?(roc)
Attachment #441120 - Flags: review?(roc)
Attached patch Patch H v2: mpath implementation (obsolete) — Splinter Review
... and here's the updated mpath patch, to fit into the fixes / refactoring from the updated main patch.
Attachment #441122 - Attachment is obsolete: true
Attachment #441757 - Flags: review?(roc)
Attachment #441122 - Flags: review?(roc)
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.
Attachment #441756 - Flags: review?(roc)
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.
Attachment #441756 - Attachment is obsolete: true
Attachment #441864 - Flags: review?(roc)
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")
Attachment #441757 - Attachment is obsolete: true
Attachment #441865 - Flags: review?(roc)
Attachment #441757 - Flags: review?(roc)
Attachment #441865 - Attachment description: Patch E1 v1: keyPoints attr → Patch F v1: keyPoints attr
This spun-off patch adds support for the |path| attribute.
Attachment #441866 - Flags: review?(roc)
Attachment #441867 - Flags: review?(roc)
Attachment #441122 - Attachment description: Patch F v1: mpath implementation → Patch H v1: mpath implementation
Attachment #441757 - Attachment description: Patch F v2: mpath implementation → Patch H v2: mpath implementation
(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 on attachment 441742 [details] [diff] [review]
Patch C3: Add "TreatSingleValueAsStatic" boolean method

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

lose extra parens
Attachment #441742 - Flags: review?(roc) → review+
-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 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.
+  mHrefTarget.Reset(aParent, targetURI);

What if aParent is null, does this work? Probably should test <mpath> as the root element...
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
OK, those changes look good. r=me on the remaining patches.
(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 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)
Attachment #441864 - Flags: review?(roc) → review+
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)
Attachment #441867 - Flags: review?(roc) → review+
(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/)
Yes, I think you should land these patches now.
Woo! :-)
Depends on: 684780
Blocks: 665392
Depends on: CVE-2011-3654
Blocks: 825254
Blocks: 974710
Blocks: 974698
Blocks: 966634
Blocks: 1752912
You need to log in before you can comment on or make changes to this bug.