Closed Bug 1345017 Opened 7 years ago Closed 7 years ago

Add SampleAnimation support

Categories

(Core :: Graphics: WebRender, enhancement)

Product:
Core
Component:
Graphics: WebRender
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: pchang, Assigned: pchang)

References

Details

Attachments

(6 obsolete files) 

After bug 1337889 (CompositorAnimationStorage) is landed, we can start to add sampleanimation support in Quantum Render project.
Assignee: nobody → howareyou322
Blocks: 1333355
Status: NEW → ASSIGNED
Now I can save the animation data in CompositorAnimationStorage in WebRenderBridgeParent. Now I'm working on pass the sampling value/id pair to WebRender.
Have a prototype to pass the sampling value to webrender. Because of the c binding interface, there is two data copy in gecko before sending to WR.

copy sampling data from CompositorAnimationStorage to binding.rs
copy sampling data to rust data structure(DynamicProperties) in binding.rs to WebRender
OMTA with WR is working now. Will clean up patches for reviewing.
I'm cleaning up the last patch that involves sample animation logic and call new WebRender API for OMTA.
(In reply to Peter Chang[:pchang] from comment #6)
> I'm cleaning up the last patch that involves sample animation logic and call
> new WebRender API for OMTA.
Now we save animations data from client layer to CompositorStorage, then we need to deal with the lifetime issue of this animations because client layer could be destroyed easily.

Trying to solve the lifetime problem.
Comment on attachment 8852813 [details]
Bug 1345017 - pass animation data from content to WebRenderBridgeParent,

https://reviewboard.mozilla.org/r/124978/#review127712

r+ with comment addressed. If for some reason what I suggested doesn't work please let me know.

::: gfx/layers/wr/WebRenderBridgeParent.cpp:365
(Diff revision 1)
> +          CompositorAnimationStorage* storage =
> +            mCompositorBridge->GetAnimationStorage(mPipelineId.mHandle);

I think it would be better to use "mWidget ? 0 : mPipelineId.mHandle" as the argument here, to make it clearer that the child WebRenderBridgeParent instances are using the handle as the "layersid" while the root doesn't need one. Then we can remove the change you made to GetAnimationStorage and just keep the existing assert the way it is. Does that seem reasonable to you?
Attachment #8852813 - Flags: review?(bugmail) → review+
Comment on attachment 8852814 [details]
Bug 1345017 - Update WebRenderAPIs to enable OMTA,

https://reviewboard.mozilla.org/r/124980/#review128066

Some notes below. I'd like to see the comments addressed before giving r+. Also I'm not as comfortable with the rust side of things so I'm flagging nical for review as well.

::: gfx/webrender_bindings/WebRenderAPI.cpp:173
(Diff revision 1)
> +                                        !aOpacityArray.IsEmpty() ?
> +                                          aOpacityArray.Elements() : nullptr,
> +                                        aOpacityArray.Length(),
> +                                        !aTransformArray.IsEmpty() ?
> +                                          aTransformArray.Elements() : nullptr,
> +                                        aTransformArray.Length());

Invert the conditions here, so:

aOpacityArray.IsEmpty() ? nullptr
  : aOpacityArray.Elements()

It's a little easier to read without the !.

::: gfx/webrender_bindings/WebRenderAPI.cpp:460
(Diff revision 1)
> +}
> +void

nit: add a blank line between functions

::: gfx/webrender_bindings/WebRenderAPI.cpp:469
(Diff revision 1)
> -  wr_dp_push_stacking_context(mWrState, aBounds, aOverflow, aMask, aOpacity,
> -                              &aTransform.components[0], aMixBlendMode);
> +  int discardAnimationId = 0;
> +  wr_dp_push_stacking_context(mWrState, aBounds, aOverflow, aMask, discardAnimationId,
> +                              &aOpacity, &aTransform.components[0], aMixBlendMode);

Would be slightly better I think to just invoke the other function:

PushStackingContext(aBounds, aOverflow, aMask, 0, &aOpacity, &aTransform, aMixBlendMode);

Less code duplication that way. Not a big deal though.

::: gfx/webrender_bindings/WebRenderTypes.h:325
(Diff revision 1)
>    WrExternalImageId id;
>    id.id = aID;
>    return id;
>  }
>  
> +static inline WrTransformProperty ToWrTransformProperty(uint64_t id, gfx::Matrix4x4& transform)

This doesn't appear to be used anywhere

::: gfx/webrender_bindings/WebRenderTypes.h:333
(Diff revision 1)
> +  prop.id = id;
> +  prop.transform = &transform.components[0];
> +  return prop;
> +}
> +
> +static inline WrOpacityProperty ToWrOpacityProperty(uint64_t id, const float opacity)

Also not used. If you're going to use it in another patch, move it to that patch.

::: gfx/webrender_bindings/src/bindings.rs:903
(Diff revision 1)
>  #[no_mangle]
> +pub extern "C" fn wr_api_generate_frame_with_properties(api: &mut RenderApi,
> +                                                        opacity_array: *mut WrOpacityProperty,
> +                                                        opacity_count: usize,
> +                                                        transform_array: *mut WrTransformProperty,
> +                                                        transform_count: usize){

nit: space between ){

::: gfx/webrender_bindings/src/bindings.rs:911
(Diff revision 1)
> +        floats: Vec::new()
> +    };
> +
> +    if transform_count > 0 {
> +        let mut transform_vector = Vec::new();
> +        let transform_slice = unsafe { slice::from_raw_parts(transform_array, transform_count as usize) };

Do you need the "as usize" here? It's already a usize. Same for opacity_count below

::: gfx/webrender_bindings/src/bindings.rs:912
(Diff revision 1)
> +        transform_vector.extend_from_slice(transform_slice);
> +
> +        for index in transform_vector {

Do you need to create this intermediate vector? You should be able to iterate over the slice directly. And anyway you're just creating new PropertyValue items out of the elements, so lifetimes shouldn't be a problem here.

::: gfx/webrender_bindings/src/bindings.rs:1116
(Diff revision 1)
> +            PropertyBinding::Binding(PropertyBindingKey::new(animationId))));
> +    }
> +
> +    let maybetransform;
> +    if !transform.is_null() {
> +        maybetransform = PropertyBinding::Value(unsafe {*transform} );

nit: adjust spaces like so

::Value(unsafe { *transform });
Attachment #8852814 - Flags: review?(bugmail)
Attachment #8852814 - Flags: review?(nical.bugzilla) → review?(rhunt)
I changed it to Ryan since Nical is on PTO.
Comment on attachment 8852814 [details]
Bug 1345017 - Update WebRenderAPIs to enable OMTA,

https://reviewboard.mozilla.org/r/124980/#review128186

::: gfx/webrender_bindings/WebRenderAPI.h:148
(Diff revision 1)
> +  void PushStackingContext(const WrRect& aBounds, // TODO: We should work with strongly typed rects
> +                           const WrRect& aOverflow,
> +                           const WrImageMask* aMask, // TODO: needs a wrapper.
> +                           const uint64_t& aAnimationId,
> +                           const float* aOpacity,
> +                           const gfx::Matrix4x4* aTransform,

Could aOpacity and aTransform be renamed to specify that they are optional? Maybe to 'aMaybeOpacity', etc?

::: gfx/webrender_bindings/src/bindings.rs:1079
(Diff revision 1)
>  #[no_mangle]
>  pub extern "C" fn wr_dp_push_stacking_context(state: &mut WrState,
>                                                bounds: WrRect,
>                                                overflow: WrRect,
>                                                mask: *const WrImageMask,
> -                                              opacity: f32,
> +                                              animationId: u64,

nit: should be animation_id

::: gfx/webrender_bindings/src/bindings.rs:1104
(Diff revision 1)
>  
>      let clip_region2 = state.frame_builder.dl_builder.new_clip_region(&overflow, vec![], None);
>      let clip_region = state.frame_builder.dl_builder.new_clip_region(&overflow, vec![], mask);
>  
>      let mut filters: Vec<FilterOp> = Vec::new();
> -    if opacity < 1.0 {
> +    if !opacity.is_null() {

I think this could be reorganized like,

let opacity = unsafe { opacity.as_ref() }
if let Some(opacity) = opacity {
    if *opacity < 1.0 {
        filters.push(..);
    }
} else {
    filters.push(..);
}

A match statement would also work.

::: gfx/webrender_bindings/src/bindings.rs:1114
(Diff revision 1)
> +    } else {
> +        filters.push(FilterOp::Opacity(
> +            PropertyBinding::Binding(PropertyBindingKey::new(animationId))));
> +    }
> +
> +    let maybetransform;

I think this could also be reorganized like above,

let transform = unsafe { transform.as_ref() };
let transform_binding = match transform {
    Some(transform) => PropertyBinding::Value(..),
    None => PropertyBinding::Binding(..),
};

You can reuse the binding for transform, so you don't need to call it maybetransform. We do this elsewhere in this file when we convert from bindings types to webrender types.

::: gfx/webrender_bindings/webrender_ffi.h:446
(Diff revision 1)
>      size_t length;
>      size_t capacity;
>  };
>  
> +struct WrTransformProperty {
> +  uint64_t id;

It would be nice to have a type for this ID, but it looks non trivial so this is okay.

::: gfx/webrender_bindings/webrender_ffi.h:639
(Diff revision 1)
>  WR_FUNC;
>  
>  WR_INLINE void
>  wr_dp_push_stacking_context(WrState *wrState, WrRect bounds,
>                              WrRect overflow, const WrImageMask *mask,
> -                            float opacity, const float* matrix,
> +                            uint64_t aAnimationId, const float* opacity,

nit: rename to animationId

The style of arguments to functions in webrender_ffi.h is very inconsistent, but I'd like in the same function to be similar.
I agree with the comments kats made, especially with the intermediate vector, and would like to see that addressed before giving and r+.
Comment on attachment 8852813 [details]
Bug 1345017 - pass animation data from content to WebRenderBridgeParent,

https://reviewboard.mozilla.org/r/124978/#review127712

> I think it would be better to use "mWidget ? 0 : mPipelineId.mHandle" as the argument here, to make it clearer that the child WebRenderBridgeParent instances are using the handle as the "layersid" while the root doesn't need one. Then we can remove the change you made to GetAnimationStorage and just keep the existing assert the way it is. Does that seem reasonable to you?

Fixed, thanks for suggestion.
Comment on attachment 8852814 [details]
Bug 1345017 - Update WebRenderAPIs to enable OMTA,

https://reviewboard.mozilla.org/r/124980/#review128066

> Invert the conditions here, so:
> 
> aOpacityArray.IsEmpty() ? nullptr
>   : aOpacityArray.Elements()
> 
> It's a little easier to read without the !.

Good point

> nit: add a blank line between functions

Done

> Would be slightly better I think to just invoke the other function:
> 
> PushStackingContext(aBounds, aOverflow, aMask, 0, &aOpacity, &aTransform, aMixBlendMode);
> 
> Less code duplication that way. Not a big deal though.

Agree, fixed.

> Also not used. If you're going to use it in another patch, move it to that patch.

Moved to next patch

> nit: space between ){

fixed

> Do you need to create this intermediate vector? You should be able to iterate over the slice directly. And anyway you're just creating new PropertyValue items out of the elements, so lifetimes shouldn't be a problem here.

I just removed the intermediate vector

> nit: adjust spaces like so
> 
> ::Value(unsafe { *transform });

Done
Comment on attachment 8852813 [details]
Bug 1345017 - pass animation data from content to WebRenderBridgeParent,

https://reviewboard.mozilla.org/r/124978/#review130384

::: gfx/layers/wr/WebRenderContainerLayer.cpp:45
(Diff revision 2)
>                                 GetLocalOpacity(),
> -                               //GetLayer()->GetAnimations(),
>                                 transform,
>                                 mixBlendMode);
> +
> +  if (GetCompositorAnimationsId()) {

I just noticed we're only doing this in WebRenderContainerLayer. Can't other layer types also have compositor animations?
Comment on attachment 8852814 [details]
Bug 1345017 - Update WebRenderAPIs to enable OMTA,

https://reviewboard.mozilla.org/r/124980/#review130386
Attachment #8852814 - Flags: review?(bugmail) → review+
Comment on attachment 8852814 [details]
Bug 1345017 - Update WebRenderAPIs to enable OMTA,

Looks good, thanks!
Attachment #8852814 - Flags: review?(rhunt) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #17)
> Comment on attachment 8852813 [details]
> Bug 1345017 - pass animation data from content to WebRenderBridgeParent,
> 
> https://reviewboard.mozilla.org/r/124978/#review130384
> 
> ::: gfx/layers/wr/WebRenderContainerLayer.cpp:45
> (Diff revision 2)
> >                                 GetLocalOpacity(),
> > -                               //GetLayer()->GetAnimations(),
> >                                 transform,
> >                                 mixBlendMode);
> > +
> > +  if (GetCompositorAnimationsId()) {
> 
> I just noticed we're only doing this in WebRenderContainerLayer. Can't other
> layer types also have compositor animations?

Yes, gecko creates container layer for nsDisplayOpacity and nsDisplayTransform in [1] and [2].

[1]http://searchfox.org/mozilla-central/source/layout/painting/nsDisplayList.cpp#5644
[2]http://searchfox.org/mozilla-central/source/layout/painting/nsDisplayList.cpp#7285

Markus, I think we don't want to add compositor animation for other layer types. Am I right?
Flags: needinfo?(mstange)
Comment on attachment 8857398 [details]
Bug 1345017 - Add animation sampling for WR,

https://reviewboard.mozilla.org/r/129392/#review131942

::: gfx/layers/AnimationHelper.cpp:542
(Diff revision 1)
> +        gfx::Matrix4x4 transform =
> +          nsDisplayTransform::GetResultingTransformMatrix(props, origin,
> +                                                          transformData.appUnitsPerDevPixel(),
> +                                                          0, &transformData.bounds());
> +        gfx::Matrix4x4 frameTransform = transform;
> +

The code here was copied from SampleAnimation and ApplyAnimatedValue in AsyncCompositionManager.cpp.

Now the OMTA with WR could work but the offset of transform was wrong because we skip offset here.

I would like to open another follow-up to fix it.
After adding the sample animation for WR, I found the a problem regarding the animation lifetime in CompositorAnimationStorage. 

In content side, it is easy to remove the animation in CSS which also removes the containerlayer in client side, but we don't have layer in WR parent. So it doesn't remove the mapped animation data in CompositorAnimationStorage in parent side.

There is possible solution that we can save the CompositorAnimationId for above removed containerlayer in WebRenderLayerManager and notify WebRenderBridgeParent in next EndTransaction.

Kats, do you have any concern about this approach or other suggestion?
Flags: needinfo?(bugmail)
Comment on attachment 8857397 [details]
Bug 1345017 - Disable OMTA with webrender by default,

https://reviewboard.mozilla.org/r/129390/#review131996
Attachment #8857397 - Flags: review?(bugmail) → review+
(In reply to Peter Chang[:pchang] from comment #20)
> > I just noticed we're only doing this in WebRenderContainerLayer. Can't other
> > layer types also have compositor animations?
> 
> Yes, gecko creates container layer for nsDisplayOpacity and
> nsDisplayTransform in [1] and [2].
> 
> [1]http://searchfox.org/mozilla-central/source/layout/painting/nsDisplayList.
> cpp#5644
> [2]http://searchfox.org/mozilla-central/source/layout/painting/nsDisplayList.
> cpp#7285
> 
> Markus, I think we don't want to add compositor animation for other layer
> types. Am I right?

Not sure, forwarding the question to Matt.
It's true that Gecko currently only sets compositor animations on ContainerLayers, but that's not really enforced by the layers API and just happens to be true at the moment because of how we use layers. If it simplifies things a lot, I suppose we could bake this restriction into the layers API officially.
Flags: needinfo?(mstange) → needinfo?(matt.woodrow)
Comment on attachment 8857398 [details]
Bug 1345017 - Add animation sampling for WR,

https://reviewboard.mozilla.org/r/129392/#review132038

This looks reasonable with comments addressed. You'll need to rebase some of the earlier patches on top of the binding generator stuff that landed.

::: gfx/layers/AnimationHelper.h:113
(Diff revision 1)
> +  {
> +    return mAnimatedValues.ConstIter();
> +  }
> +
> +  /**
> +   * Return the count of animated values

A comment like this is not very useful. The name of the function makes it clear enough.

::: gfx/layers/AnimationHelper.h:177
(Diff revision 1)
>                  InfallibleTArray<AnimData>& aAnimData,
>                  StyleAnimationValue& aBaseAnimationStyle);
>    static uint64_t GetNextCompositorAnimationsId();
> +
> +  static void
> +  SampleAnimations(CompositorAnimationStorage* aStorage,

On the other hand, functions like these in AnimationHelper could really use comments that describe what they do. It's not at all obvious from looking at them which parameters are inputs, which are outputs, and what the functions are intended to do. What does the "node" in "SampleAnimationForEachNode" refer to? That's not clear to me at all, even after looking around at how the code is used.

::: gfx/layers/AnimationHelper.cpp:8
(Diff revision 1)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "AnimationHelper.h"
> +#include "nsDisplayList.h"              // for nsDisplayTransform, etc

Move this down, alphabetical sort

::: gfx/layers/AnimationHelper.cpp:37
(Diff revision 1)
> +}
>  
> +void
> +CompositorAnimationStorage::ClearById(const uint64_t& aId)
> +{
> +  mAnimatedValues.Remove(aId);

This should have a compositor thread assertion as well.

::: gfx/layers/AnimationHelper.cpp:490
(Diff revision 1)
>    return nextId;
>  }
>  
> +void
> +AnimationHelper::SampleAnimations(CompositorAnimationStorage* aStorage,
> +                                  TimeStamp aPoint)

s/aPoint/aTime/ to be clearer. I realize this is just copied from the other function but we might as well improve it here.

::: gfx/layers/AnimationHelper.cpp:509
(Diff revision 1)
> +    AnimationHelper::SampleAnimationForEachNode(
> +                                                aPoint,

Move aPoint up to previous line

::: gfx/layers/AnimationHelper.cpp:563
(Diff revision 1)
> +        aStorage->SetAnimatedValue(iter.Key(),
> +                                   Move(transform), Move(frameTransform),
> +                                   transformData);
> +        break;
> +      }
> +      defalult:

typo of "default"

::: gfx/layers/wr/WebRenderContainerLayer.cpp:45
(Diff revision 1)
>      CompositorAnimations anim;
>      anim.animations() = GetAnimations();
>      anim.id() = GetCompositorAnimationsId();
>      WrBridge()->AddWebRenderParentCommand(OpAddCompositorAnimations(anim));
> +
> +    float opacity = GetLocalOpacity();

move this one up to be next to the |transform| declaration, since it's used in both parts of the if condition
Attachment #8857398 - Flags: review?(bugmail) → review+
(In reply to Peter Chang[:pchang] from comment #24)
> There is possible solution that we can save the CompositorAnimationId for
> above removed containerlayer in WebRenderLayerManager and notify
> WebRenderBridgeParent in next EndTransaction.
> 
> Kats, do you have any concern about this approach or other suggestion?

This approach sounds reasonable. We just need to make sure we handle all the edge cases, like if the layer tree is torn down on the client side while we are in the middle of an animation. We might not even get another transaction coming over to the parent side, and we should make sure that in this scenario we still clear out the animation data on the parent side somehow. Most likely this will happen when we Clear() the entire compositor animation storage, but just need to make sure it happens.
Flags: needinfo?(bugmail)
(In reply to Markus Stange [:mstange] from comment #26)
> (In reply to Peter Chang[:pchang] from comment #20)
> > > I just noticed we're only doing this in WebRenderContainerLayer. Can't other
> > > layer types also have compositor animations?
> > 
> > Yes, gecko creates container layer for nsDisplayOpacity and
> > nsDisplayTransform in [1] and [2].
> > 
> > [1]http://searchfox.org/mozilla-central/source/layout/painting/nsDisplayList.
> > cpp#5644
> > [2]http://searchfox.org/mozilla-central/source/layout/painting/nsDisplayList.
> > cpp#7285
> > 
> > Markus, I think we don't want to add compositor animation for other layer
> > types. Am I right?
> 
> Not sure, forwarding the question to Matt.
> It's true that Gecko currently only sets compositor animations on
> ContainerLayers, but that's not really enforced by the layers API and just
> happens to be true at the moment because of how we use layers. If it
> simplifies things a lot, I suppose we could bake this restriction into the
> layers API officially.

Yes, I think we should move the Layers animations APIs onto ContainerLayer to avoid this confusion. I don't think we have any plans to animate other types.
Flags: needinfo?(matt.woodrow)
Comment on attachment 8857398 [details]
Bug 1345017 - Add animation sampling for WR,

https://reviewboard.mozilla.org/r/129392/#review132038

> A comment like this is not very useful. The name of the function makes it clear enough.

Removed

> On the other hand, functions like these in AnimationHelper could really use comments that describe what they do. It's not at all obvious from looking at them which parameters are inputs, which are outputs, and what the functions are intended to do. What does the "node" in "SampleAnimationForEachNode" refer to? That's not clear to me at all, even after looking around at how the code is used.

Sure, I will add comment for the API usage.

> Move this down, alphabetical sort

Done

> s/aPoint/aTime/ to be clearer. I realize this is just copied from the other function but we might as well improve it here.

Done.

> Move aPoint up to previous line

Done

> move this one up to be next to the |transform| declaration, since it's used in both parts of the if condition

Fixed.
Comment on attachment 8857398 [details]
Bug 1345017 - Add animation sampling for WR,

https://reviewboard.mozilla.org/r/129392/#review131942

> The code here was copied from SampleAnimation and ApplyAnimatedValue in AsyncCompositionManager.cpp.
> 
> Now the OMTA with WR could work but the offset of transform was wrong because we skip offset here.
> 
> I would like to open another follow-up to fix it.

Will create another bug for this.
Comment on attachment 8852814 [details]
Bug 1345017 - Update WebRenderAPIs to enable OMTA,

https://reviewboard.mozilla.org/r/124980/#review132890

::: gfx/webrender_bindings/WebRenderAPI.cpp:547
(Diff revision 3)
> +                                        const uint64_t& aAnimationId,
> +                                        const float* aOpacity,
> +                                        const gfx::Matrix4x4* aTransform,
> +                                        const WrMixBlendMode& aMixBlendMode)
> +{
> +  WrMatrix matrix;

Kats, I changed here a little bit to align the WrMatrix. Please let me know if you have any comment for this.

::: gfx/webrender_bindings/webrender_ffi_generated.h:124
(Diff revision 3)
>  
>    bool operator==(const WrBuiltDisplayListDescriptor& aOther) const {
>      return display_list_items_size == aOther.display_list_items_size;
>    }
>  };
>  

These changes are generated from wr-binding.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #28)
> (In reply to Peter Chang[:pchang] from comment #24)
> > There is possible solution that we can save the CompositorAnimationId for
> > above removed containerlayer in WebRenderLayerManager and notify
> > WebRenderBridgeParent in next EndTransaction.
> > 
> > Kats, do you have any concern about this approach or other suggestion?
> 
> This approach sounds reasonable. We just need to make sure we handle all the
> edge cases, like if the layer tree is torn down on the client side while we
> are in the middle of an animation. We might not even get another transaction
> coming over to the parent side, and we should make sure that in this
> scenario we still clear out the animation data on the parent side somehow.
> Most likely this will happen when we Clear() the entire compositor animation
> storage, but just need to make sure it happens.

attachment 8858242 [details] just added the mechanism to discard compositor animations when layer in client side was gone.

For the whole layer tree is torn down, the CompositorBridgeParent per window should be destroyed as well.
Then the destructor of compositor animation storage will be trigger to clean up the animation data.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #25)
> Comment on attachment 8857397 [details]
> Bug 1345017 - Reuse CompositorAnimationsId to save
> CompositorAnimationStorage space,
> 
> https://reviewboard.mozilla.org/r/129390/#review131996

I found this patch caused try failure in [1] with wr disabled, I guess we got out-of-date animated value after reusing the compositor animation id. I would defter this patch to another follow up bug.

[1]https://treeherder.mozilla.org/#/jobs?repo=try&revision=e10267076a8fb7994103146581bb4abd2e418058
Comment on attachment 8857398 [details]
Bug 1345017 - Add animation sampling for WR,

https://reviewboard.mozilla.org/r/129392/#review133324

::: gfx/layers/AnimationHelper.cpp:544
(Diff revision 3)
> +          nsDisplayTransform::GetResultingTransformMatrix(props, origin,
> +                                                          transformData.appUnitsPerDevPixel(),
> +                                                          0, &transformData.bounds());
> +        gfx::Matrix4x4 frameTransform = transform;
> +
> +        //TODO how do we support this without layer information

About the try failures with WR enabled, they were related to the following TODO.
I just added a quick fix to apply the translate and postscale if have. I hope this should be the final patch to land OMTA with WR.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9e1681b0be12ce119922289dec870f350a0a18a
(In reply to Peter Chang[:pchang] from comment #46)
> Comment on attachment 8857398 [details]
> Bug 1345017 - Add sample animation for WR,
> 
> https://reviewboard.mozilla.org/r/129392/#review133324
> 
> ::: gfx/layers/AnimationHelper.cpp:544
> (Diff revision 3)
> > +          nsDisplayTransform::GetResultingTransformMatrix(props, origin,
> > +                                                          transformData.appUnitsPerDevPixel(),
> > +                                                          0, &transformData.bounds());
> > +        gfx::Matrix4x4 frameTransform = transform;
> > +
> > +        //TODO how do we support this without layer information
> 
> About the try failures with WR enabled, they were related to the following
> TODO.
> I just added a quick fix to apply the translate and postscale if have. I
> hope this should be the final patch to land OMTA with WR.
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=c9e1681b0be12ce119922289dec870f350a0a18a

I still got 4 reftest failures. As discussed with kats, I will land this bug with preference off first.
Comment on attachment 8852814 [details]
Bug 1345017 - Update WebRenderAPIs to enable OMTA,

https://reviewboard.mozilla.org/r/124980/#review133412

::: gfx/webrender_bindings/WebRenderAPI.cpp:552
(Diff revision 5)
> +    wr_dp_push_stacking_context(mWrState, aBounds, aAnimationId, aOpacity,
> +                                maybeTransform, aMixBlendMode);

nit: fix indentation here

::: gfx/webrender_bindings/src/bindings.rs:619
(Diff revision 5)
>  
> +#[repr(C)]
> +#[derive(Copy, Clone, Debug)]
> +pub struct WrTransformProperty {
> +    pub id: u64,
> +    pub transform: *const f32,

Please use a WrMatrix here, passing a f32 pointer and casting it to a LayoutTransform pointer is not FFI safe.
Attachment #8852814 - Flags: review+ → review-
Comment on attachment 8857398 [details]
Bug 1345017 - Add animation sampling for WR,

https://reviewboard.mozilla.org/r/129392/#review133414

::: commit-message-0257d:1
(Diff revision 5)
> +Bug 1345017 - Add sample animation for WR, r?kats

s/sample animation/animation sampling/

::: gfx/layers/AnimationHelper.h:160
(Diff revision 5)
>  {
>  public:
> +
> +
> +  /*
> +   * TODO Bug 1356509 Once we decopule the compositor animations and layers

s/decopule/decouple/

::: gfx/layers/AnimationHelper.h:176
(Diff revision 5)
>                               AnimationArray& aAnimations,
>                               InfallibleTArray<AnimData>& aAnimationData,
>                               StyleAnimationValue& aAnimationValue,
>                               bool& aHasInEffectAnimations);
> -
> +  /*
> +   * TODO Bug 1356509 Once we decopule the compositor animations and layers

s/decopule/decouple/

::: gfx/layers/AnimationHelper.h:180
(Diff revision 5)
> -
> +  /*
> +   * TODO Bug 1356509 Once we decopule the compositor animations and layers
> +   * in parent side, the API will be called inside SampleAnimations.
> +   * Before this, we expose this API for AsyncCompositionManager.
> +   *
> +   * Prepare another animation structure(AnimData) for sample animation

This is still not super helpful. Maybe something like "populates AnimData stuctures into aAnimData based on aAnimations" would be more useful.

::: gfx/layers/AnimationHelper.h:188
(Diff revision 5)
>    SetAnimations(AnimationArray& aAnimations,
>                  InfallibleTArray<AnimData>& aAnimData,
>                  StyleAnimationValue& aBaseAnimationStyle);
> +
> +  /*
> +   * Get a unique id to represent the compositir animation between child

s/compositir/compositor/

::: gfx/layers/AnimationHelper.h:190
(Diff revision 5)
>                  StyleAnimationValue& aBaseAnimationStyle);
> +
> +  /*
> +   * Get a unique id to represent the compositir animation between child
> +   * and parent side. This id will be used as a key to store animation
> +   * data in the CompositorAnimationStorage per windows(widget).

instead of saying "per windows(widget)" you can say "per compositor"

::: gfx/layers/AnimationHelper.h:191
(Diff revision 5)
> +   * Each layer in content side only calls once even they get new
> +   * animation data.

"Each layer on the content side only calls this once even if it gets new animation data."
Comment on attachment 8858241 [details]
Bug 1345017 - Discard the unchanged animated value during sample animations,

https://reviewboard.mozilla.org/r/130218/#review133416

::: commit-message-53200:1
(Diff revision 4)
> +Bug 1345017 - Discard the unchanged animated value during sample animations, r?kats

This commit message needs more explanation. Why are you discarding the unchanged values? Is it an optimization? For speed or memory usage? Or is it a correctness bug?

::: gfx/layers/AnimationHelper.cpp:579
(Diff revision 4)
> +  // Clean up the unchanged animations
> +  for (const uint64_t& key : unchangedAnimations) {
> +    aStorage->ClearById(key);

Doesn't this remove the animation entirely? What happens if you have an animation that's so slow that two consecutive frames have the same value? Won't you effectively abort the animation at that point because you will consider it "unchanged" and remove it?
Attachment #8858241 - Flags: review?(bugmail) → review-
Comment on attachment 8858242 [details]
Bug 1345017 - Discard compositor animations on the next layer transaction,

https://reviewboard.mozilla.org/r/130220/#review133420

::: commit-message-77e26:3
(Diff revision 4)
> +Animations in content side could be removed easily by changing CSS, but the
> +CompositorAnimationStorage in parent side doesn't get updated. Therefore,
> +we store the layer's CompositorAnimationsId before layer is destroyed in
> +WebRenderLayerManager and then send out these discarded ids to parent on
> +the next layer transaction.

Thanks for putting this in the commit message!
Attachment #8858242 - Flags: review?(bugmail) → review+
Comment on attachment 8857397 [details]
Bug 1345017 - Disable OMTA with webrender by default,

https://reviewboard.mozilla.org/r/129390/#review133424

Looks like MozReview carried the r+ from some other patch onto this one. But yeah, r+, this patch looks fine.
Comment on attachment 8858241 [details]
Bug 1345017 - Discard the unchanged animated value during sample animations,

https://reviewboard.mozilla.org/r/130218/#review133416

> This commit message needs more explanation. Why are you discarding the unchanged values? Is it an optimization? For speed or memory usage? Or is it a correctness bug?

When I verified OMTA in my local, I noticed some animations kept alive in CompositorAnimationStorage and the animated values were unchanged. Since we disable OMTA by default, we don't need to deal with the animation lifetime for this case right now. I will investgiate lifetime problem later.
Blocks: 1357320
Attachment #8858241 - Attachment is obsolete: true
Comment on attachment 8852814 [details]
Bug 1345017 - Update WebRenderAPIs to enable OMTA,

https://reviewboard.mozilla.org/r/124980/#review133736
Attachment #8852814 - Flags: review?(bugmail) → review+
(In reply to Peter Chang[:pchang] from comment #65)
> Since we
> disable OMTA by default, we don't need to deal with the animation lifetime
> for this case right now. I will investgiate lifetime problem later.

Sounds good, thanks.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/8798fd1fe09b
pass animation data from content to WebRenderBridgeParent, r=kats
https://hg.mozilla.org/projects/graphics/rev/918c74101435
Update WebRenderAPIs to enable OMTA, r=kats
https://hg.mozilla.org/projects/graphics/rev/0643a5f1e592
Add animation sampling for WR, r=kats
https://hg.mozilla.org/projects/graphics/rev/5fa3e998d3e5
Discard compositor animations on the next layer transaction, r=kats
https://hg.mozilla.org/projects/graphics/rev/c8dae3630db6
Disable OMTA with webrender by default, r=kats
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Blocks: 1358437
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/e849a232de22
Fix android build bustage due to missing include. r=bustage
Attachment #8852813 - Attachment is obsolete: true
Attachment #8852814 - Attachment is obsolete: true
Attachment #8857398 - Attachment is obsolete: true
Attachment #8858242 - Attachment is obsolete: true
Attachment #8857397 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: