Bug 1402439 - Redo how we discard compositor animation ids. r=pchang
authorKartikaya Gupta <kgupta@mozilla.com>
Fri, 22 Sep 2017 16:39:57 -0400
changeset 420465 4480c624a5541676a995e2209c784dbaeaf0e483
parent 420464 26fb75dd655c5002521503dcf2eea1b1011bd784
child 420466 b50170822d21c1ea1a55a60579f94b4fa6d16f1a
push idunknown
push userunknown
push dateunknown
reviewerspchang
bugs1402439
milestone58.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1402439 - Redo how we discard compositor animation ids. r=pchang Instead of always discarding the compositor animation id, and then sometimes un-discarding it (which involves a linear lookup in nsTArray), this patch now has the WebRenderLayerManager keep a set of active animation ids, and uses that to avoid discarding the same animation twice. In addition, because the display item can be destroyed at any time (e.g. in the middle of an animation), we were previously "leaking" compositor animations in that the compositor side never got notified to discard the IDs. This resulted in infinite composition loops. This patch solves this problem by having any unused WebRenderAnimationData trigger discard of the animation id during destruction. This way, even if the nsDisplayItem is deleted in the middle of the animation we have a fallback mechanism to discard the id. MozReview-Commit-ID: 8G3EYHcg9Kl
gfx/layers/AnimationInfo.cpp
gfx/layers/wr/WebRenderLayerManager.cpp
gfx/layers/wr/WebRenderLayerManager.h
gfx/layers/wr/WebRenderUserData.cpp
gfx/layers/wr/WebRenderUserData.h
layout/painting/nsDisplayList.cpp
--- a/gfx/layers/AnimationInfo.cpp
+++ b/gfx/layers/AnimationInfo.cpp
@@ -38,21 +38,16 @@ AnimationInfo::AddAnimation()
   // Here generates a new id when the first animation is added and
   // this id is used to represent the animations in this layer.
   EnsureAnimationsId();
 
   MOZ_ASSERT(!mPendingAnimations, "should have called ClearAnimations first");
 
   Animation* anim = mAnimations.AppendElement();
 
-  if (mManager->AsWebRenderLayerManager()) {
-    mManager->AsWebRenderLayerManager()->
-      KeepCompositorAnimationsIdAlive(mCompositorAnimationsId);
-  }
-
   mMutated = true;
 
   return anim;
 }
 
 Animation*
 AnimationInfo::AddAnimationForNextTransaction()
 {
@@ -68,21 +63,16 @@ void
 AnimationInfo::ClearAnimations()
 {
   mPendingAnimations = nullptr;
 
   if (mAnimations.IsEmpty() && mAnimationData.IsEmpty()) {
     return;
   }
 
-  if (mManager->AsWebRenderLayerManager()) {
-    mManager->AsWebRenderLayerManager()->
-      AddCompositorAnimationsIdForDiscard(mCompositorAnimationsId);
-  }
-
   mAnimations.Clear();
   mAnimationData.Clear();
 
   mMutated = true;
 }
 
 void
 AnimationInfo::ClearAnimationsForNextTransaction()
--- a/gfx/layers/wr/WebRenderLayerManager.cpp
+++ b/gfx/layers/wr/WebRenderLayerManager.cpp
@@ -104,16 +104,22 @@ WebRenderLayerManager::DoDestroy(bool aI
     // Just clear ImageKeys, they are deleted during WebRenderAPI destruction.
     mImageKeysToDeleteLater.Clear();
     mImageKeysToDelete.Clear();
     // CompositorAnimations are cleared by WebRenderBridgeParent.
     mDiscardedCompositorAnimationsIds.Clear();
     WrBridge()->Destroy(aIsSync);
   }
 
+  // Clear this before calling RemoveUnusedAndResetWebRenderUserData(),
+  // otherwise that function might destroy some WebRenderAnimationData instances
+  // which will put stuff back into mDiscardedCompositorAnimationsIds. If
+  // mActiveCompositorAnimationIds is empty that won't happen.
+  mActiveCompositorAnimationIds.clear();
+
   mLastCanvasDatas.Clear();
   RemoveUnusedAndResetWebRenderUserData();
 
   if (mTransactionIdAllocator) {
     // Make sure to notify the refresh driver just in case it's waiting on a
     // pending transaction. Do this at the top of the event loop so we don't
     // cause a paint to occur during compositor shutdown.
     RefPtr<TransactionIdAllocator> allocator = mTransactionIdAllocator;
@@ -986,25 +992,40 @@ WebRenderLayerManager::DiscardImages()
     resources.DeleteImage(key);
   }
   mImageKeysToDeleteLater.Clear();
   mImageKeysToDelete.Clear();
   WrBridge()->UpdateResources(resources);
 }
 
 void
-WebRenderLayerManager::AddCompositorAnimationsIdForDiscard(uint64_t aId)
+WebRenderLayerManager::AddActiveCompositorAnimationId(uint64_t aId)
 {
-  mDiscardedCompositorAnimationsIds.AppendElement(aId);
+  // In layers-free mode we track the active compositor animation ids on the
+  // client side so that we don't try to discard the same animation id multiple
+  // times. We could just ignore the multiple-discard on the parent side, but
+  // checking on the content side reduces IPC traffic.
+  MOZ_ASSERT(IsLayersFreeTransaction());
+  mActiveCompositorAnimationIds.insert(aId);
 }
 
 void
-WebRenderLayerManager::KeepCompositorAnimationsIdAlive(uint64_t aId)
+WebRenderLayerManager::AddCompositorAnimationsIdForDiscard(uint64_t aId)
 {
-  mDiscardedCompositorAnimationsIds.RemoveElement(aId);
+  if (!IsLayersFreeTransaction()) {
+    // For layers-full we don't track the active animation id in
+    // mActiveCompositorAnimationIds, we just call this on layer destruction and
+    // don't need to worry about discarding the same id multiple times.
+    mDiscardedCompositorAnimationsIds.AppendElement(aId);
+  } else if (mActiveCompositorAnimationIds.erase(aId)) {
+    // For layers-free ensure we don't try to discard an animation id that wasn't
+    // active. We also remove it from mActiveCompositorAnimationIds so we don't
+    // discard it again unless it gets re-activated.
+    mDiscardedCompositorAnimationsIds.AppendElement(aId);
+  }
 }
 
 void
 WebRenderLayerManager::DiscardCompositorAnimations()
 {
   if (WrBridge()->IPCOpen() &&
       !mDiscardedCompositorAnimationsIds.IsEmpty()) {
     WrBridge()->
--- a/gfx/layers/wr/WebRenderLayerManager.h
+++ b/gfx/layers/wr/WebRenderLayerManager.h
@@ -1,16 +1,17 @@
 /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
  * 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/. */
 
 #ifndef GFX_WEBRENDERLAYERMANAGER_H
 #define GFX_WEBRENDERLAYERMANAGER_H
 
+#include <unordered_set>
 #include <vector>
 
 #include "gfxPrefs.h"
 #include "Layers.h"
 #include "mozilla/MozPromise.h"
 #include "mozilla/layers/APZTestData.h"
 #include "mozilla/layers/FocusTarget.h"
 #include "mozilla/layers/StackingContextHelper.h"
@@ -166,22 +167,21 @@ public:
   { return mPaintedLayerCallbackData; }
 
   // adds an imagekey to a list of keys that will be discarded on the next
   // transaction or destruction
   void AddImageKeyForDiscard(wr::ImageKey);
   void DiscardImages();
   void DiscardLocalImages();
 
-  // Before destroying a layer with animations, add its compositorAnimationsId
-  // to a list of ids that will be discarded on the next transaction
+  // Methods to manage the compositor animation ids. Active animations are still
+  // going, and when they end we discard them and remove them from the active
+  // list.
+  void AddActiveCompositorAnimationId(uint64_t aId);
   void AddCompositorAnimationsIdForDiscard(uint64_t aId);
-  // If the animations are valid and running on the compositor,
-  // we should keep the compositorAnimationsId alive on the compositor side.
-  void KeepCompositorAnimationsIdAlive(uint64_t aId);
   void DiscardCompositorAnimations();
 
   WebRenderBridgeChild* WrBridge() const { return mWrChild; }
 
   virtual void Mutated(Layer* aLayer) override;
   virtual void MutatedSimple(Layer* aLayer) override;
 
   void Hold(Layer* aLayer);
@@ -293,16 +293,22 @@ private:
 private:
   nsIWidget* MOZ_NON_OWNING_REF mWidget;
   nsTArray<wr::ImageKey> mImageKeysToDelete;
   // TODO - This is needed because we have some code that creates image keys
   // and enqueues them for deletion right away which is bad not only because
   // of poor texture cache usage, but also because images end up deleted before
   // they are used. This should hopfully be temporary.
   nsTArray<wr::ImageKey> mImageKeysToDeleteLater;
+
+  // Set of compositor animation ids for which there are active animations (as
+  // of the last transaction) on the compositor side.
+  std::unordered_set<uint64_t> mActiveCompositorAnimationIds;
+  // Compositor animation ids for animations that are done now and that we want
+  // the compositor to discard information for.
   nsTArray<uint64_t> mDiscardedCompositorAnimationsIds;
 
   /* PaintedLayer callbacks; valid at the end of a transaciton,
    * while rendering */
   DrawPaintedLayerCallback mPaintedLayerCallback;
   void *mPaintedLayerCallbackData;
 
   RefPtr<WebRenderBridgeChild> mWrChild;
--- a/gfx/layers/wr/WebRenderUserData.cpp
+++ b/gfx/layers/wr/WebRenderUserData.cpp
@@ -215,16 +215,28 @@ WebRenderFallbackData::SetGeometry(nsAut
 
 WebRenderAnimationData::WebRenderAnimationData(WebRenderLayerManager* aWRManager, nsDisplayItem* aItem,
                                                WebRenderUserDataRefTable* aTable)
   : WebRenderUserData(aWRManager, aItem, aTable)
   , mAnimationInfo(aWRManager)
 {
 }
 
+WebRenderAnimationData::~WebRenderAnimationData()
+{
+  // It may be the case that nsDisplayItem that created this WebRenderUserData
+  // gets destroyed without getting a chance to discard the compositor animation
+  // id, so we should do it as part of cleanup here.
+  uint64_t animationId = mAnimationInfo.GetCompositorAnimationsId();
+  // animationId might be 0 if mAnimationInfo never held any active animations.
+  if (animationId) {
+    mWRManager->AddCompositorAnimationsIdForDiscard(animationId);
+  }
+}
+
 WebRenderCanvasData::WebRenderCanvasData(WebRenderLayerManager* aWRManager, nsDisplayItem* aItem,
                                          WebRenderUserDataRefTable* aTable)
   : WebRenderUserData(aWRManager, aItem, aTable)
 {
 }
 
 WebRenderCanvasData::~WebRenderCanvasData()
 {
--- a/gfx/layers/wr/WebRenderUserData.h
+++ b/gfx/layers/wr/WebRenderUserData.h
@@ -135,17 +135,17 @@ protected:
   bool mInvalid;
 };
 
 class WebRenderAnimationData : public WebRenderUserData
 {
 public:
   explicit WebRenderAnimationData(WebRenderLayerManager* aWRManager, nsDisplayItem* aItem,
                                   WebRenderUserDataRefTable* aTable);
-  virtual ~WebRenderAnimationData() {}
+  virtual ~WebRenderAnimationData();
 
   virtual UserDataType GetType() override { return UserDataType::eAnimation; }
   static UserDataType Type() { return UserDataType::eAnimation; }
   AnimationInfo& GetAnimationInfo() { return mAnimationInfo; }
 
 protected:
   AnimationInfo mAnimationInfo;
 };
--- a/layout/painting/nsDisplayList.cpp
+++ b/layout/painting/nsDisplayList.cpp
@@ -6260,27 +6260,32 @@ nsDisplayOpacity::CreateWebRenderCommand
   float* opacityForSC = &mOpacity;
 
   RefPtr<WebRenderAnimationData> animationData = aManager->CreateOrRecycleWebRenderUserData<WebRenderAnimationData>(this);
   AnimationInfo& animationInfo = animationData->GetAnimationInfo();
   AddAnimationsForProperty(Frame(), aDisplayListBuilder,
                            this, eCSSProperty_opacity,
                            animationInfo, false);
   animationInfo.StartPendingAnimations(aManager->GetAnimationReadyTime());
-  uint64_t animationsId = 0;
+
+  // Note that animationsId can be 0 (uninitialized in AnimationInfo) if there
+  // are no active animations.
+  uint64_t animationsId = animationInfo.GetCompositorAnimationsId();
 
   if (!animationInfo.GetAnimations().IsEmpty()) {
-    animationsId = animationInfo.GetCompositorAnimationsId();
     opacityForSC = nullptr;
     OptionalOpacity opacityForCompositor = mOpacity;
 
     OpAddCompositorAnimations
       anim(CompositorAnimations(animationInfo.GetAnimations(), animationsId),
            void_t(), opacityForCompositor);
     aManager->WrBridge()->AddWebRenderParentCommand(anim);
+    aManager->AddActiveCompositorAnimationId(animationsId);
+  } else if (animationsId) {
+    aManager->AddCompositorAnimationsIdForDiscard(animationsId);
   }
 
   nsTArray<mozilla::wr::WrFilterOp> filters;
   StackingContextHelper sc(aSc,
                            aBuilder,
                            aDisplayListBuilder,
                            this,
                            &mList,
@@ -8023,34 +8028,38 @@ nsDisplayTransform::CreateWebRenderComma
 
   RefPtr<WebRenderAnimationData> animationData = aManager->CreateOrRecycleWebRenderUserData<WebRenderAnimationData>(this);
 
   AnimationInfo& animationInfo = animationData->GetAnimationInfo();
   AddAnimationsForProperty(Frame(), aDisplayListBuilder,
                            this, eCSSProperty_transform,
                            animationInfo, false);
   animationInfo.StartPendingAnimations(aManager->GetAnimationReadyTime());
-  uint64_t animationsId = 0;
+
+  // Note that animationsId can be 0 (uninitialized in AnimationInfo) if there
+  // are no active animations.
+  uint64_t animationsId = animationInfo.GetCompositorAnimationsId();
 
   if (!animationInfo.GetAnimations().IsEmpty()) {
-    animationsId = animationInfo.GetCompositorAnimationsId();
-
     // Update transfrom as nullptr in stacking context if there exists
     // transform animation, the transform value will be resolved
     // after animation sampling on the compositor
     transformForSC = nullptr;
 
     // Pass default transform to compositor in case gecko fails to
     // get animated value after animation sampling.
     OptionalTransform transformForCompositor = newTransformMatrix;
 
     OpAddCompositorAnimations
       anim(CompositorAnimations(animationInfo.GetAnimations(), animationsId),
            transformForCompositor, void_t());
     aManager->WrBridge()->AddWebRenderParentCommand(anim);
+    aManager->AddActiveCompositorAnimationId(animationsId);
+  } else if (animationsId) {
+    aManager->AddCompositorAnimationsIdForDiscard(animationsId);
   }
 
   gfx::Matrix4x4Typed<LayerPixel, LayerPixel> boundTransform = ViewAs<gfx::Matrix4x4Typed<LayerPixel, LayerPixel>>(newTransformMatrix);
 
   nsTArray<mozilla::wr::WrFilterOp> filters;
   StackingContextHelper sc(aSc,
                            aBuilder,
                            aDisplayListBuilder,