Deal with the lifetime of animation data in CompositorAnimationStorage

RESOLVED FIXED in Firefox 55

Status

()

Core
Graphics: WebRender
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: pchang, Assigned: pchang)

Tracking

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [gfx-noted])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

6 months ago
+++ 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.

Comment 1

6 months ago
According to the above comment, I will denote it as gfx-noted.
Whiteboard: [gfx-noted]
(Assignee)

Updated

6 months ago
Assignee: nobody → howareyou322
(Assignee)

Updated

6 months ago
Blocks: 1358437
Comment hidden (mozreview-request)

Comment 3

6 months ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

6 months ago
mozreview-review-reply
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 7

6 months ago
mozreview-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+

Comment 8

6 months ago
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
Last Resolved: 6 months ago
Resolution: --- → FIXED
(Assignee)

Comment 9

6 months ago
mozreview-review-reply
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
https://hg.mozilla.org/mozilla-central/rev/ccd2f70fc8d4
https://hg.mozilla.org/mozilla-central/rev/588a50945a7f
status-firefox55: affected → fixed
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.