Bug 1260324. Don't draw garbage to the screen if an image doesn't happen to be decoded. r=seth, a=ritu
authorTimothy Nikkel <tnikkel@gmail.com>
Wed, 24 Aug 2016 00:02:19 -0500
changeset 350202 41184c8c63b17e62605e0f9840d8e3350ac2275a
parent 350201 8fe577bafb7837433a3d88515baf0c4155dadf90
child 350203 961603912962c62f9715751518358edc82f7afdf
push id1230
push userjlund@mozilla.com
push dateMon, 31 Oct 2016 18:13:35 +0000
treeherdermozilla-release@5e06e3766db2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersseth, ritu
bugs1260324
milestone50.0a2
Bug 1260324. Don't draw garbage to the screen if an image doesn't happen to be decoded. r=seth, a=ritu 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/reftests/table-background/reftest.list
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
@@ -391,17 +391,17 @@ RasterImage::GetCurrentFrameIndex() cons
 }
 
 uint32_t
 RasterImage::GetRequestedFrameIndex(uint32_t aWhichFrame) const
 {
   return aWhichFrame == FRAME_FIRST ? 0 : GetCurrentFrameIndex();
 }
 
-NS_IMETHODIMP_(bool)
+bool
 RasterImage::IsOpaque()
 {
   if (mError) {
     return false;
   }
 
   Progress progress = mProgressTracker->GetProgress();
 
@@ -409,16 +409,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(),
+                                                   /* aFrameNum */ 0));
+  MatchType matchType = result.Type();
+  if (matchType == MatchType::NOT_FOUND || matchType == MatchType::PENDING ||
+      !result.DrawableRef()->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
@@ -436,16 +436,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
@@ -250,19 +250,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/reftests/table-background/reftest.list
+++ b/layout/reftests/table-background/reftest.list
@@ -40,23 +40,23 @@ skip-if(B2G||Mulet) == border-collapse-t
 skip-if(B2G||Mulet) == border-collapse-table-row-group.html border-collapse-table-row-group-ref.html # bug 773482 # Initial mulet triage: parity with B2G/B2G Desktop
 == border-collapse-table-row.html border-collapse-table-row-ref.html
 == border-collapse-table.html border-collapse-table-ref.html
 skip-if(B2G||Mulet) fuzzy-if(d2d,1,1083) fuzzy-if(skiaContent,1,2200) == border-collapse-opacity-table-cell.html border-collapse-opacity-table-cell-ref.html # bug 773482 # Initial mulet triage: parity with B2G/B2G Desktop
 fails == border-collapse-opacity-table-column-group.html border-collapse-opacity-table-column-group-ref.html # bug 424274
 fails == border-collapse-opacity-table-column.html border-collapse-opacity-table-column-ref.html # bug 424274
 skip-if(B2G||Mulet) fuzzy-if(d2d,1,16359) fuzzy-if(skiaContent,1,17000) == border-collapse-opacity-table-row-group.html border-collapse-opacity-table-row-group-ref.html # bug 773482 # Initial mulet triage: parity with B2G/B2G Desktop
 skip-if(B2G||Mulet) fuzzy-if(d2d,1,5453) fuzzy-if(skiaContent,1,5500) == border-collapse-opacity-table-row.html border-collapse-opacity-table-row-ref.html # bug 773482 # Initial mulet triage: parity with B2G/B2G Desktop
-skip-if(B2G||Mulet) fuzzy-if(d2d,1,29973) fuzzy-if(skiaContent,1,60000) == border-collapse-opacity-table.html border-collapse-opacity-table-ref.html # bug 773482 # Initial mulet triage: parity with B2G/B2G Desktop
+skip-if(B2G||Mulet) fuzzy-if(d2d||skiaContent,1,60000) == border-collapse-opacity-table.html border-collapse-opacity-table-ref.html # bug 773482 # Initial mulet triage: parity with B2G/B2G Desktop
 skip-if(B2G||Mulet) fuzzy-if(d2d,1,2478) fuzzy-if(skiaContent,1,2500) == border-separate-opacity-table-cell.html border-separate-opacity-table-cell-ref.html # bug 773482 # Initial mulet triage: parity with B2G/B2G Desktop
 fails == border-separate-opacity-table-column-group.html border-separate-opacity-table-column-group-ref.html # bug 424274
 fails == border-separate-opacity-table-column.html border-separate-opacity-table-column-ref.html # bug 424274
 fuzzy-if(d2d,1,37170) fuzzy-if(skiaContent,1,38000) == border-separate-opacity-table-row-group.html border-separate-opacity-table-row-group-ref.html
 skip-if(B2G||Mulet) fuzzy-if(d2d,1,12390) fuzzy-if(skiaContent,1,13000) == border-separate-opacity-table-row.html border-separate-opacity-table-row-ref.html # bug 773482 # Initial mulet triage: parity with B2G/B2G Desktop
-skip-if(B2G||Mulet) fuzzy-if(skiaContent,1,95000) == border-separate-opacity-table.html border-separate-opacity-table-ref.html # bug 773482 # Initial mulet triage: parity with B2G/B2G Desktop
+skip-if(B2G||Mulet) fuzzy-if(d2d||skiaContent,1,95000) == border-separate-opacity-table.html border-separate-opacity-table-ref.html # bug 773482 # Initial mulet triage: parity with B2G/B2G Desktop
 != scrollable-rowgroup-collapse-background.html scrollable-rowgroup-collapse-notref.html 
 != scrollable-rowgroup-collapse-border.html scrollable-rowgroup-collapse-notref.html     
 != scrollable-rowgroup-separate-background.html scrollable-rowgroup-separate-notref.html
 == scrollable-rowgroup-separate-border.html scrollable-rowgroup-separate-notref.html # scrolling rowgroups were removed in bug 28800
 == empty-cells-default-1.html empty-cells-default-1-ref.html
 == empty-cells-default-2.html empty-cells-default-2-ref.html
 fuzzy-if(OSX,1,113) fuzzy-if(winWidget,1,12) fuzzy-if(Android,1,39) fuzzy-if(winWidget&&!layersGPUAccelerated,82,116) fuzzy-if(skiaContent,77,5400) == table-row-opacity-dynamic-1.html table-row-opacity-dynamic-1-ref.html
 == table-row-opacity-dynamic-2.html table-row-opacity-dynamic-2-ref.html
--- a/layout/style/nsStyleStruct.cpp
+++ b/layout/style/nsStyleStruct.cpp
@@ -2211,17 +2211,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);