Bug 1543584 - Always rasterize SVGs but clamp the maximum size. r=tnikkel
authorAndrew Osmond <aosmond@mozilla.com>
Thu, 11 Apr 2019 15:30:48 -0400
changeset 470052 3502a44129b0a87c0a57c5c02643210d3410d8c6
parent 470051 8eeaf7c73026c4c16b479ddf061751e03f358ca6
child 470053 854896d598c17d7254157ad1d4e15a8eb1b46514
push id35887
push userapavel@mozilla.com
push dateFri, 19 Apr 2019 03:54:33 +0000
treeherdermozilla-central@f7a15eb24f3d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel
bugs1543584
milestone68.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 1543584 - Always rasterize SVGs but clamp the maximum size. r=tnikkel 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/PresShell.h"
 #include "mozilla/RefPtr.h"
 #include "mozilla/Tuple.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
@@ -4810,19 +4810,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);