Closed Bug 1340005 Opened 7 years ago Closed 7 years ago

stylo: Switch to Servo style backend for compositor animations

Categories

(Core :: CSS Parsing and Computation, defect, P4)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: hiro, Assigned: boris)

References

Details

Attachments

(9 files)

Bug 1340005 - Part 4: Retrieve transform list from AnimationValue.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1340005 - Part 1: Implement AnimationValue::Opacity.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1340005 - Part 2: Implement AnimationValue::Transform.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1340005 - Part 3: Use AnimationValue on the compositor thread.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1340005 - Part 5: Implement SampleValue for Servo backend.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1340005 - Part 6: Move AppendTransformFunction into AnimationValue struct.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1340005 - Part 7: Switch compositor animations to Servo backend for desktop.
59 bytes, text/x-review-board-request
birtles
: review+
hiro
: review+
nical
: review+
Details
Bug 1340005 - Part 8: Remove tolerance for omta tests.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Servo PR, #19042
41 bytes, text/x-github-pull-request
Details | Review 
Then, we can use servo's interpolation on the compositor and we can eliminate StyleAnimationValue on the compositor.

I think this is a low priority because we can run animations on the compositor without this.

This is actually M6 in stylo-animation etherpad [1]
[1] https://public.etherpad-mozilla.org/p/stylo-animation
Would you mind pointing me to the related code/file?
Flags: needinfo?(boris.chiou)
Hi Shing,

Sure. Our final target is to obsolete StyleAnimationValue in Compositor thread, so you can follow these steps:

1. We use layers::Animatable to pass the animation data from content threads to the compositor thread, so first you can check how do we get the transform data from layers::Animatable in AnimationHelper.cpp and its caller. 

2. Then you can add a FFI to convert the Animatable::TArrayOfTransformFunction into ServoAnimationValue. (We convert Animatable into StyleAnimationValue now because we still use StyleAnimationValue to do interpolation. [1])
(Note: we only support opacity and transform for OMTA, and opacity is just a f32 value, so all we need to do is convert transform data.)

3. Finally, reuse the interpolation FFI on ServoAnimationValue [2] to get the interpolated value.

[1] http://searchfox.org/mozilla-central/rev/9c1c7106eef137e3413fd867fc1ddfb1d3f6728c/gfx/layers/AnimationHelper.cpp#346
[2] http://searchfox.org/mozilla-central/rev/9c1c7106eef137e3413fd867fc1ddfb1d3f6728c/servo/ports/geckolib/glue.rs#236-249


Then we can use the interpolated value (type: RefPtr<RawServoAnimationValue>) in compositor thread. We can talk about this later. (I'm not sure whether we have to do that in this bug. Maybe you can file another bug for this).
Flags: needinfo?(boris.chiou)
Priority: -- → P3
Priority: P3 → P2
Blocks: 1361663
No longer blocks: 1334036
Component: DOM: Animation → CSS Parsing and Computation
Assignee: nobody → boris.chiou
A possible way to fix Bug 1361663 is that we should use ServoAnimationValue in both main thread and compositor thread, so it seems to me that we need to fix this first.
Status: NEW → ASSIGNED
Summary: stylo: Need a way to convert Animatable::TArrayOfTransformFunction (or Animatable::Tfloat) to servo's AnimationValue → stylo: Need a way to convert Animatable::TArrayOfTransformFunction and Animatable::Tfloat to servo's AnimationValue
We use a temporary way to fix Bug 1361663 now, so downgrade the priority of this bug. However, eventually, we still need to fix this to eliminate the difference between compositor thread and main thread (if without QuantumRender).
Priority: P2 → P3
And if we support mismatched transfrom list on Servo side, this might be easier because we need to convert a transfrom list (i.e. AnimationValue::Transform(...)) into a 3d matrix, so compositor can use this matrix.
See Also: → https://github.com/servo/servo/issues/13267
Priority: P3 → --
Not sure if it is worth to do this for now. If we want to remove Gecko style system, it seems we have to fix this ASAP.
OK, this is worth to do because it seems web-renderer doesn't replace SampleAnimations with rust code.
Comment on attachment 8910191 [details]
Bug 1340005 - Part 4: Retrieve transform list from AnimationValue.

https://reviewboard.mozilla.org/r/181656/#review187282

::: layout/painting/nsDisplayList.cpp:566
(Diff revision 1)
> +  animation->styleBackend() =
> +    aFrame->StyleContext()->StyleSource().IsServoComputedValues()
> +    ? (uint8_t)StyleBackendType::Servo
> +    : (uint8_t)StyleBackendType::Gecko;
>  

Having the backend type in each animation is really inefficient.  It will be better to have the backend in each layer. But still I think it's not so good.

One question, if we use servo's interpolation code on the compositor if the stylo pref is enabled and even if the document is not styled by stylo, what happens?  I think it should work.
Comment on attachment 8910191 [details]
Bug 1340005 - Part 4: Retrieve transform list from AnimationValue.

https://reviewboard.mozilla.org/r/181656/#review187282

> Having the backend type in each animation is really inefficient.  It will be better to have the backend in each layer. But still I think it's not so good.
> 
> One question, if we use servo's interpolation code on the compositor if the stylo pref is enabled and even if the document is not styled by stylo, what happens?  I think it should work.

> It will be better to have the backend in each layer. But still I think it's not so good.

I guess after dropping Gecko style system, we could drop this backend flag because at that memont, we always use stylo. So this is a short-term solution. I will update it according to your suggestion. Thanks.

> One question, if we use servo's interpolation code on the compositor if the stylo pref is enabled and even if the document is not styled by stylo, what happens?  I think it should work.

Me, too. If the document is not styled by stylo, its layers shouldn't styled by stylo, so the backend flag should be Gecko in all the layers in this document. If there is any exception (i.e. layer A is styled by Gecko and layer B is styled by Stylo), the visual result is still acceptable because I think their difference is ignoreable.
So, to be clear my suggestion is to check the pref value on the compositor instead of passing the backend to the compositor. :)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #14)
> So, to be clear my suggestion is to check the pref value on the compositor
> instead of passing the backend to the compositor. :)

Haha. OK. I will try that.
Summary: stylo: Need a way to convert Animatable::TArrayOfTransformFunction and Animatable::Tfloat to servo's AnimationValue → stylo: Switch to Servo style backend for compositor aniamtions
status-firefox54: affected → ---
Summary: stylo: Switch to Servo style backend for compositor aniamtions → stylo: Switch to Servo style backend for compositor animations
After discussing with Morris and Peter, I think we may pass the "isServo" flag per transaction on Gecko layer system. For Webrenderer, we will deprecate layers, so it would be better to store this flag into CompositorAnimationStorage (on the parent side).
Attachment #8915926 - Flags: review?(hikezoe)
I'd like to see the try result. If it looks good, we can review other patches.
Comment on attachment 8915926 [details]
Bug 1340005 - Part 8: Remove tolerance for omta tests.

https://reviewboard.mozilla.org/r/187184/#review192204
Attachment #8915926 - Flags: review?(hikezoe) → review+
Attachment #8910192 - Flags: review?(bbirtles)
Attachment #8910193 - Flags: review?(bbirtles)
Attachment #8911095 - Flags: review?(bbirtles)
Attachment #8910191 - Flags: review?(bbirtles)
Attachment #8911096 - Flags: review?(bbirtles)
Attachment #8915912 - Flags: review?(howareyou322)
Attachment #8915912 - Flags: review?(bbirtles)
Attachment #8915913 - Flags: review?(howareyou322)
Attachment #8915913 - Flags: review?(bbirtles)
Attachment #8910192 - Flags: review?(bbirtles)
Attachment #8910193 - Flags: review?(bbirtles)
Attachment #8911095 - Flags: review?(bbirtles)
Attachment #8910191 - Flags: review?(bbirtles)
Attachment #8911096 - Flags: review?(bbirtles)
Attachment #8915913 - Flags: review?(howareyou322)
Attachment #8915913 - Flags: review?(bbirtles)
Attachment #8915912 - Flags: review?(howareyou322)
Attachment #8915912 - Flags: review?(bbirtles)
Sorry for the spam. We decide to use Servo backend on the compositor directly now, so need to update the patches.
See Also: → 1293492
Blocks: 1409278
Comment on attachment 8915913 [details]
Bug 1340005 - Part 7: Switch compositor animations to Servo backend for desktop.

https://reviewboard.mozilla.org/r/187166/#review195324

::: commit-message-1dfb2:5
(Diff revision 3)
> +3. Servo backend uses "rotate3d" for rotate, rotatex, rotatey, and rotatez, so
> +   we should convert them into rotate3d.
> +4. Servo backend uses "skew" for skewx and skewy, so we should convert them
> +   into skew.

reference:
http://searchfox.org/mozilla-central/rev/a984558fa2bbde6492d3fb918496fc0b0835b2ce/servo/components/style/properties/gecko.mako.rs#3085-3094
Not sure if bug 1391145 affects that last patch.
(In reply to Brian Birtles (:birtles) from comment #48)
> Not sure if bug 1391145 affects that last patch.

Oh yes. bug 1391145 makes _Part 7_ simpler, i.e. I don't have to update CreateCSSValueList().
Comment on attachment 8910192 [details]
Bug 1340005 - Part 1: Implement AnimationValue::Opacity.

https://reviewboard.mozilla.org/r/181658/#review195658

::: servo/components/style/gecko/generated/bindings.rs:2607
(Diff revision 4)
>      pub fn Servo_AnimationValue_GetOpacity(value:
>                                                 RawServoAnimationValueBorrowed)
>       -> f32;
>  }
>  extern "C" {
> +    pub fn Servo_AnimationValue_FromOpacity(arg1: f32)

I was thinking this name was a little odd--it sounds like we're getting the opacity from somewhere rather than supplying it--and that Servo_AnimationValue_Opacity would be better, but given that we have AnimationValue::FromOpacity I guess this is fine unless you want to rename AnimationValue::FromOpacity to AnimationValue::Opacity too.

It's a bit odd because FromString makes sense--you're extracting information from the string--but in the case of opacity you're just setting the value directly.

What do you think about renaming Animation::FromOpacity to AnimationValue::Opacity and then renaming this accordingly?

::: servo/ports/geckolib/glue.rs:649
(Diff revision 4)
>  
>  #[no_mangle]
> +pub extern "C" fn Servo_AnimationValue_FromOpacity(
> +    opacity: f32
> +) -> RawServoAnimationValueStrong {
> +    debug_assert!(opacity >= 0.0f32 && opacity <= 1.0f32);

Is this needed?
Attachment #8910192 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #50)
> Comment on attachment 8910192 [details]
> Bug 1340005 - Part 1: Implement AnimationValue::FromOpacity.
> 
> https://reviewboard.mozilla.org/r/181658/#review195658
> 
> ::: servo/components/style/gecko/generated/bindings.rs:2607
> (Diff revision 4)
> >      pub fn Servo_AnimationValue_GetOpacity(value:
> >                                                 RawServoAnimationValueBorrowed)
> >       -> f32;
> >  }
> >  extern "C" {
> > +    pub fn Servo_AnimationValue_FromOpacity(arg1: f32)
> 
> I was thinking this name was a little odd--it sounds like we're getting the
> opacity from somewhere rather than supplying it--and that
> Servo_AnimationValue_Opacity would be better, but given that we have
> AnimationValue::FromOpacity I guess this is fine unless you want to rename
> AnimationValue::FromOpacity to AnimationValue::Opacity too.
> 
> It's a bit odd because FromString makes sense--you're extracting information
> from the string--but in the case of opacity you're just setting the value
> directly.
> 
> What do you think about renaming Animation::FromOpacity to
> AnimationValue::Opacity and then renaming this accordingly?

Don't feel obliged to do this. I'm not sure which I prefer, actually. So, it's up to you.
Comment on attachment 8910193 [details]
Bug 1340005 - Part 2: Implement AnimationValue::Transform.

https://reviewboard.mozilla.org/r/181664/#review195666

::: layout/style/StyleAnimationValue.h:643
(Diff revision 5)
> +  static AnimationValue FromTransform(StyleBackendType aBackendType,
> +                                      nsCSSValueSharedList* aList);

Can we make this a reference?

I know this is a contentious point but I had to check all the places this is used to see if they accept a null value. They don't and they will break badly if they are given one. It makes sense to me to just check for null at the call site.

If we can't make this a reference then at least we need to add an assertion it's not null in the implementation.

::: layout/style/StyleAnimationValue.cpp:5522
(Diff revision 5)
> +    case StyleBackendType::Servo: {
> +      result.mServo = Servo_AnimationValue_FromTransform(aList).Consume();
> +      break;
> +    }

Are the {} needed here?

::: servo/components/style/properties/gecko.mako.rs:3106
(Diff revision 5)
> -            return computed_value::T(None);
> +            return longhands::transform::computed_value::T(None);
>          }
>          let list = unsafe { (*self.gecko.mSpecifiedTransform.to_safe().get()).mHead.as_ref() };
> -        let result = list.map(|list| {
> -            list.into_iter()
> -                .map(|value| Self::clone_single_transform_function(value))
> +        Self::clone_transform_from_list(list)
> +    }
> +    pub fn clone_transform_from_list(list: Option< &structs::root::nsCSSValueList>)

Is the space after < needed here?

::: servo/components/style/properties/gecko.mako.rs:3108
(Diff revision 5)
> -        });
> -        computed_value::T(result)
> +        let result = match list {
> +            Some(list) => {
> +                let vec: Vec<_> = list
> +                    .into_iter()
> +                    .filter_map(|value| {
> +                        // Handle none transform.
> +                        if value.is_none() {
> +                            None
> +                        } else {
> +                            Some(Self::clone_single_transform_function(value))
> +                        }
> +                    })
> +                    .collect();
> +                if vec.len() > 0 { Some(vec) } else { None }
> +            },
> +            _ => None,
> +        };

Doesn't list.map(|l| ...) work here instead of the match?

::: servo/components/style/properties/gecko.mako.rs:3121
(Diff revision 5)
> +                        } else {
> +                            Some(Self::clone_single_transform_function(value))
> +                        }
> +                    })
> +                    .collect();
> +                if vec.len() > 0 { Some(vec) } else { None }

Replace len() > 0 with !is_empty() ?
Attachment #8910193 - Flags: review?(bbirtles) → review+
Comment on attachment 8911095 [details]
Bug 1340005 - Part 3: Use AnimationValue on the compositor thread.

https://reviewboard.mozilla.org/r/182590/#review195674
Attachment #8911095 - Flags: review?(bbirtles) → review+
Comment on attachment 8910191 [details]
Bug 1340005 - Part 4: Retrieve transform list from AnimationValue.

https://reviewboard.mozilla.org/r/181656/#review195678

::: layout/painting/nsDisplayList.h:5214
(Diff revision 4)
> -    FrameTransformProperties(nsCSSValueSharedList* aTransformList,
> +    FrameTransformProperties(RefPtr<const nsCSSValueSharedList>&&
> +                               aTransformList,
>                               const Point3D& aToTransformOrigin)
>        : mFrame(nullptr)
> -      , mTransformList(aTransformList)
> +      , mTransformList(mozilla::Move(aTransformList))
>        , mToTransformOrigin(aToTransformOrigin)
>      {}
>  
>      const nsIFrame* mFrame;
> -    RefPtr<nsCSSValueSharedList> mTransformList;
> +    const RefPtr<const nsCSSValueSharedList> mTransformList;

I suppose the use of Move() here is to avoid unnecessary refcount traffic.

I'm not sure the 'const RefPtr' is needed but I guess if the compiler doesn't complain (i.e. we're not trying to copy construct these objects anywhere) then it's ok.

::: layout/style/StyleAnimationValue.h:614
(Diff revision 4)
>    bool IsNull() const { return mGecko.IsNull() && !mServo; }
>  
>    float GetOpacity() const;
>  
> -  // Returns the scale for mGecko or mServo, which are calculated with
> +  // Return the transform list as a RefPtr.
> +  already_AddRefed<const nsCSSValueSharedList> GetTransformList() const;

(I was surprised that using const here works but I see that RefPtr includes special ConstRemovingRefPtrTraits machinery for this.)

I guess this makes sense because at least in the servo case, the value is a copy so mutating it is meaningless.
Attachment #8910191 - Flags: review?(bbirtles) → review+
Comment on attachment 8911096 [details]
Bug 1340005 - Part 5: Implement SampleValue for Servo backend.

https://reviewboard.mozilla.org/r/182592/#review195682

I need to look at this more thoroughly still but if you agree with the refactoring suggested in the comments below then it will be easier to review after that.

::: servo/ports/geckolib/glue.rs:412
(Diff revision 4)
>      // If compute_squared_distance() failed, this function will return negative value
>      // in order to check whether we support the specified paced animation values.
>      from_value.compute_squared_distance(to_value).map(|d| d.sqrt()).unwrap_or(-1.0)
>  }
>  
> +/// Compute the composited value.

Compute one of the endpoints for the interpolation interval, compositing it with the underlying value if needed.

::: servo/ports/geckolib/glue.rs:414
(Diff revision 4)
>      from_value.compute_squared_distance(to_value).map(|d| d.sqrt()).unwrap_or(-1.0)
>  }
>  
> +/// Compute the composited value.
> +/// A return value of None means, "Just use keyframe_value as-is."
> +fn compute_composited_value(

composite_endpoint?

::: servo/ports/geckolib/glue.rs:414
(Diff revision 4)
> +fn compute_composited_value(
> +    keyframe_value: Option<&RawOffsetArc<AnimationValue>>,
> +    underlying_value: Option<&AnimationValue>,
> +    last_value: Option<&AnimationValue>,
> +    composite: CompositeOperation,
> +    current_iteration: u64

This is a lot of arguments to be passing along, it feels like the function is doing two different things, and we have this slightly odd behavior where the value of last_value determines if we do iteration composite behavior or not.

I think this code would be a lot clearer if we had two separate methods: one for doing the the effect composition, and one for doing the iteration composition (like in the original). Perhaps call one composite_endpoint and another accumulate_endpoint?

That would seem like a better separation of concerns to me. What do you think?

::: servo/ports/geckolib/glue.rs:507
(Diff revision 4)
> +    let composited_from_value = compute_composited_value(keyframe_from_value,
> +                                                         underlying_value,
> +                                                         last_value,
> +                                                         segment.mFromComposite,
> +                                                         current_iteration);
> +    let composited_to_value = compute_composited_value(keyframe_to_value,
> +                                                       underlying_value,
> +                                                       last_value,
> +                                                       segment.mToComposite,
> +                                                       current_iteration);

(This might not be necessary given the refactoring suggested above, but rather than pass mostly the same list of arguments twice I wonder if adding a tiny lambda that wraps up the common arguments would make this code easier to read/maintain.)
Attachment #8911096 - Flags: review?(bbirtles)
Comment on attachment 8915912 [details]
Bug 1340005 - Part 6: Move AppendTransformFunction into AnimationValue struct.

https://reviewboard.mozilla.org/r/187164/#review195686
Attachment #8915912 - Flags: review?(bbirtles) → review+
Comment on attachment 8910192 [details]
Bug 1340005 - Part 1: Implement AnimationValue::Opacity.

https://reviewboard.mozilla.org/r/181658/#review195658

> I was thinking this name was a little odd--it sounds like we're getting the opacity from somewhere rather than supplying it--and that Servo_AnimationValue_Opacity would be better, but given that we have AnimationValue::FromOpacity I guess this is fine unless you want to rename AnimationValue::FromOpacity to AnimationValue::Opacity too.
> 
> It's a bit odd because FromString makes sense--you're extracting information from the string--but in the case of opacity you're just setting the value directly.
> 
> What do you think about renaming Animation::FromOpacity to AnimationValue::Opacity and then renaming this accordingly?

I don't any preference either, so using Opacity is ok to me. I will change FromOpacity and FromTransform into Opacity and Transform together.

> Is this needed?

This API is used only by the compositor, and if the main thread always sends a valid opacity, this asseertion is not needed. I can remove this because we clamp the opacity in the very earily stage (i.e compute the specific value into computed value.)
Comment on attachment 8910193 [details]
Bug 1340005 - Part 2: Implement AnimationValue::Transform.

https://reviewboard.mozilla.org/r/181664/#review195666

> Can we make this a reference?
> 
> I know this is a contentious point but I had to check all the places this is used to see if they accept a null value. They don't and they will break badly if they are given one. It makes sense to me to just check for null at the call site.
> 
> If we can't make this a reference then at least we need to add an assertion it's not null in the implementation.

I will try to use a reference and check for null at the call site. Thanks for the checking.

> Are the {} needed here?

Oh. we don't need it.

> Is the space after < needed here?

Yes, I could try again by the newer rust compilar. Last time I compiled this by rust 1.18 and got errors because rust compilar cannot parse this correctly.

> Doesn't list.map(|l| ...) work here instead of the match?

I tried that but it seems the closure in map() can only return a specific type, i.e. we only return a Vec<...> or an Option. So we might write this as ```list.map(|v| if ... { Some(v) } else { None } )```, and the ```result``` becomes ```Some(Some(Vec))``` or ```None```, too many Option wrappers, I think. Therefore, I use match here.

> Replace len() > 0 with !is_empty() ?

Oh yes, it is better. Thanks.
Comment on attachment 8911096 [details]
Bug 1340005 - Part 5: Implement SampleValue for Servo backend.

https://reviewboard.mozilla.org/r/182592/#review195682

> This is a lot of arguments to be passing along, it feels like the function is doing two different things, and we have this slightly odd behavior where the value of last_value determines if we do iteration composite behavior or not.
> 
> I think this code would be a lot clearer if we had two separate methods: one for doing the the effect composition, and one for doing the iteration composition (like in the original). Perhaps call one composite_endpoint and another accumulate_endpoint?
> 
> That would seem like a better separation of concerns to me. What do you think?

OK, let's add two separate methods, composite_endpoint and accumulate_endpoint, and use a closure to put them together, so we don't need to pass two many arguments. (Let me try to refactor this.) Thanks for the suggestion.
Comment on attachment 8911096 [details]
Bug 1340005 - Part 5: Implement SampleValue for Servo backend.

https://reviewboard.mozilla.org/r/182592/#review195716

::: gfx/layers/AnimationHelper.cpp:250
(Diff revision 4)
>      uint32_t segmentIndex = 0;
>      size_t segmentSize = animation.segments().Length();
>      AnimationSegment* segment = animation.segments().Elements();
>      while (segment->endPortion() < computedTiming.mProgress.Value() &&
>             segmentIndex < segmentSize - 1) {
>        ++segment;
>        ++segmentIndex;
>      }

(I wonder if this handles zero-length segments properly --- looks like it might not.)

::: layout/style/ServoBindingList.h:462
(Diff revision 4)
>                     nsCSSPropertyID property,
>                     RawGeckoAnimationPropertySegmentBorrowed animation_segment,
>                     RawGeckoAnimationPropertySegmentBorrowed last_segment,
>                     RawGeckoComputedTimingBorrowed computed_timing,
> -                   mozilla::dom::IterationCompositeOperation iteration_composite)
> +                   mozilla::dom::IterationCompositeOperation iter_composite)
> +SERVO_BINDING_FUNC(Servo_AnimationCompose_Compositor,

While you're working on this call this:

  Servo_ComposeAnimationSegment

This could do with a comment too.
Attachment #8915913 - Flags: review?(bbirtles)
According to the discussion today, we decide to add a gfxPref, which is enabled on desktop, and disabled on Android. If this is enabled, we use Servo style backend to compute the animation value and do interpolation/accumulation/composition by ServoAnimationValue; otherwise, we use Gecko StyleAnimationValue. i.e. desktop => Stylo; Android => Gecko.
Comment on attachment 8911096 [details]
Bug 1340005 - Part 5: Implement SampleValue for Servo backend.

https://reviewboard.mozilla.org/r/182592/#review196622

This was a big patch so I'm not entirely confident I've covered everything, but it seems reasonable.

::: layout/style/ServoBindingList.h:462
(Diff revision 6)
> +// Compose an animation segment with the underlying_value and last_value if
> +// needed. We use |iter_composite| and current_iteration to check if we need to
> +// do accumulation on iteration composition by |last_value|.

Calculate the result of interpolating given animation segment at the given progress and current iteration.
This includes combining the segment endpoints with the underlying value and/or last value depending the composite modes specified on the segment endpoints and the supplied iteration composite mode.
The caller is responsible for providing an underlying value and last value in all situations where there are needed.

::: servo/ports/geckolib/glue.rs:416
(Diff revision 6)
>      from_value.compute_squared_distance(to_value).map(|d| d.sqrt()).unwrap_or(-1.0)
>  }
>  
> +/// Compute one of the endpoints for the interpolation interval, compositing it with the
> +/// underlying value if needed.
> +/// An None |composited_value| means, "Just use keyframe_value as-is."

We should probably add a comment after the second sentence,

"It is the responsibility of the caller to ensure that underlying_value is provided when it will be used."

::: servo/ports/geckolib/glue.rs:417
(Diff revision 6)
> +fn composite_endpoint(
> +    keyframe_value: Option<&RawOffsetArc<AnimationValue>>,

Given that this is called composite_endpoint, I guess keyframe_value should be called endpoint or endpoint_value?

Or better still, perhaps we should just call this composite_animation_value? And call the parameter value or animation_value.

::: servo/ports/geckolib/glue.rs:426
(Diff revision 6)
> +                    debug_assert!(underlying_value.is_some(),
> +                                  "We should have an underlying value");
> +                    underlying_value.unwrap().animate(keyframe_value, Procedure::Add).ok()
> +                },
> +                CompositeOperation::Accumulate => {
> +                    debug_assert!(underlying_value.is_some(),
> +                                  "We should have an underlying value");
> +                    underlying_value
> +                        .unwrap()
> +                        .animate(keyframe_value, Procedure::Accumulate { count: 1 })
> +                        .ok()
> +                },
> +                _ => None,
> +            }
> +        },
> +        None => {
> +            debug_assert!(underlying_value.is_some(),
> +                          "We should have an underlying value");

I don't know that these debug_asserts make sense anymore. They made sense when we had need_underlying_value available since they would more clearly point to the root of the problem. Now that we don't have that, though, we should probably just rely on unwrap() to panic.

::: servo/ports/geckolib/glue.rs:449
(Diff revision 6)
> +            underlying_value.map(|v| v.clone())
> +        },
> +    }
> +}
> +
> +/// Accumulate the animation value on one of the endpoints.

"Accumulate one of the endpoints of the animation interval."?

::: servo/ports/geckolib/glue.rs:476
(Diff revision 6)
> +/// Compose the animation segment. We composite it with the underlying_value and last_value if
> +/// needed.

Again, we need to call out that the caller is responsible for supplying the underlying_value and last_value when needed.

(Originally the check for whether or not we needed an underlying value and the code that used it were very close to each other so it was easy to keep them in sync. Now, however, we are separating this out so this dependency is stretched across several functions. Normally we should make the API more robust in that case but at very least we should add appropriate documentation identifying that dependency.)

::: servo/ports/geckolib/glue.rs:522
(Diff revision 6)
> +        let apply_iteration_composite = |keyframe_value: Option<&RawOffsetArc<AnimationValue>>,
> +                                         composited_value: Option<AnimationValue>|
> +                                         -> Option<AnimationValue> {
> +            accumulate_endpoint(keyframe_value, composited_value, last_value, current_iteration)
> +        };
> +
> +        composited_from_value = apply_iteration_composite(keyframe_from_value,
> +                                                          composited_from_value);
> +        composited_to_value = apply_iteration_composite(keyframe_to_value, composited_to_value);

I understand apply_iteration_composite it a convenience to avoid passing 'last_value' and 'current_iteration' twice, but I think in this case it probably makes this harder to read.

::: servo/ports/geckolib/glue.rs:627
(Diff revision 6)
> -    if segment.mToKey == segment.mFromKey {
> -        if progress < 0. {
> +    let position = if segment.mFromKey < segment.mToKey {
> +        unsafe { Gecko_GetPositionInSegment(segment, progress, computed_timing.mBeforeFlag) }
> -            value_map.insert(property, from_value.clone());
> -        } else {
> +    } else {
> -            value_map.insert(property, to_value.clone());
> -        }
> -        return;
> +        // Note: compose_animation_segment doesn't use this value
> +        // if segment.mFromKey == segment.mToKey, so assigning |progress| directly is fine.
> +        progress

Given the comment here, wouldn't it make more sense to check for segment.mFromKey != segment.mToKey here?
Attachment #8911096 - Flags: review?(bbirtles) → review+
Comment on attachment 8915913 [details]
Bug 1340005 - Part 7: Switch compositor animations to Servo backend for desktop.

https://reviewboard.mozilla.org/r/187166/#review195720

Looks fine but I'd like Hiro to have a look at the computed_operation_arm part since he knows that code better than I do.

And someone from gfx team should check the gfxPrefs part.

::: gfx/thebes/gfxPrefs.h:434
(Diff revision 5)
> +#if defined(ANDROID)
> +  DECL_GFX_PREF(Once, "gfx.compositor.servo.enabled",          CompositorServoAnimation, bool, false);
> +#else
> +  DECL_GFX_PREF(Once, "gfx.compositor.servo.enabled",          CompositorServoAnimation, bool, true);
> +#endif

Please get someone from gfx to review this. I'm not familiar with gfxPrefs.

It probably makes sense to have the pref and name match though, I guess.
Attachment #8915913 - Flags: review?(bbirtles) → review+
Attachment #8915913 - Flags: review?(nical.bugzilla)
Attachment #8915913 - Flags: review?(hikezoe)
Hi hiro, could you please take a look at the change in gecko.mako.rs?

Hi nical, could you please take a look at the change in gfxPrefs.h?

Thanks.
Comment on attachment 8915913 [details]
Bug 1340005 - Part 7: Switch compositor animations to Servo backend for desktop.

https://reviewboard.mozilla.org/r/187166/#review196636
Attachment #8915913 - Flags: review?(hikezoe) → review+
Comment on attachment 8911096 [details]
Bug 1340005 - Part 5: Implement SampleValue for Servo backend.

https://reviewboard.mozilla.org/r/182592/#review196622

> Given that this is called composite_endpoint, I guess keyframe_value should be called endpoint or endpoint_value?
> 
> Or better still, perhaps we should just call this composite_animation_value? And call the parameter value or animation_value.

OK, I will rename this as ```composite_animation_value()``` and call the parameter value ```animation_value```

> I don't know that these debug_asserts make sense anymore. They made sense when we had need_underlying_value available since they would more clearly point to the root of the problem. Now that we don't have that, though, we should probably just rely on unwrap() to panic.

ok, I will remove this debug_assert(). Thanks.

> I understand apply_iteration_composite it a convenience to avoid passing 'last_value' and 'current_iteration' twice, but I think in this case it probably makes this harder to read.

ok, I will remove the clousure, and just pass ```last_value``` and ```current_iteration``` twice for readability.

> Given the comment here, wouldn't it make more sense to check for segment.mFromKey != segment.mToKey here?

OK. I will update the if condition.
Comment on attachment 8915913 [details]
Bug 1340005 - Part 7: Switch compositor animations to Servo backend for desktop.

https://reviewboard.mozilla.org/r/187166/#review195720

> Please get someone from gfx to review this. I'm not familiar with gfxPrefs.
> 
> It probably makes sense to have the pref and name match though, I guess.

OK. I will update the pref function name as ```CompositorServoEnabled```
Comment on attachment 8911096 [details]
Bug 1340005 - Part 5: Implement SampleValue for Servo backend.

https://reviewboard.mozilla.org/r/182592/#review196622

> "Accumulate one of the endpoints of the animation interval."?

Again, I'd like to update the function name as ```accumulate_animation_value``` and the first argument as ```animation_value```. So the comment becomes: "Accumulate one of the endpoints (i.e. animation_value) of the animation interval."
(In reply to Boris Chiou [:boris] (away 24 Oct – 12 Nov) from comment #78)
> Comment on attachment 8911096 [details]
> Bug 1340005 - Part 5: Implement SampleValue for Servo backend.
> 
> https://reviewboard.mozilla.org/r/182592/#review196622
> 
> > Given that this is called composite_endpoint, I guess keyframe_value should be called endpoint or endpoint_value?
> > 
> > Or better still, perhaps we should just call this composite_animation_value? And call the parameter value or animation_value.
> 
> OK, I will rename this as ```composite_animation_value()``` and call the
> parameter value ```animation_value```

Oh, you can ignore that comment. I thought I deleted it but I think MozReview might have tricked me.
(In reply to Brian Birtles (:birtles) from comment #81)
> (In reply to Boris Chiou [:boris] (away 24 Oct – 12 Nov) from comment #78)
> > Comment on attachment 8911096 [details]
> > Bug 1340005 - Part 5: Implement SampleValue for Servo backend.
> > 
> > https://reviewboard.mozilla.org/r/182592/#review196622
> > 
> > > Given that this is called composite_endpoint, I guess keyframe_value should be called endpoint or endpoint_value?
> > > 
> > > Or better still, perhaps we should just call this composite_animation_value? And call the parameter value or animation_value.
> > 
> > OK, I will rename this as ```composite_animation_value()``` and call the
> > parameter value ```animation_value```
> 
> Oh, you can ignore that comment. I thought I deleted it but I think
> MozReview might have tricked me.

OK. I keep using composite_endpoint and accumulate_endpoint. Thanks.
Comment on attachment 8911096 [details]
Bug 1340005 - Part 5: Implement SampleValue for Servo backend.

https://reviewboard.mozilla.org/r/182592/#review196622

> Again, we need to call out that the caller is responsible for supplying the underlying_value and last_value when needed.
> 
> (Originally the check for whether or not we needed an underlying value and the code that used it were very close to each other so it was easy to keep them in sync. Now, however, we are separating this out so this dependency is stretched across several functions. Normally we should make the API more robust in that case but at very least we should add appropriate documentation identifying that dependency.)

I hope we can make composite_endpoint and accumulate_endpoint as the methods of AnimationValue in the future, so we could reuse the code on both Servo browser and Firefox browser. For now, just move them into separate local functions.
Comment on attachment 8915913 [details]
Bug 1340005 - Part 7: Switch compositor animations to Servo backend for desktop.

https://reviewboard.mozilla.org/r/187166/#review196670

::: gfx/layers/AnimationHelper.cpp:280
(Diff revision 6)
>        static_cast<dom::CompositeOperation>(segment->startComposite());
>      animSegment.mToComposite =
>        static_cast<dom::CompositeOperation>(segment->endComposite());
>  
>      // interpolate the property
> -    bool isServo = animSegment.mFromValue.mServo ||
> +    if (gfxPrefs::CompositorServoEnabled()) {

I think that prefs are not accessible from the GPU process. You may need to use [gfxVars](https://searchfox.org/mozilla-central/rev/72fe012899a1b27d34838ab463ad1ae5b116d76b/gfx/config/gfxVars.h#28) instead.

::: gfx/thebes/gfxPrefs.h:435
(Diff revision 6)
>  #else
>    DECL_GFX_PREF(Once, "gfx.blocklist.all",                     BlocklistAll, int32_t, 0);
>  #endif
>    DECL_GFX_PREF(Live, "gfx.compositor.clearstate",             CompositorClearState, bool, false);
> +#if defined(ANDROID)
> +  DECL_GFX_PREF(Once, "gfx.compositor.servo.enabled",          CompositorServoEnabled, bool, false);

nit: could we use the term stylo instead of servo here? with webrender being integrated in the compositor, "servo" is very ambiguous.
Comment on attachment 8915913 [details]
Bug 1340005 - Part 7: Switch compositor animations to Servo backend for desktop.

https://reviewboard.mozilla.org/r/187166/#review196670

> nit: could we use the term stylo instead of servo here? with webrender being integrated in the compositor, "servo" is very ambiguous.

Sure. I will move this into gfxVars, and use "stylo", e.g. ```_(UseStyloOnCompositor, bool, false)```
Hi, Nicolas,

I updated part 7 and use gfxVars. Could you please take a look at it again? Thanks.
Comment on attachment 8915913 [details]
Bug 1340005 - Part 7: Switch compositor animations to Servo backend for desktop.

https://reviewboard.mozilla.org/r/187166/#review197136

Looks good, although I don't see any code to change the gfxVar's value. If you don't need to do that you might as well just do ```if (STYLO_DEFAULT) { ... }``` instead of using gfxVars.
If you do, then you need some glue code somewhere in the parent process that reads from a pref and calls ```gfxVars::SetUseStyloOnCompositor```. We typically have code for that kind of thing in gfxPlatform.
Comment on attachment 8915913 [details]
Bug 1340005 - Part 7: Switch compositor animations to Servo backend for desktop.

https://reviewboard.mozilla.org/r/187166/#review197136

> If you don't need to do that you might as well just do if (STYLO_DEFAULT) { ... } instead of using gfxVars.

Actually, we don't need to update the variable. All we need is a flag which can tell us: are we on Android platform? Because we shouldn't use stylo on the compositor on Android. i.e. Only run stylo on desktops. Therefore, it looks like using the defined flag is ok.
Hi, Nicolas,

I updated part 7 by using the #define flag directly. Could you please take a look at it again? Thanks.
Comment on attachment 8915913 [details]
Bug 1340005 - Part 7: Switch compositor animations to Servo backend for desktop.

https://reviewboard.mozilla.org/r/187166/#review198988
Attachment #8915913 - Flags: review?(nical.bugzilla) → review+
Attached file Servo PR, #19042 — 

    
    

    
  
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/86ef7d8d75e4
Part 1: Implement AnimationValue::Opacity. r=birtles
https://hg.mozilla.org/integration/autoland/rev/143169e5a42d
Part 2: Implement AnimationValue::Transform. r=birtles
https://hg.mozilla.org/integration/autoland/rev/b43c82ffe651
Part 3: Use AnimationValue on the compositor thread. r=birtles
https://hg.mozilla.org/integration/autoland/rev/6ba6dfa9f229
Part 4: Retrieve transform list from AnimationValue. r=birtles
https://hg.mozilla.org/integration/autoland/rev/ebe3f98312ec
Part 5: Implement SampleValue for Servo backend. r=birtles
https://hg.mozilla.org/integration/autoland/rev/80316da01761
Part 6: Move AppendTransformFunction into AnimationValue struct. r=birtles
https://hg.mozilla.org/integration/autoland/rev/3b54675c1eee
Part 7: Switch compositor animations to Servo backend for desktop. r=birtles,hiro,nical
https://hg.mozilla.org/integration/autoland/rev/b0de4b584e6d
Part 8: Remove tolerance for omta tests. r=hiro
Depends on: 1418806

    
    

    
  
Comment on attachment 8915913 [details]
Bug 1340005 - Part 7: Switch compositor animations to Servo backend for desktop.

https://reviewboard.mozilla.org/r/187166/#review259998

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: