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