From ad8174a7aee44f79fad72b56c4a1c311624d3a82 Mon Sep 17 00:00:00 2001 From: Boris Chiou Date: Wed, 26 Jun 2024 21:33:10 +0000 Subject: [PATCH] Bug 1626165 - Part 2: Replace the start value and start time of the transition on the compositor. r=layout-reviewers,firefox-animation-reviewers,hiro Although we re-compute the start value of the transition when sending it to the compositor, we still want to use the last sampled animation value as the new start value, to avoid any possible jittery. Also, we replace the start time with the previous sample time on the compositor as well to make sure we use the proper start time for the transition if the main thread is busy. Differential Revision: https://phabricator.services.mozilla.com/D209889 --- dom/animation/CSSTransition.cpp | 8 +- dom/animation/CSSTransition.h | 4 +- gfx/layers/AnimationHelper.cpp | 34 +++- gfx/layers/AnimationHelper.h | 4 +- gfx/layers/AnimationInfo.cpp | 16 +- gfx/layers/CompositorAnimationStorage.cpp | 48 +++-- gfx/layers/CompositorAnimationStorage.h | 36 +++- gfx/layers/ipc/LayersMessages.ipdlh | 2 + gfx/layers/wr/OMTASampler.cpp | 4 +- layout/style/test/animation_utils.js | 166 +++++++++--------- layout/style/test/mochitest.toml | 3 + ...itions_replacement_on_busy_frame_omta.html | 112 ++++++++++++ 12 files changed, 326 insertions(+), 111 deletions(-) create mode 100644 layout/style/test/test_transitions_replacement_on_busy_frame_omta.html diff --git a/dom/animation/CSSTransition.cpp b/dom/animation/CSSTransition.cpp index 949604011177..1ebb8c0f1c42 100644 --- a/dom/animation/CSSTransition.cpp +++ b/dom/animation/CSSTransition.cpp @@ -276,14 +276,14 @@ double CSSTransition::CurrentValuePortion() const { return computedTiming.mProgress.Value(); } -void CSSTransition::UpdateStartValueFromReplacedTransition() { +bool CSSTransition::UpdateStartValueFromReplacedTransition() { MOZ_ASSERT(mEffect && mEffect->AsKeyframeEffect() && mEffect->AsKeyframeEffect()->HasAnimationOfPropertySet( nsCSSPropertyIDSet::CompositorAnimatables()), "Should be called for compositor-runnable transitions"); if (!mReplacedTransition) { - return; + return false; } // We don't set |mReplacedTransition| if the timeline of this transition is @@ -308,6 +308,8 @@ void CSSTransition::UpdateStartValueFromReplacedTransition() { mReplacedTransition->mTimingFunction, computedTiming.mProgress.Value(), computedTiming.mBeforeFlag); + // FIXME: Bug 1634945. We may have to use the last value on the compositor + // to replace the start value. const AnimationValue& replacedFrom = mReplacedTransition->mFromValue; const AnimationValue& replacedTo = mReplacedTransition->mToValue; AnimationValue startValue; @@ -321,6 +323,8 @@ void CSSTransition::UpdateStartValueFromReplacedTransition() { } mReplacedTransition.reset(); + + return true; } void CSSTransition::SetEffectFromStyle(KeyframeEffect* aEffect) { diff --git a/dom/animation/CSSTransition.h b/dom/animation/CSSTransition.h index b2a76d34164c..0f828893adbd 100644 --- a/dom/animation/CSSTransition.h +++ b/dom/animation/CSSTransition.h @@ -143,7 +143,7 @@ class CSSTransition final : public Animation { // For a new transition interrupting an existing transition on the // compositor, update the start value to match the value of the replaced // transitions at the current time. - void UpdateStartValueFromReplacedTransition(); + bool UpdateStartValueFromReplacedTransition(); protected: virtual ~CSSTransition() { @@ -222,6 +222,8 @@ class CSSTransition final : public Animation { // for the third transition (from 0px/2px to 10px) will be 0.8. double mReversePortion = 1.0; + // The information of the old transition we'd like to replace with "this" + // transition. Maybe mReplacedTransition; }; diff --git a/gfx/layers/AnimationHelper.cpp b/gfx/layers/AnimationHelper.cpp index 5156f5173136..e30cf5764ee8 100644 --- a/gfx/layers/AnimationHelper.cpp +++ b/gfx/layers/AnimationHelper.cpp @@ -5,6 +5,7 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ #include "AnimationHelper.h" +#include "CompositorAnimationStorage.h" #include "base/process_util.h" #include "gfx2DGlue.h" // for ThebesRect #include "gfxLineSegment.h" // for gfxLineSegment @@ -445,7 +446,9 @@ static bool HasTransformLikeAnimations(const AnimationArray& aAnimations) { #endif AnimationStorageData AnimationHelper::ExtractAnimations( - const LayersId& aLayersId, const AnimationArray& aAnimations) { + const LayersId& aLayersId, const AnimationArray& aAnimations, + const CompositorAnimationStorage* aStorage, + const TimeStamp& aPreviousSampleTime) { AnimationStorageData storageData; storageData.mLayersId = aLayersId; @@ -537,12 +540,37 @@ AnimationStorageData AnimationHelper::ExtractAnimations( propertyAnimation->mScrollTimelineOptions = animation.scrollTimelineOptions(); + RefPtr startValue; + if (animation.replacedTransitionId()) { + if (const auto* animatedValue = + aStorage->GetAnimatedValue(*animation.replacedTransitionId())) { + startValue = animatedValue->AsAnimationValue(animation.property()); + // Basically, the timeline time is increasing monotonically, so it may + // not make sense to have a negative start time (i.e. the case when + // aPreviousSampleTime is behind the origin time). Therefore, if the + // previous sample time is less than the origin time, we skip the + // replacement of the start time. + if (!aPreviousSampleTime.IsNull() && + (aPreviousSampleTime >= animation.originTime())) { + propertyAnimation->mStartTime = + Some(aPreviousSampleTime - animation.originTime()); + } + + MOZ_ASSERT(animation.segments().Length() == 1, + "The CSS Transition only has one segement"); + } + } + nsTArray& segmentData = propertyAnimation->mSegments; for (const AnimationSegment& segment : animation.segments()) { segmentData.AppendElement(PropertyAnimation::SegmentData{ - AnimationValue::FromAnimatable(animation.property(), - segment.startState()), + // Note that even though we re-compute the start value on the main + // thread, we still replace it with the last sampled value, to avoid + // any possible lag. + startValue ? startValue + : AnimationValue::FromAnimatable(animation.property(), + segment.startState()), AnimationValue::FromAnimatable(animation.property(), segment.endState()), segment.sampleFn(), segment.startPortion(), segment.endPortion(), diff --git a/gfx/layers/AnimationHelper.h b/gfx/layers/AnimationHelper.h index 175791c72bb1..b3484c2cba95 100644 --- a/gfx/layers/AnimationHelper.h +++ b/gfx/layers/AnimationHelper.h @@ -145,7 +145,9 @@ class AnimationHelper { * 3. background color property: background-color. */ static AnimationStorageData ExtractAnimations( - const LayersId& aLayersId, const AnimationArray& aAnimations); + const LayersId& aLayersId, const AnimationArray& aAnimations, + const CompositorAnimationStorage* aStorage, + const TimeStamp& aPreviousSampleTime); /** * Get a unique id to represent the compositor animation between child diff --git a/gfx/layers/AnimationInfo.cpp b/gfx/layers/AnimationInfo.cpp index b6816ffc732b..427c52b5660f 100644 --- a/gfx/layers/AnimationInfo.cpp +++ b/gfx/layers/AnimationInfo.cpp @@ -418,8 +418,16 @@ void AnimationInfo::AddAnimationForProperty( // since after generating the new transition other requestAnimationFrame // callbacks may run that introduce further lag between the main thread and // the compositor. + // + // Note that we will replace the start value with the last sampled animation + // value on the compositor. + // The computation here is for updating the keyframe values, to make sure the + // computed values on the main thread don't behind the rendering result on the + // compositor too much. + bool needReplaceTransition = false; if (dom::CSSTransition* cssTransition = aAnimation->AsCSSTransition()) { - cssTransition->UpdateStartValueFromReplacedTransition(); + needReplaceTransition = + cssTransition->UpdateStartValueFromReplacedTransition(); } animation->originTime() = @@ -463,6 +471,12 @@ void AnimationInfo::AddAnimationForProperty( animation->isNotAnimating() = false; animation->scrollTimelineOptions() = GetScrollTimelineOptions(aAnimation->GetTimeline()); + // We set this flag to let the compositor know that the start value of this + // transition is replaced. The compositor may replace the start value with its + // last sampled animation value, instead of using the segment.mFromValue we + // send to the compositor, to avoid any potential lag. + animation->replacedTransitionId() = + needReplaceTransition ? Some(GetCompositorAnimationsId()) : Nothing(); TransformReferenceBox refBox(aFrame); diff --git a/gfx/layers/CompositorAnimationStorage.cpp b/gfx/layers/CompositorAnimationStorage.cpp index ebe21a3d2944..b8ccd49fe8ea 100644 --- a/gfx/layers/CompositorAnimationStorage.cpp +++ b/gfx/layers/CompositorAnimationStorage.cpp @@ -54,6 +54,31 @@ namespace layers { using gfx::Matrix4x4; +already_AddRefed AnimatedValue::AsAnimationValue( + nsCSSPropertyID aProperty) const { + RefPtr result; + mValue.match( + [&](const AnimationTransform& aTransform) { + // Linear search. It's likely that the length of the array is one in + // most common case, so it shouldn't have much performance impact. + for (const auto& value : Transform().mAnimationValues) { + AnimatedPropertyID property(eCSSProperty_UNKNOWN); + Servo_AnimationValue_GetPropertyId(value, &property); + if (property.mID == aProperty) { + result = value; + break; + } + } + }, + [&](const float& aOpacity) { + result = Servo_AnimationValue_Opacity(aOpacity).Consume(); + }, + [&](const nscolor& aColor) { + result = Servo_AnimationValue_Color(aProperty, aColor).Consume(); + }); + return result.forget(); +} + void CompositorAnimationStorage::ClearById(const uint64_t& aId) { MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread()); MutexAutoLock lock(mLock); @@ -120,20 +145,21 @@ OMTAValue CompositorAnimationStorage::GetOMTAValue(const uint64_t& aId) const { void CompositorAnimationStorage::SetAnimatedValue( uint64_t aId, AnimatedValue* aPreviousValue, - const gfx::Matrix4x4& aFrameTransform, const TransformData& aData) { + const gfx::Matrix4x4& aFrameTransform, const TransformData& aData, + SampledAnimationArray&& aValue) { mLock.AssertCurrentThreadOwns(); if (!aPreviousValue) { MOZ_ASSERT(!mAnimatedValues.Contains(aId)); mAnimatedValues.InsertOrUpdate( - aId, - MakeUnique(gfx::Matrix4x4(), aFrameTransform, aData)); + aId, MakeUnique(gfx::Matrix4x4(), aFrameTransform, aData, + std::move(aValue))); return; } MOZ_ASSERT(aPreviousValue->Is()); MOZ_ASSERT(aPreviousValue == GetAnimatedValue(aId)); - aPreviousValue->SetTransform(aFrameTransform, aData); + aPreviousValue->SetTransform(aFrameTransform, aData, std::move(aValue)); } void CompositorAnimationStorage::SetAnimatedValue(uint64_t aId, @@ -168,14 +194,15 @@ void CompositorAnimationStorage::SetAnimatedValue(uint64_t aId, aPreviousValue->SetOpacity(aOpacity); } -void CompositorAnimationStorage::SetAnimations(uint64_t aId, - const LayersId& aLayersId, - const AnimationArray& aValue) { +void CompositorAnimationStorage::SetAnimations( + uint64_t aId, const LayersId& aLayersId, const AnimationArray& aValue, + const TimeStamp& aPreviousSampleTime) { MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread()); MutexAutoLock lock(mLock); - mAnimations[aId] = std::make_unique( - AnimationHelper::ExtractAnimations(aLayersId, aValue)); + mAnimations[aId] = + std::make_unique(AnimationHelper::ExtractAnimations( + aLayersId, aValue, this, aPreviousSampleTime)); PROFILER_MARKER("SetAnimation", GRAPHICS, MarkerInnerWindowId(mCompositorBridge->GetInnerWindowId()), @@ -277,7 +304,8 @@ void CompositorAnimationStorage::StoreAnimatedValue( } } - SetAnimatedValue(aId, aAnimatedValueEntry, frameTransform, transformData); + SetAnimatedValue(aId, aAnimatedValueEntry, frameTransform, transformData, + std::move(aAnimationValues)); break; } default: diff --git a/gfx/layers/CompositorAnimationStorage.h b/gfx/layers/CompositorAnimationStorage.h index 2422f86b5bf4..41fe07424f08 100644 --- a/gfx/layers/CompositorAnimationStorage.h +++ b/gfx/layers/CompositorAnimationStorage.h @@ -39,6 +39,15 @@ struct AnimationTransform { */ gfx::Matrix4x4 mFrameTransform; TransformData mData; + + /* + * Store the previous sampled transform-like animation values. + * It's unfortunate we have to keep the previous sampled animation value for + * replacing the running transition, because we can not re-create the + * AnimationValues from the matrix. + * Note: We expect the length is one in most cases. + */ + SampledAnimationArray mAnimationValues; }; struct AnimatedValue final { @@ -57,22 +66,27 @@ struct AnimatedValue final { AnimatedValue(const gfx::Matrix4x4& aTransformInDevSpace, const gfx::Matrix4x4& aFrameTransform, - const TransformData& aData) - : mValue(AsVariant(AnimationTransform{aTransformInDevSpace, - aFrameTransform, aData})) {} + const TransformData& aData, SampledAnimationArray&& aValue) + : mValue(AsVariant(AnimationTransform{ + aTransformInDevSpace, aFrameTransform, aData, std::move(aValue)})) { + } explicit AnimatedValue(const float& aValue) : mValue(AsVariant(aValue)) {} explicit AnimatedValue(nscolor aValue) : mValue(AsVariant(aValue)) {} + // Note: Only transforms need to store the sampled AnimationValue because it's + // impossible to re-create the AnimationValue from the matrix. void SetTransform(const gfx::Matrix4x4& aFrameTransform, - const TransformData& aData) { + const TransformData& aData, + SampledAnimationArray&& aValue) { MOZ_ASSERT(mValue.is()); AnimationTransform& previous = mValue.as(); previous.mFrameTransform = aFrameTransform; if (previous.mData != aData) { previous.mData = aData; } + previous.mAnimationValues = std::move(aValue); } void SetOpacity(float aOpacity) { MOZ_ASSERT(mValue.is()); @@ -83,6 +97,8 @@ struct AnimatedValue final { mValue.as() = aColor; } + already_AddRefed AsAnimationValue(nsCSSPropertyID) const; + private: AnimatedValueType mValue; }; @@ -129,7 +145,8 @@ class CompositorAnimationStorage final { * Set the animations based on the unique id */ void SetAnimations(uint64_t aId, const LayersId& aLayersId, - const AnimationArray& aAnimations); + const AnimationArray& aAnimations, + const TimeStamp& aPreviousSampleTime); /** * Sample animation based the given timestamps and store them in this @@ -154,14 +171,14 @@ class CompositorAnimationStorage final { */ void ClearById(const uint64_t& aId); - private: - ~CompositorAnimationStorage() = default; - /** * Return the animated value if a given id can map to its animated value */ AnimatedValue* GetAnimatedValue(const uint64_t& aId) const; + private: + ~CompositorAnimationStorage() = default; + /** * Set the animation transform based on the unique id and also * set up |aFrameTransform| and |aData| for OMTA testing. @@ -171,7 +188,8 @@ class CompositorAnimationStorage final { */ void SetAnimatedValue(uint64_t aId, AnimatedValue* aPreviousValue, const gfx::Matrix4x4& aFrameTransform, - const TransformData& aData); + const TransformData& aData, + SampledAnimationArray&& aValue); /** * Similar to above but for opacity. diff --git a/gfx/layers/ipc/LayersMessages.ipdlh b/gfx/layers/ipc/LayersMessages.ipdlh index a0e417350770..2f9df94b2786 100644 --- a/gfx/layers/ipc/LayersMessages.ipdlh +++ b/gfx/layers/ipc/LayersMessages.ipdlh @@ -240,6 +240,8 @@ struct Animation { Animatable baseStyle; // An optional data specific for transform like properies. TransformData? transformData; + // This indicates that a transition whose start value should be replaced. + uint64_t? replacedTransitionId; // If this is present, the animation is driven by a ScrollTimeline, and // this structure contains information about that timeline. ScrollTimelineOptions? scrollTimelineOptions; diff --git a/gfx/layers/wr/OMTASampler.cpp b/gfx/layers/wr/OMTASampler.cpp index c9616e87e103..a2b9ee2df146 100644 --- a/gfx/layers/wr/OMTASampler.cpp +++ b/gfx/layers/wr/OMTASampler.cpp @@ -167,8 +167,8 @@ void OMTASampler::SetAnimations( const nsTArray& aAnimations) { MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread()); MutexAutoLock lock(mStorageLock); - - mAnimStorage->SetAnimations(aId, aLayersId, aAnimations); + MutexAutoLock timeLock(mSampleTimeLock); + mAnimStorage->SetAnimations(aId, aLayersId, aAnimations, mPreviousSampleTime); } bool OMTASampler::HasAnimations() const { diff --git a/layout/style/test/animation_utils.js b/layout/style/test/animation_utils.js index 7239885e7c2e..bf24a679c945 100644 --- a/layout/style/test/animation_utils.js +++ b/layout/style/test/animation_utils.js @@ -208,6 +208,90 @@ function findKeyframesRule(name) { return undefined; } +function isOMTAWorking() { + function waitForDocumentLoad() { + return new Promise(function (resolve, reject) { + if (document.readyState === "complete") { + resolve(); + } else { + window.addEventListener("load", resolve); + } + }); + } + + function loadPaintListener() { + return new Promise(function (resolve, reject) { + if (typeof window.waitForAllPaints !== "function") { + var script = document.createElement("script"); + script.onload = resolve; + script.onerror = function () { + reject(new Error("Failed to load paint listener")); + }; + script.src = "/tests/SimpleTest/paint_listener.js"; + var firstScript = document.scripts[0]; + firstScript.parentNode.insertBefore(script, firstScript); + } else { + resolve(); + } + }); + } + + // Create keyframes rule + const animationName = "a6ce3091ed85"; // Random name to avoid clashes + var ruleText = + "@keyframes " + + animationName + + " { from { opacity: 0.5 } to { opacity: 0.5 } }"; + var style = document.createElement("style"); + style.appendChild(document.createTextNode(ruleText)); + document.head.appendChild(style); + + // Create animation target + var div = document.createElement("div"); + document.body.appendChild(div); + + // Give the target geometry so it is eligible for layerization + div.style.width = "100px"; + div.style.height = "100px"; + div.style.backgroundColor = "white"; + + var utils = SpecialPowers.DOMWindowUtils; + + // Common clean up code + var cleanUp = function () { + div.remove(); + style.remove(); + if (utils.isTestControllingRefreshes) { + utils.restoreNormalRefresh(); + } + }; + + return waitForDocumentLoad() + .then(loadPaintListener) + .then(function () { + // Put refresh driver under test control and flush all pending style, + // layout and paint to avoid the situation that waitForPaintsFlush() + // receives unexpected MozAfterpaint event for those pending + // notifications. + utils.advanceTimeAndRefresh(0); + return waitForPaintsFlushed(); + }) + .then(function () { + div.style.animation = animationName + " 10s"; + + return waitForPaintsFlushed(); + }) + .then(function () { + var opacity = utils.getOMTAStyle(div, "opacity"); + cleanUp(); + return Promise.resolve(opacity == 0.5); + }) + .catch(function (err) { + cleanUp(); + return Promise.reject(err); + }); +} + // Checks if off-main thread animation (OMTA) is available, and if it is, runs // the provided callback function. If OMTA is not available or is not // functioning correctly, the second callback, aOnSkip, is run instead. @@ -261,88 +345,6 @@ function runOMTATest(aTestFunction, aOnSkip, specialPowersForPrefs) { ok(false, err); aOnSkip(); }); - - function isOMTAWorking() { - // Create keyframes rule - const animationName = "a6ce3091ed85"; // Random name to avoid clashes - var ruleText = - "@keyframes " + - animationName + - " { from { opacity: 0.5 } to { opacity: 0.5 } }"; - var style = document.createElement("style"); - style.appendChild(document.createTextNode(ruleText)); - document.head.appendChild(style); - - // Create animation target - var div = document.createElement("div"); - document.body.appendChild(div); - - // Give the target geometry so it is eligible for layerization - div.style.width = "100px"; - div.style.height = "100px"; - div.style.backgroundColor = "white"; - - // Common clean up code - var cleanUp = function () { - div.remove(); - style.remove(); - if (utils.isTestControllingRefreshes) { - utils.restoreNormalRefresh(); - } - }; - - return waitForDocumentLoad() - .then(loadPaintListener) - .then(function () { - // Put refresh driver under test control and flush all pending style, - // layout and paint to avoid the situation that waitForPaintsFlush() - // receives unexpected MozAfterpaint event for those pending - // notifications. - utils.advanceTimeAndRefresh(0); - return waitForPaintsFlushed(); - }) - .then(function () { - div.style.animation = animationName + " 10s"; - - return waitForPaintsFlushed(); - }) - .then(function () { - var opacity = utils.getOMTAStyle(div, "opacity"); - cleanUp(); - return Promise.resolve(opacity == 0.5); - }) - .catch(function (err) { - cleanUp(); - return Promise.reject(err); - }); - } - - function waitForDocumentLoad() { - return new Promise(function (resolve, reject) { - if (document.readyState === "complete") { - resolve(); - } else { - window.addEventListener("load", resolve); - } - }); - } - - function loadPaintListener() { - return new Promise(function (resolve, reject) { - if (typeof window.waitForAllPaints !== "function") { - var script = document.createElement("script"); - script.onload = resolve; - script.onerror = function () { - reject(new Error("Failed to load paint listener")); - }; - script.src = "/tests/SimpleTest/paint_listener.js"; - var firstScript = document.scripts[0]; - firstScript.parentNode.insertBefore(script, firstScript); - } else { - resolve(); - } - }); - } } // Common architecture for setting up a series of asynchronous animation tests diff --git a/layout/style/test/mochitest.toml b/layout/style/test/mochitest.toml index 15cc339749a6..03a66d19ba5f 100644 --- a/layout/style/test/mochitest.toml +++ b/layout/style/test/mochitest.toml @@ -696,6 +696,9 @@ fail-if = ["xorigin"] ["test_transitions_replacement_on_busy_frame.html"] +["test_transitions_replacement_on_busy_frame_omta.html"] +skip-if = ["os == 'linux' && tsan && verify"] # intermittent in chaos mode + ["test_transitions_replacement_with_setKeyframes.html"] ["test_transitions_step_functions.html"] diff --git a/layout/style/test/test_transitions_replacement_on_busy_frame_omta.html b/layout/style/test/test_transitions_replacement_on_busy_frame_omta.html new file mode 100644 index 000000000000..c2c7b26e01c0 --- /dev/null +++ b/layout/style/test/test_transitions_replacement_on_busy_frame_omta.html @@ -0,0 +1,112 @@ + + + + + + Test for bug 1626165 + + + + + + + +
+ + +