Closed Bug 1357320 Opened 3 years ago Closed 3 years ago

Deal with the lifetime of animation data in CompositorAnimationStorage

Categories

(Core :: Graphics: WebRender, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: pchang, Assigned: pchang)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files)

+++ This bug was initially created as a clone of Bug #1345017 comment 65 +++

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.

Need to figure out where did these unchanged animations come from.
According to the above comment, I will denote it as gfx-noted.
Whiteboard: [gfx-noted]
Assignee: nobody → howareyou322
Blocks: 1358437
Comment on attachment 8861344 [details]
Bug 1357320 - Deal with the lifetime of compositor animations,

https://reviewboard.mozilla.org/r/133334/#review136362

::: gfx/layers/wr/WebRenderContainerLayer.cpp:20
(Diff revision 1)
>  namespace layers {
> +void
> +WebRenderContainerLayer::ClearAnimations()
> +{
> +
> +  if (GetAnimations().Length()) {

nit: use !GetAnimations().IsEmpty() as it's a little clearer. Also put a blank line between the "namespace layers {" and the function, and remove the blank line at the start of the function

::: gfx/layers/wr/WebRenderLayerManager.cpp:468
(Diff revision 1)
>    for (auto id : mDiscardedCompositorAnimationsIds) {
> -    WrBridge()->AddWebRenderParentCommand(OpRemoveCompositorAnimations(id));
> +    WrBridge()->SendDeleteCompositorAnimations(id);
>    }

Can we make mDiscardedCompositorAnimationsIds an nsTArray instead of an std::vector, and then change the IPDL message to just take the entire array? That we don't call SendDeleteCompositorAnimations over and over. If you do this, please do it as a separate patch on this bug since it's conceptually a cleanup patch.
Attachment #8861344 - Flags: review?(bugmail) → review+
Comment on attachment 8861344 [details]
Bug 1357320 - Deal with the lifetime of compositor animations,

https://reviewboard.mozilla.org/r/133334/#review136362

> nit: use !GetAnimations().IsEmpty() as it's a little clearer. Also put a blank line between the "namespace layers {" and the function, and remove the blank line at the start of the function

Fixed.

> Can we make mDiscardedCompositorAnimationsIds an nsTArray instead of an std::vector, and then change the IPDL message to just take the entire array? That we don't call SendDeleteCompositorAnimations over and over. If you do this, please do it as a separate patch on this bug since it's conceptually a cleanup patch.

Good suggestion, I just created another patch for review.
Comment on attachment 8862690 [details]
Bug 1357320 - Dispatches the discarded compositor animations id list in one async call,

https://reviewboard.mozilla.org/r/134556/#review137662

Thanks!

::: gfx/layers/wr/WebRenderBridgeParent.cpp:283
(Diff revision 1)
> +  CompositorAnimationStorage* storage =
> +    mCompositorBridge->GetAnimationStorage(storageId);
>    MOZ_ASSERT(storage);
> -  storage->ClearById(aId);
>  
> +  for (uint32_t i=0; i < aIds.Length(); i++) {

nit: add spaces around '='
Attachment #8862690 - Flags: review?(bugmail) → review+
Pushed by pchang@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/ccd2f70fc8d4
Deal with the lifetime of compositor animations, r?kats
https://hg.mozilla.org/projects/graphics/rev/588a50945a7f
Dispatches the discarded compositor animations id list in one async call, r?kats
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Comment on attachment 8862690 [details]
Bug 1357320 - Dispatches the discarded compositor animations id list in one async call,

https://reviewboard.mozilla.org/r/134556/#review137662

> nit: add spaces around '='

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