Bug 57085 – SVG no fallback to discrete animation on attribute 'values' for SVGString

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Wataru Shimizu]

Created attachment 86898 [details]
font-style animation without calcMode attribute

Attahced file is the test case (simplified version of
LayoutTests/W3C-SVG-1.1/animate-elem-46-t.svg).

I think changes of font-style and translate should occur simultaneously as follows (column1: second, 2: translate, 3: font-style).
0.0-1.0s  0 normal
1.0-2.0s 30 italic
2.0-3.0s 60 normal
3.0-     60 normal

However, the font-style animation seems incorrect. Here is the result of my investigation.
0.0-1.0s  0 normal
1.0-1.5s 30 normal
1.5-2.0s 30 italic
2.0-3.0s 60 italic
3.0-     60 normal

If calcMode="discrete" is explicitly added to the <animate> element, it works well.

I saw this problem on nightly builds (Win: r81845 and Mac: r81829).

[Dirk Schulze]

(In reply to comment #0)
> Created an attachment (id=86898) [details]
> font-style animation without calcMode attribute
> 
> Attahced file is the test case (simplified version of
> LayoutTests/W3C-SVG-1.1/animate-elem-46-t.svg).
> 
> I think changes of font-style and translate should occur simultaneously as follows (column1: second, 2: translate, 3: font-style).
> 0.0-1.0s  0 normal
> 1.0-2.0s 30 italic
> 2.0-3.0s 60 normal
> 3.0-     60 normal
> 
> However, the font-style animation seems incorrect. Here is the result of my investigation.
> 0.0-1.0s  0 normal
> 1.0-1.5s 30 normal
> 1.5-2.0s 30 italic
> 2.0-3.0s 60 italic
> 3.0-     60 normal
> 
> If calcMode="discrete" is explicitly added to the <animate> element, it works well.
> 
> I saw this problem on nightly builds (Win: r81845 and Mac: r81829).

Yes, I can confirm this bug. I am working on a fix but it might take some time, since we have to fix other things first on animation. But IIRC the svg-wg decided to change this test in the meantime, since more than one viewer fail on this tests. Also some implementers doubt that this case is specified enough by the specification. If you put calcMode="discrete" on the certain subtests, the test should work.

[Dirk Schulze]

Created attachment 97993 [details]
Patch

[WebKit Review Bot]

Comment on attachment 97993 [details]
Patch

Attachment 97993 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8913954

New failing tests:
svg/W3C-SVG-1.1/animate-elem-31-t.svg

[WebKit Review Bot]

Created attachment 97996 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Nikolas Zimmermann]

Comment on attachment 97993 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97993&action=review

I still have a question:

> Source/WebCore/ChangeLog:11
> +        
> +        The patch also changes behavior on String animation. The animation code doesn't handle inheritance

s/on/for/

> Source/WebCore/svg/SVGAnimateElement.h:62
> -    
> +

Unrelated but ok.

> Source/WebCore/svg/SVGAnimationElement.cpp:492
> +        if (attributeType == AnimatedBoolean
> +            || attributeType == AnimatedEnumeration
> +            || attributeType == AnimatedPreserveAspectRatio
> +            || attributeType == AnimatedString)
> +            calcMode = CalcModeDiscrete;

How often is this called? If that's hot, you could create a static HashSet and add "static inline bool useStringAnimationForAttributeType(FooEnumType)" ?

[Dirk Schulze]

(In reply to comment #5)
> (From update of attachment 97993 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97993&action=review
> 
> > Source/WebCore/svg/SVGAnimationElement.cpp:492
> > +        if (attributeType == AnimatedBoolean
> > +            || attributeType == AnimatedEnumeration
> > +            || attributeType == AnimatedPreserveAspectRatio
> > +            || attributeType == AnimatedString)
> > +            calcMode = CalcModeDiscrete;
> 
> How often is this called? If that's hot, you could create a static HashSet and add "static inline bool useStringAnimationForAttributeType(FooEnumType)" ?

Once per animateElement.

[Rob Buis]

Comment on attachment 97993 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97993&action=review

Overall LGTM but the HashSet thing needs to be cleared up...

> Source/WebCore/ChangeLog:12
> +        for strings anymore. Let CSS parser handle inheritance itself if we don't have proceeding animations.

You may want to rephrase the "proceeding animations" bit.

[Rob Buis]

Comment on attachment 97993 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97993&action=review

Overall LGTM but the HashSet thing needs to be cleared up... After talking with Dirk, this would be a small win if any and more confusing code. So only the ChangeLog entry can be a bit improved, but r=me.

>> Source/WebCore/ChangeLog:12
>> +        for strings anymore. Let CSS parser handle inheritance itself if we don't have proceeding animations.
> 
> You may want to rephrase the "proceeding animations" bit.

You may want to rephrase the "proceeding animations" bit.

[Dirk Schulze]

Committed r89367: <http://trac.webkit.org/changeset/89367>

[Nikolas Zimmermann]

(In reply to comment #9)
> Committed r89367: <http://trac.webkit.org/changeset/89367>

Dirk, you did not fix the HashSet problem that Rob and me moaned about :-)
Can you please fix it in a follow-up?

[Nikolas Zimmermann]

(In reply to comment #8)
> (From update of attachment 97993 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97993&action=review
> 
> Overall LGTM but the HashSet thing needs to be cleared up... After talking with Dirk, this would be a small win if any and more confusing code. So only the ChangeLog entry can be a bit improved, but r=me.
> 

Moving code to a helper function is always a benefit IMHO. It makes Dirks change easier to read, as teh set of ifs is replaced with a descriptive function call.

I just read that it's called once per animation. So I think it's fine to leave it as-is for now.