Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1942703002:
Fix handling of multiple keyframes at same offset (Closed)
|
Created:
4 years, 7 months ago by suzyh_UTC10 (ex-contributor) Modified:
4 years, 7 months ago Reviewers:
alancutter (OOO until 2018) CC:
darktears, blink-reviews, blink-reviews-animation_chromium.org, blink-reviews-w3ctests_chromium.org, chromium-reviews, Eric Willigers, rjwright, shans, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix handling of multiple keyframes at same offset
When multiple keyframes have the same offset, we cannot always ignore
all but the last-specified keyframe. In particular, if iteration
progress is negative (e.g. due to easing), the first keyframe with
offset 0 is used. See spec:
https://w3c.github.io/web-animations/#the-effect-value-of-a-keyframe-animation-effect
This patch fixes our implementation in order to pass the W3C
web-animations/keyframe-effect/keyframe-handling.html test.
It involves
(a) not removing as redundant those end keyframes that have the same
offset as their neighbours,
(b) allowing interpolations with the same offset at both ends, and
(c) using the same keyframe as both endpoints in interpolations
where the offset is the same for both ends.
BUG=600248
Committed: https://crrev.com/aa5bd02c9b5ffebfbd942512534b7e10aba8a821
Cr-Commit-Position: refs/heads/master@{#391464}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Remove unneeded 0<offset<1 case for equal offsets #
Total comments: 3
Patch Set 3 : Rewrite loop over keyframes #
Total comments: 6
Patch Set 4 : Response to review #
Messages
Total messages: 27 (7 generated)
suzyh@chromium.org changed reviewers: + alancutter@chromium.org
The CQ bit was checked by suzyh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1942703002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1942703002/1
https://codereview.chromium.org/1942703002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp (right): https://codereview.chromium.org/1942703002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp:218: } else { Can this be "else if (offset == 1)"? https://codereview.chromium.org/1942703002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp:266: bool hasSameOffsetAsNextNeighbor = m_keyframes[i + 1]->offset() == offset; Much nicer!
https://codereview.chromium.org/1942703002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp (right): https://codereview.chromium.org/1942703002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp:218: } else { On 2016/05/02 at 07:35:35, alancutter wrote: > Can this be "else if (offset == 1)"? Hmm... I think that at this point, offset can still be something other than 0 and 1. My reading of the spec was that only offset 0 is special, so concluded that other offsets should behave like 1 in this case. But now that you mention it, maybe for equal offsets other than 0 and 1 we should just not add an extra interpolation, because it never applies. Can you step through an example on paper, working off the spec, and see what you think the appropriate behaviour is? Then we can compare notes and see where to go from there.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/05/02 at 08:34:10, suzyh wrote: > https://codereview.chromium.org/1942703002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp (right): > > https://codereview.chromium.org/1942703002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp:218: } else { > On 2016/05/02 at 07:35:35, alancutter wrote: > > Can this be "else if (offset == 1)"? > > Hmm... I think that at this point, offset can still be something other than 0 and 1. My reading of the spec was that only offset 0 is special, so concluded that other offsets should behave like 1 in this case. > > But now that you mention it, maybe for equal offsets other than 0 and 1 we should just not add an extra interpolation, because it never applies. > > Can you step through an example on paper, working off the spec, and see what you think the appropriate behaviour is? Then we can compare notes and see where to go from there. I just seems odd to me that only the second keyframe is used for any offsets other than zero while it makes sense for offset 1. I trust that our tests will catch if there's any issues with the suggested change.
On 2016/05/03 at 01:29:00, alancutter wrote: > On 2016/05/02 at 08:34:10, suzyh wrote: > > https://codereview.chromium.org/1942703002/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp (right): > > > > https://codereview.chromium.org/1942703002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp:218: } else { > > On 2016/05/02 at 07:35:35, alancutter wrote: > > > Can this be "else if (offset == 1)"? > > > > Hmm... I think that at this point, offset can still be something other than 0 and 1. My reading of the spec was that only offset 0 is special, so concluded that other offsets should behave like 1 in this case. > > > > But now that you mention it, maybe for equal offsets other than 0 and 1 we should just not add an extra interpolation, because it never applies. > > > > Can you step through an example on paper, working off the spec, and see what you think the appropriate behaviour is? Then we can compare notes and see where to go from there. > > I just seems odd to me that only the second keyframe is used for any offsets other than zero while it makes sense for offset 1. I trust that our tests will catch if there's any issues with the suggested change. OK, looks like all our tests are passing if I don't add the extra interpolation for cases other than 0 and 1. Done.
The CQ bit was checked by suzyh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1942703002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1942703002/20001
lgtm
https://codereview.chromium.org/1942703002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp (right): https://codereview.chromium.org/1942703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp:217: // - 0 < offset < 1, never used in sampling This comment is a touch close to being a straight copy of the code below but I'm not going to block landing this on it.
https://codereview.chromium.org/1942703002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp (right): https://codereview.chromium.org/1942703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp:217: // - 0 < offset < 1, never used in sampling On 2016/05/04 at 01:32:31, alancutter wrote: > This comment is a touch close to being a straight copy of the code below but I'm not going to block landing this on it. Possible suggestion: if (offset == 0) { ASSERT(applyFrom == -inf && applyTo == 0); addInterpolation... } else if (offset == 1) { ASSERT(applyFrom == 1 && applyTo == inf); addInterpolation... } // else the interpolation will never be used in sampling.
https://codereview.chromium.org/1942703002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp (right): https://codereview.chromium.org/1942703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp:217: // - 0 < offset < 1, never used in sampling On 2016/05/04 at 01:35:32, alancutter wrote: > On 2016/05/04 at 01:32:31, alancutter wrote: > > This comment is a touch close to being a straight copy of the code below but I'm not going to block landing this on it. Fair. I was trying to explain why the first two cases use the keyframe-pair values they do, and why the third case was not present, but I can see it may have ended up a little too close to copying the code. > Possible suggestion: > if (offset == 0) { > ASSERT(applyFrom == -inf && applyTo == 0); > addInterpolation... > } else if (offset == 1) { > ASSERT(applyFrom == 1 && applyTo == inf); > addInterpolation... > } > // else the interpolation will never be used in sampling. Hmm, yeah, I hadn't thought of using ASSERTs to capture this. Good idea. But now that I look at it, this makes the definitions of applyFrom and applyTo unnecessarily repetitive. I'll try rewriting the whole loop and see if I can make it cleaner.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/05/04 at 01:41:00, suzyh wrote: > https://codereview.chromium.org/1942703002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp (right): > > https://codereview.chromium.org/1942703002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp:217: // - 0 < offset < 1, never used in sampling > On 2016/05/04 at 01:35:32, alancutter wrote: > > On 2016/05/04 at 01:32:31, alancutter wrote: > > > This comment is a touch close to being a straight copy of the code below but I'm not going to block landing this on it. > > Fair. I was trying to explain why the first two cases use the keyframe-pair values they do, and why the third case was not present, but I can see it may have ended up a little too close to copying the code. > > > > Possible suggestion: > > if (offset == 0) { > > ASSERT(applyFrom == -inf && applyTo == 0); > > addInterpolation... > > } else if (offset == 1) { > > ASSERT(applyFrom == 1 && applyTo == inf); > > addInterpolation... > > } > > // else the interpolation will never be used in sampling. > > Hmm, yeah, I hadn't thought of using ASSERTs to capture this. Good idea. > > But now that I look at it, this makes the definitions of applyFrom and applyTo unnecessarily repetitive. I'll try rewriting the whole loop and see if I can make it cleaner. I've rewritten the loop over keyframes to make the logic (I hope) clearer. It's more verbose for perhaps little benefit, but I think it's an improvement. What do you think of this version?
I think this is a good improvement. I'm happy to see those ternaries go, this code is easier to follow than it was. https://codereview.chromium.org/1942703002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp (right): https://codereview.chromium.org/1942703002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp:210: size_t startKeyframe = i; I'd call these startIndex and endIndex to avoid confusing them with Keyframes. https://codereview.chromium.org/1942703002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp:221: ASSERT(endKeyframe + 1 < keyframes.size() No need to check this, WTF::Vector indexing comes with bounds checking. https://codereview.chromium.org/1942703002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp:230: ASSERT(startKeyframe > 1 Won't this fail with [{}, {offset: 1}, {}]? I don't think this check is necessary with bounds checking.
https://codereview.chromium.org/1942703002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp (right): https://codereview.chromium.org/1942703002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp:210: size_t startKeyframe = i; On 2016/05/04 at 07:00:03, alancutter wrote: > I'd call these startIndex and endIndex to avoid confusing them with Keyframes. Done. https://codereview.chromium.org/1942703002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp:221: ASSERT(endKeyframe + 1 < keyframes.size() On 2016/05/04 at 07:00:03, alancutter wrote: > No need to check this, WTF::Vector indexing comes with bounds checking. Done. https://codereview.chromium.org/1942703002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp:230: ASSERT(startKeyframe > 1 On 2016/05/04 at 07:00:03, alancutter wrote: > Won't this fail with [{}, {offset: 1}, {}]? > I don't think this check is necessary with bounds checking. Ah, typo, that should have been startKeyframe >= 1 (or > 0) but at any rate it is now obsolete.
lgtm
The CQ bit was checked by suzyh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1942703002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1942703002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix handling of multiple keyframes at same offset When multiple keyframes have the same offset, we cannot always ignore all but the last-specified keyframe. In particular, if iteration progress is negative (e.g. due to easing), the first keyframe with offset 0 is used. See spec: https://w3c.github.io/web-animations/#the-effect-value-of-a-keyframe-animatio... This patch fixes our implementation in order to pass the W3C web-animations/keyframe-effect/keyframe-handling.html test. It involves (a) not removing as redundant those end keyframes that have the same offset as their neighbours, (b) allowing interpolations with the same offset at both ends, and (c) using the same keyframe as both endpoints in interpolations where the offset is the same for both ends. BUG=600248 ========== to ========== Fix handling of multiple keyframes at same offset When multiple keyframes have the same offset, we cannot always ignore all but the last-specified keyframe. In particular, if iteration progress is negative (e.g. due to easing), the first keyframe with offset 0 is used. See spec: https://w3c.github.io/web-animations/#the-effect-value-of-a-keyframe-animatio... This patch fixes our implementation in order to pass the W3C web-animations/keyframe-effect/keyframe-handling.html test. It involves (a) not removing as redundant those end keyframes that have the same offset as their neighbours, (b) allowing interpolations with the same offset at both ends, and (c) using the same keyframe as both endpoints in interpolations where the offset is the same for both ends. BUG=600248 Committed: https://crrev.com/aa5bd02c9b5ffebfbd942512534b7e10aba8a821 Cr-Commit-Position: refs/heads/master@{#391464} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/aa5bd02c9b5ffebfbd942512534b7e10aba8a821 Cr-Commit-Position: refs/heads/master@{#391464} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
