Closed
Bug 1357320
Opened 7 years ago
Closed 7 years ago
Deal with the lifetime of animation data in CompositorAnimationStorage
Categories
(Core :: Graphics: WebRender, enhancement)
Core
Graphics: WebRender
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.
Comment 1•7 years ago
|
||
According to the above comment, I will denote it as gfx-noted.
Whiteboard: [gfx-noted]
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → howareyou322
Comment hidden (mozreview-request) |
Comment 3•7 years 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•7 years 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•7 years 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+
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: 7 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 9•7 years 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
Comment 10•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ccd2f70fc8d4 https://hg.mozilla.org/mozilla-central/rev/588a50945a7f
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•