607537 - SVG SMIL: Support paint servers an animation values

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Brian Birtles (:birtles)]

Attachment: Test case

Currently we don't allow paint servers as animation values, e.g.

    <set attributeName="fill" to="url(#lime)"/>

SVG seems to allow this: http://dev.w3.org/SVG/profiles/1.1F2/publish/animate.html#Animatable

However, such values are not additive since they can't be converted to an RGB colour (unless we want to get really picky and special-case single stop gradients!)

See the attached test case. Opera and Webkit give us a solid lime fill (the gradient paint server) but Firefox gives us an ominous red.

This, of course, is a known shortcoming indicated very helpfully by the FIXME here:

http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleAnimation.cpp#2640

"FIXME: At some point in the future, we should animate gradients."

Note that bug 552087 has also been filed but, while related, appears to be more concerned with interpolating between gradients and colours. From what I can make of SVG at this point such behaviour is not required. If one wishes to interpolate between gradients they can animate the <stop> elements as necessary.

I think what is required here is simply being able to set a fill/stroke/etc. attribute to point to a paint server. i.e. discrete animation only, no interpolation. Testing Opera and WebKit it appears they only do discrete animation here.

Comment 1

[Brian Birtles (:birtles)]

Attachment: Interpolation test case

As mentioned in the above description, although this animation describes interpolation is should probably fall back to discrete animation. This is what Opera and WebKit do and I believe that is what is required by SVG.

Comment 2

[jonathan chetwynd]

I cannot say whether 552087 is related, the CSS_SVG wrangle is already confused.

but isn't this bug merely the converse 603917.
ie one is gradient to fill, the other fill to gradient?

Comment 3

[Brian Birtles (:birtles)]

(In reply to comment #2)
> but isn't this bug merely the converse 603917.
> ie one is gradient to fill, the other fill to gradient?

Yes, conceptually that's so (although the solution to bug 603917 reaches beyond just paint servers to all un-animatable base values). From an implementation point of view, however, they're different.

Comment 4

[jonathan chetwynd]

Attachment: converse?

Comment 5

[jonathan chetwynd]

apologies if the attachment is bug-spam,

the use-case peepo.com. relies on this bug being fixed in the simple case
ie set, and I cannot fathom a workaround.
click on 'study' select a famous professional game and play through it, all moves are black, rather than black-white-black. declarative animation only.

Comment 6

[will69]

The morphing example on carto:net (http://www.carto.net/papers/svg/samples/path_morphing.shtml) now works with respect to morphing thanks to Jonathan's work on bug #522306, but the pattern animation doesn't work (Swiss flag not shown). Would that be in the scope of this bug?

Comment 7

[Brian Birtles (:birtles)]

(In reply to comment #6)
> The morphing example on carto:net
> (http://www.carto.net/papers/svg/samples/path_morphing.shtml) now works with
> respect to morphing thanks to Jonathan's work on bug #522306, but the pattern
> animation doesn't work (Swiss flag not shown). Would that be in the scope of
> this bug?

Yes, that example appears to be covered by this bug.

Comment 8

[Brian Birtles (:birtles)]

Attachment: WIP patch v1

Progress so far. Still to do:
* Add some NULL checks
* Check we behave correctly with regards to character sets, document origin, relative paths
* Check we haven't messed up CSS animation in the process

Comment 9

[Brian Birtles (:birtles)]

Attachment: WIP test case

WIP in progress test file. With WIP patch v1 all tests pass.

Comment 10

[Brian Birtles (:birtles)]

(In reply to comment #9)
> WIP in progress test file. With WIP patch v1 all tests pass.

That is, if you also apply the patch from bug 614879.

Comment 11

[Brian Birtles (:birtles)]

Attachment: Patch v1a

Proposed patch.

Comment 12

[Brian Birtles (:birtles)]

(In reply to comment #6)
> The morphing example on carto:net
> (http://www.carto.net/papers/svg/samples/path_morphing.shtml) now works with
> respect to morphing thanks to Jonathan's work on bug #522306, but the pattern
> animation doesn't work (Swiss flag not shown). Would that be in the scope of
> this bug?

With the attached patch the Swiss flag is now shown. However, there are gaps between the flag and the country border in some cases. I've filed bug 615788 to track that problem.

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 494275 [details] [diff] [review]
Patch v1a

Looks good, generally!

>+++ b/layout/reftests/svg/smil/anim-paintserver-1.svg
>+  <!-- 2. Get paint server. (We're looking for a code path that will fetch the

Nit: This comment needs a ")" close-paren somewhere. :)

>+  <!-- 6. Check by-addition behaves (3): from-by paint server -->
>+  <rect x="200" y="100" width="100" height="100" fill="#0f0">
>+    <animate attributeName="fill" from="url(#lime)" by="url(#red)"/>
>+  </rect>

We probably want from="url(#red)" (or some other non-lime value), becuase the point here is that neither the from value nor the by value should be applied.

>+  <!-- 7. Check that by-addition without a paint server is ok though -->
>+  <rect x="0" y="200" width="100" height="100" fill="red">
>+    <animate attributeName="fill" from="#0f0" by="#00f"/>
>+  </rect>

So my first reaction to this was: "Wait -- doesn't this end up at #0ff instead of lime?"

But then I realized that it's got an indefinite duration (by default), so I guess it just sets the 'from' value (0f0 = lime) and never adds anything.

Might be good to add a brief comment there to explain that, so that other people less familiar with the ins & outs of SMIL can tell what's going on.

>+       value instead. (SMIL 3 12.6.3 says, "[The accumulate] attribute is
>+       ignored if the target attribute does not support additive animation." -->
>+  <rect x="100" y="200" width="100" height="100" fill="red">
>+    <animate attributeName="fill" values="url(#lime)" additive="sum"/>

I think you meant s/accumulate/additive/ there -- your test never uses 'accumulate', though the next few examples do use 'additive'.

You also need a closing-paren at the end of that comment. :)

>diff --git a/layout/style/nsStyleAnimation.cpp b/layout/style/nsStyleAnimation.cpp
>+static nsStringBuffer*
>+GetURIAsUtf16StringBuffer(nsIURI* aUri)

I think the return-type here should be "already_AddRefed<nsStringBuffer>" instead of "nsStringBuffer*", to make the addrefing responsibilities more explicit.  (See Bug 615977 -- if dbaron agrees with me there, then your patch here should layer on top of that one.)

>@@ -2602,26 +2614,45 @@ nsStyleAnimation::ExtractComputedValue(n
>       if (paint.mType == eStyleSVGPaintType_None) {
>         aComputedValue.SetNoneValue();
>         return PR_TRUE;
>       }
>-      return PR_FALSE;
>+      NS_ABORT_IF_FALSE(paint.mType == eStyleSVGPaintType_Server,
>+          "Unexpected SVG paint type");
>+      NS_ABORT_IF_FALSE(paint.mPaint.mPaintServer, "Null paint server");

So, right now the "fallback" behavior (for any unexpected "paint.mType" values) is this new paint-server behavior.

I'd prefer that we make the fallback behavior be the much-safer "none" case. In the unexpected situation that paint.mType is a bogus value (or if we add a new PaintType value and forget to update this code), the "none" behavior will be much safer, since it doesn't touch |paint|'s data at all.

So you'd want to structure it like this (starting just after the "_Color" case's closing-paren):
   if (paint.mType == eStyleSVGPaintType_Server) {
     // NEW CODE ALL GOES HERE
     return PR_TRUE 
   }
   NS_ABORT_IF_FALSE(paint.mType == eStyleSVGPaintType_None,
                    "Unexpected SVG paint type");
   aComputedValue.SetNoneValue();
   return PR_TRUE;

>+      nsRefPtr<nsStringBuffer> uriAsStringBuffer =
>+        getter_AddRefs(GetURIAsUtf16StringBuffer(paint.mPaint.mPaintServer));

getter_AddRefs will no longer be needed here, once GetURIAsUtf16StringBuffer returns an already_AddRefed pointer (per my suggestion above).

r=dholbert with that.

Comment 14

[Brian Birtles (:birtles)]

Attachment: Patch v1b; r=dholbert

Address review feedback.

(In reply to comment #13)
> >+++ b/layout/reftests/svg/smil/anim-paintserver-1.svg
> >+  <!-- 2. Get paint server. (We're looking for a code path that will fetch the
> 
> Nit: This comment needs a ")" close-paren somewhere. :)
Fixed.

> >+  <!-- 6. Check by-addition behaves (3): from-by paint server -->
> >+  <rect x="200" y="100" width="100" height="100" fill="#0f0">
> >+    <animate attributeName="fill" from="url(#lime)" by="url(#red)"/>
> >+  </rect>
> 
> We probably want from="url(#red)" (or some other non-lime value), becuase the
> point here is that neither the from value nor the by value should be applied.

Yep, definitely. This was a last minute change.

> >+  <!-- 7. Check that by-addition without a paint server is ok though -->
> >+  <rect x="0" y="200" width="100" height="100" fill="red">
> >+    <animate attributeName="fill" from="#0f0" by="#00f"/>
> >+  </rect>
> 
> So my first reaction to this was: "Wait -- doesn't this end up at #0ff instead
> of lime?"
> 
> But then I realized that it's got an indefinite duration (by default), so I
> guess it just sets the 'from' value (0f0 = lime) and never adds anything.
> 
> Might be good to add a brief comment there to explain that, so that other
> people less familiar with the ins & outs of SMIL can tell what's going on.

Added and also added dur="indefinite" to make it clearer.

> >+       value instead. (SMIL 3 12.6.3 says, "[The accumulate] attribute is
> >+       ignored if the target attribute does not support additive animation." -->
> >+  <rect x="100" y="200" width="100" height="100" fill="red">
> >+    <animate attributeName="fill" values="url(#lime)" additive="sum"/>
> 
> I think you meant s/accumulate/additive/ there -- your test never uses
> 'accumulate', though the next few examples do use 'additive'.

Umm, yep :)

> >diff --git a/layout/style/nsStyleAnimation.cpp b/layout/style/nsStyleAnimation.cpp
> >+static nsStringBuffer*
> >+GetURIAsUtf16StringBuffer(nsIURI* aUri)
> 
> I think the return-type here should be "already_AddRefed<nsStringBuffer>"
> instead of "nsStringBuffer*", to make the addrefing responsibilities more
> explicit.  (See Bug 615977 -- if dbaron agrees with me there, then your patch
> here should layer on top of that one.)

Now layered on top as suggested. I'll wait for that bug to land before trying to land this (assuming both that patch and this are approved).

> >@@ -2602,26 +2614,45 @@ nsStyleAnimation::ExtractComputedValue(n
> >       if (paint.mType == eStyleSVGPaintType_None) {
> >         aComputedValue.SetNoneValue();
> >         return PR_TRUE;
> >       }
> >-      return PR_FALSE;
> >+      NS_ABORT_IF_FALSE(paint.mType == eStyleSVGPaintType_Server,
> >+          "Unexpected SVG paint type");
> >+      NS_ABORT_IF_FALSE(paint.mPaint.mPaintServer, "Null paint server");
> 
> So, right now the "fallback" behavior (for any unexpected "paint.mType" values)
> is this new paint-server behavior.
> 
> I'd prefer that we make the fallback behavior be the much-safer "none" case.

Good call, fixed.

Requesting approval to land. Our current implementation is incomplete and this feature is already being used in the wild (comment 6). Includes tests.

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 494906 [details] [diff] [review]
Patch v1b; r=dholbert

>+  <!-- 7. Check that by-addition without a paint server is ok though.
>+       (Since we animation has indefinite simple duration we'll never get past
>+       the first value which is is lime green.) -->

s/since we/since our/ ? :)

> > I'd prefer that we make the fallback behavior be the much-safer "none" case.
> Good call, fixed

Looks like this still isn't fixed in the latest attachment, actually. (perhaps you forgot a qrefresh?)

Comment 16

[Brian Birtles (:birtles)]

Attachment: Patch v1c; r=dholbert

(In reply to comment #15)
...
> >+       (Since we animation has indefinite simple duration we'll never get past
> >+       the first value which is is lime green.) -->
> 
> s/since we/since our/ ? :)
Umm, yeah :) "Since the"

> > > I'd prefer that we make the fallback behavior be the much-safer "none" case.
> > Good call, fixed
> 
> Looks like this still isn't fixed in the latest attachment, actually. (perhaps
> you forgot a qrefresh?)

Worse, forgot a ":w" :(

Comment 17

[Brian Birtles (:birtles)]

Requesting approval to land. Our current implementation is incomplete and this
feature is already being used in the wild (comment 6). Parity Opera and WebKit. Includes tests.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

-      // FIXME: At some point in the future, we should animate gradients.

I think this is still a valid comment.

+        NS_ABORT_IF_FALSE(paint.mPaint.mPaintServer, "Null paint server");
+        // Play it safe since nsStyleSVGPaint doesn't give us any guarantees
+        if (!paint.mPaint.mPaintServer)
+          return PR_FALSE;

I'm confused. If nsStyleSVGPaint doesn't give us any guarantees, why are we aborting?

Should dbaron have reviewed this?

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to comment #18)
> I'm confused. If nsStyleSVGPaint doesn't give us any guarantees, why are we
> aborting?

Mm, good point.

> Should dbaron have reviewed this?

Sorry, you're right - he should take a look at it before it lands. I thought about suggesting that as part of my review comments, but I didn't end up mentioning it because I've written/reviewed a lot of the code there and was pretty comfortable with how the patch looked.

But I think dbaron's more familiar with how nsStyleSVGPaint works than I am, and this could have implications for CSS transitions, too (though probably not because we still can't interpolate paint servers?), so it'd definitely be good to get his review on this.

Comment 20

[Brian Birtles (:birtles)]

(In reply to comment #18)
> -      // FIXME: At some point in the future, we should animate gradients.
> 
> I think this is still a valid comment.

Why is that? This comment is part of the eStyleAnimType_PaintServer block which, so far as I can tell, we only use for SVG paint servers (and not CSS gradients). As per comment 1, I don't think SVG expects us to interpolate smoothly between paint servers but simply treat them like strings.

This is based on the table here: http://dev.w3.org/SVG/profiles/1.1F2/publish/animate.html#AnimationAttributesAndProperties

There's no category for "interpolatable" but non-colour paint values are clearly not additive and I'm taking this to mean not interpolatable either in this context. Opera and WebKit do likewise.

As per comment 1, if we want to interpolate gradient stops we can do that by attaching <animate> elements to the <stop> elements to move them around and change colours and the like.

So in this part of the code we're handling an nsStyleSVGPaint which has three possible types:
eStyleSVGPaintType_None,
eStyleSVGPaintType_Color,
eStyleSVGPaintType_Server

I think the comment was added since we weren't handling the third case but we now do.

> +        NS_ABORT_IF_FALSE(paint.mPaint.mPaintServer, "Null paint server");
> +        // Play it safe since nsStyleSVGPaint doesn't give us any guarantees
> +        if (!paint.mPaint.mPaintServer)
> +          return PR_FALSE;
> 
> I'm confused. If nsStyleSVGPaint doesn't give us any guarantees, why are we
> aborting?

The thought was, we're not ever expecting to have an nsStyleSVGPaint of type eStyleSVGPaintType_Server where the paint server is nsnull. However, it's possible that code might be added in the future that produces this case without it being detected upstream since nsStyleSVGPaint doesn't do any of these kind of consistency checks. So if we failed to test this new code path in combination with animation we'd end up crashing on a production build.

So I was looking for behaviour that aborts on debug (since such a case is probably a bug and we want to know about it as soon as possible) but fails safely on release.

Is an NS_WARNING better?

> Should dbaron have reviewed this?

That'd be great!

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #20)
> So in this part of the code we're handling an nsStyleSVGPaint which has three
> possible types:
> eStyleSVGPaintType_None,
> eStyleSVGPaintType_Color,
> eStyleSVGPaintType_Server
> 
> I think the comment was added since we weren't handling the third case but we
> now do.

OK.

> Is an NS_WARNING better?

Yes.

Comment 22

[Brian Birtles (:birtles)]

Attachment: Patch v1d; r=dholbert

(In reply to comment #21)
> > Is an NS_WARNING better?
> 
> Yes.

Fixed.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 495738 [details] [diff] [review]
Patch v1d; r=dholbert

I was thinking of suggesting the following, but I think I've decided things are ok as is:  instead of making these eUnit_CSSValuePair, could you add a new unit in nsStyleAnimation::Unit for paint servers (still nsCSSValuePair*), and make that unit not interpolatable (i.e., return false from AddWeighted and ComputeDistance, but with the addition of the FIXME comment that you removed).  You'll need to add additional cases (falling through to the current eUnit_CSSValuePair one in all except UncomputeValue, I think).

However, could you please readd the FIXME in ComputeDistance and AddWeighted, and fix up the comment in UncomputeValue?

This would also be a bit cleaner if I got http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/01874e991681/unapplied.pass-cssvalue-url-through landed.

Comment 24

[Brian Birtles (:birtles)]

(In reply to comment #23)
> However, could you please readd the FIXME in ComputeDistance and AddWeighted,
> and fix up the comment in UncomputeValue?

Thanks for the review. I'm a little bit unsure about this comment though.

This patch doesn't remove any FIXME from ComputeDistance or AddWeighted, nor does it touch UncomputeValue so I'm not quite sure what's required here.

Regarding the FIXME that this patch removes from ExtractComputedValue, I thought that FIXME was no longer needed as discussed in the first part of comment 20. Please let me know if that is incorrect.

Comment 25

[Brian Birtles (:birtles)]

Attachment: Patch v1e; r=dholbert,dbaron

Fix compile error on non-Windows platforms.

I'm going to go ahead and re-request landing approval now that this has r=dbaron and since the only review feedback pertains to comments.

Rationale is as with comment 6: Our current implementation is incomplete and this feature is already being used in the wild (comment 6). Parity Opera and WebKit. Includes tests.

Comment 26

[Brian Birtles (:birtles)]

(In reply to comment #25)
> Fix compile error on non-Windows platforms.

Note that comparing patch v1d and v1e with bugzilla shows now difference but in fact there is a difference. I've changed one line from:
nsAutoPtr<nsCSSValuePair> pair = new nsCSSValuePair;
to:
nsAutoPtr<nsCSSValuePair> pair(new nsCSSValuePair);

(For anyone who's curious).

Comment 27

[Brian Birtles (:birtles)]

I've pushed the patch as is: http://hg.mozilla.org/mozilla-central/rev/dd88bfc07421

The only outstanding review feedback pertains to comments. Once I hear back from dbaron I'll push a follow-up comments-only patch. Until then I'm leaving this bug open.

Comment 28

[Robert Longson [:longsonr]]

If it's fixed, it's fixed :-). You can push comment only changes anytime.