Bug 1543584 - Always rasterize SVGs but clamp the maximum size. r=tnikkel a=pascalc
authorAndrew Osmond <aosmond@mozilla.com>
Thu, 11 Apr 2019 15:30:48 -0400
changeset 523300 b73974383bce7dd7bddb7f59a371ff766a1f4369
parent 523299 3ca335918c716fff83dc6cc079f719f73dfa8651
child 523301 ec55d1e895f99a9cc3085d2bcf54dc962abdbdcc
push id11138
push userarchaeopteryx@coole-files.de
push dateTue, 23 Apr 2019 19:02:09 +0000
treeherdermozilla-beta@c53e3fd76964 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel, pascalc
bugs1543584
milestone67.0
Bug 1543584 - Always rasterize SVGs but clamp the maximum size. r=tnikkel a=pascalc SVG performance with the fallback path with WebRender is very bad. This patch avoids fallback by always producing a rasterized surface we store in SurfaceCache, but also clamping the size consistently to a configured maximum. This will cause us to upscale rasterized SVGs which is undesirable visually but is a lower risk change that we can uplift to beta than fixing the underlying performance issue. Differential Revision: https://phabricator.services.mozilla.com/D27159
image/LookupResult.h
image/SurfaceCache.cpp
image/SurfaceCache.h
image/VectorImage.cpp
image/VectorImage.h
modules/libpref/init/all.js
--- a/image/LookupResult.h
+++ b/image/LookupResult.h
@@ -57,16 +57,23 @@ class MOZ_STACK_CLASS LookupResult {
     MOZ_ASSERT(!mSurface || !(mMatchType == MatchType::NOT_FOUND ||
                               mMatchType == MatchType::PENDING),
                "Only NOT_FOUND or PENDING make sense with no surface");
     MOZ_ASSERT(mSurface || mMatchType == MatchType::NOT_FOUND ||
                    mMatchType == MatchType::PENDING,
                "NOT_FOUND or PENDING do not make sense with a surface");
   }
 
+  LookupResult(MatchType aMatchType, const gfx::IntSize& aSuggestedSize)
+      : mMatchType(aMatchType), mSuggestedSize(aSuggestedSize) {
+    MOZ_ASSERT(
+        mMatchType == MatchType::NOT_FOUND || mMatchType == MatchType::PENDING,
+        "Only NOT_FOUND or PENDING make sense with no surface");
+  }
+
   LookupResult(DrawableSurface&& aSurface, MatchType aMatchType,
                const gfx::IntSize& aSuggestedSize)
       : mSurface(std::move(aSurface)),
         mMatchType(aMatchType),
         mSuggestedSize(aSuggestedSize) {
     MOZ_ASSERT(!mSurface || !(mMatchType == MatchType::NOT_FOUND ||
                               mMatchType == MatchType::PENDING),
                "Only NOT_FOUND or PENDING make sense with no surface");
--- a/image/SurfaceCache.cpp
+++ b/image/SurfaceCache.cpp
@@ -506,36 +506,18 @@ class ImageSurfaceCache {
     // if we discarded surfaces due to the volatile buffers getting released,
     // it is possible.
     AfterMaybeRemove();
   }
 
   IntSize SuggestedSize(const IntSize& aSize) const {
     IntSize suggestedSize = SuggestedSizeInternal(aSize);
     if (mIsVectorImage) {
-      // Whether or not we are in factor of 2 mode, vector image rasterization
-      // is clamped at a configured maximum if the caller is willing to accept
-      // substitutes.
-      MOZ_ASSERT(SurfaceCache::IsLegalSize(suggestedSize));
-
-      // If we exceed the maximum, we need to scale the size downwards to fit.
-      // It shouldn't get here if it is significantly larger because
-      // VectorImage::UseSurfaceCacheForSize should prevent us from requesting
-      // a rasterized version of a surface greater than 4x the maximum.
-      int32_t maxSizeKB = gfxPrefs::ImageCacheMaxRasterizedSVGThresholdKB();
-      int32_t proposedKB = suggestedSize.width * suggestedSize.height / 256;
-      if (maxSizeKB >= proposedKB) {
-        return suggestedSize;
-      }
-
-      double scale = sqrt(double(maxSizeKB) / proposedKB);
-      suggestedSize.width = int32_t(scale * suggestedSize.width);
-      suggestedSize.height = int32_t(scale * suggestedSize.height);
+      suggestedSize = SurfaceCache::ClampVectorSize(suggestedSize);
     }
-
     return suggestedSize;
   }
 
   IntSize SuggestedSizeInternal(const IntSize& aSize) const {
     // When not in factor of 2 mode, we can always decode at the given size.
     if (!mFactor2Mode) {
       return aSize;
     }
@@ -959,17 +941,19 @@ class SurfaceCacheImpl final : public ns
 
   LookupResult LookupBestMatch(const ImageKey aImageKey,
                                const SurfaceKey& aSurfaceKey,
                                const StaticMutexAutoLock& aAutoLock,
                                bool aMarkUsed) {
     RefPtr<ImageSurfaceCache> cache = GetImageCache(aImageKey);
     if (!cache) {
       // No cached surfaces for this image.
-      return LookupResult(MatchType::NOT_FOUND);
+      return LookupResult(
+          MatchType::NOT_FOUND,
+          SurfaceCache::ClampSize(aImageKey, aSurfaceKey.Size()));
     }
 
     // Repeatedly look up the best match, trying again if the resulting surface
     // has been freed by the operating system, until we can either lock a
     // surface for drawing or there are no matching surfaces left.
     // XXX(seth): This is O(N^2), but N is expected to be very small. If we
     // encounter a performance problem here we can revisit this.
 
@@ -978,17 +962,17 @@ class SurfaceCacheImpl final : public ns
     MatchType matchType = MatchType::NOT_FOUND;
     IntSize suggestedSize;
     while (true) {
       Tie(surface, matchType, suggestedSize) =
           cache->LookupBestMatch(aSurfaceKey);
 
       if (!surface) {
         return LookupResult(
-            matchType);  // Lookup in the per-image cache missed.
+            matchType, suggestedSize);  // Lookup in the per-image cache missed.
       }
 
       drawableSurface = surface->GetDrawableSurface();
       if (drawableSurface) {
         break;
       }
 
       // The surface was released by the operating system. Remove the cache
@@ -1642,10 +1626,37 @@ bool SurfaceCache::IsLegalSize(const Int
       CheckedInt32(aSize.width) * CheckedInt32(aSize.height) * 4;
   if (MOZ_UNLIKELY(!requiredBytes.isValid())) {
     NS_WARNING("width or height too large");
     return false;
   }
   return true;
 }
 
+IntSize SurfaceCache::ClampVectorSize(const IntSize& aSize) {
+  // If we exceed the maximum, we need to scale the size downwards to fit.
+  // It shouldn't get here if it is significantly larger because
+  // VectorImage::UseSurfaceCacheForSize should prevent us from requesting
+  // a rasterized version of a surface greater than 4x the maximum.
+  int32_t maxSizeKB = gfxPrefs::ImageCacheMaxRasterizedSVGThresholdKB();
+  if (maxSizeKB <= 0) {
+    return aSize;
+  }
+
+  int32_t proposedKB = int32_t(int64_t(aSize.width) * aSize.height / 256);
+  if (maxSizeKB >= proposedKB) {
+    return aSize;
+  }
+
+  double scale = sqrt(double(maxSizeKB) / proposedKB);
+  return IntSize(int32_t(scale * aSize.width), int32_t(scale * aSize.height));
+}
+
+IntSize SurfaceCache::ClampSize(ImageKey aImageKey, const IntSize& aSize) {
+  if (aImageKey->GetType() != imgIContainer::TYPE_VECTOR) {
+    return aSize;
+  }
+
+  return ClampVectorSize(aSize);
+}
+
 }  // namespace image
 }  // namespace mozilla
--- a/image/SurfaceCache.h
+++ b/image/SurfaceCache.h
@@ -429,16 +429,26 @@ struct SurfaceCache {
    */
   static size_t MaximumCapacity();
 
   /**
    * @return true if the given size is valid.
    */
   static bool IsLegalSize(const IntSize& aSize);
 
+  /**
+   * @return clamped size for the given vector image size to rasterize at.
+   */
+  static IntSize ClampVectorSize(const IntSize& aSize);
+
+  /**
+   * @return clamped size for the given image and size to rasterize at.
+   */
+  static IntSize ClampSize(const ImageKey aImageKey, const IntSize& aSize);
+
  private:
   virtual ~SurfaceCache() = 0;  // Forbid instantiation.
 };
 
 }  // namespace image
 }  // namespace mozilla
 
 #endif  // mozilla_image_SurfaceCache_h
--- a/image/VectorImage.cpp
+++ b/image/VectorImage.cpp
@@ -12,16 +12,17 @@
 #include "gfxUtils.h"
 #include "imgFrame.h"
 #include "mozilla/AutoRestore.h"
 #include "mozilla/MemoryReporting.h"
 #include "mozilla/dom/Event.h"
 #include "mozilla/dom/SVGSVGElement.h"
 #include "mozilla/dom/SVGDocument.h"
 #include "mozilla/gfx/2D.h"
+#include "mozilla/gfx/gfxVars.h"
 #include "mozilla/PendingAnimationTracker.h"
 #include "mozilla/RefPtr.h"
 #include "mozilla/Tuple.h"
 #include "nsIPresShell.h"
 #include "nsIStreamListener.h"
 #include "nsMimeTypes.h"
 #include "nsPresContext.h"
 #include "nsRect.h"
@@ -750,20 +751,16 @@ VectorImage::GetFrameInternal(const IntS
   if (mError) {
     return MakeTuple(ImgDrawResult::BAD_IMAGE, aSize, RefPtr<SourceSurface>());
   }
 
   if (!mIsFullyLoaded) {
     return MakeTuple(ImgDrawResult::NOT_READY, aSize, RefPtr<SourceSurface>());
   }
 
-  // We don't allow large surfaces to be rasterized on the Draw and
-  // GetImageContainerAtSize paths, because those have alternatives. If we get
-  // here however, then we know it came from GetFrame(AtSize) and that path does
-  // not have any fallback method, so we don't check UseSurfaceCacheForSize.
   RefPtr<SourceSurface> sourceSurface;
   IntSize decodeSize;
   Tie(sourceSurface, decodeSize) =
       LookupCachedSurface(aSize, aSVGContext, aFlags);
   if (sourceSurface) {
     return MakeTuple(ImgDrawResult::SUCCESS, decodeSize,
                      std::move(sourceSurface));
   }
@@ -848,31 +845,26 @@ VectorImage::GetImageContainer(LayerMana
 
 //******************************************************************************
 NS_IMETHODIMP_(bool)
 VectorImage::IsImageContainerAvailableAtSize(LayerManager* aManager,
                                              const IntSize& aSize,
                                              uint32_t aFlags) {
   // Since we only support image containers with WebRender, and it can handle
   // textures larger than the hw max texture size, we don't need to check aSize.
-  return !aSize.IsEmpty() && UseSurfaceCacheForSize(aSize) &&
-         IsImageContainerAvailable(aManager, aFlags);
+  return !aSize.IsEmpty() && IsImageContainerAvailable(aManager, aFlags);
 }
 
 //******************************************************************************
 NS_IMETHODIMP_(ImgDrawResult)
 VectorImage::GetImageContainerAtSize(layers::LayerManager* aManager,
                                      const gfx::IntSize& aSize,
                                      const Maybe<SVGImageContext>& aSVGContext,
                                      uint32_t aFlags,
                                      layers::ImageContainer** aOutContainer) {
-  if (!UseSurfaceCacheForSize(aSize)) {
-    return ImgDrawResult::NOT_SUPPORTED;
-  }
-
   Maybe<SVGImageContext> newSVGContext;
   MaybeRestrictSVGContext(newSVGContext, aSVGContext, aFlags);
 
   // The aspect ratio flag was already handled as part of the SVG context
   // restriction above.
   uint32_t flags = aFlags & ~(FLAG_FORCE_PRESERVEASPECTRATIO_NONE);
   return GetImageContainerImpl(aManager, aSize,
                                newSVGContext ? newSVGContext : aSVGContext,
@@ -945,19 +937,23 @@ VectorImage::Draw(gfxContext* aContext, 
   if (mAnimationConsumers == 0) {
     SendOnUnlockedDraw(aFlags);
   }
 
   // We should bypass the cache when:
   // - We are using a DrawTargetRecording because we prefer the drawing commands
   //   in general to the rasterized surface. This allows blob images to avoid
   //   rasterized SVGs with WebRender.
-  // - The size exceeds what we are will to cache as a rasterized surface.
+  // - The size exceeds what we are willing to cache as a rasterized surface.
+  //   We don't do this for WebRender because the performance of the fallback
+  //   path is quite bad and upscaling the SVG from the clamped size is better
+  //   than bringing the browser to a crawl.
   if (aContext->GetDrawTarget()->GetBackendType() == BackendType::RECORDING ||
-      !UseSurfaceCacheForSize(aSize)) {
+      (!gfxVars::UseWebRender() &&
+       aSize != SurfaceCache::ClampVectorSize(aSize))) {
     aFlags |= FLAG_BYPASS_SURFACE_CACHE;
   }
 
   MOZ_ASSERT(!(aFlags & FLAG_FORCE_PRESERVEASPECTRATIO_NONE) ||
                  (aSVGContext && aSVGContext->GetViewportSize()),
              "Viewport size is required when using "
              "FLAG_FORCE_PRESERVEASPECTRATIO_NONE");
 
@@ -1015,34 +1011,16 @@ already_AddRefed<gfxDrawable> VectorImag
     const SVGDrawingParameters& aParams) {
   RefPtr<gfxDrawingCallback> cb = new SVGDrawingCallback(
       mSVGDocumentWrapper, aParams.viewportSize, aParams.size, aParams.flags);
 
   RefPtr<gfxDrawable> svgDrawable = new gfxCallbackDrawable(cb, aParams.size);
   return svgDrawable.forget();
 }
 
-bool VectorImage::UseSurfaceCacheForSize(const IntSize& aSize) const {
-  int32_t maxSizeKB = gfxPrefs::ImageCacheMaxRasterizedSVGThresholdKB();
-  if (maxSizeKB <= 0) {
-    return true;
-  }
-
-  if (!SurfaceCache::IsLegalSize(aSize)) {
-    return false;
-  }
-
-  // With factor of 2 mode, we should be willing to use a surface which is up
-  // to twice the width, and twice the height, of the maximum sized surface
-  // before switching to drawing to the target directly. That means the size in
-  // KB works out to be:
-  //   width * height * 4 [bytes/pixel] * / 1024 [bytes/KB] <= 2 * 2 * maxSizeKB
-  return aSize.width * aSize.height / 1024 <= maxSizeKB;
-}
-
 Tuple<RefPtr<SourceSurface>, IntSize> VectorImage::LookupCachedSurface(
     const IntSize& aSize, const Maybe<SVGImageContext>& aSVGContext,
     uint32_t aFlags) {
   // If we're not allowed to use a cached surface, don't attempt a lookup.
   if (aFlags & FLAG_BYPASS_SURFACE_CACHE) {
     return MakeTuple(RefPtr<SourceSurface>(), aSize);
   }
 
--- a/image/VectorImage.h
+++ b/image/VectorImage.h
@@ -98,19 +98,16 @@ class VectorImage final : public ImageRe
   bool MaybeRestrictSVGContext(Maybe<SVGImageContext>& aNewSVGContext,
                                const Maybe<SVGImageContext>& aSVGContext,
                                uint32_t aFlags);
 
   /// Create a gfxDrawable which callbacks into the SVG document.
   already_AddRefed<gfxDrawable> CreateSVGDrawable(
       const SVGDrawingParameters& aParams);
 
-  /// Returns true if we use the surface cache to store rasterized copies.
-  bool UseSurfaceCacheForSize(const IntSize& aSize) const;
-
   /// Rasterize the SVG into a surface. aWillCache will be set to whether or
   /// not the new surface was put into the cache.
   already_AddRefed<SourceSurface> CreateSurface(
       const SVGDrawingParameters& aParams, gfxDrawable* aSVGDrawable,
       bool& aWillCache);
 
   /// Send a frame complete notification if appropriate. Must be called only
   /// after all drawing has been completed.
--- a/modules/libpref/init/all.js
+++ b/modules/libpref/init/all.js
@@ -4813,19 +4813,19 @@ pref("image.animated.resume-from-last-di
 
 // Maximum number of surfaces for an image before entering "factor of 2" mode.
 // This in addition to the number of "native" sizes of an image. A native size
 // is a size for which we can decode a frame without up or downscaling. Most
 // images only have 1, but some (i.e. ICOs) may have multiple frames for the
 // same data at different sizes.
 pref("image.cache.factor2.threshold-surfaces", 4);
 
-// Maximum number of pixels in either dimension that we are willing to upscale
-// an SVG to when we are in "factor of 2" mode.
-pref("image.cache.max-rasterized-svg-threshold-kb", 92160);
+// Maximum size of a surface in KB we are willing to produce when rasterizing
+// an SVG.
+pref("image.cache.max-rasterized-svg-threshold-kb", 204800);
 
 // The maximum size, in bytes, of the decoded images we cache
 pref("image.cache.size", 5242880);
 
 // A weight, from 0-1000, to place on time when comparing to size.
 // Size is given a weight of 1000 - timeweight.
 pref("image.cache.timeweight", 500);