946540 - Foxkeh can no longer loop-the-loop (animateMotion stops part way)

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Brian Birtles (:birtles)]

Attachment: Test graphic

In the attached test case foxkeh stops animating part way through the loop and stays there.

I've narrowed the regression range down to:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a475f94bb1b1&tochange=beddd6d4bcdf

Some suspects in that range include:

Bug 937994 - Make the SMIL animate motion code use a Moz2D PathBuilder instead of gfxContext
Bug 938569 - Path fails to render, with comma between move command and implied line command
Bug 938388 - Convert all remaining code for calculating path lengths and position at an offset along a path in content/svg to Moz2D (kill off all uses of gfxPath)
Bug 930577 - Make SVGPathData::ToPath use the Moz2D version of SVGPathData::ConstructPath so that it no longer expecting a device space path from Moz2D backed gfxContext

Currently trying to track down further.

Comment 1

[Brian Birtles (:birtles)]

Attachment: More reduced test case

Comment 2

[Brian Birtles (:birtles)]

I notice that with this more reduced test case that the extent to which the animation tracks its motion path has degraded significantly.

Comment 3

[Brian Birtles (:birtles)]

Attachment: Reduced test case

This is a much simpler file and slows down the animation so you can see the approximation taking place in the loop more clearly.

Comment 4

[Brian Birtles (:birtles)]

For my own reference, this is probably why Parapara animation is broken in nightlies:
http://parapara.mozlabs.jp/walls/kobe-manga

Comment 5

[Brian Birtles (:birtles)]

This may be due to similar issues with the path flattening code as bug 941585.

Comment 6

[Alice0775 White]

Last Good: a08baab2e6f3
First Bad: 4f086025350f
Triggered by:
4f086025350f	Jonathan Watt — Bug 930577 - Convert much of the SVG code for calculating path lengths and position at an offset along a path to Moz2D. r=heycam

Comment 7

[Brian Birtles (:birtles)]

Attachment: Circle (works well)

This circle test case is actually a lot smoother in nightlies so it's not the case they we're always approximating paths too coarsely but seems to be the particular path in the test case that is problematic.

Comment 8

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #7)
> seems to be the particular path in the test case that is problematic.

That path, by the way, looks like this:

  m-36.17 294.5
  c42.67  26     96.58  61     164.7  61
   69     0      170    -9     207.2  -25.66
   58.42  -26.15 81.18  -61.17 81.78  -90.02
   1.007  -48.77 -23.97 -51.96 -36.72 -52.2
   -16.28 -0.305 -35.31 16.89  -35.31 54.37
   0      25.17  10.8   46.48  22.38  56
   34.22  28.16  75.68  35.52  102.6  38.52
  s113.1  7.168  170.3  1.999
  c79.31  -7.164 97.36  -10.12 179.7  -35
   42.34  -12.79 72.76  -17.46 145    -45.06

That third segment, "(c)58.42  -26.15 81.18  -61.17 81.78  -90.02" is where things start getting wacky. And if I change the first coordinate to 68.42 it's completely wrong.

The second last segment, "c79.31  -7.164 97.36  -10.12 179.7  -35" is where it stops. If I change the first coordinate to 89.31 it continues but deviates from the actual path.

Comment 9

[Brian Birtles (:birtles)]

Thanks Alice for tracking down the offending changeset!

Comment 10

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #4)
> For my own reference, this is probably why Parapara animation is broken in
> nightlies:
> http://parapara.mozlabs.jp/walls/kobe-manga

Confirming that this breakage has the same regression window so is likely the same bug.

Comment 11

[Brian Birtles (:birtles)]

Requesting tracking for Firefox 28.

animateMotion is fairly badly broken. Apart from the test case here, comment 8 shows that changing the coordinates of the path produces all sorts of other broken cases.

As for production sites http://parapara.mozlabs.jp/ is broken by this (animated characters do not appear at all--at least for the space there, e.g. http://parapara.mozlabs.jp/walls/kobe-manga).

We need to either fix the path flattening (which may have the side effect of fixing bug 941585) or back out bug 930577 and all its dependents (of which there are quite a few).

Comment 12

[Bas Schouten (:bas.schouten)]

I've root-caused one part of this problem. That part, at least, is not related to bug 941585.

Comment 13

[Bas Schouten (:bas.schouten)]

Attachment: Deal with inflection points that both lie outside of (0, 1)

This fixes all the problems with this path. The problem occurs when theoretical inflection points are found on the bezier curve, but both of them lie outside of the (0, 1) range. This patch addresses that issue.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8346634 [details] [diff] [review]
Deal with inflection points that both lie outside of (0, 1)

Review of attachment 8346634 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/Path.cpp
@@ +405,5 @@
>    if (!FindInflectionPoints(aControlPoints, &t1, &t2, &count)) {
>      aSink->LineTo(aControlPoints.mCP4);
>      return;
>    }
>  

Add something like:
// check that at least one of the inflection points is inside [0..1]

Comment 15

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #14)
> Comment on attachment 8346634 [details] [diff] [review]
> Deal with inflection points that both lie outside of (0, 1)
> 
> Review of attachment 8346634 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/Path.cpp
> @@ +405,5 @@
> >    if (!FindInflectionPoints(aControlPoints, &t1, &t2, &count)) {
> >      aSink->LineTo(aControlPoints.mCP4);
> >      return;
> >    }
> >  
> 
> Add something like:
> // check that at least one of the inflection points is inside [0..1]

And make sure you add a test

Comment 16

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #8)
> That third segment, "(c)58.42  -26.15 81.18  -61.17 81.78  -90.02" is where
> things start getting wacky. And if I change the first coordinate to 68.42
> it's completely wrong.
> 
> The second last segment, "c79.31  -7.164 97.36  -10.12 179.7  -35" is where
> it stops. If I change the first coordinate to 89.31 it continues but
> deviates from the actual path.

I tried these combinations with the patch applied and they look good. Thanks Bas!

Comment 17

[Robert Longson [:longsonr]]

It might help if you could knock up a reftest for Bas, Brian

Comment 18

[Bas Schouten (:bas.schouten)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5f9dee7531e

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a5f9dee7531e

Comment 20

[Brian Birtles (:birtles)]

Bas, can you land this on Aurora too? Thanks!

Comment 21

[Brian Birtles (:birtles)]

Attachment: Reftest

(In reply to Robert Longson from comment #17)
> It might help if you could knock up a reftest for Bas, Brian

How about this?

Comment 22

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #21)
> Created attachment 8347887 [details] [diff] [review]
> Reftest
> 
> (In reply to Robert Longson from comment #17)
> > It might help if you could knock up a reftest for Bas, Brian
> 
> How about this?

Hmmm... seems to be flaky: https://tbpl.mozilla.org/?tree=Try&rev=ba71af2814ef

Comment 23

[Robert Longson [:longsonr]]

Seems less like it's flaky and more like it just doesn't work at all on Windows 8. Perhaps you should land it as fails-if on that platform and raise another bug (for Bas perhaps?) to investigate why this is so.

Comment 24

[Brian Birtles (:birtles)]

(In reply to Robert Longson from comment #23)
> Seems less like it's flaky and more like it just doesn't work at all on
> Windows 8. Perhaps you should land it as fails-if on that platform and raise
> another bug (for Bas perhaps?) to investigate why this is so.

Yes, that's right. I just tweaked the test case to sample a few seconds after the animation has finished in case its an artifact of having just finished the animation. Waiting to see if it makes any difference otherwise I'll do as you say: https://tbpl.mozilla.org/?tree=Try&rev=a74f28d6fc4d

Comment 25

[Brian Birtles (:birtles)]

Comment on attachment 8346634 [details] [diff] [review]
Deal with inflection points that both lie outside of (0, 1)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): regressed by bug 930577
User impact if declined: Many sites using animateMotion will be broken
Testing completed (on m-c, etc.): Has had nearly a week of bake time on m-c
Risk to taking this patch (and alternatives if risky): Minimal change
String or IDL/UUID changes made by this patch: None

Comment 26

[Brian Birtles (:birtles)]

(In reply to Robert Longson from comment #23)
> Seems less like it's flaky and more like it just doesn't work at all on
> Windows 8. Perhaps you should land it as fails-if on that platform and raise
> another bug (for Bas perhaps?) to investigate why this is so.

Filed bug 951541. Giving the test one more pass through try server with fails-if Windows 8 before landing: https://tbpl.mozilla.org/?tree=Try&rev=abd25c0c43b8

Comment 27

[Brian Birtles (:birtles)]

Pushed to Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/503d63c0d198

Comment 28

[Brian Birtles (:birtles)]

Pushed reftest to m-i: https://hg.mozilla.org/integration/mozilla-inbound/rev/fb1b9398aa86

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fb1b9398aa86