Bug 1536997 - Fix broken assert for recycling animated images with WebRender. r=kats
authorAndrew Osmond <aosmond@mozilla.com>
Fri, 24 May 2019 15:46:38 -0400
changeset 475671 29c76bc4b5901f6fd331435e6db5ff47bcde04bb
parent 475670 cb677c177529541d4bbdd2c98f33bd605de749fd
child 475771 92582acb077d723e32adbda4d368e4a8cca34876
child 475786 e198d4820748d4708dff1967a9d87658011291e3
push id36071
push userrgurzau@mozilla.com
push dateMon, 27 May 2019 21:53:12 +0000
treeherdermozilla-central@29c76bc4b590 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskats
bugs1536997
milestone69.0a1
first release with
nightly linux32
29c76bc4b590 / 69.0a1 / 20190527215312 / files
nightly linux64
29c76bc4b590 / 69.0a1 / 20190527215312 / files
nightly mac
29c76bc4b590 / 69.0a1 / 20190527215312 / files
nightly win32
29c76bc4b590 / 69.0a1 / 20190527215312 / files
nightly win64
29c76bc4b590 / 69.0a1 / 20190527215312 / files
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
releases
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1536997 - Fix broken assert for recycling animated images with WebRender. r=kats By default, recycling frames for animated images is enabled. However the first frame is never recycled because we may need the first frame immediately if the animation is reset. If only the first frame of an animation is displayed before shutting down a tab, then it will not correctly register with the layer state manager prior to its destruction. This trips an assert incorrectly. Now we just always register with the layer state manager if recycling is enabled, and never if recycling is disabled. This allows us to remove from state information at the cost of requiring a restart to toggle to recycling pref (which is almost never done now that the feature is stable.) Differential Revision: https://phabricator.services.mozilla.com/D32526
gfx/layers/ipc/SharedSurfacesChild.cpp
gfx/layers/ipc/SharedSurfacesChild.h
modules/libpref/init/StaticPrefList.h
--- a/gfx/layers/ipc/SharedSurfacesChild.cpp
+++ b/gfx/layers/ipc/SharedSurfacesChild.cpp
@@ -8,16 +8,17 @@
 #include "SharedSurfacesParent.h"
 #include "CompositorManagerChild.h"
 #include "mozilla/gfx/gfxVars.h"
 #include "mozilla/image/RecyclingSourceSurface.h"
 #include "mozilla/layers/IpcResourceUpdateQueue.h"
 #include "mozilla/layers/SourceSurfaceSharedData.h"
 #include "mozilla/layers/WebRenderBridgeChild.h"
 #include "mozilla/layers/RenderRootStateManager.h"
+#include "mozilla/StaticPrefs.h"  // for StaticPrefs
 #include "mozilla/SystemGroup.h"  // for SystemGroup
 
 namespace mozilla {
 namespace layers {
 
 using namespace mozilla::gfx;
 
 /* static */
@@ -475,28 +476,25 @@ nsresult SharedSurfacesChild::UpdateAnim
   SharedSurfacesAnimation* anim = aContainer->EnsureSharedSurfacesAnimation();
   MOZ_ASSERT(anim);
 
   return anim->SetCurrentFrame(aSurface, sharedSurface, aDirtyRect);
 }
 
 AnimationImageKeyData::AnimationImageKeyData(RenderRootStateManager* aManager,
                                              const wr::ImageKey& aImageKey)
-    : SharedSurfacesChild::ImageKeyData(aManager, aImageKey),
-      mRecycling(false) {}
+    : SharedSurfacesChild::ImageKeyData(aManager, aImageKey) {}
 
 AnimationImageKeyData::AnimationImageKeyData(AnimationImageKeyData&& aOther)
     : SharedSurfacesChild::ImageKeyData(std::move(aOther)),
-      mPendingRelease(std::move(aOther.mPendingRelease)),
-      mRecycling(aOther.mRecycling) {}
+      mPendingRelease(std::move(aOther.mPendingRelease)) {}
 
 AnimationImageKeyData& AnimationImageKeyData::operator=(
     AnimationImageKeyData&& aOther) {
   mPendingRelease = std::move(aOther.mPendingRelease);
-  mRecycling = aOther.mRecycling;
   SharedSurfacesChild::ImageKeyData::operator=(std::move(aOther));
   return *this;
 }
 
 AnimationImageKeyData::~AnimationImageKeyData() = default;
 
 SharedSurfacesAnimation::~SharedSurfacesAnimation() {
   MOZ_ASSERT(mKeys.IsEmpty());
@@ -512,37 +510,33 @@ void SharedSurfacesAnimation::Destroy() 
   }
 
   if (mKeys.IsEmpty()) {
     return;
   }
 
   for (const auto& entry : mKeys) {
     MOZ_ASSERT(!entry.mManager->IsDestroyed());
-    if (entry.mRecycling) {
+    if (StaticPrefs::ImageAnimatedDecodeOnDemandRecycle()) {
       entry.mManager->DeregisterAsyncAnimation(entry.mImageKey);
     }
     entry.mManager->AddImageKeyForDiscard(entry.mImageKey);
   }
 
   mKeys.Clear();
 }
 
 void SharedSurfacesAnimation::HoldSurfaceForRecycling(
     AnimationImageKeyData& aEntry, SourceSurface* aParentSurface,
     SourceSurfaceSharedData* aSurface) {
   if (aParentSurface == static_cast<SourceSurface*>(aSurface)) {
     return;
   }
 
-  if (!aEntry.mRecycling) {
-    aEntry.mManager->RegisterAsyncAnimation(aEntry.mImageKey, this);
-    aEntry.mRecycling = true;
-  }
-
+  MOZ_ASSERT(StaticPrefs::ImageAnimatedDecodeOnDemandRecycle());
   aEntry.mPendingRelease.AppendElement(aParentSurface);
 }
 
 nsresult SharedSurfacesAnimation::SetCurrentFrame(
     SourceSurface* aParentSurface, SourceSurfaceSharedData* aSurface,
     const gfx::IntRect& aDirtyRect) {
   MOZ_ASSERT(aSurface);
 
@@ -621,16 +615,20 @@ nsresult SharedSurfacesAnimation::Update
       aKey = entry.mImageKey;
       found = true;
       break;
     }
   }
 
   if (!found) {
     aKey = aManager->WrBridge()->GetNextImageKey();
+    if (StaticPrefs::ImageAnimatedDecodeOnDemandRecycle()) {
+      aManager->RegisterAsyncAnimation(aKey, this);
+    }
+
     AnimationImageKeyData data(aManager, aKey);
     HoldSurfaceForRecycling(data, aParentSurface, aSurface);
     mKeys.AppendElement(std::move(data));
     aResources.AddExternalImage(mId, aKey);
   }
 
   return NS_OK;
 }
--- a/gfx/layers/ipc/SharedSurfacesChild.h
+++ b/gfx/layers/ipc/SharedSurfacesChild.h
@@ -195,17 +195,16 @@ class AnimationImageKeyData final : publ
                         const wr::ImageKey& aImageKey);
 
   virtual ~AnimationImageKeyData();
 
   AnimationImageKeyData(AnimationImageKeyData&& aOther);
   AnimationImageKeyData& operator=(AnimationImageKeyData&& aOther);
 
   AutoTArray<RefPtr<gfx::SourceSurface>, 2> mPendingRelease;
-  bool mRecycling;
 };
 
 /**
  * This helper class owns a single ImageKey which will map to different external
  * image IDs representing different frames in an animation.
  */
 class SharedSurfacesAnimation final {
  public:
--- a/modules/libpref/init/StaticPrefList.h
+++ b/modules/libpref/init/StaticPrefList.h
@@ -2854,20 +2854,20 @@ VARCACHE_PREF(
 VARCACHE_PREF(
   Live,
   "image.animated.decode-on-demand.batch-size",
   ImageAnimatedDecodeOnDemandBatchSize,
   RelaxedAtomicUint32, 6
 )
 
 VARCACHE_PREF(
-  Live,
+  Once,
   "image.animated.decode-on-demand.recycle",
   ImageAnimatedDecodeOnDemandRecycle,
-  RelaxedAtomicBool, false
+  bool, false
 )
 
 VARCACHE_PREF(
   Live,
   "image.animated.resume-from-last-displayed",
   ImageAnimatedResumeFromLastDisplayed,
   RelaxedAtomicBool, false
 )