This is a prerequisite for effect composition. From the spec; If there is no keyframe in property-specific keyframes with a computed keyframe offset of 0, create a new keyframe with a computed keyframe offset of 0, a property value set to the neutral value for composition, and a composite operation of add, and prepend it to the beginning of property-specific keyframes And the neutral value; Define the neutral value for composition as a value which, when combined with an underlying value using the add composite operation, produces the underlying value. We don't actually need add operation to define the neutral value because the value is the zero element for each property in add operation space. I thought initially we don't need the neutral value either because we can just use the underlying value itself without add operation but it turns out the neutral value is necessary to calculate cumulative change hint. So, what I plan to do here is; 1) Obtain underlying style and store somewhere. Currently the underlying style is obtained in UpdateCascadeResults() and stored as StyleAnimationValue in EffectSet. 2) Define neutral values for each property.

I came up with another idea that is to store the underlying style value in each AnimationPropertySegment's mFromValue or mToValue and use them as it is in ComposeStyle() if we have not composed style for the property. In this way, we don't need the neutral values for each property at all.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1) > I came up with another idea that is to store the underlying style value in > each AnimationPropertySegment's mFromValue or mToValue and use them as it is > in ComposeStyle() if we have not composed style for the property. In this > way, we don't need the neutral values for each property at all. Just to make sure we are using the same terms: * base value -- the computed property value before applying any animations * underlying value -- the value passed-in to each effect in the stack representing the value to add to for additive/cumulative animations. For the effect at the bottom of the stack this is the base value. For all other effects it is the result of the effect immediately beneath it in the stack. I think we only need to cache the base value (and only for properties where all effects in the stack have a non-replace value)? In terms of removing the need to define a neutral value--isn't it enough to just substitute in the passed-in underlying value for the missing values (and force additive to 'replace')?

(In reply to Brian Birtles (:birtles) from comment #2) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #1) > > I came up with another idea that is to store the underlying style value in > > each AnimationPropertySegment's mFromValue or mToValue and use them as it is > > in ComposeStyle() if we have not composed style for the property. In this > > way, we don't need the neutral values for each property at all. > > Just to make sure we are using the same terms: > > * base value -- the computed property value before applying any animations > > * underlying value -- the value passed-in to each effect in the stack > representing the value to add to for additive/cumulative animations. For the > effect at the bottom of the stack this is the base value. For all other > effects it is the result of the effect immediately beneath it in the stack. Thanks for the glossary. > I think we only need to cache the base value (and only for properties where > all effects in the stack have a non-replace value)? Yes. > In terms of removing the need to define a neutral value--isn't it enough to > just substitute in the passed-in underlying value for the missing values > (and force additive to 'replace')? And yes. What I intended is that we can store the base value in mFromValue or mToValue for missing keyframe. If we can store it in them we don't need to add extra code in CalculateCumulativeChangeHint() to handle the base value, but after more thought we can't detect that the mFromValue or mToValue was brought by missing keyframe without a new flag in AnimationPropertySegment. We should store the base value in EffectSet. Sigh...

There are other cases we need the neutral values, it's KeyframeEffectReadOnly.getProperties(). I ended up returning back to the original plan...

This needs reversing composite order (lowest to highest).

Here are a couple of things I've noticed. * neutral values Some neutral values for properties that additive operation behaves like discreate animation type can not be pre-defined because there is no neutral values such as 'base value adds neutral value = base value'. For example, align-content: 'flex-start adds flex-end = flex-end', conversely 'flex-end adds flex-start = flex-start'. To follow this rule, 'flex-start adds neutral = neutral'. We can't get flex-start. So I am going to use uninitialize StyleAnimationValue to represent these kind of neutral values. For mCumulativeChangeHint, if any segment has the uninitialized StyleAnimationValue, we should not optimize, also for KeyframeEffect.getProperties(), I am going to return 'neutral' string for these kind of neutrtal values for now. * paused or zero playbackRate animations on the compositor We should also send them to the compositor since add or accumulate composition should be done onto those animations, and need some tweaks on the compositor, we can use initialCurrentTime here.

To compose correct style on the compositor we need fillMode flag on the compositor (part 3 patch for bug 1223658).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c40ae482fbf0 I think these patches are almost ready to be reviewed. One thing I am concerned is that it will not work with stylo because a patch uses ResolveStyleWithoutAnimation(). https://hg.mozilla.org/try/rev/2b495459acdc9d576b37e0bd667fc4f063b576e8#l2.30

Brian, I am sorry for review request bombs. I don't rush to be reviewed all of patches, but please let me know there are any suspicions that are not good ways.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #23) > Brian, I am sorry for review request bombs. I don't rush to be reviewed all > of patches, but please let me know there are any suspicions that are not > good ways. No problem. Thanks for doing all this work. Unfortunately I'm very busy with MozFest preparation so it might take up to 2 weeks before I get to these reviews since I'll try to prioritize the smaller/more urgent reviews in my queue.

Comment on attachment 8803145 [details] Bug 1305325 - Part 1: Move test cases in file_partial_keyframes.html into file_disable_animations_api_core.html. https://reviewboard.mozilla.org/r/87368/#review86500 LGTM.

Comment on attachment 8803146 [details] Bug 1305325 - Part 3: Make AnimationPropertyValueDetails::mValue optional. https://reviewboard.mozilla.org/r/87370/#review90616 I need to read the rest of the patches to see if this approach makes sense, but at least for now I think there's an issue with transform values. ::: layout/style/StyleAnimationValue.h:545 (Diff revision 1) > + // Create a neutral value for |aProperty|. > + // Returns uninitialized StyleAnimationValue if neutral value is indefinable > + // for |aProperty|, e.g. discrete type animation. > + static StyleAnimationValue NeutralValue(nsCSSPropertyID aProperty); See comments on transform below, but the spec defines the neutral value for composition as, "a value which, when combined with an underlying value using the add composite operation, produces the underlying value"[1] I think we need to take particular care of any properties that are interpolable for some values but not others (e.g. certain keywords that remain keywords even in the computed value). Basically, we need to guarantee the following: AddWeighted(aProperty, 0.0, NeutralValue(aProperty), 1.0, someKeywordValue, aResult); MOZ_ASSERT(aResult == someKeywordValue); Another approach might be just to define a StyleAnimationValue unit of NeutralValue and make GetCommonUnit and AddWeighted deal with it appropriately. Or perhaps just re-use the null unit for that? I'm not sure what's going to be easier / more robust but at this stage that seems simpler. [1] https://w3c.github.io/web-animations/#neutral-value-for-composition ::: layout/style/StyleAnimationValue.cpp:4902 (Diff revision 1) > + case eCSSProperty_transform: { > + // transform: matrix3d(1, 0, 0, 0, > + // 0, 1, 0, 0, > + // 0, 0, 1, 0, > + // 0, 0, 0, 1) > + nsAutoPtr<nsCSSValueList> value; > + nsCSSValueList **tail = getter_Transfers(value); > + RefPtr<nsCSSValue::Array> arr = > + AppendTransformFunction(eCSSKeyword_matrix3d, tail); Shouldn't the neutral value just be 'none' ? As I understand it, if we have keyframes: { offset: 1.0, transform: 'scale(1)' } Then we'd synthesize a keyframe: { offset: 0.0, transform: <neutral value>, composite: 'add' } If <neutral value> here is a transform list with value 'none' then, when we add it to an underlying value of 'none', we'd get 'none'. Then when we go to interpolate in AddWeighted we'd turn it into: transform: 'scale(1)', coeff: 0 transform: 'scale(1)', coeff: 1 And correctly produce a value like: transform: 'scale(0.5)' If we use a matrix value here, it seems like we'll need to do more work to turn that back into a scale value (and might not do the right thing at all for rotate values). Likewise, if we are interpolating from additive <neutral> to 'none', we should get 'none'.

Thinking about this a bit more, it seems like we could just use null to represent a neutral value. Then, in AddWeighted, do some generic null handling like we do for 'none' transform lists.

Comment on attachment 8803147 [details] Bug 1305325 - Part 2: Add AnimValuesStyleRule::GetValue and HasValue to get the last composed style. https://reviewboard.mozilla.org/r/87372/#review90640 ::: dom/animation/AnimValuesStyleRule.h:48 (Diff revision 1) > // For the following functions, it there is already a value for |aProperty| it > // will be replaced with |aValue|. > void AddValue(nsCSSPropertyID aProperty, const StyleAnimationValue &aValue); > void AddValue(nsCSSPropertyID aProperty, StyleAnimationValue&& aValue); > > + bool HasPropertyValue(nsCSSPropertyID aProperty) const { Nit: I think HasValue would be more consistent with the other methods in this file.

Comment on attachment 8803148 [details] Bug 1305325 - Part 4: Add a function that returns the resolved base style on an element for a given property with nsStyleContext. https://reviewboard.mozilla.org/r/87374/#review90644 In general, I really like this patch. However, we need to work out if we can do this from UpdateProperties or not (see below) so I'm clearing the review request for now. From the commit message: > Now EffectSet has a hashtable to store non-animating base value for each > property as StyleAnimationValue. StyleAnimationValue is convinient to > be used in KeyframeEffectReadOnly::ComposeStyle(). "This patch adds a hashtable to store the non-animated base value of each property animated by the effects in EffectSet. The values are stored as StyleAnimationValue objects since they are intended to be used in KeyframeEffectReadOnly::ComposeStyle." > The hashtable is updated in UpdateCascadeResults(). We already have context-sensitive animation values (like em-based values) that need to be updated any time the parent or ancestor style context changes. It seems like base values should be updated at the same time? > The reason why we don't update the hashtable in UpdateProperties() is; > > 1) We have to update it even if there is no animation properties changes > because changing non-animating style value does not cause any animation > property changes. If we do it in Updateproperties() it's apparently less > effective. We already need to call UpdateProperties() even when animation properties have not changed (e.g. when the parent font-size has changed). > 2) There is the case that we have no EffectSet in UpdateProperties(), e.g. > creating an animation. > What happens on Element.animate(); > i) KeyframeEffect constructor > i-1) UpdateProperties() > ii) Animation constructor with the effect > ii-1) KeyframeEffectReadOnly::SetAnimation() > ii-2) The effect is added in EffectSet We should probably just make sure UpdateProperties (or UpdateBaseStyleSet) is called in that case. ::: dom/animation/EffectCompositor.cpp:756 (Diff revision 1) > > + if (aStyleContext) { > + UpdateBaseStyleSet(aEffectSet, > + aElement, > + aStyleContext, > + animatedProperties); > + } Obviously this will change if we end up doing this from UpdateProperties, but I wonder if we can avoid calling this if we don't have any additive animations? The check wouldn't have to be perfect, it could be as simple as KeyframeEffectReadOnly::NeedsBaseValues() or something that returns true if the effect has at least one keyframe with a composite value that isn't replace or is missing a 0%/100% value for any of its animated properties (and hence will require a composite value). If we do this inside UpdateProperties then we could iterate over mProperties and calculate this even more easily. I'm just concerned about calling ResolveStyleWithoutAnimation unnecessarily (and it will be unnecessary 90% or more of the time, I think). What do you think? ::: dom/animation/EffectCompositor.cpp:844 (Diff revision 1) > + for (size_t iHigh = 0; iHigh < nsCSSPropertyIDSet::kChunkCount; ++iHigh) { > + if (!aTargetProperties.HasPropertyInChunk(iHigh)) { > + continue; > + } > + for (size_t iLow = 0; iLow < nsCSSPropertyIDSet::kBitsInChunk; ++iLow) { > + if (!aTargetProperties.HasPropertyAt(iHigh, iLow)) { > + continue; > + } As a separate patch/bug, could we create an iterator class for nsCSSPropertyIDSet to encapsulate this looping? Then we could use it here and in: nsCSSExpandedDataBlock::ComputeNumProps nsCSSExpandedDataBlock::Compress nsCSSExpandedDataBlock::Clear as well as (probably) places like EffectCompositor::GetOverriddenProperties where we currently create a nsCSSPropertyIDSet *and* a separate AutoTArray just for the purpose of iterating over it in nsRuleNode::ComputePropertiesOverridingAnimation. (EffectSet already includes an example of an iterator that works with range-based for loops although it's a bit wasteful the way it represents end iterators.) ::: dom/animation/EffectSet.h:210 (Diff revision 1) > + BaseStyleValueSet& GetBaseStyleValues() > + { > + return mBaseStyleValues; > + } (I'm curious what we need this for. It seems like it would be a lot neater if we could avoid exposing this, but maybe we need it. I'll have to review the other patches to find out!) ::: dom/animation/EffectSet.h:263 (Diff revision 1) > // Specifies the properties for which the result will be added to the > // animations level of the cascade and hence should be skipped when we are > // composing the animation style for the transitions level of the cascede. > nsCSSPropertyIDSet mPropertiesForAnimationsLevel; > > + BaseStyleValueSet mBaseStyleValues; This probably deserves a comment. e.g. // The non-animated value for properties animated by effects in this set // that contain at least one animation value that is composited with the // underlying value. Or something like that, depending on how strictly we limit what values we store here.

Comment on attachment 8803149 [details] Bug 1305325 - Part 6: Handle missing keyframe whose offset 0 or 1 on the main thread. https://reviewboard.mozilla.org/r/87376/#review90634 This is good but I'd like to see it again with the following changes, particularly those to BuildSegmentsFromValueEntries. ::: dom/animation/KeyframeEffectReadOnly.h:128 (Diff revision 1) > + // NOTE: In case that no keyframe for 0 or 1 offset is specified and > + // additive operation for the property is not defined, the unit of > + // mFromValue or mToValue is eUnit_Null because we can't define neutral > + // values for such properties. I think we won't need this comment (or we might need a different one) depending on what we do with part 2. ::: dom/animation/KeyframeEffectReadOnly.h:135 (Diff revision 1) > + Maybe<dom::CompositeOperation> mFromComposite; > + Maybe<dom::CompositeOperation> mToComposite; > + Why don't we just resolve this down to the concrete composite operation when we perform UpdateProperties? i.e. drop the Maybe<> here? We'll need to make setting effect.composite call UpdateProperties and we might need to update some tests for GetProperties() but overall I think it will be simpler. ::: dom/animation/KeyframeEffectReadOnly.h:371 (Diff revision 1) > + // Returns composited value for |aProperty| in consideration of > + // |aCompositeOperation|. > + void GetCompositedValue( > + nsCSSPropertyID aProperty, > + const RefPtr<AnimValuesStyleRule>& aStyleRule, > + const StyleAnimationValue& aInputValue, > + CompositeOperation aCompositeOperation, > + StyleAnimationValue& aResult); Can we call |aStyleRule|, |aAnimationRule| ? And |aInputValue|, |aValueToComposite| (the spec calls is "value to combine" so that would be fine too I guess)? Then make the comment: // Composites |aValueToComposite| using |aCompositeOperation| onto the // value for |aProperty| in |aAnimationRule|, or, if there is no suitable // value in |aAnimationRule|, uses the base value for the property recorded // on the target element's EffectSet. Oh, and perhaps just call it 'CompositeValue' ? Could it return StyleAnimationValue as its return value? I think we could avoid unnecessary copies easily enough? ::: dom/animation/KeyframeEffectReadOnly.cpp:349 (Diff revision 1) > + EffectSet* effectSet = > + EffectSet::GetEffectSet(mTarget->mElement, mTarget->mPseudoType); (Do we need to assert that mTarget is set? Or do our call sites already check that?) ::: dom/animation/KeyframeEffectReadOnly.cpp:357 (Diff revision 1) > + // If we are composing with composite operation that is not 'replace' > + // and we have not composed style for the property yet, we have to get > + // the base style for the property. > + DebugOnly<bool> success = > + effectSet->GetBaseStyleAnimationValue(aProperty, aResult); > + MOZ_ASSERT(success, "The base style should have already set"); Nit: "The base style should be set" ::: dom/animation/KeyframeEffectReadOnly.cpp:360 (Diff revision 1) > + if (aResult.IsNull()) { > + // We'd like to fail silently. > + aResult = aInputValue; > + return; > + } This is going to change, I guess, depending on how we handle part 2. Perhaps we won't need this check at all. ::: dom/animation/KeyframeEffectReadOnly.cpp:817 (Diff revision 1) > { > aResult.mOffset = aOffset; > > nsString stringValue; > + if (aValue.IsNull()) { > + stringValue.AssignLiteral("neutral"); I don't know why but I feel like "null" would be better here somehow. I actually wonder if we should change the IDL to make the member a nullable string and return an actual null value here? There might be some issues with old versions of DevTools not handling these null values correctly, we'd have to check, but it's possibly ok. ::: dom/animation/KeyframeEffectReadOnly.cpp:1328 (Diff revision 1) > + // In case of neutral values for properties that additive operation is not > + // defined, we can't decide what the exact neutral value is until we > + // compose it onto underlying value. For this reason, we don't optimize > + // such properties. I think we don't know the exact value for any additive properties until we composite them right? Even if the additive operation is defined? Maybe I am misunderstanding this code? ::: dom/animation/KeyframeUtils.cpp:1135 (Diff revision 1) > eCSSPropertyExtra_no_properties; > } > > +static void > +AppendInitialSegment(AnimationProperty* aAnimationProperty, > + KeyframeValueEntry& aEntry) s/aEntry/aFirstEntry/ ? ::: dom/animation/KeyframeUtils.cpp:1141 (Diff revision 1) > +{ > + AnimationPropertySegment* segment = > + aAnimationProperty->mSegments.AppendElement(); > + segment->mFromKey = 0.0f; > + segment->mFromValue = > + Move(StyleAnimationValue::NeutralValue(aAnimationProperty->mProperty)); Is Move() needed here? Isn't the result already an rvalue? Likewise below. (Then again, depending on part 2, we might not need to call anything here and could just leave this as an uninitialized/null StyleAnimationValue.) ::: dom/animation/KeyframeUtils.cpp:1144 (Diff revision 1) > + segment->mFromKey = 0.0f; > + segment->mFromValue = > + Move(StyleAnimationValue::NeutralValue(aAnimationProperty->mProperty)); > + segment->mToKey = aEntry.mOffset; > + segment->mToValue = aEntry.mValue; > + segment->mTimingFunction = aEntry.mTimingFunction; Is this right? Shouldn't we use linear timing here? The timing function specified for the first keyframe should be from that point on, right? ::: dom/animation/KeyframeUtils.cpp:1145 (Diff revision 1) > + segment->mFromValue = > + Move(StyleAnimationValue::NeutralValue(aAnimationProperty->mProperty)); > + segment->mToKey = aEntry.mOffset; > + segment->mToValue = aEntry.mValue; > + segment->mTimingFunction = aEntry.mTimingFunction; > + segment->mFromComposite.emplace(dom::CompositeOperation::Add); Do we need to set mToComposite to aEntry.mComposite? (Or is that done in a later patch, perhaps?) ::: dom/animation/KeyframeUtils.cpp:1160 (Diff revision 1) > + segment->mFromValue = aEntry.mValue; > + segment->mToKey = 1.0f; > + segment->mToValue = > + Move(StyleAnimationValue::NeutralValue(aAnimationProperty->mProperty)); > + segment->mTimingFunction = aEntry.mTimingFunction; > + segment->mToComposite.emplace(dom::CompositeOperation::Add); Do we need to set mFromComposite or is that done in a later patch? ::: dom/animation/KeyframeUtils.cpp:1195 (Diff revision 1) > // Typically, for each property in |aEntries|, we expect there to be at least > // one KeyframeValueEntry with offset 0.0, and at least one with offset 1.0. > // However, since it is possible that when building |aEntries|, the call to > // StyleAnimationValue::ComputeValues might fail, this can't be guaranteed. > // Furthermore, since we don't yet implement additive animation and hence > // don't have sensible fallback behavior when these values are missing, the > // following loop takes care to identify properties that lack a value at > // offset 0.0/1.0 and drops those properties from |aResult|. I think we need to update/drop this comment. ::: dom/animation/KeyframeUtils.cpp:1210 (Diff revision 1) > AnimationProperty* animationProperty = nullptr; > > size_t i = 0, n = aEntries.Length(); > > while (i < n) { > // Check that the last property ends with an entry at offset 1. // If we've reached the end of the array of entries, synthesize final (and // initial) segment if necessary ::: dom/animation/KeyframeUtils.cpp:1212 (Diff revision 1) > - if (aEntries[i].mOffset != 1.0f && animationProperty) { > - aResult.RemoveElementAt(aResult.Length() - 1); > - animationProperty = nullptr; > + if (aEntries[i].mOffset != 1.0f) { > + if (!animationProperty) { > + animationProperty = aResult.AppendElement(); I think we only want to do this if additive animation is enabled? ::: dom/animation/KeyframeUtils.cpp:1212 (Diff revision 1) > - if (aEntries[i].mOffset != 1.0f && animationProperty) { > - aResult.RemoveElementAt(aResult.Length() - 1); > - animationProperty = nullptr; > + if (aEntries[i].mOffset != 1.0f) { > + if (!animationProperty) { > + animationProperty = aResult.AppendElement(); > + animationProperty->mProperty = aEntries[i].mProperty; > + } > + // If we have the only one entry whose offset is neither 1 nor 0, > + // we need to append both of the initial and the final segment. > + if (n == 1 && aEntries[i].mOffset != 0.0f) { > + AppendInitialSegment(animationProperty, aEntries[i]); > + } > + AppendFinalSegment(animationProperty, aEntries[i]); > + } else if (!animationProperty) { // 1 offset > + // If the last entry with offset 1 for the property has not yet > + // appended, that means this is only one entry for the property, > + // append the initial segment with 0 offset. > + animationProperty = aResult.AppendElement(); > + animationProperty->mProperty = aEntries[i].mProperty; > + AppendInitialSegment(animationProperty, aEntries[i]); > } > break; > } (I think this is mostly right. However, I wonder if there's any simpler way we can do it? This code is pretty tricky and probably even more complicated once we switch behavior depending on if additive animation is enabled or not. Also, we've had security bugs in this area before so we need to be really careful here. If you manage to find a way to make it simpler that would be great!) ::: dom/animation/KeyframeUtils.cpp:1217 (Diff revision 1) > - if (aEntries[i].mOffset != 1.0f && animationProperty) { > - aResult.RemoveElementAt(aResult.Length() - 1); > - animationProperty = nullptr; > + if (aEntries[i].mOffset != 1.0f) { > + if (!animationProperty) { > + animationProperty = aResult.AppendElement(); > + animationProperty->mProperty = aEntries[i].mProperty; > + } > + // If we have the only one entry whose offset is neither 1 nor 0, Nit: If we have only one entry... ::: dom/animation/KeyframeUtils.cpp:1218 (Diff revision 1) > - aResult.RemoveElementAt(aResult.Length() - 1); > - animationProperty = nullptr; > + if (!animationProperty) { > + animationProperty = aResult.AppendElement(); > + animationProperty->mProperty = aEntries[i].mProperty; > + } > + // If we have the only one entry whose offset is neither 1 nor 0, > + // we need to append both of the initial and the final segment. Nit: s/both of the/both the/ ::: dom/animation/KeyframeUtils.cpp:1219 (Diff revision 1) > - animationProperty = nullptr; > + animationProperty = aResult.AppendElement(); > + animationProperty->mProperty = aEntries[i].mProperty; > + } > + // If we have the only one entry whose offset is neither 1 nor 0, > + // we need to append both of the initial and the final segment. > + if (n == 1 && aEntries[i].mOffset != 0.0f) { I don't think this is right. n is not the number of entries for this property, it's the number of entries in total. If we have only one value for the last property in the sequence, we should still add both segments. e.g. { offset: 0.0, left: '10px' } { offset: 1.0, left: '100px' } { offset: 0.5, right: '50px' } In this case, n == 3 but we should still add both an initial and final segment. ::: dom/animation/KeyframeUtils.cpp:1238 (Diff revision 1) > - // Skip properties that don't have an entry with offset 0. > + // Append the initial segment with offset 0 if offset of the first entry > + // for the property is not 0. > if (aEntries[i].mProperty != lastProperty && > aEntries[i].mOffset != 0.0f) { Again, should we only do this if additive animation is enabled? ::: dom/animation/test/chrome/test_animation_properties.html:661 (Diff revision 1) > { desc: 'a property that can\'t be resolved to computed values in' > + ' final keyframe where it forms the last segment in the series', > frames: [ { margin: '5px' }, > - { margin: '5px', > + { marginLeft: '5px', > - marginLeft: '5px', > marginRight: '5px', > - marginBottom: '5px', > + marginBottom: '5px' } ], > - // margin-top sorts last and only it will be missing since > - // the other longhand components are specified > - simulateComputeValuesFailure: true } ], > expected: [ { property: 'margin-bottom', > values: [ value(0, '5px', 'replace', 'linear'), > value(1, '5px', 'replace') ] }, > { property: 'margin-left', > values: [ value(0, '5px', 'replace', 'linear'), > value(1, '5px', 'replace') ] }, > { property: 'margin-right', > values: [ value(0, '5px', 'replace', 'linear'), > - value(1, '5px', 'replace') ] } ] > + value(1, '5px', 'replace') ] }, > + { property: 'margin-top', > + values: [ value(0, '5px', 'replace', 'linear'), > + value(1, '0px', 'add') ] } ] As discussed, I'm not sure what we should do here. So long as we are still shipping without additive animation, perhaps we should keep the simulateComputeValuesFailure tests (and move them to a separate file where we turn off the pref that turns on additive animation?) Without that, these tests are really just testing that we fill in keyframes correctly--which is still important to test, but it doesn't match the test description. The same applies to all the test changes in this file, I think. So, one approach might be: 1. Copy the simulateComputeValuesFailure tests to a separate test file. 2. Run that test file with the pref turned off. 3. Keep the changes to this file but update the test description. 4. Check that there are no missing tests / tests out of order regarding filling in missing keyframe values. ::: dom/animation/test/chrome/test_restyles.html:814 (Diff revision 1) > + // Tests that animations that have no keyframe whose offset 0 or 1 and > + // its neutral value is unable to pre-defined don't throttle at all. As discussed, I think this applies to all animation properties, not just those whose animation type is discrete. ::: dom/animation/test/chrome/test_restyles.html:816 (Diff revision 1) > } > ); > > + // Tests that animations that have no keyframe whose offset 0 or 1 and > + // its neutral value is unable to pre-defined don't throttle at all. > + add_task(function* no_throttling_animations_out_of_view_element() { Nit: Should we start using async/await in future? e.g. see bug 1313956

Comment on attachment 8803150 [details] Bug 1305325 - Part 7: The expected value of offset is 1.0 for input value whose offset is 1.0. https://reviewboard.mozilla.org/r/87378/#review90688

Comment on attachment 8803151 [details] Bug 1305325 - Part 8: Tests composed style for missing keyframe for properties running on the main thread. https://reviewboard.mozilla.org/r/87380/#review90692 r=me with the following changes (as you see fit) ::: dom/animation/test/style/file_missing-keyframe.html:5 (Diff revision 1) > +div { > + /* Element needs geometry to be eligible for layerization */ > + width: 100px; > + height: 100px; > + background-color: white; > +} Is this needed? ::: dom/animation/test/style/file_missing-keyframe.html:22 (Diff revision 1) > + var div = addDiv(t, { style: 'margin-left: 100px' }); > + div.animate([{ marginLeft: '200px' }], 100 * MS_PER_SEC); > + > + assert_equals(getComputedStyle(div).marginLeft, '100px', > + 'The initial margin-left value should be the base value'); > +}, 'Initial margin-left value that the animation has no keyframe whose ' + s/that the animation/for an animation that/ Likewise elsewhere in this file. ::: dom/animation/test/style/file_missing-keyframe.html:35 (Diff revision 1) > +}, 'Initial margin-left value that the animation has no keyframe whose ' + > + 'offset 0 on a lower-priority animation'); Initial margin-left value for an animation with no keyframe at offset 0 is that of lower-priority animations. ::: dom/animation/test/style/file_missing-keyframe.html:39 (Diff revision 1) > + var div = addDiv(t, { style: 'margin-left: 100px;' + > + 'transition: margin-left 100s'}); > + flushComputedStyle(div); > + > + div.style.marginLeft = '200px'; > + flushComputedStyle(div); > + > + div.animate([{ marginLeft: '300px' }], 100 * MS_PER_SEC); > + > + assert_equals(getComputedStyle(div).marginLeft, '100px', If we use a negative transition delay, then we could be extra sure we're getting the transition style? (and not simply that we just failed to update marginLeft?) ::: dom/animation/test/style/file_missing-keyframe.html:58 (Diff revision 1) > + fill: 'forwards' }); // Without fill:forwards > + // we can't tell whether > + // the final value is > + // composed by animations > + // or not. > + > + animation.currentTime = 100 * MS_PER_SEC; Why not seek to 50 * MS_PER_SEC and check that instead? Then we'll be sure that we don't accidentally pass by failing to implement fill 'forwards' correctly? ::: dom/animation/test/style/file_missing-keyframe.html:72 (Diff revision 1) > + var lowerAnimation = div.animate([{ offset: 0, marginLeft: '200px' }, > + { offset: 1, marginLeft: '300px' }], > + { duration: 100 * MS_PER_SEC, > + fill: 'forwards' }); > + var higherAnimation = div.animate([{ offset: 0, marginLeft: '400px' }], > + { duration: 100 * MS_PER_SEC, > + fill: 'forwards' }); > + > + lowerAnimation.currentTime = 100 * MS_PER_SEC; > + higherAnimation.currentTime = 100 * MS_PER_SEC; Likewise here, it might be more interesting to seek the animations to 50% and check we get the expected result? ::: dom/animation/test/style/file_missing-keyframe.html:94 (Diff revision 1) > + div.style.marginLeft = '300px'; > + flushComputedStyle(div); > + > + div.animate([{ offset: 0, marginLeft: '200px' }], > + { duration: 100 * MS_PER_SEC, > + fill: 'forwards' }); // Without fill:forwards we can't tell > + // whether the final value is composed by > + // animations or not. > + > + div.getAnimations().forEach(animation => { > + animation.currentTime = 100 * MS_PER_SEC; > + }); > + > + assert_equals(getComputedStyle(div).marginLeft, '300px', > + 'The final margin-left value should be the final value ' + > + 'of the transition'); Again, if you agree, I think we could just test at 50% ? That would still tell us if we're setting the correct 100% value.

Comment on attachment 8803152 [details] Bug 1305325 - Part 11: Cache non-animated base values. https://reviewboard.mozilla.org/r/87382/#review90714 See comments below, but, as discussed, not only is pausing problematic, it's not clear to me that we do the correct thing when all animations are filling forwards, i.e. keep them on the compositor until all animations on that element/property have finished/paused. Roughly speaking I think we might want the behavior to be something like: 1. If we have at least one running animation, pass all animations to the compositor. 2. If all animations are paused/finished, pass none to the compositor. In future we can refine (1) as follows: 1a. Only pass paused/finished animations if we have a least one animation with a non-replace composite mode. 1b. Do the check per-property. e.g. if we have a running 'opacity' animation and a finished 'transform' animation, we don't need to pass the 'transform' animation to the compositor. (1b) is particularly unimportant at the moment but in the future when the number of compositor-animatable properties grow, it might become worthwhile. ::: dom/animation/Animation.h:284 (Diff revision 1) > (PlayState() == AnimationPlayState::Running || > - mPendingState == PendingState::PlayPending) && > + mPendingState == PendingState::PlayPending || > + IsPausedOrPausing()) && Expanding these three lines: PlayState() == AnimationPlayState::Running || mPendingState == PendingState::PlayPending || IsPausedOrPausing() is equal to: PlayState() == AnimationPlayState::Running || mPendingState == PendingState::PlayPending || (PlayState() == AnimationPlayState::Paused || mPendingState == PendingState::PausePending) i.e. PlayState() == AnimationPlayState::Running || PlayState() == AnimationPlayState::Paused || mPendingState == PendingState::PlayPending || mPendingState == PendingState::PausePending which can be written: PlayState() == AnimationPlayState::Running || PlayState() == AnimationPlayState::Paused || PlayState() == AnimationPlayState::Pending since the first three lines of PlayState() are: if (mPendingState != PendingState::NotPending) { return AnimationPlayState::Pending; } and there are only three values of mPendingState: enum class PendingState { NotPending, PlayPending, PausePending }; PendingState mPendingState; There are only five values of AnimationPlayState (ignoring EndGuard_): enum class AnimationPlayState : uint32_t { Idle, Pending, Running, Paused, Finished, EndGuard_ }; So the only cases *not* covered above are Idle and Finished. Now HasCurrentEffect is: bool HasCurrentEffect() const { return GetEffect() && GetEffect()->IsCurrent(); } And IsCurrent is: bool AnimationEffectReadOnly::IsCurrent() const { if (!mAnimation || mAnimation->PlayState() == AnimationPlayState::Finished) { return false; } ComputedTiming computedTiming = GetComputedTiming(); return computedTiming.mPhase == ComputedTiming::AnimationPhase::Before || computedTiming.mPhase == ComputedTiming::AnimationPhase::Active; } So if we're Finished, IsCurrent will return false. Furthermore, if we're Idle, inside GetComputedTimine, |aLocalTime| will be null and we have the lines: // The default constructor for ComputedTiming sets all other members to // values consistent with an animation that has not been sampled. if (aLocalTime.IsNull()) { return result; } At this point, result.mPhase has not been set so it will be the value set in its member initializer: AnimationPhase mPhase = AnimationPhase::Null; i.e. neither Before nor Active and so IsCurrent() will return false. As a result, the following: return HasCurrentEffect() && (PlayState() == AnimationPlayState::Running || mPendingState == PendingState::PlayPending || IsPausedOrPausing()) && !GetEffect()->IsActiveDurationZero(); Is equivalent to: return HasCurrentEffect() && !GetEffect()->IsActiveDurationZero(); That said, as discussed, I think this will produce a performance regression for content that sets up a lot of paused animations on document load, and then unpauses them in response to events. We could perhaps: a. Only send paused animations when we know we have at least one animation in the effect set that uses 'composite' of something other than 'replace'. b. Only send paused animations when we have at least one *running* animation in the effect set c. Add logic to the compositor to not require ticks when all animations are paused. I was thinking (c), but now I think (b) might actually be best, or perhaps some combination of (a) and (b). See my comments at the start of this review which are probably more accurate. I was thinking we could do this in a separate bug but, as discussed, it seems like this could be a significant performance regression. i.e. pages with many paused animations will suddenly make the compositor continuously busy. ::: gfx/layers/composite/AsyncCompositionManager.cpp:675 (Diff revision 1) > - (aPoint - animation.startTime()).MultDouble(animation.playbackRate()); > + ? (aPoint - animation.startTime()).MultDouble(animation.playbackRate()) > + : animation.initialCurrentTime() + animation.delay(); Nit I think the lines beginning with ? and : should be indented. ::: gfx/layers/composite/AsyncCompositionManager.cpp:676 (Diff revision 1) > > MOZ_ASSERT(!animation.startTime().IsNull(), > "Failed to resolve start time of pending animations"); > - TimeDuration elapsedDuration = > - (aPoint - animation.startTime()).MultDouble(animation.playbackRate()); > + TimeDuration elapsedDuration = animation.playbackRate() != 0 > + ? (aPoint - animation.startTime()).MultDouble(animation.playbackRate()) > + : animation.initialCurrentTime() + animation.delay(); For my own reference, when the playback rate is set to zero, we store the current time as the hold time and use that as the current time. See: https://w3c.github.io/web-animations/#updating-the-playback-rate-of-an-animation Which calls: https://w3c.github.io/web-animations/#silently-set-the-current-time Then we initialize the current time in nsDisplayList.cpp as: animation->initialCurrentTime() = aAnimation->GetCurrentTime().Value() - timing.mDelay; So here we are just adding it back so that we end up passing the current time to GetComputedTimingAt below. ::: layout/base/nsDisplayList.cpp:382 (Diff revision 1) > MOZ_ASSERT(!aAnimation->GetCurrentOrPendingStartTime().IsNull() || > + aAnimation->IsPausedOrPausing() || > (aAnimation->GetTimeline() && > aAnimation->GetTimeline()->TracksWallclockTime()), > "Animation should either have a resolved start time or " > "a timeline that tracks wallclock time"); I think this assertion might be wrong. It was added back in [1] with the changeset, "Handle Timeline() returning null", and looking at nsDisplayList.cpp at that time[2] it seems like we were concerned about the following lines: Nullable<TimeDuration> startTime = aAnimation->GetCurrentOrPendingStartTime(); animation->startTime() = startTime.IsNull() ? TimeStamp() : aAnimation->Timeline()->ToTimeStamp( startTime.Value() + timing.mDelay); I think what I was trying to ensure was that if startTime was *NOT* null, that aAnimation->Timeline() would be true and that ToTimeStamp() would not return null. I think we can change this assertion to: MOZ_ASSERT(aAnimation->GetCurrentOrPendingStartTime().IsNull() || (aAnimation->GetTimeline() && aAnimation->GetTimeline()->TracksWallclockTime()), "If the animation has a resolved start time it should " "have a timeline capable of converting it to a TimeStamp"); What do you think? [1] https://hg.mozilla.org/mozilla-central/rev/1781b7760ddb5f7dae5399dc8e5df17d6893024c [2] https://hg.mozilla.org/mozilla-central/file/1781b7760ddb/layout/base/nsDisplayList.cpp ::: layout/base/nsDisplayList.cpp:437 (Diff revision 1) > - animation->playbackRate() = aAnimation->PlaybackRate(); > + // On the compositor we regard zero playback rate as paused. > + animation->playbackRate() = > + aAnimation->IsPausedOrPausing() ? 0 : aAnimation->PlaybackRate(); Nice. Perhaps the comment could be: // We don't have a separate 'paused' field since we can achieve // the same result by using a zero playback rate. Or something like that?

Comment on attachment 8803153 [details] Bug 1305325 - Part 9: Send animations even if it's paused, finished or zero playback rate. . https://reviewboard.mozilla.org/r/87384/#review90722 I haven't reviewed the tests part since I think this will change based on the issues discussed in the previous patch. ::: dom/animation/Animation.h:284 (Diff revision 1) > - return HasCurrentEffect() && > - (PlayState() == AnimationPlayState::Running || > + return (HasCurrentEffect() || IsInEffect()) && > + PlayState() != AnimationPlayState::Idle; > - mPendingState == PendingState::PlayPending || > - IsPausedOrPausing()) && > - !GetEffect()->IsActiveDurationZero(); Oh, you worked out the same thing as me. Except, I'm pretty sure both HasCurrentEffect() and IsInEffect() will return false when PlayState() is Idle so we don't need this last check. ::: dom/animation/test/chrome/test_running_on_compositor.html:967 (Diff revision 1) > +promise_test(function(t) { > + var div = addDiv(t); > + var animation = div.animate({ opacity: [ 0, 1 ] }, > + { duration: 0, fill: 'forwards' }); > + > + return animation.ready.then(function() { > + assert_animation_is_running_on_compositor(animation, > + 'Zero duration animation with fill:forwards reports that it is ' + > + 'running on the compositor'); > + }); > +}, 'Zero duration animation with fill:forwards is running on the compositor'); As discussed in the previous patch, I don't think we want this to run on the compositor. When it's finished and nothing else on the element is moving, we should take it off the compositor. ::: gfx/layers/composite/AsyncCompositionManager.cpp:688 (Diff revision 1) > + ? (aPoint - animation.startTime()).MultDouble(animation.playbackRate()) > + : animation.initialCurrentTime() + animation.delay(); This indentation needs to be fixed ::: gfx/layers/composite/AsyncCompositionManager.cpp:690 (Diff revision 1) > + if (animation.playbackRate() > 0 && > + elapsedDuration > timing.EndTime()) { > + elapsedDuration = timing.EndTime(); So this is because the compositor doesn't do finishing behavior I guess. I think this isn't strictly correct since it's possible to seek an animation past its end time and have it still contribute to the composited result. We probably need to find another mechanism for this such as passing paused/finished animations along with a flag indicating they are paused and including their current time.

Comment on attachment 8803154 [details] Bug 1305325 - Part 12: Pass base value for opacity or transform to the compositor. https://reviewboard.mozilla.org/r/87386/#review91116 I think Markus or Matt should review this. I really don't know our layers code well enough. One bit of feedback, however, is that I find the function names here a little long and confusing. We have * GetBaseOpacityForAnimation * GetBaseOpacityStyleAnimationValue * SetBaseOpacityForAnimation * SetBaseOpacityStyleAnimationValue * GetBaseTransformForAnimation * GetBaseTransformStyleAnimationValue * SetBaseTransformForAnimation * SetBaseTransformStyleAnimationValue Since converting opacity to a StyleAnimationValue is cheap, I wonder if we need to store a StyleAnimationValue for that or if we could just create one on each sample? Then, what if we name the transform ones follows: * SetAnimationBaseTransformAsList (takes array of TransformFunction) * SetAnimationBaseTransformAsValue (takes StyleAnimationValue) Perhaps we could even drop the 'Animated' part and just put that in a comment with the member. That would leave you with: * GetBaseOpacity * SetBaseOpacity * GetBaseTransformAsList * GetBaseTransformAsValue * SetBaseTransformAsList * SetBaseTransformAsValue Or perhaps there's another way still?

Comment on attachment 8803155 [details] Bug 1305325 - Part 14: Compose base values on the compositor. https://reviewboard.mozilla.org/r/87388/#review91034 This looks good but I'd like to check again with the following changes. ::: gfx/layers/composite/AsyncCompositionManager.cpp:577 (Diff revision 1) > static void > -SampleValue(float aPortion, Animation& aAnimation, > - const StyleAnimationValue& aStart, const StyleAnimationValue& aEnd, > - const StyleAnimationValue& aLastValue, uint64_t aCurrentIteration, > - Animatable* aValue, Layer* aLayer) > -{ > - NS_ASSERTION(aStart.GetUnit() == aEnd.GetUnit() || > +GetCompositedValue(Layer* aLayer, > + const Animation& aAnimation, > + const StyleAnimationValue& aInputValue, > + dom::CompositeOperation aCompositeOperation, > + const StyleAnimationValue& aUnderlyingValue, > + StyleAnimationValue& aResult) Can we make this return a StyleAnimationValue and call it CompositeValue? ::: gfx/layers/composite/AsyncCompositionManager.cpp:580 (Diff revision 1) > } > > static void > -SampleValue(float aPortion, Animation& aAnimation, > - const StyleAnimationValue& aStart, const StyleAnimationValue& aEnd, > - const StyleAnimationValue& aLastValue, uint64_t aCurrentIteration, > +GetCompositedValue(Layer* aLayer, > + const Animation& aAnimation, > + const StyleAnimationValue& aInputValue, As with the main thread, I think |aValueToComposite| would be more clear here (or |aValueToCombine| if you want to match the spec). ::: gfx/layers/composite/AsyncCompositionManager.cpp:591 (Diff revision 1) > + aResult = aUnderlyingValue; > + switch (aCompositeOperation) { > + case dom::CompositeOperation::Add: > + // So far nothing to do since we come to here only in case of missing > + // keyframe, that means we have only to use the base value or the > + // underlying value as the composited value. > + // FIXME: Bug 1311620: Once we implement additive operation, we need to > + // calculate it here. > + break; > + case dom::CompositeOperation::Accumulate: > + // FIXME: Bug 1291468: Implement accumulate operation. > + MOZ_ASSERT_UNREACHABLE("Not implemented yet"); > + break; > + case dom::CompositeOperation::Replace: > + MOZ_ASSERT_UNREACHABLE("Replace should have already handled"); > + break; > + default: > + MOZ_ASSERT_UNREACHABLE(""); > + break; > + } This looks pretty similar to the last part of KeyframeEffectReadOnly::GetCompositedValue in part 5. Would it make sense to factor this out into a static method that we call from both places? ::: gfx/layers/composite/AsyncCompositionManager.cpp:613 (Diff revision 1) > +static void > +SampleValue(float aPortion, > + const Animation& aAnimation, > + const AnimData& aAnimData, > + const AnimationSegment* aSegment, > + uint32_t aSegmentIndex, > + uint64_t aCurrentIteration, > + Animatable* aValue, > + StyleAnimationValue& aUnderlyingValue, > + Layer* aLayer) > +{ I think I prefer the previous API here--passing AnimData and an index into its data feels a little awkward somehow. But I suppose we want to avoid adding two more parameters to represent the different composite modes? Could we re-use the AnimationPropertySegment type and pass that instead? We could leave the mTimingFunction member as None(). Also, I think this API would be better if we: 1. Make it return interpolatedValue (a StyleAnimationValue) and use the result as the underlying value for the next call. 2. Do the conversion from StyleAnimationValue to Animatable at the end of the loop in SampleAnimations. It seems like we current convert from StyleAnimationValue to InfallibleTArray<TransformFunction> for all the intermediate animations when we only need to do it at the end? ::: gfx/layers/composite/AsyncCompositionManager.cpp:679 (Diff revision 1) > aPortion, interpolatedValue); > MOZ_ASSERT(uncomputeResult, "could not uncompute value"); > > if (aAnimation.property() == eCSSProperty_opacity) { > *aValue = interpolatedValue.GetFloatValue(); > + // Pass interporated value for the next animation Nit: interpolated ::: gfx/layers/composite/AsyncCompositionManager.cpp:710 (Diff revision 1) > flags, &data.bounds()); > > InfallibleTArray<TransformFunction> functions; > functions.AppendElement(TransformMatrix(transform)); > *aValue = functions; > + // Pass interporated value for the next animation interpolated ::: layout/base/nsDisplayList.cpp:446 (Diff revision 1) > + uint8_t effectComposite = > + static_cast<uint8_t>(aAnimation->GetEffect()-> > + AsKeyframeEffect()->Composite()); We can drop this once we resolve the composite value down to the segments in UpdateProperties ::: layout/base/nsDisplayList.cpp:473 (Diff revision 1) > + animSegment->startComposite() = segment.mFromComposite ? > + static_cast<uint8_t>(segment.mFromComposite.value()) : effectComposite; > + animSegment->endComposite() = segment.mToComposite ? > + static_cast<uint8_t>(segment.mToComposite.value()) : effectComposite; This should become simpler once we resolve the composite operations down to the segments in UpdateProperties.

Comment on attachment 8803156 [details] Bug 1305325 - Part 15: Tests composed style for missing keyframe for properties runnning on the compositor. https://reviewboard.mozilla.org/r/87390/#review91122 r=me with the following changes (I suspect we don't need *all* of these tests but it's up to you if you want to drop a few.) ::: dom/animation/test/style/file_missing-keyframe-on-compositor.html:33 (Diff revision 1) > +} > + > +promise_test(t => { > + // Without this, the first test case fails on Android. > + return waitForDocumentLoad(); > +}, 'Ensure document has been loaded'); Add a comment saying, "Note that promise tests run in sequence so this ensure the document is loaded before any of the other tests run" ? ::: dom/animation/test/style/file_missing-keyframe-on-compositor.html:39 (Diff revision 1) > + > +promise_test(t => { > + setupTestRefreshMode(t); > + > + var div = addDiv(t, { style: 'opacity: 0.1' }); > + div.animate([{ opacity: 1 }], 100 * MS_PER_SEC); Can't we just write: div.animate({ opacity: 1 }, 100 * MS_PER_SEC); ? Here and below? ::: dom/animation/test/style/file_missing-keyframe-on-compositor.html:47 (Diff revision 1) > +}, 'Initial opacity value that the opacity animation has no keyframe whose ' + > + 'offset 0'); Initial opacity value for animation with no keyframe at offset 0. ::: dom/animation/test/style/file_missing-keyframe-on-compositor.html:66 (Diff revision 1) > +}, 'Initial opacity value that the opacity animation has no keyframe whose ' + > + 'offset 0 on a lower-priority animation'); Initial opacity value for animation with no keyframe at offset 0 when there is a lower-priority animation. ::: dom/animation/test/style/file_missing-keyframe-on-compositor.html:86 (Diff revision 1) > +}, 'Initial opacity value that the opacity animation has no keyframe whose ' + > + 'offset 0 on a transition'); Initial opacity value for animation with no keyframe at offset 0 when there is a transition on the same property. ::: dom/animation/test/style/file_missing-keyframe-on-compositor.html:104 (Diff revision 1) > +}, 'Final opacity value that the opacity animation has no keyframe whose ' + > + 'offset 1'); Final opacity value for animation with no keyframe at offset 1. ::: dom/animation/test/style/file_missing-keyframe-on-compositor.html:125 (Diff revision 1) > +}, 'Final opacity value that the opacity animation has no keyframe whose ' + > + 'offset 1 on a lower-priority animation'); Final opacity value for animation with no keyframe at offset 1 when there is a lower-priority animation. ::: dom/animation/test/style/file_missing-keyframe-on-compositor.html:148 (Diff revision 1) > +}, 'Final opacity value that the opacity animation has no keyframe whose ' + > + 'offset 1 on a transition'); Final opacity value for animation with no keyframe at offset 1 when there is a transition on the same property. ::: dom/animation/test/style/file_missing-keyframe-on-compositor.html:155 (Diff revision 1) > + var lowerAnimation = div.animate([{ offset: 0, opacity: 0.5 }, > + { offset: 1, opacity: 1.0 }], > + 100 * MS_PER_SEC); Would div.animate({ opacity: [ 0.5, 1 ] }, 100 * MS_PER_SEC) be more clear here? ::: dom/animation/test/style/file_missing-keyframe-on-compositor.html:162 (Diff revision 1) > + 100 * MS_PER_SEC); > + var higherAnimation = div.animate([{ opacity: 1 }], 100 * MS_PER_SEC); > + > + return waitForPaintsFlushed().then(() => { > + lowerAnimation.pause(); > + Unnecessary blank link? ::: dom/animation/test/style/file_missing-keyframe-on-compositor.html:176 (Diff revision 1) > + // (0.5 + 1.0) * (50 * MS_PER_SEC / 100 * MS_PER_SEC) = 0.75. > + assert_equals(opacity, '0.75', > + 'Composed opacity value should be composed onto the value ' + > + 'of lower-priority paused animation'); > + }); > +}, 'Opacity value onto paused animation'); Opacity value for animation with no keyframe at offset 0 at 50% when composed onto a paused underlying animation. ::: dom/animation/test/style/file_missing-keyframe-on-compositor.html:182 (Diff revision 1) > + var lowerAnimation = div.animate([{ offset: 0, opacity: 0.5 }, > + { offset: 1, opacity: 1.0 }], > + 100 * MS_PER_SEC); Again, div.animate({ opacity: [ 0.5, 1 ] }, ...) ? ::: dom/animation/test/style/file_missing-keyframe-on-compositor.html:189 (Diff revision 1) > + 100 * MS_PER_SEC); > + var higherAnimation = div.animate([{ opacity: 1 }], 100 * MS_PER_SEC); > + > + return waitForPaintsFlushed().then(() => { > + lowerAnimation.playbackRate = 0; > + Unnecessary blank link? ::: dom/animation/test/style/file_missing-keyframe-on-compositor.html:203 (Diff revision 1) > + // (0.5 + 1.0) * (50 * MS_PER_SEC / 100 * MS_PER_SEC) = 0.75. > + assert_equals(opacity, '0.75', > + 'Composed opacity value should be composed onto the value ' + > + 'of lower-priority zero playback rate animation'); > + }); > +}, 'Opacity value onto zero playback rate animation'); Opacity value for animation with no keyframe at offset 0 at 50% when composed onto a zero playback rate underlying animation. ::: dom/animation/test/style/file_missing-keyframe-on-compositor.html:209 (Diff revision 1) > + > +promise_test(t => { > + setupTestRefreshMode(t); > + > + var div = addDiv(t, { style: 'transform: translateX(100px)' }); > + div.animate([{ transform: 'translateX(200px)' }], 100 * MS_PER_SEC); As with other places in this file, I think we don't need the [] ? ::: dom/animation/test/style/file_missing-keyframe-on-compositor.html:217 (Diff revision 1) > +}, 'Initial transform value that the transform animation has no keyframe ' + > + 'whose offset 0'); For the following tests, could we use the same sort of descriptions I wrote for the opacity ones? e.g. for this one 'Initial transform value for animation with no keyframe at offset 0.' ::: dom/animation/test/style/file_missing-keyframe-on-compositor.html:426 (Diff revision 1) > + // (150px + 300px) * (50 * MS_PER_SEC / 100 * MS_PER_SEC) = 225px. > + assert_matrix_equals(transform, 'matrix(1, 0, 0, 1, 225, 0)', > + 'Composed transform value should be composed onto the value of ' + > + 'lower-priority animation with fill:forwards and negative endDelay'); > + }); > +}, 'Transform value onto animation with fill:forwards and negative endDelay'); I wonder if we also need a test for an animation with fill:forwards and an endDelay greater in magnitude than the duration? ::: dom/animation/test/testcommon.js:34 (Diff revision 1) > + * This function allows error, 0.01, because on Android we are scaling down > + * the document, it results some errors. Nit: ...because on Android when we scale down the document it results in some errors. ::: dom/animation/test/testcommon.js:39 (Diff revision 1) > + assert_regexp_match(actual, matrixRegExp, > + 'Actual value is not a matrix') > + assert_regexp_match(expected, matrixRegExp, > + 'Expected value is not a matrix'); Do we normally put the assertion description in the positive/expected form? e.g. 'Actual value should be a matrix' Also, do we need to include the |description| in the message? ::: dom/animation/test/testcommon.js:47 (Diff revision 1) > + assert_equals(actualMatrixArray.length, expectedMatrixArray.length, > + 'dimention of the matrix: ' + description); Nit: dimension (or just 'length'?) Should this be, 'Array lengths should be equal: ' + description. Or even, 'Array lengths should be equal (got \'' + expected '\' and \'' + actual '\'): ' + description ::: dom/animation/test/testcommon.js:50 (Diff revision 1) > + assert_approx_equals(actualMatrixArray[i], expectedMatrixArray[i], 0.01, > + description); We should indicate which member of the matrix differed print both |actual| and |expected| as part of the error message. ::: dom/animation/test/testcommon.js:247 (Diff revision 1) > + } > + }); > +} > + > +/* > + * Entering test refresh mode, and restore the mode when |t| finishes. s/Entering/Enters/ s/and restore/and restores/ I guess we need to be careful not to use this with async tests since they can run in parallel? Can we check that or at least add a warning about it? ::: dom/animation/test/testcommon.js:249 (Diff revision 1) > +} > + > +/* > + * Entering test refresh mode, and restore the mode when |t| finishes. > + */ > +function setupTestRefreshMode(t) { Call this useTestRefreshMode ?

Comment on attachment 8803157 [details] Bug 1305325 - Part 10: Make SampleValue return StyleAnimationValue. https://reviewboard.mozilla.org/r/87392/#review91138 As discussed, I think we need to keep this until we turn on additive animations by default.

Comment on attachment 8803158 [details] Bug 1305325 - Part 5: Add AnimationUtils::IsCoreAPIEnabled. https://reviewboard.mozilla.org/r/87394/#review91140 I think based on the changes proposed to part 2, this test no longer applies?

Part 4 still gets the base value in UpdateCascadeResults(). I have no good idea to solve situation 2) in the commit message. Main changes from previous patches: 1) Neutral value was dropped. 2) The order of the patch series has been changed due to dropping the neutral value. 3) Hold time is passed to the compositor to compose paused or finished animations. 4) The base value is represented as a union { null_t; Animatable;} on IPC. I should note about 3). Hold time basically works fine except in case of negative endDelay. In case of negative endDelay, GetComputedTimingAt() for the elapsed duration returns progress value that the nagative endDelay is not factored into it. For example an animation with duration = 1000, endDelay = -500. The progress value of this animation is 0.5 when the elapsed duration is 1000, but GetComputedTimingAt() returns 1.0 because GetComputedTimingAt() assumes |aLocalTime| is limited at end time. So if the main thread is busy when the animation finishes, we will see a jumpy animation. But anyway, an important thing is that this jumpy animation is already there without these patches.

(In reply to Brian Birtles (:birtles) from comment #36) > (Diff revision 1) > > +static void > > +SampleValue(float aPortion, > > + const Animation& aAnimation, > > + const AnimData& aAnimData, > > + const AnimationSegment* aSegment, > > + uint32_t aSegmentIndex, > > + uint64_t aCurrentIteration, > > + Animatable* aValue, > > + StyleAnimationValue& aUnderlyingValue, > > + Layer* aLayer) > > +{ > > I think I prefer the previous API here--passing AnimData and an index into > its data feels a little awkward somehow. > > But I suppose we want to avoid adding two more parameters to represent the > different composite modes? Could we re-use the AnimationPropertySegment type > and pass that instead? We could leave the mTimingFunction member as None(). I tried to use AnimationPropertySegment, but it was awkward, especially initializing. So I added a new struct named StyleAnimationValueCompositePair which has StyleAnimationValue and CompositeOperation and passed two of the struct as 'from' and 'to' values.

Comment on attachment 8803148 [details] Bug 1305325 - Part 4: Add a function that returns the resolved base style on an element for a given property with nsStyleContext. Removing review request from part 4. MozReview could not recognize 'r?'.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #54) > I should note about 3). Hold time basically works fine except in case of > negative endDelay. In case of negative endDelay, GetComputedTimingAt() for > the elapsed duration returns progress value that the nagative endDelay is > not factored into it. For example an animation with duration = 1000, > endDelay = -500. The progress value of this animation is 0.5 when the > elapsed duration is 1000, but GetComputedTimingAt() returns 1.0 because > GetComputedTimingAt() assumes |aLocalTime| is limited at end time. So if the > main thread is busy when the animation finishes, we will see a jumpy > animation. But anyway, an important thing is that this jumpy animation is > already there without these patches. Filed bug 1317832.

For reference: It turns out if we want to resolve the base values in KeyframeEffectReadOnly::UpdateProperties(), we have to store the value in KeyframeEffectReadOnly class not in EffectSet class. What I tried and failed: Resolving the base value basically in UpdateProperties() and also resolving the value at places below for the case that the KeyframeEffectReadOnly object is not included in EffectSet. * when a new KeyframeEffectReadOnly is added in EffectSet * in case of CSS animations, this causes infinite nsStyleSet::GetContext() calls. GetContext() -> UpdateAnimations() -> KeyframeEffectReadOnly::UpdateTargetRegistration() -> effectSet->AddEffect() -> UpdateBaseValues() -> ResolveStyleWithoutAnimation() -> GetContext() -> ... * in Animation::SetEffectNoUpdate(). * To avoid the infinite calls, I tried this next because this function is called after we call UpdateTargetRegistration(). But still there are some cases that EffectSet does not have the KeyframeEffectReadOnly object at this point because, yes, of relevant state. So, my conclusion is that KeyframeEffectReadOnly has to have the base value if we get the base value in UpdateProperties(). Having the base values in KeyframeEffectReadOnly is less effective than EffectSet but I am OK with it since I guess there is no big difference in most cases. Attaching file is a patch that resolves the base values in UpdateProperties(), this can be applied onto the last patch (Part 14) in those patch set.

I'd really like to avoid putting the base value in KeyframeEffectReadOnly. That sounds like the wrong place for it to be. Why is AddEffect() calling UpdateBaseValues()? Adding an effect shouldn't change the base value of an element? (Although it might change it for descendants)

(In reply to Brian Birtles (:birtles) from comment #59) > I'd really like to avoid putting the base value in KeyframeEffectReadOnly. > That sounds like the wrong place for it to be. Why is AddEffect() calling > UpdateBaseValues()? Adding an effect shouldn't change the base value of an > element? (Although it might change it for descendants) Sorry for my lack of explanation. I added it since it's a chance to resolve the base value and store it in EffectSet in the case that we have no EffectSet in UpdateProperties().

To be more precise, I modified like this; make EffectSet::AddEffect() returning boolean. and in UpdateTargetRegistration() if (effectSet->AddEffect(*this)) { UpdateBaseValues(); }

Can we just do that when we go to compose animations?

(In reply to Brian Birtles (:birtles) from comment #62) > Can we just do that when we go to compose animations? Sure we can do it in EffectCompositor::ComposeAnimationRule() or KeyframeEffectReadOnly::ComposeStyle().

The order of patch set has been reordered again. Now the base value is resolved only if needed when we compose style and stored o only for properties that can be run on the compositor. So now memory consumption for the base values gets reduced before. But yes, if there are lots of additive or accumulative animations, performance gets worse before.

Comment on attachment 8803146 [details] Bug 1305325 - Part 3: Make AnimationPropertyValueDetails::mValue optional. https://reviewboard.mozilla.org/r/87370/#review95436 Looks good to me, but this will need DOM peer review

Comment on attachment 8803148 [details] Bug 1305325 - Part 4: Add a function that returns the resolved base style on an element for a given property with nsStyleContext. https://reviewboard.mozilla.org/r/87374/#review95438 It seems like this belongs in EffectCompositor? It's the job of the EffectCompositor to get the base style, pass it to the first effect as its underlying style, take the result, pass it to the next effect as the underlying style etc.

Comment on attachment 8803158 [details] Bug 1305325 - Part 5: Add AnimationUtils::IsCoreAPIEnabled. https://reviewboard.mozilla.org/r/87394/#review95442 Nit: missing 'd' in the changeset header after 'AnimationUtils::IsCoreAPIEnable'

Comment on attachment 8803149 [details] Bug 1305325 - Part 6: Handle missing keyframe whose offset 0 or 1 on the main thread. https://reviewboard.mozilla.org/r/87376/#review95440 In general this looks good but I'm concerned that some of the tests that include empty keyframes are not actually testing anything useful. We should drop all the ones that are not testing anything useful. Also, I'd like to know how we should handle GetMinAndMaxScaleForAnimationProperty (or if we already correctly handle it). ::: dom/animation/KeyframeEffectReadOnly.h:123 (Diff revision 3) > }; > > struct AnimationPropertySegment > { > float mFromKey, mToKey; > + // NOTE: In case that no keyframe for 0 or 1 offset is specified Nit: In *the* case... ::: dom/animation/KeyframeEffectReadOnly.cpp:333 (Diff revision 3) > + > + MOZ_ASSERT(!aValueToComposite.IsNull() || > + aCompositeOperation == CompositeOperation::Add, > + "InputValue should be null only if additive composite"); > + > + StyleAnimationValue result; We should probably move this to the top and always return it to benefit from RVO. ::: dom/animation/KeyframeEffectReadOnly.cpp:928 (Diff revision 3) > + segment.mFromComposite, > fromValue); (Nit: Bring fromValue up onto the same line? Likewise below with toValue?) ::: dom/animation/KeyframeEffectReadOnly.cpp:1385 (Diff revision 3) > { > mCumulativeChangeHint = nsChangeHint(0); > > for (const AnimationProperty& property : mProperties) { > for (const AnimationPropertySegment& segment : property.mSegments) { > + // In case composite operation is not 'replace', we cant't optimize Nit: s/cant't/can't/ ::: dom/animation/KeyframeEffectReadOnly.cpp:1385 (Diff revision 3) > + // In case composite operation is not 'replace', we cant't optimize > + // because we can't tell the change hint for such properties until > + // we compose it. > + if (segment.mFromComposite != CompositeOperation::Replace || > + segment.mToComposite != CompositeOperation::Replace) { > + mCumulativeChangeHint = ~nsChangeHint_Hints_CanIgnoreIfNotVisible; > + return; > + } Is this always going to be the case, or is there some follow-up bug where we can handle some cases? s/tell the change hint/calculate the change hint/ ? Also, "can't optimize" is a bit vague. What are we optimizing? ::: dom/animation/KeyframeUtils.h:130 (Diff revision 3) > + * @param aEffectComposite The composite operation of effect level. If no > + * composite operation of keyframe level is specified, this will be used. The composite operation specified on the effect. For any keyframes in |aKeyframes| that do not specify a composite operation, this value will be used. ::: dom/animation/KeyframeUtils.cpp:1140 (Diff revision 3) > eCSSPropertyExtra_no_properties; > } > > +static void > +AppendInitialSegment(AnimationProperty* aAnimationProperty, > + const KeyframeValueEntry& aFirstEntry) Indentation here seems off by 2. ::: dom/animation/KeyframeUtils.cpp:1165 (Diff revision 3) > + segment->mToKey = 1.0f; > + segment->mToComposite = dom::CompositeOperation::Add; > + segment->mTimingFunction = aLastEntry.mTimingFunction; > +} > + > +// Returns a newly created AnimationProperty if the missing entry was filled. s/filled/filled-in/ ::: dom/animation/KeyframeUtils.cpp:1165 (Diff revision 3) > +// Returns a newly created AnimationProperty if the missing entry was filled. > +// Returns nullptr if the missing entry was not handled at all. Returns a newly created AnimationProperty if one was created to fill-in the missing keyframe, nullptr otherwise (if we decided not to fill in the keyframe because we don't support additive animation). ::: dom/animation/KeyframeUtils.cpp:1168 (Diff revision 3) > +HandleMissingOffset0(nsTArray<AnimationProperty>& aResult, > + const KeyframeValueEntry& aEntry) Can we call this HandleMissingInitialKeyframe? ::: dom/animation/KeyframeUtils.cpp:1174 (Diff revision 3) > + // If the preference of the core Web Animations API is not enabled, > + // don't fill the missing keyframe. ... since the missing keyframe requires support for additive animation which is guarded by this pref. Likewise below. ::: dom/animation/KeyframeUtils.cpp:1175 (Diff revision 3) > +{ > + MOZ_ASSERT(aEntry.mOffset != 0.0f, > + "The offset of the entry should not be 0.0"); > + > + // If the preference of the core Web Animations API is not enabled, > + // don't fill the missing keyframe. s/fill/fill in/ ::: dom/animation/KeyframeUtils.cpp:1189 (Diff revision 3) > +HandleMissingOffset1(nsTArray<AnimationProperty>& aResult, > + AnimationProperty* aCurrentAnimationProperty, > + const KeyframeValueEntry& aEntry) HandleMissingFinalKeyframe? ::: dom/animation/KeyframeUtils.cpp:1190 (Diff revision 3) > + AnimationProperty* aCurrentAnimationProperty, > + const KeyframeValueEntry& aEntry) Switch the order of these last two arguments so that the match the function above? ::: dom/animation/KeyframeUtils.cpp:1208 (Diff revision 3) > + } > + return; > + } > + > + // If |aCurrentAnimationProperty| is nullptr, that means this is the first > + // entry for the property, we have to append a new AnimationProperty for this s/, we have/so we have/ ::: dom/animation/KeyframeUtils.cpp:1269 (Diff revision 3) > - // Check that the last property ends with an entry at offset 1. > + // If we've reached the end of the array of entries, synthesize final (and > + // initial) segment if necessary. Nit: synthesize a final ... ::: dom/animation/KeyframeUtils.cpp:1275 (Diff revision 3) > + // If the last entry with offset 1 for the property has not yet > + // appended, that means this is only one entry for the property, > + // append the initial segment with 0 offset. If we have an entry with offset 1 and no animation property, that means it is the only entry for this property so append a single segment from 0 offset to |aEntry[i. ::: dom/animation/KeyframeUtils.cpp:1279 (Diff revision 3) > - animationProperty = nullptr; > + } else if (aEntries[i].mOffset == 1.0f && !animationProperty) { > + // If the last entry with offset 1 for the property has not yet > + // appended, that means this is only one entry for the property, > + // append the initial segment with 0 offset. > + Unused << HandleMissingOffset0(aResult, aEntries[i]); > } Should we set |animationProperty| to nullptr here just so it doesn't dangle? ::: dom/animation/KeyframeUtils.cpp:1287 (Diff revision 3) > > MOZ_ASSERT(aEntries[i].mProperty != eCSSProperty_UNKNOWN && > aEntries[i + 1].mProperty != eCSSProperty_UNKNOWN, > "Each entry should specify a valid property"); > > - // Skip properties that don't have an entry with offset 0. > + // Missing entry with offset 0. No keyframe for this property at offset 0. ::: dom/animation/KeyframeUtils.cpp:1294 (Diff revision 3) > aEntries[i].mOffset != 0.0f) { > - // Since the entries are sorted by offset for a given property, and > - // since we don't update |lastProperty|, we will keep hitting this > - // condition until we change property. > + animationProperty = HandleMissingOffset0(aResult, aEntries[i]); > + if (animationProperty) { > + lastProperty = aEntries[i].mProperty; > + } else { > + // Skip this entry if we did not handle the missing entry. We need to keep the existing comment here. e.g. // If we don't support additive animation we can't fill in the missing keyframes and we should just skip this property altogether. Since the entries are sorted by offset for a given property, and since we don't update |lastProperty|, we will keep hitting this condition until we change property. ::: dom/animation/KeyframeUtils.cpp:1300 (Diff revision 3) > - ++i; > + ++i; > - continue; > + continue; > - } > + } > + } > > - // Drop properties that don't end with an entry with offset 1. > + // Missing entry with offset 1. No keyframe for this property at offset 1. ::: dom/animation/KeyframeUtils.cpp:1304 (Diff revision 3) > > - // Drop properties that don't end with an entry with offset 1. > + // Missing entry with offset 1. > if (aEntries[i].mProperty != aEntries[i + 1].mProperty && > aEntries[i].mOffset != 1.0f) { > - if (animationProperty) { > - aResult.RemoveElementAt(aResult.Length() - 1); > + HandleMissingOffset1(aResult, animationProperty, aEntries[i]); > + // Move on new property. // Move on to new property. ::: dom/animation/KeyframeUtils.cpp:1429 (Diff revision 3) > if (count == 0) { > // No animation values for this property. > continue; > } > - if (count == 1) { > - // We don't support additive values and so can't support an > + > + // FIXME: Bug 1311257: Support missing keyframes for Servo backend. We also need to explain why we're checking the pref. Something like: If we only have one value, we should animate from the underlying value using additive animation--however, we don't support additive animation for the Servo backend (bug 1311257) or when the core animation API pref is switched off. ::: dom/animation/test/chrome/test_animation_properties.html:709 (Diff revision 3) > - { desc: 'a property that can\'t be resolved to computed values in' > - + ' initial keyframe', > + { desc: 'a missing property in initial keyframe', > + frames: [ { }, > - frames: [ { margin: '5px', simulateComputeValuesFailure: true }, > { margin: '5px' } ], > - expected: [ ] > + expected: [ { property: 'margin-top', > + values: [ value(0, '<not set>', 'add', 'linear'), Where does "<not set>" come from? Would using null or undefined make more sense? ::: dom/animation/test/chrome/test_animation_properties.html (Diff revision 3) > - frames: [ { margin: '5px', simulateComputeValuesFailure: true }, > { margin: '5px' }, > { margin: '5px' } ], I don't really understand why a missing keyframe is the same as a compute values failure here (and elsewhere in this file)? What are we testing? ::: dom/animation/test/chrome/test_animation_properties.html:806 (Diff revision 3) > + values: [ value(0, '5px', 'replace', 'linear'), > + value(1, '<not set>', 'add') ] }, > + { property: 'margin-right', > + values: [ value(0, '5px', 'replace', 'linear'), > + value(1, '<not set>', 'add') ] }, > + { property: 'margin-bottom', > + values: [ value(0, '5px', 'replace', 'linear'), > + value(1, '<not set>', 'add') ] }, > + { property: 'margin-left', > + values: [ value(0, '5px', 'replace', 'linear'), > + value(1, '<not set>', 'add') ] } ] (Nit: Excess space between offset and value?) ::: dom/animation/test/chrome/test_animation_properties.html:823 (Diff revision 3) > // margin-top sorts last and only it will be missing since > // the other longhand components are specified I don't think this comment makes sense any more and we should drop it from here. ::: dom/animation/test/chrome/test_animation_properties.html:920 (Diff revision 3) > - { desc: 'a property that can\'t be resolved to computed values in' > + { desc: 'a missing property in intermediate keyframe', > - + ' intermediate keyframe', > frames: [ { margin: '5px' }, > - { margin: '5px', simulateComputeValuesFailure: true }, > + { }, > { margin: '5px' } ], Here and many places in this file--this doesn't seem to be testing anything useful? ::: dom/animation/test/chrome/test_animation_properties.html:998 (Diff revision 3) > + values: [ value(0, '5px', 'replace', 'linear'), > + value(1, '<not set>', 'add') ] } ] > }, > - { desc: 'a property that can\'t be resolved to computed values in' > - + ' final keyframe along with other values where those' > - + ' values sort after the property with missing values', > + { desc: 'a missing property in final keyframe along with other values' > + + ' where those values sort after the property with missing' > + + 'values', Missing leading space ::: dom/animation/test/chrome/test_animation_properties.html:1070 (Diff revision 3) > + }, > + { desc: 'missing propertes in both of initial and final keyframe along' > + + 'with other values', > + frames: [ { left: '5px', offset: 0 }, > + { right: '5px', offset: 0.5 }, > + { left: '10px',offset: 1 } ], Whitespace after , here ::: dom/animation/test/chrome/test_restyles.html:827 (Diff revision 3) > + yield animation.ready; > + > + var markers = yield observeStyling(5); > + > + is(markers.length, 5, > + 'Discrete type of animations that have no keyframe whose offset ' + whose offset *is* ::: dom/animation/test/chrome/test_restyles.html:827 (Diff revision 3) > + yield animation.ready; > + > + var markers = yield observeStyling(5); > + > + is(markers.length, 5, > + 'Discrete type of animations that have no keyframe whose offset ' + Discrete animation that has... ::: dom/animation/test/chrome/test_restyles.html:828 (Diff revision 3) > + > + var markers = yield observeStyling(5); > + > + is(markers.length, 5, > + 'Discrete type of animations that have no keyframe whose offset ' + > + '0 or 1 in an out-of-view element should not throttle'); ...should not be throttled ::: dom/animation/test/chrome/test_restyles.html:847 (Diff revision 3) > + yield animation.ready; > + > + var markers = yield observeStyling(5); > + > + is(markers.length, 0, > + 'Discrete type of animations running on the main-thread in an ' + Discrete type animation running... ::: layout/base/nsLayoutUtils.cpp:541 (Diff revision 3) > + // In case of add or accumulate composite, StyleAnimationValue does > + // not have valid value. What happens in this case? Is the base scale etc. incorporated elsewhere? Or should we actually fetch the base value here? ::: layout/base/nsLayoutUtils.cpp:542 (Diff revision 3) > - if (prop.mProperty == eCSSProperty_transform) { > - for (uint32_t segIdx = prop.mSegments.Length(); segIdx-- != 0; ) { > - const AnimationPropertySegment& segment = prop.mSegments[segIdx]; > + if (prop.mProperty != eCSSProperty_transform) { > + continue; > + } > + for (const AnimationPropertySegment& segment : prop.mSegments) { > + // In case of add or accumulate composite, StyleAnimationValue does > + // not have valid value. ...not have a valid value ::: testing/web-platform/meta/web-animations/interfaces/KeyframeEffect/constructor.html.ini:13 (Diff revision 3) > expected: FAIL > bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1291468 > > [composite values are parsed correctly when passed to the KeyframeEffectReadOnly constructor in KeyframeTimingOptions] > expected: FAIL > - bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1216844 > + bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1291468 # This needs additive animation That bug is for accumulate... is that right?

(In reply to Brian Birtles (:birtles) from comment #82) > Also, I'd like to know how we should handle > GetMinAndMaxScaleForAnimationProperty (or if we already correctly handle it). Ah, right. We have to factor in the base value too. I will add a patch as part 15 (with a test case if I can write it). Thanks for reviewing! I think 33 open issues is a new record.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #83) > (In reply to Brian Birtles (:birtles) from comment #82) > > Also, I'd like to know how we should handle > > GetMinAndMaxScaleForAnimationProperty (or if we already correctly handle it). > > Ah, right. We have to factor in the base value too. I will add a patch as > part 15 (with a test case if I can write it). I almost give up writing the test case. I thought initially setting initial-scale will show wierd results but actually it did not. Now I am unsure that GetMinAndMaxScaleForAnimationProperty() is really helpful. A try run with no-op GetMinAndMaxScaleForAnimationProperty() [1] did not show any suspicions. Also the original bug 822231 which introduced the function and all bugs related to the original were for Firefox OS. So I am going to add a patch without any test cases here. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=18580f911fe1781aa09853f5ef93fd1d54dd608e&exclusion_profile=false

Comment on attachment 8803148 [details] Bug 1305325 - Part 4: Add a function that returns the resolved base style on an element for a given property with nsStyleContext. https://reviewboard.mozilla.org/r/87374/#review95610 Does EffectCompositor.cpp include StyleAnimationValue.h? It doesn't appear to on trunk so unless one of the earlier patches added it we should probably add it here. Otherwise looks good. Thanks for doing this.

I'm still mid-way through reviewing part 9, but I wonder if we need to expose the hold time and pass it around. Can't we just pass a 'paused' flag to the compositor and, if it's set, use the initialCurrentTime to calculate the elapsedDuration (like we currently do with the hold time)? For my own notes: I'm also trying to work out if we can do something a little simpler/clearer in FindAnimationsForCompositor.

(In reply to Brian Birtles (:birtles) from comment #101) > I'm still mid-way through reviewing part 9, but I wonder if we need to > expose the hold time and pass it around. Can't we just pass a 'paused' flag > to the compositor and, if it's set, use the initialCurrentTime to calculate > the elapsedDuration (like we currently do with the hold time)? Considering only paused animations, initialCurrentTime is sufficient to calculate the correct elapsedDuration. But for finished (but fill:forwards) animations, it's not sufficient. (In reply to Brian Birtles (:birtles) from comment #34) > ::: gfx/layers/composite/AsyncCompositionManager.cpp:690 > (Diff revision 1) > > + if (animation.playbackRate() > 0 && > > + elapsedDuration > timing.EndTime()) { > > + elapsedDuration = timing.EndTime(); > > So this is because the compositor doesn't do finishing behavior I guess. > > I think this isn't strictly correct since it's possible to seek an animation > past its end time and have it still contribute to the composited result. Without the hold time, as far as I can tell, we need to clamp the elapsedDuration like this.

Can we just overload the initialCurrentTime to represent the hold time in these cases where it is paused?

(In reply to Brian Birtles (:birtles) from comment #103) > Can we just overload the initialCurrentTime to represent the hold time in > these cases where it is paused? Ah, I got it. We can rename initialCurrentTime to holdTime and factor delay out (and factor it in on the compositor for initialCurrentTime).

(In reply to Hiroyuki Ikezoe (:hiro) from comment #104) > (In reply to Brian Birtles (:birtles) from comment #103) > > Can we just overload the initialCurrentTime to represent the hold time in > > these cases where it is paused? > > Ah, I got it. We can rename initialCurrentTime to holdTime and factor delay > out (and factor it in on the compositor for initialCurrentTime). Oops, I missed that in this case we need the 'finished' condition too. (In reply to Brian Birtles (:birtles) from comment #101) > I'm still mid-way through reviewing part 9, but I wonder if we need to > expose the hold time and pass it around. Can't we just pass a 'paused' flag > to the compositor and, I am inclined to send playState.

Comment on attachment 8803153 [details] Bug 1305325 - Part 9: Send animations even if it's paused, finished or zero playback rate. . https://reviewboard.mozilla.org/r/87384/#review95614 This looks good, but I'd like to take another look with the following changes. ::: dom/animation/Animation.h:331 (Diff revision 5) > + /** > + * Returns the play state except pending. > + * PlayPending is represented by Running, PausePending is represented by > + * Paused. > + */ > + AnimationPlayState UpcomingPlayState() const; See my comments below, but I wonder if we can we just pass a paused flag instead of adding this ::: dom/animation/EffectCompositor.cpp:135 (Diff revision 5) > + bool hasPlayingAnimations = false; > + > bool foundSome = false; > for (KeyframeEffectReadOnly* effect : *effects) { > MOZ_ASSERT(effect && effect->GetAnimation()); > Animation* animation = effect->GetAnimation(); > > if (!animation->IsPlayableOnCompositor()) { > continue; > } > > + bool isPlaying = animation->IsPlaying(); > + > + // If we are finding animations for transform, check that there are other > + // animations that should block the transform animation. e.g. geometric > + // properties' animation. This check should be done regardless of whether > + // the effect has the target property |aProperty| or not. > AnimationPerformanceWarning::Type warningType; > if (aProperty == eCSSProperty_transform && > + isPlaying && > effect->ShouldBlockAsyncTransformAnimations(aFrame, > warningType)) { > - if (aMatches) { > - aMatches->Clear(); > - } > EffectCompositor::SetPerformanceWarning( > aFrame, aProperty, > AnimationPerformanceWarning(warningType)); > - return false; > + foundSome = false; > + break; > } > > + // If the animation's effect has no |aProperty|, skip following checks. > if (!effect->HasEffectiveAnimationOfProperty(aProperty)) { > continue; > } > > + if (isPlaying && !hasPlayingAnimations) { > + hasPlayingAnimations = true; > + } > + > if (aMatches) { > aMatches->AppendElement(animation); > } > foundSome = true; > } > > + // If there is none of playing animations, we should not send those animations > + // to the compositor. > + if (!hasPlayingAnimations) { > + foundSome = false; > + } > + > MOZ_ASSERT(!foundSome || !aMatches || !aMatches->IsEmpty(), > "If return value is true, matches array should be non-empty"); > > - if (aMatches && foundSome) { > + if (aMatches) { > + if (foundSome) { > - aMatches->Sort(AnimationPtrComparator<RefPtr<dom::Animation>>()); > + aMatches->Sort(AnimationPtrComparator<RefPtr<dom::Animation>>()); > + } else { > + aMatches->Clear(); > + } > } > return foundSome; This section is a little hard to follow. As far as I can tell, for each effect there are four things that can happen: 1. We can decide it can run on the compositor 2. We can decide it can run on the compositor, if need be (e.g. a paused animation) 3. We can decide it can't run on the compositor or is irrelevant 4. We can decide it can't run on the compositor or is irrelevant but its presence means we should not run any other animations for this property on the compositor I think splitting this logic out would make this easier to read and reason about. For example, something like the following: namespace { enum class MatchForCompositor { // This animation matches and should run on the compositor if possible. Yes, // This (paused) animation matches and can be run on the compositor if there // are other animations for this property that return 'Yes'. IfNeeded, // This animation does not match or can't be run on the compositor. No, // This animation does not match or can't be run on the compositor and, // furthermore, its presence means we should not run any animations for this // property on the compositor. NoAndBlockThisProperty }; } MatchForCompositor function IsMatchForCompositor(const KeyframeEffectReadOnly& aEffect, nsCSSProperty aProperty, nsIFrame* aFrame) { const Animation* animation = effect.GetAnimation(); MOZ_ASSERT(animation); if (!animation->IsPlayableOnCompositor()) { return MatchForCompositor::No; } bool isPlaying = animation->IsPlaying(); // If we are finding animations for transform, check that there are other // animations that should block the transform animation. e.g. geometric // properties' animation. This check should be done regardless of whether // the effect has the target property |aProperty| or not. AnimationPerformanceWarning::Type warningType; if (aProperty == eCSSProperty_transform && isPlaying && effect->ShouldBlockAsyncTransformAnimations(aFrame, warningType)) { EffectCompositor::SetPerformanceWarning( aFrame, aProperty, AnimationPerformanceWarning(warningType)); return MatchForCompositor::NoAndBlockThisProperty; } if (!effect->HasEffectiveAnimationOfProperty(aProperty)) { return MatchForCompositor::No; } return isPlaying ? MatchForCompositor::Yes : MatchForCompositor::IfNeeded; } Then I think the main body loop would be something like: bool foundSome = false; for (KeyframeEffectReadOnly* effect : *effects) { MOZ_ASSERT(effect); MatchForCompositor matchResult = IsMatchForCompositor(*effect); if (matchResult == MatchForCompositor::NoAndBlockThisProperty) { if (aMatches) { aMatches->Clear(); } return false; } if (matchResult == MatchForCompositor::No) { continue; } if (aMatches) { aMatches->AppendElement(effect->GetAnimation()); } if (matchResult == MatchForCompositor::Yes) { foundSome = true; } } // If all animations we added were paused animations, don't send them // to the compositor. if (!foundSome && aMatches) { aMatches->Clear(); } MOZ_ASSERT(!foundSome || !aMatches || !aMatches->IsEmpty(), "If return value is true, matches array should be non-empty"); if (aMatches && foundSome) { aMatches->Sort(AnimationPtrComparator<RefPtr<dom::Animation>>()); } return foundSome; ::: dom/animation/EffectCompositor.cpp:169 (Diff revision 5) > + if (isPlaying && !hasPlayingAnimations) { > + hasPlayingAnimations = true; > + } hasPlayingAnimations |= isPlaying; ::: gfx/layers/ipc/LayersMessages.ipdlh:221 (Diff revision 5) > + // This uses dom::AnimationPlayState. > + uint8_t playState; Can we just use a bool here representing paused? (Where "paused" includes "actually paused--i.e. no start time and not play-pending; filling forwards etc.) Play state feels odd here since: * We're not going to handle all the possible values of play state * We shouldn't handle all the different values of play state since it's calculated from more primitive values such as start time etc. * We really only care about two states: running or not If you think "paused" is confusing, then how about "frozen", "isRunning" or "notRunning"?

Comment on attachment 8803157 [details] Bug 1305325 - Part 10: Make SampleValue return StyleAnimationValue. https://reviewboard.mozilla.org/r/87392/#review95808 r=me with the following changes. s/returning/return/ in the commit message ::: gfx/layers/composite/AsyncCompositionManager.cpp:622 (Diff revision 5) > - nsCSSValueSharedList* interpolatedList = > - interpolatedValue.GetCSSValueSharedListValue(); > +static void > +ApplyAnimatedValue(Layer* aLayer, > + Animation& aAnimation, > + const StyleAnimationValue& aValue) As far as I can tell, we need to pass |aAnimation| for two things: 1. To get the property being animated 2. To get the transform origin/bounds, i.e. TransformData in the case of a transform animation. I think it would be better to pass these separately. (And pass a null/empty/None() TransformData structure in the case of a non-transform animation.) The reason I think this is because currently it's not clear why passing animations.LastElement() is correct, or even why we need to pass it. If we pass these separately, then presumably at the call site we'll have section where we explicitly say: "Store the transform data to use when applying the transform animation. We expect this transform data to be the same for all animations." Or something like that. We can even add an #ifdef DEBUG loop to assert that the transform data is the same for all animations. That should also make it more obvious why we have the extra animations.IsEmpty() check. ::: gfx/layers/composite/AsyncCompositionManager.cpp:625 (Diff revision 5) > + const StyleAnimationValue& aValue) > +{ > + HostLayer* layerCompositor = aLayer->AsHostLayer(); > + switch (aAnimation.property()) { > + case eCSSProperty_UNKNOWN: > + break; > + case eCSSProperty_opacity: { > + MOZ_ASSERT(aValue.GetUnit() == StyleAnimationValue::eUnit_Float, > + "Interpolated value for opacity should be float"); > + layerCompositor->SetShadowOpacity(aValue.GetFloatValue()); > + layerCompositor->SetShadowOpacitySetByAnimation(true); > + break; > + } > + case eCSSProperty_transform: { > + nsCSSValueSharedList* list = aValue.GetCSSValueSharedListValue(); What do you think we should do about assertions here? We have a MOZ_ASSERT for the unit of aValue in the opacity case but not the transform case. Is that deliberate? On the other hand, StyleAnimationValue has various assertions built-in but they are NS_ASSERTION so they can be ignored. If we want more strict checks, then perhaps we should assert on GetUnit for the transform case? ::: gfx/layers/composite/AsyncCompositionManager.cpp:629 (Diff revision 5) > + case eCSSProperty_UNKNOWN: > + break; (Just curious, when does this case arise?)

Thanks for the great reviewing! (In reply to Brian Birtles (:birtles) from comment #121) > ::: dom/animation/EffectCompositor.cpp:135 > (Diff revision 5) > > + bool hasPlayingAnimations = false; > > + > > bool foundSome = false; > > for (KeyframeEffectReadOnly* effect : *effects) { > > MOZ_ASSERT(effect && effect->GetAnimation()); > > Animation* animation = effect->GetAnimation(); > > > > if (!animation->IsPlayableOnCompositor()) { > > continue; > > } > > > > + bool isPlaying = animation->IsPlaying(); > > + > > + // If we are finding animations for transform, check that there are other > > + // animations that should block the transform animation. e.g. geometric > > + // properties' animation. This check should be done regardless of whether > > + // the effect has the target property |aProperty| or not. > > AnimationPerformanceWarning::Type warningType; > > if (aProperty == eCSSProperty_transform && > > + isPlaying && > > effect->ShouldBlockAsyncTransformAnimations(aFrame, > > warningType)) { > > - if (aMatches) { > > - aMatches->Clear(); > > - } > > EffectCompositor::SetPerformanceWarning( > > aFrame, aProperty, > > AnimationPerformanceWarning(warningType)); > > - return false; > > + foundSome = false; > > + break; > > } > > > > + // If the animation's effect has no |aProperty|, skip following checks. > > if (!effect->HasEffectiveAnimationOfProperty(aProperty)) { > > continue; > > } > > > > + if (isPlaying && !hasPlayingAnimations) { > > + hasPlayingAnimations = true; > > + } > > + > > if (aMatches) { > > aMatches->AppendElement(animation); > > } > > foundSome = true; > > } > > > > + // If there is none of playing animations, we should not send those animations > > + // to the compositor. > > + if (!hasPlayingAnimations) { > > + foundSome = false; > > + } > > + > > MOZ_ASSERT(!foundSome || !aMatches || !aMatches->IsEmpty(), > > "If return value is true, matches array should be non-empty"); > > > > - if (aMatches && foundSome) { > > + if (aMatches) { > > + if (foundSome) { > > - aMatches->Sort(AnimationPtrComparator<RefPtr<dom::Animation>>()); > > + aMatches->Sort(AnimationPtrComparator<RefPtr<dom::Animation>>()); > > + } else { > > + aMatches->Clear(); > > + } > > } > > return foundSome; > > This section is a little hard to follow. As far as I can tell, for each > effect there are four things that can happen: > > 1. We can decide it can run on the compositor > 2. We can decide it can run on the compositor, if need be (e.g. a paused > animation) > 3. We can decide it can't run on the compositor or is irrelevant > 4. We can decide it can't run on the compositor or is irrelevant but its > presence means we should not run any other animations for this property on > the compositor > > I think splitting this logic out would make this easier to read and reason > about. For example, something like the following: > > namespace { > enum class MatchForCompositor { > // This animation matches and should run on the compositor if possible. > Yes, > // This (paused) animation matches and can be run on the compositor if > there > // are other animations for this property that return 'Yes'. > IfNeeded, > // This animation does not match or can't be run on the compositor. > No, > // This animation does not match or can't be run on the compositor and, > // furthermore, its presence means we should not run any animations for > this > // property on the compositor. > NoAndBlockThisProperty > }; > } > > MatchForCompositor > function IsMatchForCompositor(const KeyframeEffectReadOnly& aEffect, > nsCSSProperty aProperty, > nsIFrame* aFrame) { > const Animation* animation = effect.GetAnimation(); > MOZ_ASSERT(animation); > > if (!animation->IsPlayableOnCompositor()) { > return MatchForCompositor::No; > } > > bool isPlaying = animation->IsPlaying(); > > // If we are finding animations for transform, check that there are other > // animations that should block the transform animation. e.g. geometric > // properties' animation. This check should be done regardless of whether > // the effect has the target property |aProperty| or not. > AnimationPerformanceWarning::Type warningType; > if (aProperty == eCSSProperty_transform && > isPlaying && > effect->ShouldBlockAsyncTransformAnimations(aFrame, > warningType)) { > EffectCompositor::SetPerformanceWarning( > aFrame, aProperty, > AnimationPerformanceWarning(warningType)); > return MatchForCompositor::NoAndBlockThisProperty; > } > > if (!effect->HasEffectiveAnimationOfProperty(aProperty)) { > return MatchForCompositor::No; > } > > return isPlaying ? MatchForCompositor::Yes : > MatchForCompositor::IfNeeded; > } > > Then I think the main body loop would be something like: > > bool foundSome = false; > for (KeyframeEffectReadOnly* effect : *effects) { > MOZ_ASSERT(effect); > MatchForCompositor matchResult = IsMatchForCompositor(*effect); > > if (matchResult == MatchForCompositor::NoAndBlockThisProperty) { > if (aMatches) { > aMatches->Clear(); > } > return false; > } > > if (matchResult == MatchForCompositor::No) { > continue; > } > > if (aMatches) { > aMatches->AppendElement(effect->GetAnimation()); > } > > if (matchResult == MatchForCompositor::Yes) { > foundSome = true; > } > } > > // If all animations we added were paused animations, don't send them > // to the compositor. > if (!foundSome && aMatches) { > aMatches->Clear(); > } > > MOZ_ASSERT(!foundSome || !aMatches || !aMatches->IsEmpty(), > "If return value is true, matches array should be non-empty"); > > if (aMatches && foundSome) { > aMatches->Sort(AnimationPtrComparator<RefPtr<dom::Animation>>()); > } > > return foundSome; This looks really nice! I would like to change the 'foundSome' to 'foundRunningAnimations' too. > ::: dom/animation/EffectCompositor.cpp:169 > (Diff revision 5) > > + if (isPlaying && !hasPlayingAnimations) { > > + hasPlayingAnimations = true; > > + } > > hasPlayingAnimations |= isPlaying; > > ::: gfx/layers/ipc/LayersMessages.ipdlh:221 > (Diff revision 5) > > + // This uses dom::AnimationPlayState. > > + uint8_t playState; > > Can we just use a bool here representing paused? (Where "paused" includes > "actually paused--i.e. no start time and not play-pending; filling forwards > etc.) > > Play state feels odd here since: > * We're not going to handle all the possible values of play state > * We shouldn't handle all the different values of play state since it's > calculated from more primitive values such as start time etc. > * We really only care about two states: running or not > > If you think "paused" is confusing, then how about "frozen", "isRunning" or > "notRunning"? Yeah, the name is a bit confusing. I would like to use a word that is not mislead to 'paused' state. I think 'isStatic' matches to the state, but if you prefer to 'notRunning', I will use it.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #123) > (In reply to Brian Birtles (:birtles) from comment #121) > > If you think "paused" is confusing, then how about "frozen", "isRunning" or > > "notRunning"? > > Yeah, the name is a bit confusing. I would like to use a word that is not > mislead to 'paused' state. I think 'isStatic' matches to the state, but if > you prefer to 'notRunning', I will use it. Thanks. 'isStatic' sounds good.

Comment on attachment 8803152 [details] Bug 1305325 - Part 11: Cache non-animated base values. https://reviewboard.mozilla.org/r/87382/#review95810 I think this is ok, but I think it's based on the assumption that we send base values together with each animation we send to the compositor. If we are sending a lot of animations for a given property then that will produce unnecessary duplication. But maybe that's ok? If we decide, however, to send base values separately then I think we would want to store this "needs base value" information on the EffectSet instead? If that's possible, it seems like it would be a more natural place to store it. In fact, it might be that we can get rid of the "needs base value" information altogether and simply store the base style when we need it, and clear it otherwise. Also, where do we clear the base values when they are no longer needed? Overall, this patch is probably fine but I'd be interested to hear your answers to some of these questions (and the questions below about the cost of calling GetBaseStyle on each tick for main thread animations) before signing off on this. ::: dom/animation/EffectSet.h:202 (Diff revision 5) > + bool GetBaseStyleAnimationValue(nsCSSPropertyID aProperty, > + StyleAnimationValue& aValue) const > + { > + return mBaseStyleValues.Get(aProperty, &aValue); Would GetBaseStyle be descriptive enough? And would returning a StyleAnimationValue work (set to null in the case where the property did not have a recorded base style)? e.g. StyleAnimationValue GetBaseStyle(nsCSSPropertyID aProperty) const { StyleAnimationValue result; DebugOnly<bool> hasProperty = mBaseStyleValues.Get(aProperty, &result); MOZ_ASSERT(hasProperty || result.IsNull()); return result; } Actually, I wonder if we need the assertion. ::: dom/animation/EffectSet.h:208 (Diff revision 5) > + typedef nsDataHashtable<nsUint32HashKey, StyleAnimationValue> > + BaseStyleValueSet; > + BaseStyleValueSet& GetBaseStyleValues() > + { > + return mBaseStyleValues; > + } From what I can tell, this is only used in one place and we simply call Put on the result immediately. Why not simply add a SetBaseStyleAnimationValue() method? (Or SetBaseStyle depending on what we name the above method.) That would better encapsulate the member and mean we don't need to expose the data structure used for storing these. (I guess what I'm saying is we should probably either encapsulate this properly, or not at all.) ::: dom/animation/EffectSet.h:263 (Diff revision 5) > + // The non-animated values for properties animated by effects in this set that > + // contain at least one animation value that is composited with the underlying > + // value on the compositor if there is an additive or accumulate effect. > + BaseStyleValueSet mBaseStyleValues; This sentence is correct but the last part seems a bit redundant? Would something like this make more sense? // The non-animated values for properties animated by effects in this set // that contain at least one animation value that is composited with the // underlying value (i.e. it uses the additive or accumulate composite mode) // AND where animations for the property are run on the compositor. ::: dom/animation/KeyframeEffectReadOnly.h:335 (Diff revision 5) > + // Returns true if the effect needs base values for |aProperty| runnable on > + // the compositor. "Returns true if the effect is run on the compositor for |aProperty| and needs a base value to composite with." ::: dom/animation/KeyframeEffectReadOnly.h:413 (Diff revision 5) > + // Retain the base value if |aProperty| can be run on the compositor so that > + // we can pass the base value to the compositor when we send this effect to > + // the compositor. This might be slightly easier to follow if we change the order of the last two clauses: "Retain the base value of |aProperty| if it can be run on the compositor so that when we send this effect to the compositor, we can send the base value too." ::: dom/animation/KeyframeEffectReadOnly.h:443 (Diff revision 5) > + // A bit set that each bit represents that corresponding property needs the > + // base value or not. The bit order matches to the order of > + // LayerAnimationInfo::sRecords. A bit set where each bit represents whether or not the corresponding property requires a base value to composite with. This is only set when the property is run on the compositor. The bit order matches the order of LayerAnimationInfo::sRecords. ::: dom/animation/KeyframeEffectReadOnly.cpp:341 (Diff revision 5) > // If we are composing with composite operation that is not 'replace' > // and we have not composed style for the property yet, we have to get > // the base style for the property. > RefPtr<nsStyleContext> styleContext = GetTargetStyleContext(); > result = EffectCompositor::GetBaseValue(aProperty, > styleContext, > *mTarget->mElement); > + MaybeRetainBaseValueForCompositor(aProperty, result); This is mostly unrelated to this patch, but just thinking through this code a little more I wonder how expensive is calling GetBaseValue here? Should we be caching base values for main thread animations too? Is EffectCompositor::GetBaseValue triggering unnecessary restyling work or is it using the already calculated style? ::: dom/animation/KeyframeEffectReadOnly.cpp:1539 (Diff revision 5) > + if (LayerAnimationInfo::sRecords[i].mProperty == aProperty) { > + return mNeedsBaseValueSet[i]; > + } > + } > + MOZ_ASSERT_UNREACHABLE( > + "Shouldn't be called for property that not run on the compositor"); "Expected a property that can be run on the compositor"

(In reply to Brian Birtles (:birtles) from comment #125) > I think this is ok, but I think it's based on the assumption that we send > base values together with each animation we send to the compositor. If we > are sending a lot of animations for a given property then that will produce > unnecessary duplication. But maybe that's ok? > > If we decide, however, to send base values separately then I think we would > want to store this "needs base value" information on the EffectSet instead? > If that's possible, it seems like it would be a more natural place to store > it. In fact, it might be that we can get rid of the "needs base value" > information altogether and simply store the base style when we need it, and > clear it otherwise. Oh, never mind. I misread that part of the patch. That's not the case. We only store it if we're dealing with lowest effect in the stack for a given property. I'd still like to know about the other things though: * Where to we clear the base values? * Can we reuse this for main thread animations? Should we? * Is calling GetBaseStyle on every tick ok, if in fact that's what we do? * Is GetBaseStyle as efficient as it could be? etc.

Comment on attachment 8803154 [details] Bug 1305325 - Part 12: Pass base value for opacity or transform to the compositor. https://reviewboard.mozilla.org/r/87386/#review95812 ::: gfx/layers/Layers.cpp:426 (Diff revision 5) > +static void > +SetBaseAnimationValue(const Animatable& aAnimatable, > + StyleAnimationValue& aAnimationValue) > +{ How about making this: static StyleAnimationValue ToStyleAnimationValue(const Animatable& aAnimatable) ? For the first call site we'll use move semantics to assign the result to mBaseValue rather than just re-using mBaseValue as-is but I don't think re-using mBaseValue will actually save us any allocations in this case (since we still end up calling CreateCSSValueList which is where the main allocation happens). For the other call sites I think we'd just end up replacing: StyleAnimationValue* startValue = startValues.AppendElement(); SetBaseAnimationValue(segment.startState(), *startValue); with: startValues.AppendElement(ToStyleAnimationValue(segment.startState()); which should end up calling the overload of AppendElement that takes an rvalue reference. ::: gfx/layers/ipc/LayersMessages.ipdlh:228 (Diff revision 5) > + // This value stores base CSS value of the target element of the animation. > + // The base value is necessary for additive or accumulative animations, > + // If the animations is neither additive nor accumlative, the value is null_t. "The base value that animations should composite with. This is only set for animations with a composite mode of additive or accumulate, and only for the first animation in the set (i.e. the animation that is lowest in the stack). In all other cases the value is null_t." Is that correct? ::: layout/painting/nsDisplayList.cpp:389 (Diff revision 5) > +SetAnimatableValue(layers::Animatable& aAnimatable, > + nsCSSPropertyID aProperty, > + const StyleAnimationValue& aAnimationValue, > + nsIFrame* aFrame, > + const TransformReferenceBox& aRefBox) I think we normally put outparams last (but before optional params)? i.e. aAnimatable should go last? ::: layout/painting/nsDisplayList.cpp:423 (Diff revision 5) > +static void > +SetAnimationBaseValue(layers::AnimationBaseValue& aBaseValue, > + nsCSSPropertyID aProperty, > + nsIFrame* aFrame, > + const TransformReferenceBox& aRefBox) Likewise here, I guess aBaseValue should go last? ::: layout/painting/nsDisplayList.cpp:519 (Diff revision 5) > animation->playState() = > static_cast<uint8_t>(aAnimation->UpcomingPlayState()); > > + TransformReferenceBox refBox(aFrame); > + > + // If the animation is additive or accumulate, we need to pass its base s/accumulate/accumulates/ ?

Comment on attachment 8814295 [details] Bug 1305325 - Part 13: Factor the base value's scale in GetMinAndMaxScaleForAnimationProperty. https://reviewboard.mozilla.org/r/95526/#review95820 ::: layout/base/nsLayoutUtils.cpp:519 (Diff revision 2) > +UpdateMinMaxScales(const nsIFrame* aFrame, > + gfxSize& aMaxScale, > + gfxSize& aMinScale, Let's change the order of aMinScale and aMaxScale to match the name of the function, i.e. min comes first. Also, as before, I think outparams normally go last. Might it be worth marking this as 'inline' too? Nit: I think UpdateMinMaxScale (without an 's') might be slightly more natural. ::: layout/base/nsLayoutUtils.cpp:554 (Diff revision 2) > + // We need to factor in the scale of the base value if the value will be > + // used on the compositor. s/if the value/if the base value/ ::: layout/base/nsLayoutUtils.cpp:566 (Diff revision 2) > // In case of add or accumulate composite, StyleAnimationValue does > // not have a valid value. > if (segment.mFromComposite == dom::CompositeOperation::Replace) { Which patch did we add this check in? It seems like we could calculate the correct scale even with composite operations other than replace? Perhaps that needs to happen in a separate bug though? (It also seems like we don't calculate the correct value when we have iterationComposite set.)

Comment on attachment 8803156 [details] Bug 1305325 - Part 15: Tests composed style for missing keyframe for properties runnning on the compositor. https://reviewboard.mozilla.org/r/87390/#review95822 Nit: s/runnning/running/ in the commit message

Comment on attachment 8803155 [details] Bug 1305325 - Part 14: Compose base values on the compositor. https://reviewboard.mozilla.org/r/87388/#review95858 r=me with the following feedback addressed. In particular: * Please drop the CompositeValue wrapper in AsyncCompositionManager if possible * Please drop hasInEffectAnimations if it's not needed (I suspect it is needed, but I just don't understand why it is) ::: dom/animation/KeyframeEffectReadOnly.h:339 (Diff revision 5) > + static StyleAnimationValue CompositeValue( > + nsCSSPropertyID aProperty, > + const StyleAnimationValue& aValueToComposite, > + const StyleAnimationValue& aUnderlyingValue, > + CompositeOperation aCompositeOperation); Should we move this up closer to ComposeStyle? It seems to be closely related? ::: dom/animation/KeyframeEffectReadOnly.h:339 (Diff revision 5) > > // Returns true if the effect needs base values for |aProperty| runnable on > // the compositor. > bool NeedsBaseValue(nsCSSPropertyID aProperty) const; > > + static StyleAnimationValue CompositeValue( This needs a comment to say what it does. As far as I can tell, it doesn't handle 'replace', right? Yet we another CompositeValue method in this class that does? We should add a comment to explain that difference and what this is for? ::: dom/animation/KeyframeEffectReadOnly.cpp:315 (Diff revision 5) > + MOZ_ASSERT(aCompositeOperation == dom::CompositeOperation::Add || > + aCompositeOperation == dom::CompositeOperation::Accumulate, > + "Composite operation should be Add or Accumulate"); It seems like replace would be just "return aValueToComposite"? Should we just make it handle that to match the other CompositeValue method? ::: gfx/layers/Layers.h:1421 (Diff revision 5) > + StyleAnimationValue GetAnimationBaseValue() const > + { > + return mBaseAnimationValue; > + } We have a member that is called base-animation-value and a method called animation-base-value. We should probably make this consistent everywhere. My preference is base-animation-value (or just base-value when the context makes it obvious it's about animation). ::: gfx/layers/composite/AsyncCompositionManager.cpp:586 (Diff revision 5) > - aStart.GetUnit() == StyleAnimationValue::eUnit_None || > - aEnd.GetUnit() == StyleAnimationValue::eUnit_None, > + if (aCompositeOperation == dom::CompositeOperation::Replace) { > + // Just copy the input value in case of 'Replace'. > + return aValueToComposite; > + } I think if we move this step to KeyframeEffectReadOnly::CompositeValue we could drop this wrapper altogether? ::: gfx/layers/composite/AsyncCompositionManager.cpp:725 (Diff revision 5) > } > > InfallibleTArray<AnimData>& animationData = layer->GetAnimationData(); > > - StyleAnimationValue interpolatedValue; > + StyleAnimationValue animationValue = layer->GetAnimationBaseValue(); > + bool hasInEffectAnimations = false; Why is this needed? When will this be false (but animationValue be non-null)? ::: gfx/layers/composite/AsyncCompositionManager.cpp:787 (Diff revision 5) > double portion = > ComputedTimingFunction::GetPortion(animData.mFunctions[segmentIndex], > positionInSegment, > computedTiming.mBeforeFlag); > > - // interpolate the property > + StyleAnimationValueCompositePair from{ Nit: space between 'from' and '{' ::: gfx/layers/composite/AsyncCompositionManager.cpp:791 (Diff revision 5) > - interpolatedValue = > - SampleValue(portion, animation, > - animData.mStartValues[segmentIndex], > + animData.mStartValues[segmentIndex], > + static_cast<dom::CompositeOperation>(segment->startComposite()) > + }; > + StyleAnimationValueCompositePair to{ Likewise, whitespace here. ::: gfx/layers/composite/AsyncCompositionManager.cpp:796 (Diff revision 5) > + animationValue = Move(SampleValue(portion, > + animation, > + from, to, > - animData.mEndValues.LastElement(), > + animData.mEndValues.LastElement(), > - computedTiming.mCurrentIteration); > + computedTiming.mCurrentIteration, > + animationValue)); No need to wrap the return value of a function in Move(): it's already an rvalue.

(In reply to Brian Birtles (:birtles) from comment #126) > (In reply to Brian Birtles (:birtles) from comment #125) > > I think this is ok, but I think it's based on the assumption that we send > > base values together with each animation we send to the compositor. If we > > are sending a lot of animations for a given property then that will produce > > unnecessary duplication. But maybe that's ok? > > > > If we decide, however, to send base values separately then I think we would > > want to store this "needs base value" information on the EffectSet instead? > > If that's possible, it seems like it would be a more natural place to store > > it. In fact, it might be that we can get rid of the "needs base value" > > information altogether and simply store the base style when we need it, and > > clear it otherwise. > > Oh, never mind. I misread that part of the patch. That's not the case. We > only store it if we're dealing with lowest effect in the stack for a given > property. > > I'd still like to know about the other things though: > > * Where to we clear the base values? I was concerned that clearing them in UpdateProperties() is more than necessary in some cases, but yes, even so, it's much better than no cache at all. I will cache them for properties on the main thread.

Comment on attachment 8803155 [details] Bug 1305325 - Part 14: Compose base values on the compositor. https://reviewboard.mozilla.org/r/87388/#review95858 > We have a member that is called base-animation-value and a method called animation-base-value. We should probably make this consistent everywhere. My preference is base-animation-value (or just base-value when the context makes it obvious it's about animation). Thanks! I did rename it base-animation-style locally for consistency. > Why is this needed? When will this be false (but animationValue be non-null)? Because there is a case that all of animations there are in delay phase.

Comment on attachment 8803154 [details] Bug 1305325 - Part 12: Pass base value for opacity or transform to the compositor. https://reviewboard.mozilla.org/r/87386/#review95812 > "The base value that animations should composite with. This is only set for animations with a composite mode of additive or accumulate, and only for the first animation in the set (i.e. the animation that is lowest in the stack). In all other cases the value is null_t." > > Is that correct? Absolutely right.

Comment on attachment 8814295 [details] Bug 1305325 - Part 13: Factor the base value's scale in GetMinAndMaxScaleForAnimationProperty. https://reviewboard.mozilla.org/r/95526/#review95820 > Which patch did we add this check in? It seems like we could calculate the correct scale even with composite operations other than replace? > > Perhaps that needs to happen in a separate bug though? > > (It also seems like we don't calculate the correct value when we have iterationComposite set.) Thanks! I was actually concenred the iterationComposite case, but did not think about accumulate or additive composite at all. I should care about the case in each bug. For iterationComposite I was thinking that we can't calculate scales if iteration count is infinity, but we can calculate in other cases. I will handle it in a separate bug.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #149) > Comment on attachment 8814295 [details] > Bug 1305325 - Part 13: Factor the base value's scale in > GetMinAndMaxScaleForAnimationProperty. > > https://reviewboard.mozilla.org/r/95526/#review95820 > > > Which patch did we add this check in? It seems like we could calculate the correct scale even with composite operations other than replace? > > > > Perhaps that needs to happen in a separate bug though? > > > > (It also seems like we don't calculate the correct value when we have iterationComposite set.) > > Thanks! I was actually concenred the iterationComposite case, but did not > think about accumulate or additive composite at all. I should care about the > case in each bug. Woohoo! Calculating min/max scale there seems a hard job. It will need a utility function that does something like ComposeAnimationRule() only for transform.

Comment on attachment 8803146 [details] Bug 1305325 - Part 3: Make AnimationPropertyValueDetails::mValue optional. Olli, can you please take a look a change in KeyframeEffect.webidl? We would like to make a value in AnimationPropertyValueDetails optional. The AnimationPropertyValueDetails is used only for chrome only API. In this bug, we would like to handle the case that the value can be null.

Comment on attachment 8803146 [details] Bug 1305325 - Part 3: Make AnimationPropertyValueDetails::mValue optional. https://reviewboard.mozilla.org/r/87370/#review96220

Comment on attachment 8803154 [details] Bug 1305325 - Part 12: Pass base value for opacity or transform to the compositor. https://reviewboard.mozilla.org/r/87386/#review96616

Comment on attachment 8803153 [details] Bug 1305325 - Part 9: Send animations even if it's paused, finished or zero playback rate. . https://reviewboard.mozilla.org/r/87384/#review96656 ::: dom/animation/Animation.h:331 (Diff revision 6) > + /** > + * Returns true if the animation is paused or waiting to pause or finished. > + */ > + bool IsStatic() const; Do we need this? It seems confusing to have both IsPlaying() and IsStatic() (and if we do keep them, they should go side-by-side). As far as I can tell we have: IsPlaying mPlaybackRate != 0.0 && (PlayState() == AnimationPlayState::Running || mPendingState == PendingState::PlayPending) && !GetEffect()->IsActiveDurationZero(); IsStatic PlayState() == AnimationPlayState::Paused || PlayState() == AnimationPlayState::Finished || mPendingState == PendingState::PausePending; So I wonder if we can use !IsPlaying() instead of IsStatic(). Comparing the two we have !IsPlaying() is equivalent to: !(mPlaybackRate != 0.0 && (PlayState() == AnimationPlayState::Running || mPendingState == PendingState::PlayPending) && !GetEffect()->IsActiveDurationZero()) is equivalent to: mPlaybackRate == 0.0 || !(PlayState() == AnimationPlayState::Running || mPendingState == PendingState::PlayPending) || GetEffect()->IsActiveDurationZero(); is equivalent to: mPlaybackRate == 0.0 || (PlayState() != AnimationPlayState::Running && mPendingState != PendingState::PlayPending) || GetEffect()->IsActiveDurationZero(); which compared to: PlayState() == AnimationPlayState::Paused || PlayState() == AnimationPlayState::Finished || mPendingState == PendingState::PausePending; Means that if we use !IsPlaying() instead of IsStatic() we would *also* return true for: * idle animations * zero playback-rate animations * zero active duration animations Now, IsStatic() is only used in nsDisplayList on animations returned from GetAnimationsForCompositor. Inside GetAnimationsForCompositor we never include animations where animation->IsPlayableOnCompositor() returns false, so we should never have idle animations here. So, the only two cases where this would make a difference is zero playback-rate animations and zero active duration animations. Provided we make sure we set the start time correctly on the compositor for these two cases, then I think we don't need IsStatic(). In particular I think we'd need to make sure that the following lines work correctly: // Resolve the start time by the hold time if the animation was // waiting to start. if (anim.startTime().IsNull() && !anim.isStatic()) { anim.startTime() = aReadyTime - anim.holdTime() + anim.delay(); updated = true; } This should be fine since now we will simply *not* update the start time for these two cases which should be ok. MOZ_ASSERT(!animation.startTime().IsNull() || animation.isStatic(), "Failed to resolve start time of play pending animations"); // If the animation is static, i.e. paused or finished, then // use the hold time to stay at the same position. TimeDuration elapsedDuration = animation.isStatic() ? animation.holdTime() : (aPoint - animation.startTime()) .MultDouble(animation.playbackRate()); I *think* this will be ok. (If we do encounter issues we might consider changing the point where we set the layer animation's start time so that it is null for static animations. That would at least make all static cases behave more like paused animations.) ::: dom/animation/EffectCompositor.cpp:63 (Diff revision 6) > + // This (paused) animation matches and can be run on the compositor > + // if there > + // are other animations for this property that return 'Yes'. Line breaking here seems off. Also, maybe we should replace "(paused)" with "(currently static)" to match the terminology used elsewhere. Or, if we want to be really consistent, we could use "(not currently playing)" here? Or we could even rename IsPlaying() to IsStatic() and invert the logic and use "static" everywhere--that would probably be the best option but probably should happen as a separate patch. ::: dom/animation/EffectCompositor.cpp:67 (Diff revision 6) > + // This animation does not match or can't be run on the > + // compositor. It looks like this comment would fit on one line? ::: dom/animation/EffectCompositor.cpp:70 (Diff revision 6) > + // This animation does not match or can't be run > + // on the compositor and, > + // furthermore, its presence means we should > + // not run any animations for this > + // property on the compositor. The line breaking here seems off too. (Does vim's gq not do this for you?) ::: dom/animation/EffectCompositor.cpp:93 (Diff revision 6) > + return MatchForCompositor::No; > + } > + > + bool isPlaying = animation->IsPlaying(); > + > + // If we are finding animations for transform, check that there are other Nit: s/check that/check if/ (I wrote this comment so this is really my mistake) ::: dom/animation/EffectCompositor.cpp:107 (Diff revision 6) > + aFrame, aProperty, > + AnimationPerformanceWarning(warningType)); > + return MatchForCompositor::NoAndBlockThisProperty; > + } > + > + // If the animation's effect has no |aProperty|, skip following checks. I don't think we need this comment? (It doesn't seem to make sense either--there are no "following checks") ::: dom/animation/EffectCompositor.cpp:216 (Diff revision 6) > - MOZ_ASSERT(!foundSome || !aMatches || !aMatches->IsEmpty(), > + // If all animations we added were paused animations, don't send them to the > + // compositor. Again, we should replace "paused" with either "not currently playing" or "static". ::: gfx/layers/Layers.cpp:501 (Diff revision 6) > - anim.startTime() = aReadyTime - anim.initialCurrentTime(); > + // Resolve the start time by the hold time if the animation was > + // waiting to start. Nit: Might be easier to read as "If the animation is play-pending, resolve the start time." (I don't think we need to specifically mention the hold time? In fact, I'm not really sure we need this comment.) ::: gfx/layers/composite/AsyncCompositionManager.cpp:673 (Diff revision 6) > AnimData& animData = animationData[i]; > > activeAnimations = true; > > - MOZ_ASSERT(!animation.startTime().IsNull(), > - "Failed to resolve start time of pending animations"); > + MOZ_ASSERT(!animation.startTime().IsNull() || animation.isStatic(), > + "Failed to resolve start time of play pending animations"); Nit: s/play pending/play-pending/ ::: gfx/layers/composite/AsyncCompositionManager.cpp:674 (Diff revision 6) > - TimeDuration elapsedDuration = > - (aPoint - animation.startTime()).MultDouble(animation.playbackRate()); > + // If the animation is static, i.e. paused or finished, then > + // use the hold time to stay at the same position. Based on the above discussion, the meaning of static might extent to include zero-playback rate animations, so we should do s/i.e./e.g./ here. ::: gfx/layers/ipc/LayersMessages.ipdlh:193 (Diff revision 6) > - // of startTime such that we begin playback from initialCurrentTime. > - TimeDuration initialCurrentTime; > + // 1) Animations that are waiting to start, i.e. PlayPending. > + // Their startTime will be null. So once the animation is ready to start, > + // we calculate an appropriate value of startTime such that we begin > + // playback from holdTime. > + // 2) Paused animations to stay at this hold time. > + // 3) Animations that are finished but filling forwards to stay at this hold > + // time. Again, given that we might expand the meaning of static, we could simplify this list to just: 1) Animations that are play-pending. Initially these animations will have a null |startTime|. Once the animation is ready to start (i.e. painting has finished), we calculate an appropriate value of |startTime| such that playback begins from |holdTime|. 2) Static animations (e.g. paused and finished animations). In this case the |holdTime| represents the current time the animation will maintain. (And, writing this comment, I realize that using |holdTime| for forwards-filling animations is going to cause problems for scroll-driven animations. But we can fix that later.) ::: gfx/layers/ipc/LayersMessages.ipdlh:221 (Diff revision 6) > AnimationData data; > float playbackRate; > // This is used in the transformed progress calculation. > TimingFunction easingFunction; > uint8_t iterationComposite; > + // Represents that the animation is paused or finished but filling forwards. "True if the animation has a fixed current time (e.g. paused and forwards-filling animations)" ::: layout/painting/nsDisplayList.cpp:396 (Diff revision 6) > - MOZ_ASSERT(!aAnimation->GetCurrentOrPendingStartTime().IsNull() || > + MOZ_ASSERT((!aAnimation->GetCurrentOrPendingStartTime().IsNull() || > - (aAnimation->GetTimeline() && > + (aAnimation->GetTimeline() && > - aAnimation->GetTimeline()->TracksWallclockTime()), > - "Animation should either have a resolved start time or " > - "a timeline that tracks wallclock time"); > + aAnimation->GetTimeline()->TracksWallclockTime())) || > + !aAnimation->GetCurrentTime().IsNull(), > + "If the animation has a resolved start time it should have a " > + "timeline capable of converting it to a TimeStamp, or the " > + "animation should have a resolved current time"); This is a bit hard to follow, but I guess we have two cases where GetCurrentOrPendingStartTime() can be null: a) A play-pending animation with a null start time -- we need a suitable timeline so we can set the start time later. b) A static animation with a null start time -- all we need is a current time here Is that what we're trying to test? Because a play-pending animation will still have a non-null current time so I think adding !aAnimation->GetCurrentTime().IsNull() will make the check for (a) no longer right? Then again, I wonder why we are hitting this assertion anyway--is it due to some of our tests that manipulate the refresh driver? I'm probably misunderstanding this.

(In reply to Brian Birtles (:birtles) from comment #154) > Means that if we use !IsPlaying() instead of IsStatic() we would *also* > return true for: > > * idle animations > * zero playback-rate animations > * zero active duration animations Thinking about this some more, zero active duration animations are problematic. They are only static after are finished (in which case we'd return false anyway). I wonder if we can remove zero active duration animations from IsPlaying() now anyway? We appear to have added that check back in bug 1223658 and it makes sense--we probably don't want to run a zero-duration animation on the compositor if it's the only animation. Maybe the best thing is just to change the condition: return isPlaying ? MatchForCompositor::Yes : MatchForCompositor::IfNeeded; to return isPlaying && !zeroActiveDuration ? MatchForCompositor::Yes : MatchForCompositor::IfNeeded; We *do* want isPlaying to be true for such animations since a zero-duration 'width' animation should block transform animations while it is in its delay phase. So I think it's correct to move this check from IsPlaying(), at least here.

(In reply to Brian Birtles (:birtles) from comment #155) > Means that if we use !IsPlaying() instead of IsStatic() we would *also* > return true for: > > * idle animations > * zero playback-rate animations > * zero active duration animations > > Now, IsStatic() is only used in nsDisplayList on animations returned from > GetAnimationsForCompositor. Inside GetAnimationsForCompositor we never > include animations where animation->IsPlayableOnCompositor() returns false, > so we should never have idle animations here. > > So, the only two cases where this would make a difference is zero > playback-rate animations and zero active duration animations. > > Provided we make sure we set the start time correctly on the compositor for > these two cases, then I think we don't need IsStatic(). > > In particular I think we'd need to make sure that the following lines work > correctly: > > // Resolve the start time by the hold time if the animation was > // waiting to start. > if (anim.startTime().IsNull() && !anim.isStatic()) { > anim.startTime() = aReadyTime - anim.holdTime() + anim.delay(); > updated = true; > } > > This should be fine since now we will simply *not* update the start time for > these two cases which should be ok. > > MOZ_ASSERT(!animation.startTime().IsNull() || animation.isStatic(), > "Failed to resolve start time of play pending animations"); > // If the animation is static, i.e. paused or finished, then > // use the hold time to stay at the same position. > TimeDuration elapsedDuration = animation.isStatic() > ? animation.holdTime() > : (aPoint - animation.startTime()) > .MultDouble(animation.playbackRate()); > > I *think* this will be ok. (If we do encounter issues we might consider > changing the point where we set the layer animation's start time so that it > is null for static animations. That would at least make all static cases > behave more like paused animations.) I also think this will work. I was firstly concerned that hold time is not set for those zero active duration/playback animations, but yeah, I did use GetCurrentTime() there (it was for pending animations). I will try to use IsPlaying(). > ::: dom/animation/EffectCompositor.cpp:70 > (Diff revision 6) > > + // This animation does not match or can't be run > > + // on the compositor and, > > + // furthermore, its presence means we should > > + // not run any animations for this > > + // property on the compositor. > > The line breaking here seems off too. (Does vim's gq not do this for you?) Yes, this is really odd, I am not sure what happened. ;-P > ::: layout/painting/nsDisplayList.cpp:396 > (Diff revision 6) > > - MOZ_ASSERT(!aAnimation->GetCurrentOrPendingStartTime().IsNull() || > > + MOZ_ASSERT((!aAnimation->GetCurrentOrPendingStartTime().IsNull() || > > - (aAnimation->GetTimeline() && > > + (aAnimation->GetTimeline() && > > - aAnimation->GetTimeline()->TracksWallclockTime()), > > - "Animation should either have a resolved start time or " > > - "a timeline that tracks wallclock time"); > > + aAnimation->GetTimeline()->TracksWallclockTime())) || > > + !aAnimation->GetCurrentTime().IsNull(), > > + "If the animation has a resolved start time it should have a " > > + "timeline capable of converting it to a TimeStamp, or the " > > + "animation should have a resolved current time"); > > This is a bit hard to follow, but I guess we have two cases where > GetCurrentOrPendingStartTime() can be null: > > a) A play-pending animation with a null start time -- we need a suitable > timeline so we can set the start time later. > b) A static animation with a null start time -- all we need is a current > time here > > Is that what we're trying to test? > > Because a play-pending animation will still have a non-null current time so > I think adding !aAnimation->GetCurrentTime().IsNull() will make the check > for (a) no longer right? Exactly! Checking current time does not check at all! I didn't want to cause a case that slips thorough the current assertion. I guess maybe !IsPlaying() check is sufficient there. > b) A static animation with a null start time -- all we need is a current > time here Yes, but with zero playback-rate/active duration animations, star time might be resolved, so !IsPlaying() will be fit there.

(In reply to Brian Birtles (:birtles) from comment #155) > (In reply to Brian Birtles (:birtles) from comment #154) > > Means that if we use !IsPlaying() instead of IsStatic() we would *also* > > return true for: > > > > * idle animations > > * zero playback-rate animations > > * zero active duration animations > > Thinking about this some more, zero active duration animations are > problematic. They are only static after are finished (in which case we'd > return false anyway). > > I wonder if we can remove zero active duration animations from IsPlaying() > now anyway? We appear to have added that check back in bug 1223658 and it > makes sense--we probably don't want to run a zero-duration animation on the > compositor if it's the only animation. After thinking about this a bit more, I think it's probably ok to run a zero-duration animation on the compositor even if it's the only one. You might, for example, implement frame-by-frame animation as follows: frame1.animate({ opacity: 1 }, { fill: 'forwards' }); // delay 0, duration 0 frame2.animate({ opacity: 1 }, { fill: 'forwards', delay: 50 }); // delay 0, duration 0 frame3.animate({ opacity: 1 }, { fill: 'forwards', delay: 100 }); // delay 0, duration 0 frame4.animate({ opacity: 1 }, { fill: 'forwards', delay: 150 }); // delay 0, duration 0 I'm not sure how likely that is but if it does occur, ideally you'd want to run the animations on the compositor. If that's the case, then I think the only change would be to remove the check for zero durations from IsPlaying().

(In reply to Brian Birtles (:birtles) from comment #157) > (In reply to Brian Birtles (:birtles) from comment #155) > > (In reply to Brian Birtles (:birtles) from comment #154) > > > Means that if we use !IsPlaying() instead of IsStatic() we would *also* > > > return true for: > > > > > > * idle animations > > > * zero playback-rate animations > > > * zero active duration animations > > > > Thinking about this some more, zero active duration animations are > > problematic. They are only static after are finished (in which case we'd > > return false anyway). > > > > I wonder if we can remove zero active duration animations from IsPlaying() > > now anyway? We appear to have added that check back in bug 1223658 and it > > makes sense--we probably don't want to run a zero-duration animation on the > > compositor if it's the only animation. > > After thinking about this a bit more, I think it's probably ok to run a > zero-duration animation on the compositor even if it's the only one. You > might, for example, implement frame-by-frame animation as follows: > > frame1.animate({ opacity: 1 }, { fill: 'forwards' }); // delay 0, duration > 0 > frame2.animate({ opacity: 1 }, { fill: 'forwards', delay: 50 }); // delay > 0, duration 0 > frame3.animate({ opacity: 1 }, { fill: 'forwards', delay: 100 }); // delay > 0, duration 0 > frame4.animate({ opacity: 1 }, { fill: 'forwards', delay: 150 }); // delay > 0, duration 0 > > I'm not sure how likely that is but if it does occur, ideally you'd want to > run the animations on the compositor. > > If that's the case, then I think the only change would be to remove the > check for zero durations from IsPlaying(). Thanks. It's much simpler. Actually I was thinking last night how to leave a comment in IsMatchForCompositor. I will do it.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #158) > (In reply to Brian Birtles (:birtles) from comment #157) > > (In reply to Brian Birtles (:birtles) from comment #155) > > > (In reply to Brian Birtles (:birtles) from comment #154) > > > > Means that if we use !IsPlaying() instead of IsStatic() we would *also* > > > > return true for: > > > > > > > > * idle animations > > > > * zero playback-rate animations > > > > * zero active duration animations > > > > > > Thinking about this some more, zero active duration animations are > > > problematic. They are only static after are finished (in which case we'd > > > return false anyway). > > > > > > I wonder if we can remove zero active duration animations from IsPlaying() > > > now anyway? We appear to have added that check back in bug 1223658 and it > > > makes sense--we probably don't want to run a zero-duration animation on the > > > compositor if it's the only animation. > > > > After thinking about this a bit more, I think it's probably ok to run a > > zero-duration animation on the compositor even if it's the only one. You > > might, for example, implement frame-by-frame animation as follows: > > > > frame1.animate({ opacity: 1 }, { fill: 'forwards' }); // delay 0, duration > > 0 > > frame2.animate({ opacity: 1 }, { fill: 'forwards', delay: 50 }); // delay > > 0, duration 0 > > frame3.animate({ opacity: 1 }, { fill: 'forwards', delay: 100 }); // delay > > 0, duration 0 > > frame4.animate({ opacity: 1 }, { fill: 'forwards', delay: 150 }); // delay > > 0, duration 0 > > > > I'm not sure how likely that is but if it does occur, ideally you'd want to > > run the animations on the compositor. > > > > If that's the case, then I think the only change would be to remove the > > check for zero durations from IsPlaying(). > > Thanks. It's much simpler. Actually I was thinking last night how to leave > a comment in IsMatchForCompositor. I will do it. Oops. I did miss that a filling-forwards animation on an element is considered as 'IfNeeded', it means each animation on each frame does not run on the compositor in filling-forwards state (i.e. pulling back from the compositor). As a result this frame type animation causes flicker. I am inclined to do here: 1) Check active duration is zero in IsMatchForCompositor (as Brian suggested in comment 155) 2) Leave the problem that flickers animations on different elements (frame-by-frame animation) for a future issue. Brian, what do you think?

(In reply to Hiroyuki Ikezoe (:hiro) from comment #159) > Oops. I did miss that a filling-forwards animation on an element is > considered as 'IfNeeded', it means each animation on each frame does not run > on the compositor in filling-forwards state (i.e. pulling back from the > compositor). As a result this frame type animation causes flicker. I am > inclined to do here: > > 1) Check active duration is zero in IsMatchForCompositor (as Brian suggested > in comment 155) > 2) Leave the problem that flickers animations on different elements > (frame-by-frame animation) for a future issue. > > Brian, what do you think? What is the cause of the flicker? Removing animations from the compositor shouldn't cause flicker per-se. Do you have a test case available?

(In reply to Brian Birtles (:birtles) from comment #160) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #159) > > Oops. I did miss that a filling-forwards animation on an element is > > considered as 'IfNeeded', it means each animation on each frame does not run > > on the compositor in filling-forwards state (i.e. pulling back from the > > compositor). As a result this frame type animation causes flicker. I am > > inclined to do here: > > > > 1) Check active duration is zero in IsMatchForCompositor (as Brian suggested > > in comment 155) > > 2) Leave the problem that flickers animations on different elements > > (frame-by-frame animation) for a future issue. > > > > Brian, what do you think? > > What is the cause of the flicker? Removing animations from the compositor > shouldn't cause flicker per-se. Do you have a test case available? I don't actually see any example, but if the compositor is under heavy load at the time of finish of the animation (I mean the final style is not yet painted on the compositor), we will see flicker I think. Is this another problem?

(In reply to Hiroyuki Ikezoe (:hiro) from comment #161) > (In reply to Brian Birtles (:birtles) from comment #160) > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #159) > > > Oops. I did miss that a filling-forwards animation on an element is > > > considered as 'IfNeeded', it means each animation on each frame does not run > > > on the compositor in filling-forwards state (i.e. pulling back from the > > > compositor). As a result this frame type animation causes flicker. I am > > > inclined to do here: > > > > > > 1) Check active duration is zero in IsMatchForCompositor (as Brian suggested > > > in comment 155) > > > 2) Leave the problem that flickers animations on different elements > > > (frame-by-frame animation) for a future issue. > > > > > > Brian, what do you think? > > > > What is the cause of the flicker? Removing animations from the compositor > > shouldn't cause flicker per-se. Do you have a test case available? > > I don't actually see any example, but if the compositor is under heavy load > at the time of finish of the animation (I mean the final style is not yet > painted on the compositor), we will see flicker I think. Is this another > problem? OK. I am not yet convinced this problem will be a real problem. So let's get drop zero active duration check in IsMatchForCompositor. If we see this kind of problem later, I will re-visit it.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #162) > OK. I am not yet convinced this problem will be a real problem. So let's > get drop zero active duration check in IsMatchForCompositor. If we see this > kind of problem later, I will re-visit it. Yes, I'm not sure if it will be a problem--and if it is, I suspect it will affect any animation with a very short duration so it doesn't make sense to exclude zero-duration animations only.

Note that I did drop 'static' words all over the place. Also I should note that there is a bug about null of StyleAnimationValue between the main and the compositor thread, actually it does not matter in this bug (it will be a problem for normal additive animations), because we don't handle the null StyleAnimationValue on the compositor for now. I will handle it in implementation of additive animations.

Comment on attachment 8803153 [details] Bug 1305325 - Part 9: Send animations even if it's paused, finished or zero playback rate. . https://reviewboard.mozilla.org/r/87384/#review97326 r=me with: * IsPlayingOnCompositor() replaced with IsRelevant() [ assuming that works ] * The changes to that big assertion in nsDisplayList.cpp * Dropping the tests for zero-duration animations * Dropping the comment in test_animations_omta.html ::: dom/animation/Animation.h:280 (Diff revision 7) > * We send animations to the compositor when their target effect is 'current' > * (a definition that is roughly equivalent to when they are in their before > - * or active phase). However, we don't send animations to the compositor when > + * or active phase). > - * they are paused/pausing (including being effectively paused due to > - * having a zero playback rate), have a zero-duration active interval, or have > - * no target effect at all. > */ > bool IsPlayableOnCompositor() const > { > - return HasCurrentEffect() && > - mPlaybackRate != 0.0 && > + return HasCurrentEffect() || IsInEffect(); > + } The comment and code don't match here. I went to rewrite it and I noticed that it is basically the same as IsRelevant(). Can we just use IsRelevant() instead and drop this method? ::: layout/painting/nsDisplayList.cpp:396 (Diff revision 7) > - MOZ_ASSERT(!aAnimation->GetCurrentOrPendingStartTime().IsNull() || > + MOZ_ASSERT((!aAnimation->GetCurrentOrPendingStartTime().IsNull() || > - (aAnimation->GetTimeline() && > + (aAnimation->GetTimeline() && > - aAnimation->GetTimeline()->TracksWallclockTime()), > - "Animation should either have a resolved start time or " > - "a timeline that tracks wallclock time"); > + aAnimation->GetTimeline()->TracksWallclockTime())) || > + !aAnimation->GetCurrentTime().IsNull(), > + "If the animation has a resolved start time it should have a " > + "timeline capable of converting it to a TimeStamp, or the " > + "animation should have a resolved current time"); As per our discussion in comment 154 and 156 I think this assertion is not right. Going over that conversation again, there are two cases where GetCurrentOrPendingStartTime() can be null: > a) A play-pending animation with a null start time -- we need a suitable > timeline so we can set the start time later. > b) A static animation with a null start time -- all we need is a current > time here So this check should be: !aAnimation->GetCurrentOrPendingStartTime().IsNull() || (aAnimation->GetTimeline() && aAnimation->GetTimeline()->TracksWallclockTime()) || !aAnimation->IsPlaying() i.e. MOZ_ASSERT(!aAnimation->GetCurrentOrPendingStartTime().IsNull() || !aAnimation->IsPlaying() || (aAnimation->GetTimeline() && aAnimation->GetTimeline()->TracksWallclockTime()), "If the animation has an unresolved start time it should either" " be static (so we don't need a start time) or else have a" " timeline capable of converting TimeStamps (so we can calculate" " one later)"); ::: layout/style/test/test_animations_omta.html:1895 (Diff revision 7) > + * Note: We send the zero-duration animations to the compositor after bug > + * 1305325. I think we can drop this comment. We should keep this test because it exists in test_animations.html and we try to mirror those tests in test_animations_omta.html. The fact that we run these animations on the compositor now, is not really important to note.

Comment on attachment 8803152 [details] Bug 1305325 - Part 11: Cache non-animated base values. https://reviewboard.mozilla.org/r/87382/#review96688 This is looking good, but I wonder if we really want to clear cached base values on a per-property basis. See comments below. ::: dom/animation/EffectCompositor.h:220 (Diff revision 6) > static void SetPerformanceWarning( > const nsIFrame* aFrame, > nsCSSPropertyID aProperty, > const AnimationPerformanceWarning& aWarning); > > - // Get resolved base style on |aElement| for |aProperty| with |aStyleContext|. > + // Returns the base style on (psuedo-)element for |aProperty|. s/on/of/ s/psuedo/pseudo/ ::: dom/animation/EffectCompositor.h:224 (Diff revision 7) > > - // Get resolved base style on |aElement| for |aProperty| with |aStyleContext|. > - static StyleAnimationValue GetBaseStyle(nsCSSPropertyID aProperty, > + // Returns the base style on (psuedo-)element for |aProperty|. > + // If there is no cached base style for the property, a new base style value > + // is resolved with |aStyleContext|. The new resolved base style is cached > + // until ClearBaseStylesForAnimationProperties is called. > + static StyleAnimationValue GetCachedOrResolvedBaseStyle( I wonder if this should just be called GetBaseStyle? I understand wanting to put "Cached" in the name but the "OrResolved" doesn't make much sense to me. It seems like it would be enough to just describe in the comments what it does? What do you think? ::: dom/animation/EffectCompositor.h:231 (Diff revision 7) > + static void ClearBaseStylesForAnimationProperties( > + dom::Element& aElement, > + CSSPseudoElementType aPseudoType, > + nsTArray<AnimationProperty>& aProperties); I wonder if we really need to pass in the set of properties. In effect we iterate over each animation, and then within each animation we iterate over each property and call ClearBaseStyle. That seems like we'll often end up calling ClearBaseStyle multiple times for the same property. Can we just call ClearBaseStyles(aElement, aPseudoType) at the start of UpdateEffectProperties and blow away all the base styles on effectSet at once? ::: dom/animation/KeyframeEffectReadOnly.h:441 (Diff revision 7) > > // We need to track when we go to or from being "in effect" since > // we need to re-evaluate the cascade of animations when that changes. > bool mInEffectOnLastAnimationTimingUpdate; > > + // A bit set where bit represents whether or not the corresponding property s/where bit/where each bit/ ::: dom/animation/KeyframeEffectReadOnly.cpp:1524 (Diff revision 7) > +KeyframeEffectReadOnly::SetNeedsBaseStyleForCompositor( > + nsCSSPropertyID aProperty) Let's just call this SetNeedsBaseStyle to match NeedsBaseStyle() and because I think we intend to use this for main thread animations in future too, right?

(In reply to Brian Birtles (:birtles) from comment #181) > ::: dom/animation/EffectCompositor.h:224 > (Diff revision 7) > > > > - // Get resolved base style on |aElement| for |aProperty| with |aStyleContext|. > > - static StyleAnimationValue GetBaseStyle(nsCSSPropertyID aProperty, > > + // Returns the base style on (psuedo-)element for |aProperty|. > > + // If there is no cached base style for the property, a new base style value > > + // is resolved with |aStyleContext|. The new resolved base style is cached > > + // until ClearBaseStylesForAnimationProperties is called. > > + static StyleAnimationValue GetCachedOrResolvedBaseStyle( > > I wonder if this should just be called GetBaseStyle? > > I understand wanting to put "Cached" in the name but the "OrResolved" > doesn't make much sense to me. It seems like it would be enough to just > describe in the comments what it does? What do you think? Yeah, it the comment is sufficient, it's nice. > ::: dom/animation/EffectCompositor.h:231 > (Diff revision 7) > > + static void ClearBaseStylesForAnimationProperties( > > + dom::Element& aElement, > > + CSSPseudoElementType aPseudoType, > > + nsTArray<AnimationProperty>& aProperties); > > I wonder if we really need to pass in the set of properties. In effect we > iterate over each animation, and then within each animation we iterate over > each property and call ClearBaseStyle. That seems like we'll often end up > calling ClearBaseStyle multiple times for the same property. > > Can we just call ClearBaseStyles(aElement, aPseudoType) at the start of > UpdateEffectProperties and blow away all the base styles on effectSet at > once? Though I don't check it at all, when we change the target element, does that work? I should check it. > ::: dom/animation/KeyframeEffectReadOnly.cpp:1524 > (Diff revision 7) > > +KeyframeEffectReadOnly::SetNeedsBaseStyleForCompositor( > > + nsCSSPropertyID aProperty) > > Let's just call this SetNeedsBaseStyle to match NeedsBaseStyle() and because > I think we intend to use this for main thread animations in future too, > right? Yes, will do.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #182) > (In reply to Brian Birtles (:birtles) from comment #181) > > ::: dom/animation/EffectCompositor.h:224 > > (Diff revision 7) > > > > > > - // Get resolved base style on |aElement| for |aProperty| with |aStyleContext|. > > > - static StyleAnimationValue GetBaseStyle(nsCSSPropertyID aProperty, > > > + // Returns the base style on (psuedo-)element for |aProperty|. > > > + // If there is no cached base style for the property, a new base style value > > > + // is resolved with |aStyleContext|. The new resolved base style is cached > > > + // until ClearBaseStylesForAnimationProperties is called. > > > + static StyleAnimationValue GetCachedOrResolvedBaseStyle( > > > > I wonder if this should just be called GetBaseStyle? > > > > I understand wanting to put "Cached" in the name but the "OrResolved" > > doesn't make much sense to me. It seems like it would be enough to just > > describe in the comments what it does? What do you think? > > Yeah, it the comment is sufficient, it's nice. > > > ::: dom/animation/EffectCompositor.h:231 > > (Diff revision 7) > > > + static void ClearBaseStylesForAnimationProperties( > > > + dom::Element& aElement, > > > + CSSPseudoElementType aPseudoType, > > > + nsTArray<AnimationProperty>& aProperties); > > > > I wonder if we really need to pass in the set of properties. In effect we > > iterate over each animation, and then within each animation we iterate over > > each property and call ClearBaseStyle. That seems like we'll often end up > > calling ClearBaseStyle multiple times for the same property. > > > > Can we just call ClearBaseStyles(aElement, aPseudoType) at the start of > > UpdateEffectProperties and blow away all the base styles on effectSet at > > once? > > Though I don't check it at all, when we change the target element, does that > work? I should check it. Unnecessary styles may be left there, but it's OK.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c8043738bc7b9ca383b1e901be98ac7c1066eb8 Using IsRelevant() and clearing base styles in UpdateEffectProperties() seems to work fine on the try.