Bug 57085 - SVG no fallback to discrete animation on attribute 'values' for SVGString
Summary: SVG no fallback to discrete animation on attribute 'values' for SVGString
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Dirk Schulze
URL:
Keywords:
Depends on:
Blocks: 41761
  Show dependency treegraph
 
Reported: 2011-03-24 23:51 PDT by Wataru Shimizu
Modified: 2011-06-21 12:44 PDT (History)
6 users (show)

See Also:


Attachments
font-style animation without calcMode attribute (370 bytes, image/svg+xml)
2011-03-24 23:51 PDT, Wataru Shimizu
no flags Details
Patch (49.71 KB, patch)
2011-06-21 08:51 PDT, Dirk Schulze
rwlbuis: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.48 MB, application/zip)
2011-06-21 09:13 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Wataru Shimizu 2011-03-24 23:51:33 PDT
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).
Comment 1 Dirk Schulze 2011-03-25 00:12:03 PDT
(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.
Comment 2 Dirk Schulze 2011-06-21 08:51:53 PDT
Created attachment 97993 [details]
Patch
Comment 3 WebKit Review Bot 2011-06-21 09:13:12 PDT
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
Comment 4 WebKit Review Bot 2011-06-21 09:13:17 PDT
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
Comment 5 Nikolas Zimmermann 2011-06-21 09:45:05 PDT
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)" ?
Comment 6 Dirk Schulze 2011-06-21 09:47:13 PDT
(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.
Comment 7 Rob Buis 2011-06-21 10:33:18 PDT
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.
Comment 8 Rob Buis 2011-06-21 10:57:27 PDT
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.
Comment 9 Dirk Schulze 2011-06-21 11:15:15 PDT
Committed r89367: <http://trac.webkit.org/changeset/89367>
Comment 10 Nikolas Zimmermann 2011-06-21 12:42:43 PDT
(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?
Comment 11 Nikolas Zimmermann 2011-06-21 12:44:15 PDT
(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.