Closed Bug 1335942 Opened 7 years ago Closed 7 years ago

stylo: Need a way to convert RawServoAnimationValue to layers::Animatable

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Version:
45 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: hiro, Assigned: boris)

References

Details

Attachments

(9 files)

Half-baked patch
8.11 KB, patch
Details | Diff | Splinter Review
nsStyleTransformMatrix::GetScaleValue
2.89 KB, patch
Details | Diff | Splinter Review
Bug 1335942 - Part 1: Introduce mozilla::AnimationValue.
59 bytes, text/x-review-board-request
hiro
: review+
manishearth
: review+
Details
Bug 1335942 - Part 2: Use mozilla::AnimationValue in AnimationPropertySegment.
59 bytes, text/x-review-board-request
hiro
: review+
manishearth
: review+
Details
Bug 1335942 - Part 3: Use AnimationValue as the argument in layers::SetAnimatable.
59 bytes, text/x-review-board-request
hiro
: review+
manishearth
: review+
Details
Bug 1335942 - Part 4: Support transform in SetAnimatable.
59 bytes, text/x-review-board-request
hiro
: review+
manishearth
: review+
Details
Bug 1335942 - Part 5: Add AnimationValue::GetScaleValue().
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1335942 - Part 6: Implement GetScaleValue for RawServoAnimationValue.
59 bytes, text/x-review-board-request
hiro
: review+
manishearth
: review+
Details
Servo PR, #15442
41 bytes, text/x-github-pull-request
Details | Review 
This is a prerequisite work for bug 1335938. Also this will eliminate StyleAnimationValue on the main-thread.
Also this is needed for fixing https://github.com/servo/servo/issues/13267 in gecko side, I think.
We also need GetScaleValue() for ServoAnimationValue to eliminate StyleAnimationValue on the main-thread.

https://hg.mozilla.org/mozilla-central/file/f985243bb630/layout/style/StyleAnimationValue.cpp#l4816
Summary: Need a way to convert RawServoAnimationValue to layers::Animatable → stylo: Need a way to convert RawServoAnimationValue to layers::Animatable
Attached patch Half-baked patch — — Splinter Review 
Boris, this is the half-baked patch.

This still needs to fill transform list value into nsCSSValueSharedList.  I think we need to re-use most of process in set_transform() in gecko.mako.rs.

Actually I wanted to make Servo_AnimationValue_GetTransformList returning already_AddRefed<nsCSSValueSharedList>, but I couldn't find any way for that.

Please feel free to take this.
Thanks!
(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> Created attachment 8833150 [details] [diff] [review]
> Half-baked patch
> 
> Boris, this is the half-baked patch.
> 
> This still needs to fill transform list value into nsCSSValueSharedList.  I
> think we need to re-use most of process in set_transform() in gecko.mako.rs.
> 
> Actually I wanted to make Servo_AnimationValue_GetTransformList returning
> already_AddRefed<nsCSSValueSharedList>, but I couldn't find any way for that.
> 
> Please feel free to take this.
> Thanks!

Great! Thanks a lot, I take this.
Assignee: nobody → boris.chiou
Blocks: 1334036
Boris, I found another patch that can be used for what I commented in comment #1.
You can use this nsStyleTransformMatrix::GetScaleValue() in UpdateMinMaxScale() in nsLayoutUtils.cpp and ContainsAnimatedScale() in ActiveLayerTracker.cpp.

The latter will be moved into KeyframeEffectReadOnly.cpp in bug 1333846.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> Created attachment 8833194 [details] [diff] [review]
> nsStyleTransformMatrix::GetScaleValue
> 
> Boris, I found another patch that can be used for what I commented in
> comment #1.
> You can use this nsStyleTransformMatrix::GetScaleValue() in
> UpdateMinMaxScale() in nsLayoutUtils.cpp and ContainsAnimatedScale() in
> ActiveLayerTracker.cpp.
> 
> The latter will be moved into KeyframeEffectReadOnly.cpp in bug 1333846.

Thanks! These patches are really helpful to me to understand the basic flow. BTW, I'm tired of the pair of |StyleAnimationValue & RawServoAnimationValue|, so I'd like to introduce a struct which wrap this pair as |AnimationValue|
e.g.
struct AnimationValue
{
  StyleAnimationValue mGecko;
  RefPtr<RawServoAnimationValue> mServo;  // or maybe we also need another one for Non-owning AnimationValue,
                                          // NonOwingAnimationValue, which doesn't use RefPtr, because Keyframes hold it.
};

It's possible both mGecko and mServo are valid, so keep both of them. And rewrite PropertyStyleAnimationValuePair as
struct PropertyStyleAnimationValuePair  // or rename to PropertyAnimationValuePair
{
  nsCSSPropertyID mProperty;
  AnimationValue mValue;
};

So we can pass this struct directly to reduce as many as duplicated code and redundant function arguments.
Status: NEW → ASSIGNED
Depends on: 1332657
Comment on attachment 8833925 [details]
Bug 1335942 - Part 1: Introduce mozilla::AnimationValue.

https://reviewboard.mozilla.org/r/110046/#review111036

> Currently, we have StyleAnimationValue for Gecko and
RawServoAnimationValue for Servo, and use this struct to wrap them. One
day, we will remove StyleAnimationValue, so we could just replace
mozilla::AnimationValue with RawServoAnimationValue without doing much
refactoring.

I am not convinced that this change will reduce the refactoring efforct.  I am guessing the cost is almost the same, because we just will need to drop StyleAnimationValue codes and related branches, and replace 'mValue.mServo' with 'mValue', no? In the short term we can just drop ComputeValuesFromStyleContext() in StyleAnimationValue::ComputeValues().

That's said I am OK with this change.

::: servo/components/style/gecko_bindings/structs_debug.rs:5139
(Diff revision 1)
>          #[repr(C)]
>          #[derive(Debug, Copy, Clone)]
>          pub struct ContainerLayerParameters([u8; 0]);
>          #[repr(C)]
>          #[derive(Debug)]
> +        pub struct AnimationValue {

I am wondering we don't need to add AnimationValue into whiltelist in build_gecko.rs.
Attachment #8833925 - Flags: review?(hikezoe) → review+
Comment on attachment 8833926 [details]
Bug 1335942 - Part 2: Use mozilla::AnimationValue in AnimationPropertySegment.

https://reviewboard.mozilla.org/r/110048/#review111040
Attachment #8833926 - Flags: review?(hikezoe) → review+
Comment on attachment 8833927 [details]
Bug 1335942 - Part 3: Use AnimationValue as the argument in layers::SetAnimatable.

https://reviewboard.mozilla.org/r/110050/#review111034

::: layout/painting/nsDisplayList.cpp:440
(Diff revision 1)
>    StyleAnimationValue baseValue =
>      EffectCompositor::GetBaseStyle(aProperty, aFrame);
>    MOZ_ASSERT(!baseValue.IsNull(),
>               "The base value should be already there");
>  
> -  SetAnimatable(aProperty, baseValue, aFrame, aRefBox, aBaseStyle);
> +  // FIXME: Bug 1329878: Implement accumulate and addition on Servo

This should be bug 1311257 instead.
Attachment #8833927 - Flags: review?(hikezoe) → review+
Comment on attachment 8833928 [details]
Bug 1335942 - Part 4: Support transform in SetAnimatable.

https://reviewboard.mozilla.org/r/110052/#review111048

Gecko part looks good to me.
Attachment #8833928 - Flags: review?(hikezoe) → review+
Comment on attachment 8833929 [details]
Bug 1335942 - Part 5: Add AnimationValue::GetScaleValue().

https://reviewboard.mozilla.org/r/110054/#review111050

::: layout/style/nsStyleTransformMatrix.h:237
(Diff revision 1)
>    mozilla::gfx::Matrix CSSValueArrayTo2DMatrix(nsCSSValue::Array* aArray);
>    mozilla::gfx::Matrix4x4 CSSValueArrayTo3DMatrix(nsCSSValue::Array* aArray);
> +
> +  /// @return the scale for this value, calculated with reference to @aForFrame.
> +  gfxSize GetScaleValue(const mozilla::AnimationValue& aValue,
> +                        const nsIFrame *aForFrame);

Nit: nsIFrame* aForFrame.

::: layout/style/nsStyleTransformMatrix.cpp:1262
(Diff revision 1)
> +gfxSize
> +GetScaleValue(const AnimationValue& aValue,
> +              const nsIFrame *aForFrame)

I think this GetScaleValue(const AnimationValue& aValue, const nsIFrame* aForFrame) should be a static function in StyleAnimationValue.h because AnimationValue is defined there.  And nsStyleTransformMatrix should have GetScale(const nsCSSValueSharedList*, const nsIFrame*).

Hmm, I wrote such function in that patch, is there any strong reason StyleAnimationValue does not have such function?

::: layout/style/nsStyleTransformMatrix.cpp:1264
(Diff revision 1)
>    return m;
>  }
>  
> +gfxSize
> +GetScaleValue(const AnimationValue& aValue,
> +              const nsIFrame *aForFrame)

Nit: nsIFrame* aForFrame.
Attachment #8833929 - Flags: review?(hikezoe) → review+
Comment on attachment 8833930 [details]
Bug 1335942 - Part 6: Implement GetScaleValue for RawServoAnimationValue.

https://reviewboard.mozilla.org/r/110056/#review111052
Attachment #8833930 - Flags: review?(hikezoe) → review+
Boris, please file a bug for using RawServoAnimationValue for KeyframeEffectReadOnly::GetProperties(). Once it's fixed we don't need to store StyleAnimationValue for stylo at [1], IIUC. 

[1] https://hg.mozilla.org/mozilla-central/file/1cc159c7a044/layout/style/StyleAnimationValue.cpp#l3687
Comment on attachment 8833925 [details]
Bug 1335942 - Part 1: Introduce mozilla::AnimationValue.

https://reviewboard.mozilla.org/r/110046/#review111214

::: servo/components/style/gecko_bindings/structs_debug.rs:5139
(Diff revision 1)
>          #[repr(C)]
>          #[derive(Debug, Copy, Clone)]
>          pub struct ContainerLayerParameters([u8; 0]);
>          #[repr(C)]
>          #[derive(Debug)]
> +        pub struct AnimationValue {

Because PropertyStyleAnimationValuePair depends on it. In general bindgen will generate all dependencies of whitelisted types, except for blacklisted ones (which it usually replaces with an opaque array or something)
Attachment #8833925 - Flags: review?(manishearth) → review+
Comment on attachment 8833926 [details]
Bug 1335942 - Part 2: Use mozilla::AnimationValue in AnimationPropertySegment.

https://reviewboard.mozilla.org/r/110048/#review111216

::: layout/style/StyleAnimationValue.h:591
(Diff revision 1)
>  {
>    StyleAnimationValue mGecko;
>    RefPtr<RawServoAnimationValue> mServo;
> +
> +  bool operator==(const AnimationValue& aOther) const {
> +    // FIXME: add a deep == impl for RawServoAnimationValue

file a bug for this?
Attachment #8833926 - Flags: review+
Comment on attachment 8833926 [details]
Bug 1335942 - Part 2: Use mozilla::AnimationValue in AnimationPropertySegment.

https://reviewboard.mozilla.org/r/110048/#review111220
Comment on attachment 8833927 [details]
Bug 1335942 - Part 3: Use AnimationValue as the argument in layers::SetAnimatable.

https://reviewboard.mozilla.org/r/110050/#review111222

::: layout/painting/nsDisplayList.cpp:404
(Diff revision 1)
>      return;
>    }
>  
>    switch (aProperty) {
>      case eCSSProperty_opacity:
> -      aAnimatable = aAnimationValue.GetFloatValue();
> +      aAnimatable = aAnimationValue.mServo

nit: Could this be a method on AnimationValue?
Attachment #8833927 - Flags: review?(manishearth) → review+
Comment on attachment 8833928 [details]
Bug 1335942 - Part 4: Support transform in SetAnimatable.

https://reviewboard.mozilla.org/r/110052/#review111240

::: layout/painting/nsDisplayList.cpp:410
(Diff revision 1)
>                        ? Servo_AnimationValues_GetOpacity(aAnimationValue.mServo)
>                        : aAnimationValue.mGecko.GetFloatValue();
>        break;
>      case eCSSProperty_transform: {
>        aAnimatable = InfallibleTArray<TransformFunction>();
> +      auto addTransform = [] (const nsCSSValueSharedList* aList,

nit: Since this doesn't have a capture clause, could it just be a local static function?

::: servo/components/style/properties/gecko.mako.rs:1344
(Diff revision 1)
> -            unsafe {
> -                self.gecko.mSpecifiedTransform.clear();
> -            }
> -            return;
> -        };
>  

Should we unconditionally be calling `output.clear()` here?
Attachment #8833928 - Flags: review?(manishearth) → review+
Comment on attachment 8833930 [details]
Bug 1335942 - Part 6: Implement GetScaleValue for RawServoAnimationValue.

https://reviewboard.mozilla.org/r/110056/#review111266

::: layout/style/nsStyleTransformMatrix.cpp:1269
(Diff revision 1)
> -  MOZ_ASSERT(aValue.mGecko.GetUnit() == StyleAnimationValue::eUnit_Transform);
> -
> -  nsCSSValueSharedList* list = aValue.mGecko.GetCSSValueSharedListValue();
> -  MOZ_ASSERT(list->mHead);
>  
> +  auto getTransformScale = [] (const nsCSSValueSharedList* aList,

Again, can this be a static function above instead of a closure?
Attachment #8833930 - Flags: review?(manishearth) → review+
Comment on attachment 8833925 [details]
Bug 1335942 - Part 1: Introduce mozilla::AnimationValue.

https://reviewboard.mozilla.org/r/110046/#review111036

> I am not convinced that this change will reduce the refactoring efforct.  I am guessing the cost is almost the same, because we just will need to drop StyleAnimationValue codes and related branches, and replace 'mValue.mServo' with 'mValue', no? In the short term we can just drop ComputeValuesFromStyleContext() in StyleAnimationValue::ComputeValues().
> That's said I am OK with this change.

OK, at least it is benefit for our current code base (which holds both kinds of animation values). I will remove the comment to avoid confusing people. Thanks.
Blocks: 1337229
Comment on attachment 8833926 [details]
Bug 1335942 - Part 2: Use mozilla::AnimationValue in AnimationPropertySegment.

https://reviewboard.mozilla.org/r/110048/#review111216

> file a bug for this?

OK, filed Bug 1337229.
Comment on attachment 8833927 [details]
Bug 1335942 - Part 3: Use AnimationValue as the argument in layers::SetAnimatable.

https://reviewboard.mozilla.org/r/110050/#review111222

> nit: Could this be a method on AnimationValue?

Yes. It's better to put it on AnimationValue. Thanks.
Comment on attachment 8833927 [details]
Bug 1335942 - Part 3: Use AnimationValue as the argument in layers::SetAnimatable.

https://reviewboard.mozilla.org/r/110050/#review111034

> This should be bug 1311257 instead.

Oops. Looks like I misread the comment for GetBaseStyle(). Thanks.
Comment on attachment 8833928 [details]
Bug 1335942 - Part 4: Support transform in SetAnimatable.

https://reviewboard.mozilla.org/r/110052/#review111240

> nit: Since this doesn't have a capture clause, could it just be a local static function?

Sure, I will move it to be a local static funciton.

> Should we unconditionally be calling `output.clear()` here?

Yes, we should clear it first to prevent any possible leak.
Comment on attachment 8833929 [details]
Bug 1335942 - Part 5: Add AnimationValue::GetScaleValue().

https://reviewboard.mozilla.org/r/110054/#review111050

> I think this GetScaleValue(const AnimationValue& aValue, const nsIFrame* aForFrame) should be a static function in StyleAnimationValue.h because AnimationValue is defined there.  And nsStyleTransformMatrix should have GetScale(const nsCSSValueSharedList*, const nsIFrame*).
> 
> Hmm, I wrote such function in that patch, is there any strong reason StyleAnimationValue does not have such function?

My reason to move it out of StyleAnimationValue is that we can use GetScaleValue() not only for StyleAnimationValue, but also for RawServoAnimationValue. After checking comment 22, I change my mind: We can add a new method, GetScaleValue(), on AnimationValue, which calls the StyleAnimationValue::GetScaleValue() for Gecko or calls Servo_AnimationValues_GetTransform() for Servo, so StyleAnimationValue still has GetScaleValue(). Everything is still in StyleAnimationValue, only the ReadTransform() part is in nsStyleTransformMatrix, just like what your patch did. Thanks for the suggestion.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #18)
> Boris, please file a bug for using RawServoAnimationValue for
> KeyframeEffectReadOnly::GetProperties(). Once it's fixed we don't need to
> store StyleAnimationValue for stylo at [1], IIUC. 
> 
> [1]
> https://hg.mozilla.org/mozilla-central/file/1cc159c7a044/layout/style/
> StyleAnimationValue.cpp#l3687

File Bug 1337313. Thanks for the reminder. :)
Attached file Servo PR, #15442 — 

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/93aa6b8351e6
Part 1: Introduce mozilla::AnimationValue. r=hiro,manishearth
https://hg.mozilla.org/integration/autoland/rev/4c0cd67f48d0
Part 2: Use mozilla::AnimationValue in AnimationPropertySegment. r=hiro,manishearth
https://hg.mozilla.org/integration/autoland/rev/6f4fb1c0adbe
Part 3: Use AnimationValue as the argument in layers::SetAnimatable. r=hiro,manishearth
https://hg.mozilla.org/integration/autoland/rev/aa75aa7d8517
Part 4: Support transform in SetAnimatable. r=hiro,manishearth
https://hg.mozilla.org/integration/autoland/rev/72d2c783f673
Part 5: Add AnimationValue::GetScaleValue(). r=hiro
https://hg.mozilla.org/integration/autoland/rev/60a5029865aa
Part 6: Implement GetScaleValue for RawServoAnimationValue. r=hiro,manishearth

    
    

    
  
Yay! Thank you Boris!

    
  
Blocks: 1338087
Depends on: 1340033

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: