Bug 57085

Summary: SVG no fallback to discrete animation on attribute 'values' for SVGString
Product: WebKit Reporter: Wataru Shimizu <shimizu.wataru>
Component: SVGAssignee: Dirk Schulze <krit>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, fujisawa.jun, imaizumi.satoshi, krit, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows 7   
Bug Depends on:    
Bug Blocks: 41761    
Attachments:
Description Flags
font-style animation without calcMode attribute
none
Patch
rwlbuis: review+, webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-02 none

Wataru Shimizu
Reported 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).
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-
DetailsFormatted DiffDiff
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
View All Add attachment proposed patch, testcase, etc.
Dirk Schulze
Comment 1 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.
Dirk Schulze
Comment 2 2011-06-21 08:51:53 PDT
Created attachment 97993 [details] Patch
WebKit Review Bot
Comment 3 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
WebKit Review Bot
Comment 4 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
Nikolas Zimmermann
Comment 5 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)" ?
Dirk Schulze
Comment 6 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.
Rob Buis
Comment 7 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.
Rob Buis
Comment 8 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.
Dirk Schulze
Comment 9 2011-06-21 11:15:15 PDT
Committed r89367: <http://trac.webkit.org/changeset/89367>
Nikolas Zimmermann
Comment 10 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?
Nikolas Zimmermann
Comment 11 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.
Note You need to log in before you can comment on or make changes to this bug.