Last Comment Bug 771042 - Motion on a path hardly displays when path is not rendered and group is animated
: Motion on a path hardly displays when path is not rendered and group is animated
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Jonathan Watt [:jwatt] (back in October - email directly if necessary)
: General SVG Bugs
:
Mentors:
Depends on:
Blocks: 734082
  Show dependency treegraph
 
Reported: 2012-07-04 22:11 PDT by Brian Birtles (:birtles)
Modified: 2012-08-14 07:18 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
fixed


Attachments
Test case (654 bytes, image/svg+xml)
2012-07-04 22:11 PDT, Brian Birtles (:birtles)
no flags Details
Test case with the animation path rendered (675 bytes, image/svg+xml)
2012-07-04 22:12 PDT, Brian Birtles (:birtles)
no flags Details
patch (7.99 KB, patch)
2012-07-06 16:35 PDT, Jonathan Watt [:jwatt] (back in October - email directly if necessary)
bbirtles: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Brian Birtles (:birtles) 2012-07-04 22:11:50 PDT
Created attachment 639218 [details]
Test case

If we have animateMotion that refers to an external path (mpath), and that paths is not rendered, and the group containing the path and animation target is transformed, then the target rarely renders. By which I mean it flickers really badly and is invisible 99% of the time.

Steps to reproduce:

1. Load the attached test case
2. Watch the animation

Expected results:

The triangle shapes moves in an arc (with a bit of backwards and forwards).

Actual results:

You very occasionally see the triangle flicker once or twice but mostly it doesn't appear.

Curiously, if you make the animation path rendered (even just setting stroke to something other than none and opacity to 0.01) it displays fine.

From what I can tell this only affects Windows.

Also, disabling direct2d doesn't seem to make a difference, I think.
Comment 1 Brian Birtles (:birtles) 2012-07-04 22:12:43 PDT
Created attachment 639219 [details]
Test case with the animation path rendered
Comment 2 Brian Birtles (:birtles) 2012-07-04 22:15:31 PDT
Just running this test case with a trunk build and the arrow doesn't appear to move but if you alt+tab back and forth to force a repaint the arrow will move and sometimes flicker.
Comment 3 Daniel Holbert [:dholbert] 2012-07-05 10:49:58 PDT
I see same behavior as comment 2 on 64-bit linux.

Looks like a (lack-of) invalidation bug.

It's possible this'll be fixed by DLBI -- it'd be worth trying this in a trunk build from a few days ago, before DLBI was backed out.
Comment 4 Brian Birtles (:birtles) 2012-07-05 16:25:48 PDT
(In reply to Daniel Holbert [:dholbert] from comment #3)
> I see same behavior as comment 2 on 64-bit linux.

Interestingly, it seemed to work (prior to reducing the test case at least) on Mac.
Comment 5 Cameron McCormack (:heycam) 2012-07-05 16:29:36 PDT
I get the buggy behaviour on Mac with my June 27 nightly.
Comment 6 Brian Birtles (:birtles) 2012-07-05 16:43:10 PDT
(In reply to Daniel Holbert [:dholbert] from comment #3)
> It's possible this'll be fixed by DLBI -- it'd be worth trying this in a
> trunk build from a few days ago, before DLBI was backed out.

Just tested with a build from July 3 which was built before DLBI was backed out and I'm still seeing similar problems.
Comment 7 Brian Birtles (:birtles) 2012-07-05 16:44:44 PDT
(In reply to Cameron McCormack (:heycam) from comment #5)
> I get the buggy behaviour on Mac with my June 27 nightly.

That's good to know. It may be that the non-reduced test case featured something else that triggered invalidation on Mac (it had a big bitmap background and all sorts of other elements sharing the same space).
Comment 8 Daniel Holbert [:dholbert] 2012-07-05 18:25:02 PDT
If I enable paint flashing and load the buggy testcase, I see mostly-offscreen painted rects appearing at the top-left and top-right of the window.  They seem to correspond to the position of the arrow, but upwards by a few hundred pixels.

So -- it looks like we might be invalidating but we're not applying the correct transform when doing so, or something like that.
Comment 9 Brian Birtles (:birtles) 2012-07-05 19:24:14 PDT
I forgot to mention that this is a regression. The regression range is somewhere between Firefox 12 and current Aurora (i.e. 15).
Comment 10 Alice0775 White 2012-07-06 02:51:24 PDT
I tested with attachment 639218 [details].
Triangle flickers/disappears .

Regression window(cached m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/f2b2b99108a2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120517030029
Bad:
http://hg.mozilla.org/mozilla-central/rev/895e12563245
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120517110214
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f2b2b99108a2&tochange=895e12563245

Regression window(cached m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/37f2536e975e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120516204627
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/2c3647738e81
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120516230826
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=37f2536e975e&tochange=2c3647738e81

In local build:
Last Good:d3b11e443f04
First Bad: 05a339620439

Triggered by: 
05a339620439	Jonathan Watt — Bug 734082 - Compute and store bounds and visual overflow bounds for both SVG leaf and container frames. r=roc.
Comment 11 Brian Birtles (:birtles) 2012-07-06 05:43:17 PDT
(In reply to Alice0775 White from comment #10)
> I tested with attachment 639218 [details].
> Triangle flickers/disappears .
> ...
> In local build:
> Last Good:d3b11e443f04
> First Bad: 05a339620439
> 
> Triggered by: 
> 05a339620439	Jonathan Watt — Bug 734082 - Compute and store bounds and
> visual overflow bounds for both SVG leaf and container frames. r=roc.

Alice0775 White, thank you so much. That's a huge help.
Comment 12 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-07-06 16:35:51 PDT
Created attachment 639846 [details] [diff] [review]
patch
Comment 13 Brian Birtles (:birtles) 2012-07-08 17:39:57 PDT
Comment on attachment 639846 [details] [diff] [review]
patch

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

Looks good, thanks Jonathan.

The main issue is I discovered that none of the tests in the motion directory are being run. i.e. reftests/svg/smil/reftest.list doesn't include them.

Would you mind adding "include motion/reftest.list" to reftests/svg/smil/reftest.list ?

The good news is that pretty much none of those tests pass without this patch applied (i.e. we would have caught this regression a lot sooner if we were running those tests). The bad news is I am still seeing failures for animateMotion-rotate-2.svg even with this patch applied. One pixel is rgb(1,255,0) instead of (0,255,0)--could be a display driver thing since I get a few test failures like this in other places.

r=me assuming the animateMotion-rotate-2.svg is not related and passes on try

::: layout/reftests/svg/smil/motion/animateMotion-simple-ref.svg
@@ +1,3 @@
> +<svg xmlns="http://www.w3.org/2000/svg">
> +  <rect width="100" height="100" fill="blue"/>
> +</svg>

I'd rather we used standard reference files where possible. I guess the lime.svg which fills the viewport isn't helpful in this case?

Going forward I think a 100x100 green rect is probably the most generally-useful reference image (since filling the viewport sometimes makes it hard to test positioning). For now, perhaps we can just rename this to green-rect.svg? (and make it green :) )

::: layout/reftests/svg/smil/motion/animateMotion-simple.svg
@@ +1,1 @@
> +<!--

All the tests in this directory have a number at the end (I guess so variations on a theme are easy to identify). We should rename this to animateMotion-simple-1.svg.

::: layout/reftests/svg/smil/motion/reftest.list
@@ +1,4 @@
>  # Tests related to SVG Animation (using SMIL), focusing on the animateMotion
>  # element.
>  
> +== animateMotion-simple.svg animateMotion-simple-ref.svg

This should maintain alphabetical order.

::: layout/svg/base/src/nsSVGContainerFrame.cpp
@@ +164,5 @@
>    nsSVGElement *content = static_cast<nsSVGElement*>(mContent);
>    const SVGAnimatedTransformList *list = content->GetAnimatedTransformList();
> +  const gfxMatrix* animateMotionTransform =
> +    content->GetAnimateMotionTransform();
> +  if ((list && !list->GetAnimValue().IsEmpty()) || animateMotionTransform) {

Nit: Can we put the animateMotionTransform test first since it's less expensive.
i.e. if (animateMotionTransform || (list && !list->GetAnimValue().IsEmpty())) {

::: layout/svg/base/src/nsSVGPathGeometryFrame.cpp
@@ +94,5 @@
>    nsSVGElement *content = static_cast<nsSVGElement*>(mContent);
>    const SVGAnimatedTransformList *list = content->GetAnimatedTransformList();
> +  const gfxMatrix* animateMotionTransform =
> +    content->GetAnimateMotionTransform();
> +  if ((list && !list->GetAnimValue().IsEmpty()) || animateMotionTransform) {

Likewise here.
Comment 14 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-07-09 10:40:48 PDT
(In reply to Brian Birtles (:birtles) from comment #13)
> The main issue is I discovered that none of the tests in the motion
> directory are being run. i.e. reftests/svg/smil/reftest.list doesn't include
> them.

Doh! How did we miss that?

I'll actually just remove the new test that I added, given that - if we actually run the existing tests - most of them fail prior to this patch. I only added the new test because it seemed like they were passing and therefore not catching this bug.

> The bad news is I am still seeing failures for animateMotion-rotate-2.svg

Actually, that's due to a bug in the test. I'll fix that as a "part 2" patch.

> Would you mind adding "include motion/reftest.list" to
> reftests/svg/smil/reftest.list ?

Sure, I'll do that as a "part 3" patch.
Comment 16 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-07-09 10:45:27 PDT
(In reply to Brian Birtles (:birtles) from comment #13)
> Nit: Can we put the animateMotionTransform test first since it's less
> expensive.
> i.e. if (animateMotionTransform || (list &&
> !list->GetAnimValue().IsEmpty())) {

I didn't do this since it's a lot rarer to have an animateMotion than a simple transform. What I did instead is move the GetAnimateMotionTransform virtual call into the if so we only evaluate it when necessary. That seemed like the better course of action if perf is a concern.
Comment 17 Daniel Holbert [:dholbert] 2012-07-09 12:09:30 PDT
(In reply to Jonathan Watt [:jwatt] from comment #14)
> (In reply to Brian Birtles (:birtles) from comment #13)
> > The main issue is I discovered that none of the tests in the motion
> > directory are being run. i.e. reftests/svg/smil/reftest.list doesn't include
> > them.
> 
> Doh! How did we miss that?

Yikes!  Looks like that reftest.list tweak was missing from my original animateMotion tests checkin, somehow:
  http://hg.mozilla.org/mozilla-central/rev/2d5ccb713cd2

Not sure what happened there.  Thanks for catching & fixing that!
Comment 18 Brian Birtles (:birtles) 2012-07-09 15:50:12 PDT
(In reply to Jonathan Watt [:jwatt] from comment #16)
> I didn't do this since it's a lot rarer to have an animateMotion than a
> simple transform. What I did instead is move the GetAnimateMotionTransform
> virtual call into the if so we only evaluate it when necessary. That seemed
> like the better course of action if perf is a concern.

Ok, sounds good. Thanks Jonathan!
Comment 20 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-07-10 05:11:02 PDT
(In reply to Daniel Holbert [:dholbert] from comment #17)
> Looks like that reftest.list tweak was missing from my original
> animateMotion tests checkin

It would be nice if our tools would warn us about that. :)
Comment 21 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-07-10 05:13:54 PDT
Comment on attachment 639846 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 734082
User impact if declined: Animation along a path (one of the more interesting types of animation) is broken.
Testing completed (on m-c, etc.): Baked on m-c.
Risk to taking this patch (and alternatives if risky): Very low - it just fixes the area that is repainted.
String or UUID changes made by this patch: none
Comment 22 Alex Keybl [:akeybl] 2012-07-12 15:24:31 PDT
Comment on attachment 639846 [details] [diff] [review]
patch

[Triage Comment]
Low risk FF15 regression fix, approved for Aurora.
Comment 23 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-07-13 07:20:53 PDT
Pushed https://hg.mozilla.org/releases/mozilla-aurora/rev/9aa8643ad4ba
Comment 24 Paul Silaghi, QA [:pauly] 2012-08-14 07:18:49 PDT
Verified fixed loading the test cases on FF 15b4 on Win 7/64, Ubuntu 12.04 and Mac OS X 10.6.

Note You need to log in before you can comment on or make changes to this bug.