Bug 1260324. Don't draw garbage to the screen if an image doesn't happen to be decoded. r=seth
authorTimothy Nikkel <tnikkel@gmail.com>
Mon, 22 Aug 2016 21:15:38 -0500
changeset 310966 85cfd0bd6eb0c321ccfa105f49e2f607d88b79f9
parent 310965 bfbb874b4641d775af2db1c69f3b1ec696aaf177
child 310967 3490f90107290aa8af58be365d0ebb03d49f30be
push id30599
push userryanvm@gmail.com
push dateThu, 25 Aug 2016 12:11:16 +0000
treeherdermozilla-central@e56ac7b94f39 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersseth
bugs1260324
milestone51.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 1260324. Don't draw garbage to the screen if an image doesn't happen to be decoded. r=seth Layout has been using imgIContainer::IsOpaque to determine if the image will draw opaquely to all pixels it covers, and doing culling based on this. However imgIContainer::IsOpaque doesn't guarantee anything. It only describes if the image, when in a decoded state, has all opaque pixels. So if the image doesn't have fully decoded frames around (because they got discarded) it may not draw opaquely to all of its pixels. So we create a new function that first checks if there is a fully decoded frame.
image/DynamicImage.cpp
image/ImageWrapper.cpp
image/OrientedImage.cpp
image/RasterImage.cpp
image/RasterImage.h
image/VectorImage.cpp
image/VectorImage.h
image/imgIContainer.idl
layout/generic/nsImageFrame.cpp
layout/style/nsStyleStruct.cpp
--- a/image/DynamicImage.cpp
+++ b/image/DynamicImage.cpp
@@ -190,20 +190,18 @@ DynamicImage::GetFrameAtSize(const IntSi
 
   auto result = Draw(context, aSize, ImageRegion::Create(aSize),
                      aWhichFrame, SamplingFilter::POINT, Nothing(), aFlags);
 
   return result == DrawResult::SUCCESS ? dt->Snapshot() : nullptr;
 }
 
 NS_IMETHODIMP_(bool)
-DynamicImage::IsOpaque()
+DynamicImage::WillDrawOpaqueNow()
 {
-  // XXX(seth): For performance reasons it'd be better to return true here, but
-  // I'm not sure how we can guarantee it for an arbitrary gfxDrawable.
   return false;
 }
 
 NS_IMETHODIMP_(bool)
 DynamicImage::IsImageContainerAvailable(LayerManager* aManager, uint32_t aFlags)
 {
   return false;
 }
--- a/image/ImageWrapper.cpp
+++ b/image/ImageWrapper.cpp
@@ -180,19 +180,19 @@ NS_IMETHODIMP_(already_AddRefed<SourceSu
 ImageWrapper::GetFrameAtSize(const IntSize& aSize,
                              uint32_t aWhichFrame,
                              uint32_t aFlags)
 {
   return mInnerImage->GetFrameAtSize(aSize, aWhichFrame, aFlags);
 }
 
 NS_IMETHODIMP_(bool)
-ImageWrapper::IsOpaque()
+ImageWrapper::WillDrawOpaqueNow()
 {
-  return mInnerImage->IsOpaque();
+  return mInnerImage->WillDrawOpaqueNow();
 }
 
 NS_IMETHODIMP_(bool)
 ImageWrapper::IsImageContainerAvailable(LayerManager* aManager, uint32_t aFlags)
 {
   return mInnerImage->IsImageContainerAvailable(aManager, aFlags);
 }
 
--- a/image/OrientedImage.cpp
+++ b/image/OrientedImage.cpp
@@ -84,17 +84,17 @@ OrientedImage::GetFrame(uint32_t aWhichF
   IntSize size;
   rv = InnerImage()->GetWidth(&size.width);
   NS_ENSURE_SUCCESS(rv, nullptr);
   rv = InnerImage()->GetHeight(&size.height);
   NS_ENSURE_SUCCESS(rv, nullptr);
 
   // Determine an appropriate format for the surface.
   gfx::SurfaceFormat surfaceFormat;
-  if (InnerImage()->IsOpaque()) {
+  if (InnerImage()->WillDrawOpaqueNow()) {
     surfaceFormat = gfx::SurfaceFormat::B8G8R8X8;
   } else {
     surfaceFormat = gfx::SurfaceFormat::B8G8R8A8;
   }
 
   // Create a surface to draw into.
   RefPtr<DrawTarget> target =
     gfxPlatform::GetPlatform()->
--- a/image/RasterImage.cpp
+++ b/image/RasterImage.cpp
@@ -372,17 +372,17 @@ RasterImage::LookupFrame(const IntSize& 
   if (aFlags & (FLAG_SYNC_DECODE | FLAG_SYNC_DECODE_IF_FAST) &&
     result.Surface()->IsAborted()) {
     return DrawableSurface();
   }
 
   return Move(result.Surface());
 }
 
-NS_IMETHODIMP_(bool)
+bool
 RasterImage::IsOpaque()
 {
   if (mError) {
     return false;
   }
 
   Progress progress = mProgressTracker->GetProgress();
 
@@ -390,16 +390,49 @@ RasterImage::IsOpaque()
   if (!(progress & FLAG_DECODE_COMPLETE)) {
     return false;
   }
 
   // Other, we're opaque if FLAG_HAS_TRANSPARENCY is not set.
   return !(progress & FLAG_HAS_TRANSPARENCY);
 }
 
+NS_IMETHODIMP_(bool)
+RasterImage::WillDrawOpaqueNow()
+{
+  if (!IsOpaque()) {
+    return false;
+  }
+
+  if (mAnimationState) {
+    // We never discard frames of animated images.
+    return true;
+  }
+
+  // If we are not locked our decoded data could get discard at any time (ie
+  // between the call to this function and when we are asked to draw), so we
+  // have to return false if we are unlocked.
+  if (IsUnlocked()) {
+    return false;
+  }
+
+  LookupResult result =
+    SurfaceCache::LookupBestMatch(ImageKey(this),
+                                  RasterSurfaceKey(mSize,
+                                                   DefaultSurfaceFlags(),
+                                                   PlaybackType::eStatic));
+  MatchType matchType = result.Type();
+  if (matchType == MatchType::NOT_FOUND || matchType == MatchType::PENDING ||
+      !result.Surface()->IsFinished()) {
+    return false;
+  }
+
+  return true;
+}
+
 void
 RasterImage::OnSurfaceDiscarded()
 {
   MOZ_ASSERT(mProgressTracker);
 
   NS_DispatchToMainThread(NewRunnableMethod(mProgressTracker, &ProgressTracker::OnDiscard));
 }
 
--- a/image/RasterImage.h
+++ b/image/RasterImage.h
@@ -479,16 +479,18 @@ private: // data
     explicit HandleErrorWorker(RasterImage* aImage);
 
     RefPtr<RasterImage> mImage;
   };
 
   // Helpers
   bool CanDiscard();
 
+  bool IsOpaque();
+
 protected:
   explicit RasterImage(ImageURL* aURI = nullptr);
 
   bool ShouldAnimate() override;
 
   friend class ImageFactory;
 };
 
--- a/image/VectorImage.cpp
+++ b/image/VectorImage.cpp
@@ -673,17 +673,17 @@ VectorImage::GetFirstFrameDelay()
   }
 
   // We don't really have a frame delay, so just pretend that we constantly
   // need updates.
   return 0;
 }
 
 NS_IMETHODIMP_(bool)
-VectorImage::IsOpaque()
+VectorImage::WillDrawOpaqueNow()
 {
   return false; // In general, SVG content is not opaque.
 }
 
 //******************************************************************************
 NS_IMETHODIMP_(already_AddRefed<SourceSurface>)
 VectorImage::GetFrame(uint32_t aWhichFrame, uint32_t aFlags)
 {
--- a/image/VectorImage.h
+++ b/image/VectorImage.h
@@ -44,17 +44,17 @@ public:
                                         nsIInputStream* aInStr,
                                         uint64_t aSourceOffset,
                                         uint32_t aCount) override;
   virtual nsresult OnImageDataComplete(nsIRequest* aRequest,
                                        nsISupports* aContext,
                                        nsresult aResult,
                                        bool aLastPart) override;
 
-  void OnSurfaceDiscarded() override;
+  virtual void OnSurfaceDiscarded() override;
 
   /**
    * Callback for SVGRootRenderingObserver.
    *
    * This just sets a dirty flag that we check in VectorImage::RequestRefresh,
    * which is called under the ticks of the refresh driver of any observing
    * documents that we may have. Only then (after all animations in this image
    * have been updated) do we send out "frame changed" notifications,
--- a/image/imgIContainer.idl
+++ b/image/imgIContainer.idl
@@ -259,19 +259,22 @@ interface imgIContainer : nsISupports
    * @param aWhichFrame Frame specifier of the FRAME_* variety.
    * @param aFlags Flags of the FLAG_* variety
    */
   [noscript, notxpcom] TempRefSourceSurface getFrameAtSize([const] in nsIntSize aSize,
                                                            in uint32_t aWhichFrame,
                                                            in uint32_t aFlags);
 
   /**
-   * Whether this image is opaque (i.e., needs a background painted behind it).
+   * Returns true if this image will draw opaquely right now if asked to draw
+   * with FLAG_HIGH_QUALITY_SCALING and otherwise default flags. If this image
+   * (when decoded) is opaque but no decoded frames are available then
+   * willDrawOpaqueNow will return false.
    */
-  [notxpcom] boolean isOpaque();
+  [noscript, notxpcom] boolean willDrawOpaqueNow();
 
   /**
    * @return true if getImageContainer() is expected to return a valid
    *         ImageContainer when passed the given @Manager and @Flags
    *         parameters.
    */
   [noscript, notxpcom] boolean isImageContainerAvailable(in LayerManager aManager,
                                                          in uint32_t aFlags);
--- a/layout/generic/nsImageFrame.cpp
+++ b/layout/generic/nsImageFrame.cpp
@@ -1629,17 +1629,17 @@ nsDisplayImage::GetLayerState(nsDisplayL
 }
 
 
 /* virtual */ nsRegion
 nsDisplayImage::GetOpaqueRegion(nsDisplayListBuilder* aBuilder,
                                 bool* aSnap)
 {
   *aSnap = false;
-  if (mImage && mImage->IsOpaque()) {
+  if (mImage && mImage->WillDrawOpaqueNow()) {
     const nsRect frameContentBox = GetBounds(aSnap);
     return GetDestRect().Intersect(frameContentBox);
   }
   return nsRegion();
 }
 
 already_AddRefed<Layer>
 nsDisplayImage::BuildLayer(nsDisplayListBuilder* aBuilder,
--- a/layout/style/nsStyleStruct.cpp
+++ b/layout/style/nsStyleStruct.cpp
@@ -2213,17 +2213,17 @@ nsStyleImage::IsOpaque() const
 
   MOZ_ASSERT(mType == eStyleImageType_Image, "unexpected image type");
 
   nsCOMPtr<imgIContainer> imageContainer;
   mImage->GetImage(getter_AddRefs(imageContainer));
   MOZ_ASSERT(imageContainer, "IsComplete() said image container is ready");
 
   // Check if the crop region of the image is opaque.
-  if (imageContainer->IsOpaque()) {
+  if (imageContainer->WillDrawOpaqueNow()) {
     if (!mCropRect) {
       return true;
     }
 
     // Must make sure if mCropRect contains at least a pixel.
     // XXX Is this optimization worth it? Maybe I should just return false.
     nsIntRect actualCropRect;
     bool rv = ComputeActualCropRect(actualCropRect);