Bug 63553 – SVG animation fill="freeze" doesn't set baseVal to current animVal if animation stops before reaching the end

• Component: SVG
• Status: RESOLVED FIXED

[Dirk Schulze]

Created attachment 98958 [details]
Animation aborted by clicking on the rect

SVG animation fill="freeze" doesn't set baseVal to current animVal if animation stops before reaching the end. We just set baseVal to current animation value if 100% of the animation was reached. If we stop earlier the target attribute gets reset to its original value. We might want to check for fill="freeze" if 'end' of animation element fires.

[Dirk Schulze]

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

[Dirk Schulze]

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

[Dirk Schulze]

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

[Young Han Lee]

Created attachment 99330 [details]
Patch

[Young Han Lee]

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.

[Dirk Schulze]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Young Han Lee]

(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.

[Young Han Lee]

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.

[Young Han Lee]

Created attachment 100227 [details]
Patch

[Dirk Schulze]

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.

[Young Han Lee]

Created attachment 101117 [details]
Patch

[Young Han Lee]

(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!

[Dirk Schulze]

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.

[Young Han Lee]

Created attachment 101494 [details]
Patch

[Young Han Lee]

(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 :)

[WebKit Review Bot]

Comment on attachment 101494 [details]
Patch

Clearing flags on attachment: 101494

Committed r93517: <http://trac.webkit.org/changeset/93517>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Peter Kasting]

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.

[Peter Kasting]

Reverted r93517 for reason:

Breaks GTK Linux and Chromium Win

Committed r93532: <http://trac.webkit.org/changeset/93532>

[Dirk Schulze]

(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?

[Young Han Lee]

> 
> 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.

[Peter Kasting]

(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.

[Young Han Lee]

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");

[Dirk Schulze]

Comment on attachment 105436 [details]
Patch

Change in the test makes sense. r=me.

[WebKit Review Bot]

Comment on attachment 105436 [details]
Patch

Clearing flags on attachment: 105436

Committed r93938: <http://trac.webkit.org/changeset/93938>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.