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

RESOLVED FIXED in Firefox 28

Status

()

Core
SVG
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: birtles, Assigned: bas)

Tracking

({regression})

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

Firefox Tracking Flags

(firefox26 unaffected, firefox27 unaffected, firefox28+ fixed, firefox29 fixed)

Details

Attachments

(6 attachments)

(Reporter)

Description

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

Comment 1

5 years ago
Created attachment 8342784 [details]
More reduced test case
(Reporter)

Comment 2

5 years ago
I notice that with this more reduced test case that the extent to which the animation tracks its motion path has degraded significantly.
(Reporter)

Comment 3

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

Comment 4

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

Comment 5

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

Comment 6

5 years ago
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
(Reporter)

Comment 7

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

Comment 8

5 years ago
(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.
(Reporter)

Comment 9

5 years ago
Thanks Alice for tracking down the offending changeset!
(Reporter)

Updated

5 years ago
Depends on: 930577
(Reporter)

Comment 10

5 years ago
(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.
(Reporter)

Comment 11

5 years ago
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).
tracking-firefox28: --- → ?
(Assignee)

Updated

5 years ago
Assignee: nobody → bas
Status: NEW → ASSIGNED
(Assignee)

Comment 12

5 years ago
I've root-caused one part of this problem. That part, at least, is not related to bug 941585.
(Assignee)

Comment 13

5 years ago
Created attachment 8346634 [details] [diff] [review]
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.
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
(Reporter)

Comment 16

5 years ago
(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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
(Reporter)

Comment 20

5 years ago
Bas, can you land this on Aurora too? Thanks!
(Reporter)

Comment 21

5 years ago
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?
Attachment #8347887 - Flags: review?(longsonr)
(Reporter)

Comment 22

5 years ago
(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.
(Reporter)

Comment 24

5 years ago
(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
(Reporter)

Updated

5 years ago
status-firefox26: --- → unaffected
status-firefox27: --- → unaffected
status-firefox28: --- → affected
status-firefox29: --- → fixed
(Reporter)

Comment 25

5 years ago
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?
(Reporter)

Comment 26

5 years ago
(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
tracking-firefox28: ? → +
Attachment #8346634 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Reporter)

Comment 27

5 years ago
Pushed to Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/503d63c0d198
status-firefox28: affected → fixed
(Reporter)

Comment 28

5 years ago
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.