Reject spline animations if the last value of keyTimes is not 1 [chromium/src : main]
YISI YU (Gerrit)
YISI YU voted and added 7 comments![]( "Open in Gerrit")
Votes added by YISI YU
| Auto-Submit | +1 |
| Commit-Queue | +1 |
7 comments
Hello Fredrik. Hello Vacation. Sorry for the delay with this CL. It is now ready for review. PTAL!
The root cause of this issue is that we omitted the check for the abnormal case where the last value of keyTimes is not 1 for spline animations, which leads to us getting an incorrect keyTimes index and further causes from_percent and to_percent to be equal.
Such cases can be avoided by improving the check for keyTimes.
FIXUP: DCHECK when calculate svg animationYISI YUThis is not a great prefix. How about a more descriptive description, something along the lines of:
Fix handling of zero-length keyTimes intervals
?
Done
? std::numeric_limits<float>::epsilon()YISI YU1. The sign may change here if `from` is less than `to`. Should we keep the sign?
2. In extreme cases, the result can be infinite. Do we need the result to be finite?Do you know if we already have a function in css/svg code for percent or float interpolation?
Fredrik SöderquistThansk for your reply.
1. It appears that `to_percent` will always be greater than `from_percent`, because when parsing `keyTimes`, [ParseKeyTimes](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/svg/svg_animation_element.cc;l=193;bpv=1;bpt=1?q=ParseKeyTimes&ss=chromium) ensures that the sequence is strictly increasing.
2. CSS Keyframes has a similar issue, but the CSS specification stipulates that [all keyframe groups will be merged](https://developer.mozilla.org/en-US/docs/Web/SVG/Reference/Attribute/keyPoints). For the same property, the latter one takes effect. Its code implementation is located [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/animation/css/css_animations.cc;l=473).
3. A similar type of interpolation calculation is color interpolation ([ColorInterpolation::InterpolateColors](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/graphics/color.h;l=256;bpv=1;bpt=1)): it does not has a range restriction, but provides [Color::ResolveNonFiniteValues](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/graphics/color.h;l=397;bpv=1;bpt=1) to handle cases involving `NaN` and `inf`.
The fix now works almost the same as when Chrome's DCHECK is off, and Safari behaves pretty much the same as our current code does too.
YISI YUThis is unnecessarily complicated. If the key-times interval is zero, then the result would just map to one of the possible end-points - 0 or 1. I think I'd go with zero here.
This extreme case won't happen if `keyTimes` are well checked. The core fix is to improve the check for spline in `CheckAnimationParameters()`, rather than individually checking for these edge cases.
DCHECK_GT(to_percent, from_percent);YISI YUThis ought to be `DCHECK_GE()` I guess? At least in the key-times branch above. It probably makes sense to move the computation of `effective_percent` up into the `key_times_count` if-else statement blocks above.
`SVGAnimationElement::CheckAnimationParameters()` ensures that keyTimes ends with 1, and when combined with `SVGAnimationElement::CalculateKeyTimesIndex()`, it guarantees that `to_percent` and `from_percent` will not be equal.
YISI YUWe could make this a WPT (and the expectation file shouldn't be needed).
I have added this test case to the WPT, so this file is no longer needed.
<animateMotion path="M0,0 L100,0 L100,100 Z" keyTimes="0;0.01;0.01" keyPoints="0;0.5;1"YISI YUThis should have a value of "1" at the end to be valid (with additional adjustments rto other attributes). (It should've been rejected by `SVGAnimationElement::CheckAnimationParameters`, but it seems to be missing this case.)
This is the key to resolving this issue. Please take a look at the detailed explanation in commit msg.
// Run the test for 10ms to make sure the animation is running for few frames.YISI YUCould we try to write this as a test that (pauses and) seeks to the right point instead to trigger the same condition? That should be more predictable than this. (And could possibly be observable beyond the assertion.)
Done. This script is no longer needed.
Related details
- Fredrik Söderquist
- Xianzhu Wang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 2 comments![]( "Open in Gerrit")
Code change LG, but I think we could do better on the test side
<title>When the calcMode of animateMotion is spline, if keyTimes does not end with 1, it should be rejected without causing a crash.</title>I think we could make a more useful test now - one that verifies that the animation does not apply.
I think it could look something like this:
```
<!doctype html>
<title>keyTimes does not end with one, calcMode=spline</title>
<script src="/resources/testharness.js">
<script src="/resources/testharnessreport.js">
<svg>
<rect width="100" height="100" fill="green">
<animateMotion .../>
</rect>
</svg>
<script>
async_test(t => {
onload = t.step_func(() => {
requestAnimationFrame(t.step_func_done(() => {
const rect = document.querySelector("svg > rect");
assert_equals(rect.getCTM().e, 0, "motion path not applied");
}));
});
});
</script>
```
The path should probably be adjusted to have a non-zero initial value (I.e "M-100,-50 ..." or so).
Related details
- Xianzhu Wang
- YISI YU
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
YISI YU (Gerrit)
YISI YU voted and added 2 comments![]( "Open in Gerrit")
Votes added by YISI YU
| Auto-Submit | +1 |
2 comments
<title>When the calcMode of animateMotion is spline, if keyTimes does not end with 1, it should be rejected without causing a crash.</title>I think we could make a more useful test now - one that verifies that the animation does not apply.
I think it could look something like this:
```
<!doctype html>
<title>keyTimes does not end with one, calcMode=spline</title>
<script src="/resources/testharness.js">
<script src="/resources/testharnessreport.js">
<svg>
<rect width="100" height="100" fill="green">
<animateMotion .../>
</rect>
</svg>
<script>
async_test(t => {
onload = t.step_func(() => {
requestAnimationFrame(t.step_func_done(() => {
const rect = document.querySelector("svg > rect");
assert_equals(rect.getCTM().e, 0, "motion path not applied");
}));
});
});
</script>
```
The path should probably be adjusted to have a non-zero initial value (I.e "M-100,-50 ..." or so).
Thanks! I made a `smil_async_test` by looking at other SVG tests, and I put both the crash check and the apply validation in the same file.
Before fix: the test won't pass. It crashed and failed when `DCHECK_IS_ON` was true. The animation still got applied when `DCHECK_IS_ON` was false.
After fix: the test will pass. The spline animation won't be applied. `rect.getCMT().e` will always be zero.
Related details
- Fredrik Söderquist
- Xianzhu Wang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist voted![]( "Open in Gerrit")
- Xianzhu Wang
- YISI YU
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 1 comment![]( "Open in Gerrit")
- Fredrik Söderquist
- Philip Rogers
- Xianzhu Wang
- YISI YU
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |