Closed Bug 1317208 Opened 8 years ago Closed 7 years ago

Stylo: Store servo computed values for animation properties

Categories

(Core :: DOM: Animation, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: birtles, Assigned: manishearth)

References

Details

Attachments

(1 file)

Copying from the etherpad[1], we have the following plan for getting animation
to work:

Current Gecko flow:
1. Animation values input as "string"
2. Convert to nsCSSValue (specified value) -- stored as
   KeyframeEffectReadOnly::mKeyframes
3. Convert to StyleAnimationValue (computed value for interpolation) -- this
   happens in KeyframeEffectReadOnly::UpdateProperties, stored as mProperties
   (This happens whenever the parent style context changes, mProperties is
   basically thrown away each time)
4. Interpolate and produce an output StyleAnimationValue object
5. Uncompute to nsCSSValue for re-adding to the cascade
6. Add to cascade

Proposed Servo flow:
(Common part)
1. Animation values input as "string" [done]
2. Convert to ServoDeclarationBlock (specified value) [done]
(Branch A -- this is temporary)
3a. Convert to StyleAnimationValue (computed value) for (a) distance
    calculations, (b) interpolation on the compositor [done]
(Branch B)
3b. Convert to Servo computed values (store as opaque pointers on
    KeyframeEffectReadOnly parallel to mProperties) [this is the part Emilio was
    going to do]
4b. Interpolate Servo computed values using Servo interpolation code and produce
    new Servo computed values
5b. Uncompute Servo computed values to ServoDeclarationBlock (or something
    similar) for re-adding to the cascade [function written, needs to be used]
6b. Add to cascade (use something similar to what we do for the style attribute)

This bug is about 3b from above. Emilio added AnimationValue to servo[2] so this bug is about making KeyframeEffectReadOnly::UpdateProperties take the Servo values in mKeyframes (stuffed inside mServoDeclarationBlock in the PropertyValuePair objects), produce AnimationValue objects, and either stick them in mProperties (i.e. extend AnimationPropertySegment) or create some parallel array for storing them.

[1] https://public.etherpad-mozilla.org/p/stylo-animation
[2] https://github.com/servo/servo/pull/13694
Assignee: nobody → boris.chiou
Hi Manish,

I checked your messages in irc with Brian, and it seems you will implement another method which does what Servo_RestyleWithAddedDeclaration does but returns an AnimationValues, right? That is what I'm looking for now (e.g. getting a ComputedValue/AnimationValues from a specified value and a style context). If so, I think this bug could be fixed more easily.

Thanks.
Flags: needinfo?(manishearth)
Yep, that's what I'm working on. I'm creating a method that fills in a `Vec<AnimationValue>` by computing a propertydeclarationblock
Flags: needinfo?(manishearth)
Assignee: boris.chiou → nobody
Assignee: nobody → manishearth
This also includes https://github.com/servo/servo/pull/14684 without which the bindings don't generate.

I had to box the animation values since I can't represent the type (a Rust ADT) in C++ without Rust -> C++ bindgen. Since it was being boxed anyway, I refcounted it to avoid the need for a deep copy constructor since it's being boxed anyway.

(distance computation does copy these values, I'm not sure if a deep copy is actually necessary here. This can be revisited when we try handling distance computation)
Status: NEW → ASSIGNED
Comment on attachment 8821434 [details]
Bug 1317208 - Stylo: Store servo computed values for animation properties;

https://reviewboard.mozilla.org/r/100728/#review101206

(Publishing these and switching to r3 to review...)

::: dom/animation/KeyframeEffectReadOnly.h:23
(Diff revision 2)
> +
> +#include "mozilla/ServoBindingTypes.h"

Can drop this.

::: dom/animation/KeyframeEffectReadOnly.h:128
(Diff revision 2)
>  {
>    float mFromKey, mToKey;
>    // NOTE: In the case that no keyframe for 0 or 1 offset is specified
>    // the unit of mFromValue or mToValue is eUnit_Null.
>    StyleAnimationValue mFromValue, mToValue;
> +#ifdef MOZ_STYLO

I guess I'm not super worried about the memory usage here, so I would probably just drop the MOZ_STYLO guards here and elsewhere in the patch.  It should be fine that we might copy around null values in these RefPtrs, and we'll never actually call in to Servo_AnimationValue_AddRef/Release, right?

::: dom/animation/KeyframeUtils.cpp:637
(Diff revision 2)
>            continue;
> +#ifdef MOZ_STYLO
> +          Servo_AnimationValues_Populate(&values,
> +                                         &*pair.mServoDeclarationBlock,
> +                                         currentStyle,
> +                                         parentStyle);
> +#endif

This looks like unreachable code, coming right after the "continue"...
Comment on attachment 8821434 [details]
Bug 1317208 - Stylo: Store servo computed values for animation properties;

https://reviewboard.mozilla.org/r/100728/#review101210

::: dom/animation/KeyframeUtils.cpp:608
(Diff revision 3)
>    StyleBackendType styleBackend = aElement->OwnerDoc()->GetStyleBackendType();
>  
>    const size_t len = aKeyframes.Length();
>    nsTArray<ComputedKeyframeValues> result(len);
>  
> +  const ServoComputedValues *currentStyle, *parentStyle;

Nit: to adhere to the "* next to type" rule, I think we should prefer to have two separate declarations.

::: dom/animation/KeyframeUtils.cpp:674
(Diff revision 3)
> -        computedValues->AppendElement(value);
> +        computedValues->AppendElement(Move(value));
>          propertiesOnThisKeyframe.AddProperty(value.mProperty);

I guess this is OK because moving the PropertyStyleAnimationValuePair doesn't invalidate the mProperty value, though it does look a little strange.  Maybe swap the order of these two statements around?

::: dom/animation/KeyframeUtils.cpp
(Diff revision 3)
> +    segment->mServoFromValue = aEntries[i].mServoValue;
> +    segment->mServoToValue   = aEntries[j].mServoValue;
> +#endif
>      segment->mFromComposite = aEntries[i].mComposite;
>      segment->mToComposite = aEntries[j].mComposite;
> -

Nit: Subjective, I guess, but I thought the newline there looked appropriate given this whole |else| block we're in has some sections separated by newlines.

::: layout/style/ServoBindingTypes.h:16
(Diff revision 3)
>  #include "mozilla/UniquePtr.h"
> +#include "mozilla/ServoTypes.h"
> +#include "nsTArray.h"
>  
>  struct RawServoStyleSet;
> +struct RawServoAnimationValue;

Nit: one line up, to keep this sorted.

::: servo/ports/geckolib/glue.rs:46
(Diff revision 3)
>  use style::gecko_bindings::bindings::nsTArrayBorrowed_uintptr_t;
>  use style::gecko_bindings::structs;
>  use style::gecko_bindings::structs::{SheetParsingMode, nsIAtom, nsCSSPropertyID};
>  use style::gecko_bindings::structs::{ThreadSafePrincipalHolder, ThreadSafeURIHolder};
>  use style::gecko_bindings::structs::{nsRestyleHint, nsChangeHint};
> +use style::gecko_bindings::bindings::RawServoAnimationValueBorrowed;

This line probably needs to move up to avoid tidy errors.

::: servo/ports/geckolib/glue.rs:181
(Diff revision 3)
> +    use style::properties::animated_properties::AnimationValue;
> +    use style::properties::ComputedValues;
> +    use style::values::computed::Context;

The first two are already used at the top of the file.  Maybe the third one can go up the top of the file, too.

::: servo/ports/geckolib/glue.rs:202
(Diff revision 3)
> +        inherited_style: parent_style.unwrap_or(ComputedValues::initial_values()),
> +        style: (**style).clone(),
> +        font_metrics_provider: None,
> +    };
> +
> +    // FIXME figure out what to do about !important

They should be ignored:

https://drafts.csswg.org/css-animations-1/#keyframes

although I'm not sure from that text whether they get dropped at parse time, or should be ignored here.

::: servo/ports/geckolib/glue.rs:208
(Diff revision 3)
> +    let iter = guard.declarations
> +                    .iter()
> +                    .filter_map(|&(ref decl, _)| {
> +                        AnimationValue::from_declaration(decl, &context)
> +                    });
> +    // FIXME what if the servo iterator has a different length from the gecko one?

In |declarations|, have shorthands already been expanded out to longhands?  The place where we are calling Servo_AnimationValues_Populate is used to handle converting a single property declaration into longhand animation values.  It looks like AnimationValue::from_declaration just ignores shorthands that it encounters, so perhaps they have been expanded already, but otherwise we need to handle that.
Comment on attachment 8821434 [details]
Bug 1317208 - Stylo: Store servo computed values for animation properties;

https://reviewboard.mozilla.org/r/100728/#review101210

> In |declarations|, have shorthands already been expanded out to longhands?  The place where we are calling Servo_AnimationValues_Populate is used to handle converting a single property declaration into longhand animation values.  It looks like AnimationValue::from_declaration just ignores shorthands that it encounters, so perhaps they have been expanded already, but otherwise we need to handle that.

No, they haven't been expanded. It is the job of this loop to expand the shorthands. ComputeValues does the same. (which is why there's the whole stuff with building the `values` nsTArray and then draining it into a different array.
Comment on attachment 8821434 [details]
Bug 1317208 - Stylo: Store servo computed values for animation properties;

https://reviewboard.mozilla.org/r/100728/#review101210

> No, they haven't been expanded. It is the job of this loop to expand the shorthands. ComputeValues does the same. (which is why there's the whole stuff with building the `values` nsTArray and then draining it into a different array.

Isn't this loop operating on what should be the expanded set of longhands, and that expansion needs to be done by the stuff inside the |let iter = ...|?
Comment on attachment 8821434 [details]
Bug 1317208 - Stylo: Store servo computed values for animation properties;

https://reviewboard.mozilla.org/r/100728/#review101210

> Nit: Subjective, I guess, but I thought the newline there looked appropriate given this whole |else| block we're in has some sections separated by newlines.

Newline probably got removed during the rebase. Unintentional. :)

> Isn't this loop operating on what should be the expanded set of longhands, and that expansion needs to be done by the stuff inside the |let iter = ...|?

The expansion on the servo side happens before this loop. On the gecko side it happens during the loop (so `anim` should contain the list of longhands)

I'm not sure if this needs fixing, the fact that both servo and gecko values coexist is probably temporary.
Comment on attachment 8821434 [details]
Bug 1317208 - Stylo: Store servo computed values for animation properties;

https://reviewboard.mozilla.org/r/100728/#review101520

The Gecko side seems fine to me. I can't comment on the FFI stuff since it is completely unfamiliar to me and I've only left fairly superficial comments on the other servo changes.

The only thing is I'd like to understand the iterator sizing issue. It seems like the |gecko| array is zero-length at the point we go to iterate over it? Am I missing something?

::: dom/animation/KeyframeEffectReadOnly.h:22
(Diff revision 5)
>  #include "mozilla/KeyframeEffectParams.h"
> +
>  #include "mozilla/ServoBindingTypes.h" // RawServoDeclarationBlock and
>                                         // associated RefPtrTraits
>  #include "mozilla/StyleAnimationValue.h"

I don't think we need this extra blank link?

::: dom/animation/KeyframeUtils.cpp:275
(Diff revision 5)
>   */
>  struct KeyframeValueEntry
>  {
>    nsCSSPropertyID mProperty;
>    StyleAnimationValue mValue;
> +  RefPtr<RawServoAnimationValue> mServoValue;

(Presumably RawServoAnimationValue objects never hold references to Gecko objects so we don't need to worry about cycles here)

::: dom/animation/KeyframeUtils.cpp:612
(Diff revision 5)
> +  const ServoComputedValues* currentStyle;
> +  const ServoComputedValues* parentStyle;
> +
> +  if (styleBackend == StyleBackendType::Servo) {
> +    currentStyle = aStyleContext->StyleSource().AsServoComputedValues();
> +    parentStyle = aStyleContext->GetParent()->StyleSource().AsServoComputedValues();

This should check the result of GetParent() before dereferencing it.

I'm not sure when this will happen but if it is called GetXXX() in Gecko that means it can return null (and looking through the codebase we generally seem to handle the null case for GetParent). If we believe this will never be null then we should add an assertion to the start of this function to call it out as a precondition.

::: dom/animation/KeyframeUtils.cpp:640
(Diff revision 5)
>                CSSEnabledState::eForAllContent, aStyleContext,
>                *pair.mServoDeclarationBlock, values)) {
>            continue;
>          }
> +        Servo_AnimationValues_Populate(&values,
> +                                       &*pair.mServoDeclarationBlock,

I don't know what the bindings generate for the signature of Servo_AnimationValues_Populate (and I don't have time at the moment to try building this locally) but I'm curious to know why we need '&*' here.

::: dom/animation/KeyframeUtils.cpp:705
(Diff revision 5)
>        KeyframeValueEntry* entry = entries.AppendElement();
>        entry->mOffset = frame.mComputedOffset;
>        entry->mProperty = value.mProperty;
>        entry->mValue = value.mValue;
> +      entry->mServoValue = value.mServoValue;
> +

Nit: Not sure this extra line makes sense here (there's no real logical grouping going on here).

::: dom/animation/KeyframeUtils.cpp:1400
(Diff revision 5)
> +    segment->mServoFromValue = aEntries[i].mServoValue;
> +    segment->mServoToValue   = aEntries[j].mServoValue;

Nit: Move these up one line to be directly after mFromValue / mToValue

::: layout/style/ServoBindings.cpp:1059
(Diff revision 5)
>      MOZ_CRASH("stylo: shouldn't be calling " #name_ "in a non-stylo build");  \
>    }
>  #include "ServoBindingList.h"
>  #undef SERVO_BINDING_FUNC
>  #endif
> +

Nit: stray change here?

::: layout/style/ServoTypes.h
(Diff revision 5)
>  // descendants.
>  enum class TraversalRootBehavior {
>    Normal,
>    UnstyledChildrenOnly,
>  };
> -

Likewise here?

::: servo/components/style/build_gecko.rs:377
(Diff revision 5)
> +            "mozilla::StyleAnimationValue",
> +            "StyleAnimationValue", // pulls in a whole bunch of stuff we don't need in the bindings

I'm curious why we need both?

::: servo/components/style/properties/helpers/animated_properties.mako.rs:225
(Diff revision 5)
> +        match *decl {
> +            % for prop in data.longhands:
> +                % if prop.animatable:
> +                    PropertyDeclaration::${prop.camel_case}(ref val) => {
> +                        let computed = match *val {
> +                            DeclaredValue::WithVariables{..} => unimplemented!(),

Does this refer to variable declarations, or declarations that reference variables?

We should probably reference the issue for fixing this? Particularly if it's the latter.

::: servo/ports/geckolib/glue.rs:186
(Diff revision 5)
> +    // FIXME this might not be efficient since Populate
> +    // is called multiple times. We should precalculate the Context
> +    // and share it across Populate calls

Do we have a bug on file for doing this? It sounds like it might be worth doing but I'm not sure.

::: servo/ports/geckolib/glue.rs:208
(Diff revision 5)
> +    // FIXME what if the servo iterator has a different length from the gecko one?
> +    for (gecko, servo) in anim.iter_mut().zip(iter) {
> +        gecko.mServoValue.set_arc_leaky(Arc::new(servo))
> +    }

Can you please explain the issue here? Isn't |gecko| a zero-length array at this point?

::: servo/ports/geckolib/glue.rs
(Diff revision 5)
> -
>  #[no_mangle]

Stray change here
Comment on attachment 8821434 [details]
Bug 1317208 - Stylo: Store servo computed values for animation properties;

https://reviewboard.mozilla.org/r/100728/#review101520

> (Presumably RawServoAnimationValue objects never hold references to Gecko objects so we don't need to worry about cycles here)

(yes)

> I don't know what the bindings generate for the signature of Servo_AnimationValues_Populate (and I don't have time at the moment to try building this locally) but I'm curious to know why we need '&*' here.

My mistake. In Rust * is usually overloaded so `&*` is not a no-op and crops up at times so when I saw a type error I just put an `&` there forgetting that this was C++ code.

> I'm curious why we need both?

Just copyig what we do for `SupportsWeakPtr`. I was having namespace issues after a long day getting bindgen working again so I gave up and added both :|

> Does this refer to variable declarations, or declarations that reference variables?
> 
> We should probably reference the issue for fixing this? Particularly if it's the latter.

Declarations that reference variables.

bug 1326131

> Do we have a bug on file for doing this? It sounds like it might be worth doing but I'm not sure.

I want to wait until the rest of the animation stuff crystallizes before we figure this out.

> Can you please explain the issue here? Isn't |gecko| a zero-length array at this point?

It gets populated by ComputeValues

http://searchfox.org/mozilla-central/rev/51aa673e28802fe6ea89f4793721fc55334a6ac8/layout/style/StyleAnimationValue.cpp#3654

In the long run we probably want to get rid of ComputeValues? We'd need to move some of that checking of "enabled" properties back to KeyframeUtils though.
I'd like to use set_arc_leaky() in bug 1328787.
Blocks: 1328787
(In reply to Manish Goregaokar [:manishearth] from comment #15)
> In the long run we probably want to get rid of ComputeValues? We'd need to
> move some of that checking of "enabled" properties back to KeyframeUtils
> though.

Yes, we can drop it once we no longer need StyleAnimationValue objects. Currently, however, we need StyleAnimationValue objects for various things like distance calculations and for running animations on the compositor.
Comment on attachment 8821434 [details]
Bug 1317208 - Stylo: Store servo computed values for animation properties;

https://reviewboard.mozilla.org/r/100728/#review103152

r=me for the Gecko side with the following changes made.

I've only done a fairly brief review of the Servo side, particularly the FFI bindings since Cameron is much more familiar with that part.

::: dom/animation/KeyframeEffectReadOnly.h:24
(Diff revision 7)
>  #include "mozilla/Attributes.h"
>  #include "mozilla/ComputedTimingFunction.h"
>  #include "mozilla/EffectCompositor.h"
>  #include "mozilla/KeyframeEffectParams.h"
> -#include "mozilla/ServoBindingTypes.h" // RawServoDeclarationBlock and
> -                                       // associated RefPtrTraits
> +
> +#include "mozilla/ServoBindingTypes.h" // RawServoDeclarationBlock and associated RefPtrTraits

Please wrap to 80 chars for Gecko code.

(Sorry, perhaps my previous review comment was confusing--it had a typo too. I didn't mean to wrap the comment, but to drop the blank line before this include.)

::: dom/animation/KeyframeEffectReadOnly.h:126
(Diff revision 7)
>  {
>    float mFromKey, mToKey;
>    // NOTE: In the case that no keyframe for 0 or 1 offset is specified
>    // the unit of mFromValue or mToValue is eUnit_Null.
>    StyleAnimationValue mFromValue, mToValue;
> +  RefPtr<RawServoAnimationValue> mServoFromValue, mServoToValue;

I suppose in future we will drop mFromValue and mToValue and then we will need operator== to compare mServoFromValue and mServoToValue. Should we just add them to operator== now?

I guess pointer comparison won't be enough though so we'll need an extra FFI call to do deep comparison of the RawServoAnimationValue objects. So perhaps we should do that in a separate patch and just add a FIXME comment in this patch?

::: dom/animation/KeyframeUtils.cpp:607
(Diff revision 7)
> +  const ServoComputedValues* currentStyle;
> +  const ServoComputedValues* parentStyle;

Let's initialize these raw pointers to nullptr since we might not set them below.

::: servo/ports/geckolib/glue.rs:210
(Diff revision 7)
> +    // FIXME what if the servo iterator has a different length from the gecko one?
> +    for (gecko, servo) in anim.iter_mut().zip(iter) {
> +        gecko.mServoValue.set_arc_leaky(Arc::new(servo))
> +    }

Isn't this reasonably likely to be different? Can we at least add an assertion for this and make sure we do something sensible if it does crop up?
Attachment #8821434 - Flags: review?(bbirtles) → review+
Hi Manish, I'd like to test the interpolation part, but I have no idea now to get a valid RefPtr<RawServoAnimationValue> pairs. I think we cannot create a keyframe animation now, so how do you test these AnimationValues? Thanks.
Flags: needinfo?(manishearth)
(In reply to Boris Chiou [:boris] from comment #21)
> Hi Manish, I'd like to test the interpolation part, but I have no idea now
> to get a valid RefPtr<RawServoAnimationValue> pairs. I think we cannot
> create a keyframe animation now, so how do you test these AnimationValues?
> Thanks.

hiro suggested me use Element.animate(), and I can see the mServoFromValue/mServoToValue are set correctly in UpdateProperties(). Thanks.
Flags: needinfo?(manishearth)
Comment on attachment 8821434 [details]
Bug 1317208 - Stylo: Store servo computed values for animation properties;

https://reviewboard.mozilla.org/r/100728/#review103152

> Isn't this reasonably likely to be different? Can we at least add an assertion for this and make sure we do something sensible if it does crop up?

It will only mismatch when Servo and Gecko don't find the same longhands in a shorthand (or Servo doesn't think a shorthand subprop is animatable).

Since the ComputeValues call will be dropped in the long run, I didn't think this would be important. How hard would it be to move the distance calculation steps into servo? I can do that next if you want.
(In reply to Manish Goregaokar [:manishearth] from comment #23)
> Comment on attachment 8821434 [details]
> Bug 1317208 - Stylo: Store servo computed values for animation properties;
> 
> https://reviewboard.mozilla.org/r/100728/#review103152
> 
> > Isn't this reasonably likely to be different? Can we at least add an assertion for this and make sure we do something sensible if it does crop up?
> 
> It will only mismatch when Servo and Gecko don't find the same longhands in
> a shorthand (or Servo doesn't think a shorthand subprop is animatable).
> 
> Since the ComputeValues call will be dropped in the long run, I didn't think
> this would be important. How hard would it be to move the distance
> calculation steps into servo? I can do that next if you want.

It would be easy, I think, but I also think it's low priority since doing that would not be enough to allow us to drop the call to ComputeValues. We still need the StyleAnimationValue objects for animating on the compositor and changing that seems like a bit more work (we have to work out how to pass these opaque pointers to servo data structures to the compositor thread/process and then call into Servo from there).

If we can add an assertion for the mismatch case that would be enough for now, I think. If we start hitting that during tests then we'll have to look more closely.
Rebased and addressed.

Cameron, the Servo code changed substantially since the last reviewed by you, re-r?
Comment on attachment 8821434 [details]
Bug 1317208 - Stylo: Store servo computed values for animation properties;

https://reviewboard.mozilla.org/r/100728/#review104404

Yay!  Manish, thanks for updating!

::: servo/ports/geckolib/glue.rs:222
(Diff revision 9)
> +pub extern "C" fn Servo_AnimationValue_AddRef(sheet: RawServoAnimationValueBorrowed) -> () {
> +    unsafe { AnimationValue::addref(sheet) };
> +}

Nit: it's not 'sheet'.

::: servo/ports/geckolib/glue.rs:227
(Diff revision 9)
> +pub extern "C" fn Servo_AnimationValue_Release(sheet: RawServoAnimationValueBorrowed) -> () {
> +    unsafe { AnimationValue::release(sheet) };
> +}

Likewise.
Comment on attachment 8821434 [details]
Bug 1317208 - Stylo: Store servo computed values for animation properties;

https://reviewboard.mozilla.org/r/100728/#review104504

r=me with these fixed.

::: layout/style/ServoBindingTypes.h:12
(Diff revision 10)
>  #ifndef mozilla_ServoBindingTypes_h
>  #define mozilla_ServoBindingTypes_h
>  
>  #include "mozilla/RefPtr.h"
>  #include "mozilla/UniquePtr.h"
> +#include "mozilla/ServoTypes.h"

Nit: one line up to keep this sorted.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:260
(Diff revision 10)
> +                            DeclaredValue::Inherit => {
> +                                let inherit_struct = context.inherited_style
> +                                                            .get_${prop.style_struct.name_lower}();
> +                                inherit_struct.clone_${prop.ident}()
> +                            },
> +                            DeclaredValue::Unset => return None

Returning None for unset seems inconsistent with how we handle initial and inherit.  You should treat it like initial or inherit, depending on the property.
Attachment #8821434 - Flags: review?(cam) → review+
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0f84815541c1
Stylo: Store servo computed values for animation properties; r=birtles,heycam
https://hg.mozilla.org/mozilla-central/rev/0f84815541c1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: