Closed Bug 1458457 Opened 7 years ago Closed 7 years ago

Use the current time stamp instead of the previous time stamp on the compositor for the pending animations which were just started when it's sent to the compositor

Categories

(Core :: DOM: Animation, enhancement)

Product:
Core
Component:
DOM: Animation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(2 files)

Bug 1458457 - Move the logic that we use whether the previous time stamp or the last compose time stamp into SampleAnimationForEachNode.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1458457 - Use the current frame time stamp instead of the previous frame's time stamp in the case where the animation was play pending state when the animation was sent to the compositor.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Spun off from bug 1456679 comment 12. I did push a try with the approach I commented in bug 1456679 comment 12. It reduced failure frequency but still some failures there. So there seems other cases that we miss the initial paint on the compositor. https://treeherder.mozilla.org/#/jobs?repo=try&revision=297148c4caef6951fed8858d8c3380763de6517c
Summary: Use the current time stamp instead of the previous time stamp on the compositor for the pending animations which were just started when it's sen to the compositor → Use the current time stamp instead of the previous time stamp on the compositor for the pending animations which were just started when it's sent to the compositor
(In reply to Hiroyuki Ikezoe (:hiro) from comment #0) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=297148c4caef6951fed8858d8c3380763de6517c I'd say it also reduces bug 1395971 frequency (in M-e10s-2 in the try).
One of the other failure case will be fixed by bug 1458414. Still there are a couple of failures on Win64 though. But in any cases, those failure happens only if the second patch for bug 1456679, so I think we can land patches here without worrying about the failures. Here is the try based on patches for bug 1458414; https://treeherder.mozilla.org/#/jobs?repo=try&revision=70b0bb05f6b7683469463ccb6d7c618759ffde2b
Found the other case. The case is that we don't clear the previous time stamp when we actually didn't call SampleAnimations() in WebRenderBridgeParent::CompositeToTarget(). Here is the fix; https://hg.mozilla.org/try/rev/6b13aa6fb7d050e78fbea3911328c99bc83e83f3 And the try with the fix. https://treeherder.mozilla.org/#/jobs?repo=try&revision=65fa235a7e8a10ed31e6e92a346db2b210eb4fe2 I will open another bug for the fix.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
See Also: → 1395971
Attachment #8972790 - Flags: review?(bbirtles)
Attachment #8972791 - Flags: review?(bbirtles)
Comment on attachment 8972790 [details] Bug 1458457 - Move the logic that we use whether the previous time stamp or the last compose time stamp into SampleAnimationForEachNode. https://reviewboard.mozilla.org/r/241356/#review247766 ::: gfx/layers/AnimationHelper.h:215 (Diff revision 1) > * Sample animations based on a given time stamp for a element(layer) with > * its animation data. > * > * Returns SampleResult::None if none of the animations are producing a result > * (e.g. they are in the delay phase with no backwards fill), > * SampleResult::Skipped if the animation output did not change since the last > * call of this function, > * SampleResult::Sampled if the animation output was updated. > */ > static SampleResult > - SampleAnimationForEachNode(TimeStamp aTime, > + SampleAnimationForEachNode(TimeStamp aPreviousFrameTime, > + TimeStamp aCurrentFrameTime, We should probably update the comment here to describe the different timestamps? (If you ping me on IRC we can wordsmith a description.)
Attachment #8972790 - Flags: review?(bbirtles) → review+
The second patch seems a little unfortunate. Why is it ok to paint two frames with the same timestamp? Or have I misunderstood? If this is only to get tests to pass then I'm wondering if we can do a test-specific fix that doesn't degrade regular playback.
(In reply to Brian Birtles (:birtles) from comment #7) > The second patch seems a little unfortunate. Why is it ok to paint two > frames with the same timestamp? Or have I misunderstood? We have been doing the two frames painting since we introduced the previous timestamp there. The previous timestamp is cleared in the case where no animation exists on the compositor. It's a side-effect of using the previous timestamp, but I believe it should be fine since we have never received such bug reports since then (as far as I know). > If this is only to get tests to pass then I'm wondering if we can do a > test-specific fix that doesn't degrade regular playback. No, this is not just for making the test pass. Though this is for matching the values by getOMTAStyle() and the values we are visually seeing, this is what we've been doing. Note that on *non-WebRender* we've been painting the initial animation value on the compositor even if SampleAnimationForEachNode() doesn't sample for the animation. That's the one of the failure reason of bug 1395971 (I am now suspecting that's the exactly failure cause). On non-WebRender we paint the initial value there but we don't set the value is set by the animation (i.e. we don't call SetShadowTransformSetByAnimation(true), we call SetShadowTransformSetByAnimation(false) instead, the setting value is actually animation value though). I know we should also the non-WebRender case, but it's pretty confusing, it needs more time. I am inclined to leave as it is since it will be replaced by WebRender.
The problem here; https://searchfox.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp#748 |layer->GetBaseTransform()| includes initial animation value (computed on the main-thread). I think we have been doing this since we introduced OMTA.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #8) > I know we should also the non-WebRender case, but it's pretty confusing, it needs more time. *should also fix*
(In reply to Hiroyuki Ikezoe (:hiro) from comment #8) > (In reply to Brian Birtles (:birtles) from comment #7) > > The second patch seems a little unfortunate. Why is it ok to paint two > > frames with the same timestamp? Or have I misunderstood? > > We have been doing the two frames painting since we introduced the previous > timestamp there. The previous timestamp is cleared in the case where no > animation exists on the compositor. It's a side-effect of using the > previous timestamp, but I believe it should be fine since we have never > received such bug reports since then (as far as I know). I don't think the absence of bug reports is a particularly good indication. This change is only subtle, quite recent, and only on WebRender. > > If this is only to get tests to pass then I'm wondering if we can do a > > test-specific fix that doesn't degrade regular playback. > > No, this is not just for making the test pass. Though this is for matching > the values by getOMTAStyle() and the values we are visually seeing, this is > what we've been doing. Can you explain? > Note that on *non-WebRender* we've been painting the > initial animation value on the compositor even if > SampleAnimationForEachNode() doesn't sample for the animation. Are you saying that we already paint the same frame twice for non-WebRender? I'm not quite sure which code path you're referring to. > I know we should also the non-WebRender > case, but it's pretty confusing, it needs more time. I am inclined to leave > as it is since it will be replaced by WebRender. I agree. Sorry for all the questions. I'm just trying to convince myself this change is correct because at a glance it seems like a regression to animation responsiveness.
(In reply to Brian Birtles (:birtles) from comment #11) > > Sorry for all the questions. I'm just trying to convince myself this change > is correct because at a glance it seems like a regression to animation > responsiveness. No, no worries at all. This is quite confusing. I (we) have been cheated by this issue for long time. > (In reply to Hiroyuki Ikezoe (:hiro) from comment #8) > > (In reply to Brian Birtles (:birtles) from comment #7) > > > The second patch seems a little unfortunate. Why is it ok to paint two > > > frames with the same timestamp? Or have I misunderstood? > > > > We have been doing the two frames painting since we introduced the previous > > timestamp there. The previous timestamp is cleared in the case where no > > animation exists on the compositor. It's a side-effect of using the > > previous timestamp, but I believe it should be fine since we have never > > received such bug reports since then (as far as I know). > > I don't think the absence of bug reports is a particularly good indication. > This change is only subtle, quite recent, and only on WebRender. > > > > If this is only to get tests to pass then I'm wondering if we can do a > > > test-specific fix that doesn't degrade regular playback. > > > > No, this is not just for making the test pass. Though this is for matching > > the values by getOMTAStyle() and the values we are visually seeing, this is > > what we've been doing. > > Can you explain? > > > Note that on *non-WebRender* we've been painting the > > initial animation value on the compositor even if > > SampleAnimationForEachNode() doesn't sample for the animation. > > Are you saying that we already paint the same frame twice for non-WebRender? > I'm not quite sure which code path you're referring to. Yes, that's right. That's what we've been cheated. So first of all, the issue happens only when we need to modify the start time when we send the animation to the compositor. In such cases for a transform animation, 1) we modify the start time in AnimationInfo::StartPendingAnimations 2) at that time, we also set the base transform value to the layer and note that the base value is including the animation value 3) when we compose the animation initially on the compositor, we don't compose any animation values but instead we do set the base transform value with SetShadowTransformSetByAnimation(false) 4) so, when we call getOMTAStyle() for the transform, we don't get any valid transform values, but the animation value is being painted I don't know yet where the animation value was factored into the base transform value, I just confirmed it on the compositor side. > > (In reply to Brian Birtles (:birtles) from comment #7) > > > The second patch seems a little unfortunate. Why is it ok to paint two > > > frames with the same timestamp? Or have I misunderstood? > > > > We have been doing the two frames painting since we introduced the previous > > timestamp there. The previous timestamp is cleared in the case where no > > animation exists on the compositor. It's a side-effect of using the > > previous timestamp, but I believe it should be fine since we have never > > received such bug reports since then (as far as I know). > > I don't think the absence of bug reports is a particularly good indication. > This change is only subtle, quite recent, and only on WebRender. What I am saying about the bug report is for the side-effect of the previous timestamp, it's been there for a while.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #12) > 2) at that time, we also set the base transform value to the layer and note > that the base value is including the animation value Ok, I was misunderstanding this point. When we were talking about the base value, I thought this referred to the `Animatable baseStyle` on the Animation ipdl struct but this refers to the base transform value set on the main thread (which, I presume, is set using the computed value of the transform, which includes animations starting in this frame--although it possibly shouldn't).
Oh right. that's really confusing. In my head, at that moment, I did totally forget about the 'Animatable baseStyle'. Sorry for the confusion.
(In reply to Brian Birtles (:birtles) from comment #6) > We should probably update the comment here to describe the different > timestamps? > > (If you ping me on IRC we can wordsmith a description.) I did add the below comment in the first patch; + * Generally |aPreviousFrameTimeStamp| is used for the sampling if it's + * supplied to make the animation more in sync with other animations on the + * main-thread. And an additional comment in the second patch; + * main-thread. But in the case where the animation just started at the + * |aCurrentFrameTime|, it's used for the sampling instead to avoid flickering + * the animation.
Comment on attachment 8972791 [details] Bug 1458457 - Use the current frame time stamp instead of the previous frame's time stamp in the case where the animation was play pending state when the animation was sent to the compositor. https://reviewboard.mozilla.org/r/241358/#review247768 ::: commit-message-07769:7 (Diff revision 1) > +on the compositor. Once this situation happens, we do skip composing the > +initial animation value on the compositor because we consider the animation > +hasn't started at the previous frame time stamp so that we fail to do the > +initial paint for the animation. To prevent the failure, we do explicitly use This sentence might be more clear as, "If we were to use the previous frame timestamp in this case, then we'd end up treating the animation as if it had not yet started so we would skip its initial paint." ::: gfx/layers/AnimationHelper.cpp:181 (Diff revision 1) > // Use a previous vsync time to make main thread animations and compositor > // more in sync with each other. > // On the initial frame we use the current frame time here so the timestamp > // on the second frame are the same as the initial frame, but it does not > // matter. > - const TimeStamp& timeStamp = !aPreviousFrameTime.IsNull() > + const TimeStamp& timeStamp = > + // If the animation was play-pending state when the animation was sent to > + // the compositor, we should use the current frame time stamp to avoid > + // using the past time stamp (aPreviousFrameTime) for the animation that > + // has just started. > + !aPreviousFrameTime.IsNull() && !wasPlayPending > ? aPreviousFrameTime > : aCurrentFrameTime; We could probably make this comment/logic a little more clear with something like: > // Use the previous vsync time to make main thread animations and compositor > // more closely aligned. > // > // On the first frame where we have animations, the previous timestamp will > // not be set so we simply use the current timestamp. As a result we will > // end up painting the first frame twice. That doesn't appear to be > // noticeable, however. > // > // Likewise, if the animation is play-pending, it may have a resolved start > // time that is *after* |aPreviousFrameTime| but *before* > // |aCurrentFrameTime|. However, the base value applied to the layer as > // calculated on the main thread will already incorporate the animated value > // so if we use |aPreviousFrameTime| we end up flickering as we temporarily > // jump back to the un-animated value. Is the last part, correct, however? I would have thought that this code meant that in that case we apply the base layer value anyway? https://searchfox.org/mozilla-central/rev/b28b94dc81d60c6d9164315adbd4a5073526d372/gfx/layers/composite/AsyncCompositionManager.cpp#745-752 Then make the condition a little simpler like: > const TimeStamp& timeStamp = > aPreviousFrameTime.IsNull() || wasPlayPending > ? aCurrentFrameTime > : aPreviousFrameTime; ::: gfx/layers/AnimationHelper.cpp:194 (Diff revision 1) > + if (wasPlayPending) { > + animation.wasPlayPending() = false; > + } Is it a little odd to mutate animation here? I would expect SampleAnimationsForEachNode to be idempotent so that if we end up calling it twice in one frame it's ok. But maybe that's ok because on the second call we will pass a different `aPreviousFrameTime`? Is there any other way we can detect this case that doesn't require adding extra state? e.g. look for previous frame time < start time < current time + no stored animation value?
(In reply to Brian Birtles (:birtles) from comment #18) > ::: gfx/layers/AnimationHelper.cpp:181 > (Diff revision 1) > > // Use a previous vsync time to make main thread animations and compositor > > // more in sync with each other. > > // On the initial frame we use the current frame time here so the timestamp > > // on the second frame are the same as the initial frame, but it does not > > // matter. > > - const TimeStamp& timeStamp = !aPreviousFrameTime.IsNull() > > + const TimeStamp& timeStamp = > > + // If the animation was play-pending state when the animation was sent to > > + // the compositor, we should use the current frame time stamp to avoid > > + // using the past time stamp (aPreviousFrameTime) for the animation that > > + // has just started. > > + !aPreviousFrameTime.IsNull() && !wasPlayPending > > ? aPreviousFrameTime > > : aCurrentFrameTime; > > We could probably make this comment/logic a little more clear with something > like: > > > // Use the previous vsync time to make main thread animations and compositor > > // more closely aligned. > > // > > // On the first frame where we have animations, the previous timestamp will > > // not be set so we simply use the current timestamp. As a result we will > > // end up painting the first frame twice. That doesn't appear to be > > // noticeable, however. > > // > > // Likewise, if the animation is play-pending, it may have a resolved start > > // time that is *after* |aPreviousFrameTime| but *before* > > // |aCurrentFrameTime|. However, the base value applied to the layer as > > // calculated on the main thread will already incorporate the animated value > > // so if we use |aPreviousFrameTime| we end up flickering as we temporarily > > // jump back to the un-animated value. > > Is the last part, correct, however? I would have thought that this code meant > that in that case we apply the base layer value anyway? > > https://searchfox.org/mozilla-central/rev/ > b28b94dc81d60c6d9164315adbd4a5073526d372/gfx/layers/composite/ > AsyncCompositionManager.cpp#745-752 Yes, that's right. The code is the wallpaper of this issue. I do really want to eliminate the code at some point (if I could take time). > Then make the condition a little simpler like: > > > const TimeStamp& timeStamp = > > aPreviousFrameTime.IsNull() || wasPlayPending > > ? aCurrentFrameTime > > : aPreviousFrameTime; > > ::: gfx/layers/AnimationHelper.cpp:194 > (Diff revision 1) > > + if (wasPlayPending) { > > + animation.wasPlayPending() = false; > > + } > > Is it a little odd to mutate animation here? Yeah, I understand it's a bit odd. > I would expect SampleAnimationsForEachNode to be idempotent so that if we > end up calling it twice in one frame it's ok. But maybe that's ok because on > the second call we will pass a different `aPreviousFrameTime`? > > Is there any other way we can detect this case that doesn't require adding > extra state? e.g. look for previous frame time < start time < current time + > no stored animation value? OK, I will try that way. I am pretty sure that you understand timing handling for animations very well so I did expect you have another way for that. The timing handling, start time, hold time, etc. etc. are pretty complicated to me actually. :)
The way seems to work as expected. https://treeherder.mozilla.org/#/jobs?repo=try&revision=53eee20b3c6cec8aca42eaad92496f02a5401537 I did use the below conditions but I am not 100% sure it's correct. + const bool wasPlayPending = + !aPreviousFrameTime.IsNull() && + animation.startTime().type() != MaybeTimeDuration::Tnull_t && + !aPreviousValue && + (aPreviousFrameTime - animation.originTime() - + animation.startTime().get_TimeDuration()) < animation.holdTime();
Comment on attachment 8972791 [details] Bug 1458457 - Use the current frame time stamp instead of the previous frame's time stamp in the case where the animation was play pending state when the animation was sent to the compositor. https://reviewboard.mozilla.org/r/241358/#review248104 ::: gfx/layers/AnimationHelper.cpp:181 (Diff revision 3) > + // We consider the animation just started if all following conditions are > + // true: > + // * We have no previous animation value for this animation (it means that > + // this is the first frame for the animation since the animation was > + // sent to the compositor) > + // * The animation has a resolved start time (i.e. not in delay phase) > + // * The start time of the animation at the previous timestamp is behind > + // of the current time of the animation > + const bool wasPlayPending = > + !aPreviousValue && > + animation.startTime().type() != MaybeTimeDuration::Tnull_t && > + !aPreviousFrameTime.IsNull() && > + (aPreviousFrameTime - animation.originTime() - > + animation.startTime().get_TimeDuration()) < animation.holdTime(); > // Use a previous vsync time to make main thread animations and compositor > // more in sync with each other. > // On the initial frame we use the current frame time here so the timestamp > // on the second frame are the same as the initial frame, but it does not > // matter. > - const TimeStamp& timeStamp = !aPreviousFrameTime.IsNull() > - ? aPreviousFrameTime > - : aCurrentFrameTime; > + const TimeStamp& timeStamp = > + // If the animation was play-pending state when the animation was sent to > + // the compositor, we should use the current frame time stamp to avoid > + // using the past timestamp (aPreviousFrameTime) for the animation that > + // has just started. > + aPreviousFrameTime.IsNull() || wasPlayPending > + ? aCurrentFrameTime > + : aPreviousFrameTime; > * The animation has a resolved start time (i.e. not in delay phase) s/in delay phase/paused/ I looked into this calculation and I think it needs to factor in the playback rate. In effect, what we are trying to detect is, "Is the ready time between the previous frame and the next frame time?". Or actually just, "Is the ready time after the previous frame time"? To get to the ready time from the start time, however, we need to reverse the calculation for play-pending animations in AnimationInfo::StartPendingAnimations[1], i.e. TimeDuration readyTime = aReadyTime - anim.originTime(); anim.startTime() = dom::Animation::StartTimeFromTimelineTime( readyTime, anim.holdTime(), anim.playbackRate()); Expanding out Animation::StartTimeFromTimelineTime gives something like: TimeDuration readyTime = aReadyTime - anim.originTime(); if (anim.playbackRate() == 0) { anim.startTime() = readyTime; } else { readyTime -= anim.holdTime().MultDouble(1.0 / anim.playbackRate()); anim.startTime() = readyTime; } Solving for |aReadyTime|: if (anim.playbackRate() == 0) { aReadyTime = anim.startTime() + anim.originTime(); } else { aReadyTime = anim.startTime() + anim.originTime() + anim.holdTime().MultDouble(1.0 / anim.playbackRate()); } i.e. readyTime = anim.startTime() + anim.originTime(); if (anim.playbackRate() != 0) { readyTime += anim.holdTime().MultDouble(1.0 / anim.playbackRate()); } However, we can drop the check for `anim.playbackRate() != 0` by checking that `anim.isNotPlaying()` is false earlier. If we do that, we also don't need to check if the start time is null since we have an assertion earlier in this function that if the animation is playing it will have a resolved origin time and start time. That would give us something like: const TimeStamp readyTime = anim.startTime().get_TimeDuration() + anim.originTime() + anim.holdTime().MultDouble(1.0 / anim.playbackRate()); Then we want to compare: previous frame time < ready time i.e. aPreviousFrameTime < anim.startTime().get_TimeDuration() + anim.originTime() + anim.holdTime().MultDouble(1.0 / anim.playbackRate()) which could also be written: aPreviousFrameTime - anim.startTime().get_TimeDuration() - anim.originTime() < anim.holdTime().MultDouble(1.0 / anim.playbackRate()) which is pretty similar to the code in this patch: aPreviousFrameTime - anim.startTime().get_TimeDuration() - anim.originTime() < animation.holdTime() the only difference being that we incorporate the playback rate. So I think this patch is nearly right. Perhaps we could break up the logic as follows though: // Determine if the animation was play-pending and used a ready time later // than the previous frame time. // // To determine this, _all_ of the following conditions need to hold: // // * There was no previous animation value (i.e. this is the first frame for // the animation since it was sent to the compositor), and // * The animation is playing, and // * There is a previous frame time, and // * The ready time of the animation is ahead of the previous frame time. bool hasFutureReadyTime = false; if (!aPreviousValue && !animation.isNotPlaying() && !aPreviousFrameTime.IsNull()) { // This is the inverse of the calculation performed in // AnimationInfo::StartPendingAnimations to calculate the start time of // play-pending animations. const TimeStamp readyTime = anim.startTime().get_TimeDuration() + anim.originTime() + anim.holdTime().MultDouble(1.0 / anim.playbackRate()); hasFutureReadyTime = readyTime > aPreviousFrameTime; } // Use the previous vsync time to make main thread animations and compositor // more closely aligned. // // On the first frame where we have animations the previous timestamp will // not be set so we simply use the current timestamp. As a result we will // end up painting the first frame twice. That doesn't appear to be // noticeable, however. // // Likewise, if the animation is play-pending, it may have a ready time that // is *after* |aPreviousFrameTime| (but *before* |aCurrentFrameTime|). // To avoid flicker we need to use |aCurrentFrameTime| to avoid temporarily // jumping backwards into the range prior to when the animation starts. const TimeStamp& timeStamp = aPreviousFrameTime.IsNull() || hasFutureReadyTime ? aCurrentFrameTime : aPreviousFrameTime; [1] https://searchfox.org/mozilla-central/rev/f30847c12e5fb13791401ed4330ced3e1b9c8d81/gfx/layers/AnimationInfo.cpp#121-129
Comment on attachment 8972791 [details] Bug 1458457 - Use the current frame time stamp instead of the previous frame's time stamp in the case where the animation was play pending state when the animation was sent to the compositor. https://reviewboard.mozilla.org/r/241358/#review248106 (Clearing review request on this for now just to keep track of what I still need to review.)
Attachment #8972791 - Flags: review?(bbirtles)
Thanks for the review! I did want to avoid the reverse calculation somehow since if it's needed there we can set the value in the Animation struct in the first place, that is the same as what I did in my initial attempt. But in most cases, the reverse-calculation should be avoided with the 'if (!aPreviousValue && ..', I believe, I hope the case doesn't happen frequently.
Yeah, if that calculation does prove significant, that's a reasonable argument for adding more data to the IPDL. I suspect it won't, however.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc68c0d87c22d15e5cd64a8e91297926f605c93c the try looks fine (at least the test cases that failed in bug 1456679 passed).
Comment on attachment 8972791 [details] Bug 1458457 - Use the current frame time stamp instead of the previous frame's time stamp in the case where the animation was play pending state when the animation was sent to the compositor. https://reviewboard.mozilla.org/r/241358/#review248116 ::: gfx/layers/AnimationHelper.cpp:181 (Diff revision 4) > - // Use a previous vsync time to make main thread animations and compositor > - // more in sync with each other. > - // On the initial frame we use the current frame time here so the timestamp > - // on the second frame are the same as the initial frame, but it does not > - // matter. > - const TimeStamp& timeStamp = !aPreviousFrameTime.IsNull() > - ? aPreviousFrameTime > - : aCurrentFrameTime; > + // We consider the animation just started if all following conditions are > + // true: > + // * We have no previous animation value for this animation (it means that > + // this is the first frame for the animation since the animation was > + // sent to the compositor) > + // * The animation has a resolved start time (i.e. not in delay phase) > + // * The start time of the animation at the previous timestamp is behind > + // of the current time of the animation > + Not sure we need this comment? I think it is covered in the comments below?
Attachment #8972791 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #30) > Comment on attachment 8972791 [details] > Bug 1458457 - Use the current frame time stamp instead of the previous > frame's time stamp in the case where the animation was play pending state > when the animation was sent to the compositor. > > https://reviewboard.mozilla.org/r/241358/#review248116 > > ::: gfx/layers/AnimationHelper.cpp:181 > (Diff revision 4) > > - // Use a previous vsync time to make main thread animations and compositor > > - // more in sync with each other. > > - // On the initial frame we use the current frame time here so the timestamp > > - // on the second frame are the same as the initial frame, but it does not > > - // matter. > > - const TimeStamp& timeStamp = !aPreviousFrameTime.IsNull() > > - ? aPreviousFrameTime > > - : aCurrentFrameTime; > > + // We consider the animation just started if all following conditions are > > + // true: > > + // * We have no previous animation value for this animation (it means that > > + // this is the first frame for the animation since the animation was > > + // sent to the compositor) > > + // * The animation has a resolved start time (i.e. not in delay phase) > > + // * The start time of the animation at the previous timestamp is behind > > + // of the current time of the animation > > + > > Not sure we need this comment? I think it is covered in the comments below? Forgot to drop it. :/
Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9346c2138737 Move the logic that we use whether the previous time stamp or the last compose time stamp into SampleAnimationForEachNode. r=birtles https://hg.mozilla.org/integration/autoland/rev/6cbc76d14fef Use the current frame time stamp instead of the previous frame's time stamp in the case where the animation was play pending state when the animation was sent to the compositor. r=birtles
https://hg.mozilla.org/mozilla-central/rev/9346c2138737 https://hg.mozilla.org/mozilla-central/rev/6cbc76d14fef
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Depends on: 1459895
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: