Bug 63553

*** Bug 26008 has been marked as a duplicate of this bug. ***

*** Bug 63550 has been marked as a duplicate of this bug. ***

*** Bug 63547 has been marked as a duplicate of this bug. ***

Created attachment 99330 [details] Patch

The function for calculating animation progress is causing this issue. It returns 100% when the animation stops before reaching the end. It should return the progress value of the stop point.

Comment on attachment 99330 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99330&action=review > Source/WebCore/ChangeLog:10 > + When the animation stops before reaching the end, the elapsed time is bigger than > + m_intervalEnd and smaller than simpleDuration. In that case, the progress of > + the animation doesn't 100%, it should be the value of the stop point. Can you go a bit more into detail? I don't understand what you try to fix. What do the timings mean? To review the patch I have to know more about what we try to do here. > Source/WebCore/svg/animation/SVGSMILElement.cpp:829 > + activeTime = m_intervalEnd; Why do you set activeTime here, we haven't done it before.

Comment on attachment 99330 [details] Patch Attachment 99330 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8959720 New failing tests: svg/custom/animate-path-morphing.svg svg/custom/animate-path-discrete.svg

Created attachment 99331 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

(In reply to comment #6) > (From update of attachment 99330 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=99330&action=review > > > Source/WebCore/ChangeLog:10 > > + When the animation stops before reaching the end, the elapsed time is bigger than > > + m_intervalEnd and smaller than simpleDuration. In that case, the progress of > > + the animation doesn't 100%, it should be the value of the stop point. > > Can you go a bit more into detail? I don't understand what you try to fix. What do the timings mean? To review the patch I have to know more about what we try to do here. I'm sorry for the undetailed comment. I believe this phenomenon is caused by the miscalculated progress value. The animated position of an SVGElement is updated in SMILTimeContainer::updateAnimations() in every frame. When an animation with fill=freeze option stops before reaching the end, the animated position should be updated to somewhere in the animation route, but now it is updated to the end of the animation. The newly updated position is come from SVGSMILElement::progress(), in which the position is calculated based on the progress value returned from SVGSMILElement::calculateAnimationPercentAndRepeat(). The calculateAnimationPercentAndRepeat() returns 100% in the attached testcase, so consequently the last animated position is updated to the end of the animation. I think the calculateAnimationPercentAndRepeat() have to return the progress of the stopping point on the animation, not 100%. > > > Source/WebCore/svg/animation/SVGSMILElement.cpp:829 > > + activeTime = m_intervalEnd; > > Why do you set activeTime here, we haven't done it before. This change makes the progress value is calculated with the stopping point when the elapsed value exceeds the stopping point. Thus, the function will return the progress of the stopping point if the animation stops before the end. But I missed something in there, the activeTime should be m_intervalEnd - m_intervalBegin. I will upload the new patch if you agree with the approach I wrote.

Comment on attachment 99330 [details] Patch I found a problem in this patch, so I will upload a new one again maybe in this weekend.

Created attachment 100227 [details] Patch

Comment on attachment 100227 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=100227&action=review I'm confused about your ChangeLog entry since it is the same entry like on another bug. r- because of the layouttest. > LayoutTests/svg/animations/script-tests/animate-end-attribute.js:33 > + ok = isCloseEnough(rect.x.baseVal.value, 200, 0.2); > + if (ok) > + testPassed("rect.x.baseVal.value is almost 200"); > + else > + testFailed("rect.x.baseVal.value is NOT almost 200, as expected"); Please don't do it manually we have a function called shouldBeCloseEnough() with three arguments. Take a look at other tests. > LayoutTests/svg/animations/script-tests/animate-end-attribute.js:41 > + ok = isCloseEnough(rect.x.baseVal.value, 200, 0.2); > + if (ok) > + testPassed("rect.x.baseVal.value is almost 200"); > + else > + testFailed("rect.x.baseVal.value is NOT almost 200, as expected"); Ditto. > LayoutTests/svg/animations/script-tests/animate-end-attribute.js:49 > + ["animation", 2.0, "rect", sample2], > + ["animation", 3.0, "rect", sample3] You can reuse sample3 for 2s. Can you please add an animation step at 0s to verify that we start from baseVal? > Source/WebCore/ChangeLog:9 > + calculateAnimationPercentAndRepeat() is returning 1, which means 100%, whenever > + elapsed >= m_intervalEnd, but this is wrong because m_intervalEnd can be before Hm, wasn't it fixed with bug 63911? > Source/WebCore/svg/animation/SVGSMILElement.cpp:833 > + return 1.f; just use 1, no .f after numbers.

Created attachment 101117 [details] Patch

(In reply to comment #12) > (From update of attachment 100227 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=100227&action=review > > I'm confused about your ChangeLog entry since it is the same entry like on another bug. r- because of the layouttest. Would you check this again? This changeLog could be difficult to understand, I will rewrite the log if you meant it, but the log is not the same with any other bug's changeLog. Did you mean it is the same with the changeLog on the first patch I uploaded in this bug? > > > LayoutTests/svg/animations/script-tests/animate-end-attribute.js:33 > > + ok = isCloseEnough(rect.x.baseVal.value, 200, 0.2); > > + if (ok) > > + testPassed("rect.x.baseVal.value is almost 200"); > > + else > > + testFailed("rect.x.baseVal.value is NOT almost 200, as expected"); > > Please don't do it manually we have a function called shouldBeCloseEnough() with three arguments. Take a look at other tests. Done. > > > LayoutTests/svg/animations/script-tests/animate-end-attribute.js:41 > > + ok = isCloseEnough(rect.x.baseVal.value, 200, 0.2); > > + if (ok) > > + testPassed("rect.x.baseVal.value is almost 200"); > > + else > > + testFailed("rect.x.baseVal.value is NOT almost 200, as expected"); > > Ditto. Done. > > > LayoutTests/svg/animations/script-tests/animate-end-attribute.js:49 > > + ["animation", 2.0, "rect", sample2], > > + ["animation", 3.0, "rect", sample3] > > You can reuse sample3 for 2s. Can you please add an animation step at 0s to verify that we start from baseVal? Done. > > > Source/WebCore/ChangeLog:9 > > + calculateAnimationPercentAndRepeat() is returning 1, which means 100%, whenever > > + elapsed >= m_intervalEnd, but this is wrong because m_intervalEnd can be before > > Hm, wasn't it fixed with bug 63911? No, bug 63911 doesn't fix a problem related to this bug. The function is still returning 1, after the bug 63911 is applied. > > > Source/WebCore/svg/animation/SVGSMILElement.cpp:833 > > + return 1.f; > > just use 1, no .f after numbers. Done. Thanks for the comments!

Comment on attachment 101117 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=101117&action=review Sorry for the delay. I'm a bit busy at the moment. r- to get it from pending review list. Needs more discussion. > Source/WebCore/ChangeLog:10 > + elapsed >= m_intervalEnd, but this is wrong because m_intervalEnd can be before > + the end of the animation. (e.g. begin="0s" end="2s" dur="3s") I'm confused about this interpretation. For me the ending of the interval is at 3s, even if we end the animation before that time. What about this example: <rect width="0" height="100"> <animate attributeName="width" from="0" to="300" begin="0s" dur="3s" end="2s" repeat="3"/> </rect> With your interpretation of interval end we would repeat the animation after 2s, not after 3s. I'd interprete the animation with start every 3 seconds but animate just 2 seconds. Maybe I just misunderstand your intention. > Source/WebCore/ChangeLog:14 > + This change makes the function return 100% only when > + m_intervalEnd - m_intervalBegin == simpleDuration, which means the animation stops > + at the end of the animation. This also does some refactoring to remove unnecessary this sounds more reasonable. > Source/WebCore/ChangeLog:17 > + m_intervalEnd - m_intervalBegin. I believe this is more intuitive because the animation > + cannot progress beyond it. Can you add an ASSERT if the animation shouldn't proceed beyond that time? > Source/WebCore/svg/animation/SVGSMILElement.cpp:826 > + SMILTime activeTime = min(elapsed, m_intervalEnd) - m_intervalBegin; Still unsure about this.The variable is called activeTime. Is intervalBegin the relative time from the current repeat level or the absolute time like elapsedTime? It must be the abs time, otherwise it wouldn't make sense anyway. And then activeTime would be the relative time from the current repeated animation. Anyway why do you take intervalEnd if elapsedTime is bigger than elapsedTime? We should differ between the end of animation process (when end=.. is set) and the end of the current interval. Otherwise the example above would repeat every 2s instead of every 3s.

Created attachment 101494 [details] Patch

(In reply to comment #15) > (From update of attachment 101117 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=101117&action=review > > Sorry for the delay. I'm a bit busy at the moment. r- to get it from pending review list. Needs more discussion. No problem. I appreciate for the detailed comments :) > > > Source/WebCore/ChangeLog:10 > > + elapsed >= m_intervalEnd, but this is wrong because m_intervalEnd can be before > > + the end of the animation. (e.g. begin="0s" end="2s" dur="3s") > > I'm confused about this interpretation. For me the ending of the interval is at 3s, even if we end the animation before that time. What about this example: > <rect width="0" height="100"> > <animate attributeName="width" from="0" to="300" begin="0s" dur="3s" end="2s" repeat="3"/> > </rect> > > With your interpretation of interval end we would repeat the animation after 2s, not after 3s. I'd interprete the animation with start every 3 seconds but animate just 2 seconds. Maybe I just misunderstand your intention. The interval indicates a time from the beginning to end of the animation, not one iteration. The important thing is when the animation ends. Let's see some examples. <animate attributeName="width" from="0" to="300" begin="0s" dur="3s" end="2s" repeat="3"/> You misunderstood the end attribute at this example. This animation doesn't repeat. The end attribute means absolute end time of the animation, not the end of the iteration. (You can find similar example with an explanation in here[1].) So, the end of the interval is at 2s in this case. <animate attributeName="width" from="0" to="300" begin="0s" dur="3s" repeat="3"/> In this example, the end of the interval is at 9s because the animation ends at 9s. <animate attributeName="width" from="0" to="300" begin="0s;10s" dur="3s" repeat="3"/> Here is the example have more than one interval. As the animation has two begin values, it restarts after the first end. The end of the first interval is at 9s and the end of the second interval is at 19s. Here[2] is an explanation of the begin-time list and interval. Now, looking my comments. + calculateAnimationPercentAndRepeat() is returning 1, which means 100%, whenever + elapsed >= m_intervalEnd, but this is wrong because m_intervalEnd can be before + the end of the animation. (e.g. begin="0s" end="2s" dur="3s") Like examples given above, m_intervalEnd is 2s in this case, so when the elapsed time becomes bigger than 2, the function returns 1, but it should be 0.66. And the comment has fixed, as m_intervalEnd is just the end of the animation. [1] http://www.w3.org/TR/smil-animation/#EndActive [2] http://www.w3.org/TR/smil-animation/#Timing-EvaluationOfBeginEndTimeLists > > > Source/WebCore/ChangeLog:14 > > + This change makes the function return 100% only when > > + m_intervalEnd - m_intervalBegin == simpleDuration, which means the animation stops > > + at the end of the animation. This also does some refactoring to remove unnecessary > > this sounds more reasonable. > > > Source/WebCore/ChangeLog:17 > > + m_intervalEnd - m_intervalBegin. I believe this is more intuitive because the animation > > + cannot progress beyond it. > > Can you add an ASSERT if the animation shouldn't proceed beyond that time? This change has disappeared. > > > Source/WebCore/svg/animation/SVGSMILElement.cpp:826 > > + SMILTime activeTime = min(elapsed, m_intervalEnd) - m_intervalBegin; > > Still unsure about this.The variable is called activeTime. Is intervalBegin the relative time from the current repeat level or the absolute time like elapsedTime? It must be the abs time, otherwise it wouldn't make sense anyway. And then activeTime would be the relative time from the current repeated animation. Anyway why do you take intervalEnd if elapsedTime is bigger than elapsedTime? We should differ between the end of animation process (when end=.. is set) and the end of the current interval. Otherwise the example above would repeat every 2s instead of every 3s. Ditto. I removed the refactoring parts from the patch as it breaks some concepts you told and caused confusion. I should have done this earlier :)

Comment on attachment 101494 [details] Patch Clearing flags on attachment: 101494 Committed r93517: <http://trac.webkit.org/changeset/93517>

All reviewed patches have been landed. Closing bug.

This patch seems to have broken Chromium Win and GTK Linux. Both are failing the "close enough to 200" test. Chromium Win: 196.89999389648438 GTK Linux 32-bit: 196.38299560546875 GTK Linux 64-bit: 197.6179962158203 There are also downstream failures on the Chromium bots: XP, Vista: 198.39999389648438 Windows 7: 196.8000030517578 Linux: 192.15499877929688 Mac 10.6: 191.6490020751953 I can't tell if the test is being too stringent in its "close enough" check, the code is not doing the correct rounding, the values are being checked at the wrong time, etc. I'm going to roll back this change.

Reverted r93517 for reason: Breaks GTK Linux and Chromium Win Committed r93532: <http://trac.webkit.org/changeset/93532>

(In reply to comment #20) > This patch seems to have broken Chromium Win and GTK Linux. Both are failing the "close enough to 200" test. > > Chromium Win: 196.89999389648438 > GTK Linux 32-bit: 196.38299560546875 > GTK Linux 64-bit: 197.6179962158203 > > There are also downstream failures on the Chromium bots: > > XP, Vista: 198.39999389648438 > Windows 7: 196.8000030517578 > Linux: 192.15499877929688 > Mac 10.6: 191.6490020751953 > > I can't tell if the test is being too stringent in its "close enough" check, the code is not doing the correct rounding, the values are being checked at the wrong time, etc. > > I'm going to roll back this change. Hi Peter. Which test (tests) failed after committing Young Hans patch?

> > Hi Peter. Which test (tests) failed after committing Young Hans patch? It must be the new test I added in this patch. I just reproduced the failure in my GTK port. I don't know why it is failing as it passed when I upload the patch. There could be some changes landed recently which affect the test. Anyway, I will find out the problem within this week.

(In reply to comment #23) > > Hi Peter. Which test (tests) failed after committing Young Hans patch? > > It must be the new test I added in this patch. Yes, sorry I wasn't clearer.

Created attachment 105436 [details] Patch animate.setAttribute("begin", "click"); animate.setAttribute("end", "2s"); The test was wrong. The SVG animation in the test started when a 'click' event occurs and ended at '2s'. It was a mistake because I actually wanted to stop the animation 2 seconds after the animation starts. I changed the test as follows. animate.setAttribute("begin", "click"); animate.setAttribute("end", "click+2s");

Comment on attachment 105436 [details] Patch Change in the test makes sense. r=me.

Comment on attachment 105436 [details] Patch Clearing flags on attachment: 105436 Committed r93938: <http://trac.webkit.org/changeset/93938>

All reviewed patches have been landed. Closing bug.