Enable WebRender OMTA by default

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: pchang, Assigned: pchang)

Tracking

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(4 attachments)

Assignee: nobody → howareyou322
Depends on: 1357320
These failure reftests were adding the animation delay, therefore we didn't run OMTA during the testing.
In content side, we detected there was an animation and we passed the compostiorAnimationID instead of transform to compositor.
But we found out the animation delay state during sampling animation, so we couldn't provide the correct transform in compositor(because the correct transform was in content side).

I'm checking how to solve this problem.
Summary: Eanble WebRender OMTA by default → Enable WebRender OMTA by default
Comment on attachment 8862836 [details]
Bug 1358437 - Setup correct opacity/transform in stacking context when there exists opacity/transform aniamtions.

https://reviewboard.mozilla.org/r/134752/#review137672

Shoot, sorry! I screwed this up in cset 007c9fb80cfa with bad refactoring :(
Attachment #8862836 - Flags: review?(bugmail) → review+
Comment on attachment 8862837 [details]
Bug 1358437 - pass layer's transform/opacity to compositor,

https://reviewboard.mozilla.org/r/134754/#review137680

::: gfx/layers/AnimationHelper.h:88
(Diff revision 1)
>    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(CompositorAnimationStorage)
>  public:
>  
>    /**
> -   * Set the animation transform based on the unique id
> +   * Set the animation transform based on the unique id and also
> +   * setup |aFrameTransform| and |aData| for OMTA testing

s/setup/set up/

::: gfx/layers/wr/WebRenderBridgeParent.cpp:505
(Diff revision 1)
> +            // Store the default opacity
> +            if (op.opacity().type() == OptionalOpacity::Tfloat) {
> +              storage->SetAnimatedValue(data.id(), op.opacity().get_float());
> +            }

What happens if the animation is removed on the client side? The type() will change from e.g. Tfloat on one transaction to void on the next transaction. Should we be clearing the previously-saved default opacity? Or maybe it doesn't matter because the stacking context will have the opacity and no animation id?
Attachment #8862837 - Flags: review?(bugmail) → review+
Comment on attachment 8862838 [details]
Bug 1358437 - enable WebRender OMTA by default,

https://reviewboard.mozilla.org/r/134756/#review137682
Attachment #8862838 - Flags: review?(bugmail) → review+
Comment on attachment 8862835 [details]
Bug 1358437 - pass layer's transform attributes for transform animation,

https://reviewboard.mozilla.org/r/134758/#review137664

I don't really understand how perspective and that stuff works so I'm assuming the functional effects of PostTranslate/PostScale are ok.

::: gfx/layers/wr/WebRenderContainerLayer.cpp:33
(Diff revision 1)
>  }
>  
>  void
> +WebRenderContainerLayer::UpdateTransformDataForAnimation()
> +{
> +  for(Animation& animation : mAnimations) {

nit: add space before '('

::: gfx/layers/wr/WebRenderContainerLayer.cpp:38
(Diff revision 1)
> +      if (GetParent() && GetParent()->GetTransformIsPerspective()) {
> +        transformData.hasPerspectiveParent() = true;

IMO it would be better to just unconditionally assign this:

transform.hasPerspectiveParent() = GetParent() && GetParent()->GetTransformIsPerspective();

that way it's correct even if it changes from having a perspective parent on one iteration to not having it on the next.
Attachment #8862835 - Flags: review?(bugmail) → review+
Comment on attachment 8862837 [details]
Bug 1358437 - pass layer's transform/opacity to compositor,

https://reviewboard.mozilla.org/r/134754/#review137680

> What happens if the animation is removed on the client side? The type() will change from e.g. Tfloat on one transaction to void on the next transaction. Should we be clearing the previously-saved default opacity? Or maybe it doesn't matter because the stacking context will have the opacity and no animation id?

Basically, we share the same animation id for the deafult value and animated value after animation sampling. So the default value will be replaced when aniatmion got removed on the client side via layer::ClearAnimation calls or new animated value is generated after sampling. Besides, the stacking context will have the opacity instead of animation id when animation is removed on the client side.
Comment on attachment 8862835 [details]
Bug 1358437 - pass layer's transform attributes for transform animation,

https://reviewboard.mozilla.org/r/134758/#review137664

Basically I jost copied the logic from ApplyAnimatedValue func in AsyncCompositionManager

> nit: add space before '('

Done

> IMO it would be better to just unconditionally assign this:
> 
> transform.hasPerspectiveParent() = GetParent() && GetParent()->GetTransformIsPerspective();
> 
> that way it's correct even if it changes from having a perspective parent on one iteration to not having it on the next.

Good point. Fixed.
Pushed by pchang@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/f877064cf25c
pass layer's transform attributes for transform animation, r?kats
https://hg.mozilla.org/projects/graphics/rev/53f5de187374
Setup correct opacity/transform in stacking context when there exists opacity/transform aniamtions. r?kats
https://hg.mozilla.org/projects/graphics/rev/6ba01d83eb61
pass layer's transform/opacity to compositor, r?kats
https://hg.mozilla.org/projects/graphics/rev/77880d70c875
enable WebRender OMTA by default, r?kats
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Pushed by pchang@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/ae9f92d42542
Correct the way to use override pref for WebRender OMTA, r=me
(In reply to Pulsebot from comment #14)
> Pushed by pchang@mozilla.com:
> https://hg.mozilla.org/projects/graphics/rev/22ed1758f789
> fix compiler error, r=me

(In reply to Pulsebot from comment #15)
> Pushed by pchang@mozilla.com:
> https://hg.mozilla.org/projects/graphics/rev/ae9f92d42542
> Correct the way to use override pref for WebRender OMTA, r=me

Kats, sorry to land above additional commits. I should verify in my local before pushing commits every time.
(In reply to Peter Chang[:pchang] from comment #16)
> Kats, sorry to land above additional commits. I should verify in my local
> before pushing commits every time.

Yes please. Also in the future please be sure to update r?kats to r=kats when landing.
You need to log in before you can comment on or make changes to this bug.