Closed Bug 946540 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox26 --- unaffected
firefox27 --- unaffected
firefox28 + fixed
firefox29 --- fixed

People

(Reporter: birtles, Assigned: bas.schouten)

References

Details

(Keywords: regression)

Attachments

(6 files)

Attached image 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.
Attached image More reduced test case
I notice that with this more reduced test case that the extent to which the animation tracks its motion path has degraded significantly.
Attached image 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.
For my own reference, this is probably why Parapara animation is broken in nightlies:
http://parapara.mozlabs.jp/walls/kobe-manga
This may be due to similar issues with the path flattening code as bug 941585.
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
Attached image 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.
(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.
Thanks Alice for tracking down the offending changeset!
Depends on: 930577
(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.
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).
Assignee: nobody → bas
Status: NEW → ASSIGNED
I've root-caused one part of this problem. That part, at least, is not related to bug 941585.
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.
Attachment #8346634 - Flags: review?(jmuizelaar)
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]
Attachment #8346634 - Flags: review?(jmuizelaar) → review+
(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
(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!
It might help if you could knock up a reftest for Bas, Brian
https://hg.mozilla.org/mozilla-central/rev/a5f9dee7531e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Bas, can you land this on Aurora too? Thanks!
Attached patch ReftestSplinter Review
(In reply to Robert Longson from comment #17)
> It might help if you could knock up a reftest for Bas, Brian

How about this?
Attachment #8347887 - Flags: review?(longsonr)
(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
Attachment #8347887 - Flags: review?(longsonr) → review+
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.
(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 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
Attachment #8346634 - Flags: approval-mozilla-aurora?
(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
Attachment #8346634 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Pushed reftest to m-i: https://hg.mozilla.org/integration/mozilla-inbound/rev/fb1b9398aa86
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.