Bug 1397235 - Ensure that we reset factor-of-2 mode for an ImageSurfaceCache when it becomes empty. r=tnikkel
authorAndrew Osmond <aosmond@mozilla.com>
Wed, 06 Sep 2017 20:16:31 -0700
changeset 379413 354981c2ee1ff9102350393ac2c262691cf72c3d
parent 379412 e4fe078dc0134815c9e19e671ab3d3ea69979955
child 379414 1e6956da301f796a885e0810090d104eab13a097
push id32453
push userarchaeopteryx@coole-files.de
push dateThu, 07 Sep 2017 10:39:44 +0000
treeherdermozilla-central@37b95547f0d2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel
bugs1397235
milestone57.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 1397235 - Ensure that we reset factor-of-2 mode for an ImageSurfaceCache when it becomes empty. r=tnikkel An ImageSurfaceCache cannot enter factor-of-2 mode without a minimum number of surfaces being present in its cache. However those surfaces can be purged from the cache through various means (expire due to being disuse, volatile buffers purged, etc). Also, it is entirely possible that all the surfaces get purged, but the cache itself remains. Since factor-of-2 mode requires at least one surface (to get the owning image and its native size), we need to handle the case when the cache is emptied appropriately. As such, we now reset the factor-of-2 mode (and its pruned state) to the default (false) if we transition from non-empty to empty. MozReview-Commit-ID: EVaEqW59Asv
image/SurfaceCache.cpp
--- a/image/SurfaceCache.cpp
+++ b/image/SurfaceCache.cpp
@@ -274,16 +274,28 @@ public:
 
   already_AddRefed<CachedSurface> Remove(NotNull<CachedSurface*> aSurface)
   {
     MOZ_ASSERT(mSurfaces.GetWeak(aSurface->GetSurfaceKey()),
         "Should not be removing a surface we don't have");
 
     RefPtr<CachedSurface> surface;
     mSurfaces.Remove(aSurface->GetSurfaceKey(), getter_AddRefs(surface));
+
+    if (IsEmpty() && mFactor2Mode) {
+      // The last surface for this cache was removed. This can happen if the
+      // surface was stored in a volatile buffer and got purged, or the surface
+      // expired from the cache. If the cache itself lingers for some reason
+      // (e.g. in the process of performing a lookup, the cache itself is
+      // locked), then we need to reset the factor of 2 state because it
+      // requires at least one surface present to get the native size
+      // information from the image.
+      mFactor2Mode = mFactor2Pruned = false;
+    }
+
     return surface.forget();
   }
 
   already_AddRefed<CachedSurface> Lookup(const SurfaceKey& aSurfaceKey,
                                          bool aForAccess)
   {
     RefPtr<CachedSurface> surface;
     mSurfaces.Get(aSurfaceKey, getter_AddRefs(surface));
@@ -529,16 +541,18 @@ public:
 
   IntSize SuggestedSize(const IntSize& aSize) const
   {
     // When not in factor of 2 mode, we can always decode at the given size.
     if (!mFactor2Mode) {
       return aSize;
     }
 
+    // We cannot enter factor of 2 mode unless we have a minimum number of
+    // surfaces, and we should have left it if the cache was emptied.
     MOZ_ASSERT(!IsEmpty());
 
     // This bit of awkwardness gets the largest native size of the image.
     auto iter = ConstIter();
     NotNull<CachedSurface*> firstSurface = WrapNotNull(iter.UserData());
     Image* image = static_cast<Image*>(firstSurface->GetImageKey());
     IntSize factorSize;
     if (NS_FAILED(image->GetWidth(&factorSize.width)) ||
@@ -753,17 +767,17 @@ public:
       }
       return InsertOutcome::FAILURE;
     }
 
     StartTracking(surface, aAutoLock);
     return InsertOutcome::SUCCESS;
   }
 
-  bool Remove(NotNull<CachedSurface*> aSurface,
+  void Remove(NotNull<CachedSurface*> aSurface,
               const StaticMutexAutoLock& aAutoLock)
   {
     ImageKey imageKey = aSurface->GetImageKey();
 
     RefPtr<ImageSurfaceCache> cache = GetImageCache(imageKey);
     MOZ_ASSERT(cache, "Shouldn't try to remove a surface with no image cache");
 
     // If the surface was not a placeholder, tell its image that we discarded it.
@@ -777,20 +791,17 @@ public:
     mCachedSurfacesDiscard.AppendElement(cache->Remove(aSurface));
 
     // Remove the per-image cache if it's unneeded now. Keep it if the image is
     // locked, since the per-image cache is where we store that state. Note that
     // we don't push it into mImageCachesDiscard because all of its surfaces
     // have been removed, so it is safe to free while holding the lock.
     if (cache->IsEmpty() && !cache->IsLocked()) {
       mImageCaches.Remove(imageKey);
-      return true;
     }
-
-    return false;
   }
 
   void StartTracking(NotNull<CachedSurface*> aSurface,
                      const StaticMutexAutoLock& aAutoLock)
   {
     CostEntry costEntry = aSurface->GetCostEntry();
     MOZ_ASSERT(costEntry.GetCost() <= mAvailableCost,
                "Cost too large and the caller didn't catch it");
@@ -906,19 +917,17 @@ public:
 
       drawableSurface = surface->GetDrawableSurface();
       if (drawableSurface) {
         break;
       }
 
       // The surface was released by the operating system. Remove the cache
       // entry as well.
-      if (Remove(WrapNotNull(surface), aAutoLock)) {
-        break;
-      }
+      Remove(WrapNotNull(surface), aAutoLock);
     }
 
     MOZ_ASSERT_IF(matchType == MatchType::EXACT,
                   surface->GetSurfaceKey() == aSurfaceKey);
     MOZ_ASSERT_IF(matchType == MatchType::SUBSTITUTE_BECAUSE_NOT_FOUND ||
                   matchType == MatchType::SUBSTITUTE_BECAUSE_PENDING,
       surface->GetSurfaceKey().SVGContext() == aSurfaceKey.SVGContext() &&
       surface->GetSurfaceKey().Playback() == aSurfaceKey.Playback() &&