Bug 1257101. imgFrame::IsImageComplete says whether we've had pixels decoded to the whole image rect, but it's used to check if the frame is finished decoding. These are different things when the image has more than one progress pass. r=seth

This means that in RasterImage::LookupFrame when we are asked to do a sync decode (if needed) we use WaitUntilComplete to wait until the frame is finished decoding.  But we would actually return after the next progressive pass notified the monitor to wake up. Thus, we would draw a not-fully-decoded image even though the sync decode flag was passed.

The change in FrameAnimator means that we won't draw the next frame in an animated image until all progressive passes of that image are complete. This seems like what we want anyways.

There is one real use of IsImageComplete left, in imgFrame::Draw, where we need to know if the decoded image data covers the whole image frame. (There are a couple of uses of IsImageComplete in asserts.)

image/FrameAnimator.cpp

@@ -143,7 +143,7 @@ FrameAnimator::AdvanceFrame(TimeStamp aTime)
   // If we aren't, we only display fully-downloaded frames; everything else
   // gets delayed.
   bool canDisplay = mDoneDecoding ||
-                    (nextFrame && nextFrame->IsImageComplete());
+                    (nextFrame && nextFrame->IsFinished());
 
   if (!canDisplay) {
     // Uh oh, the frame we want to show is currently being decoded (partial)

image/RasterImage.cpp

@@ -365,7 +365,7 @@ RasterImage::LookupFrame(uint32_t aFrameNum,
   // async decoder that's currently running, the contents of the frame may not
   // be available yet. Make sure we get everything.
   if (mHasSourceData && (aFlags & FLAG_SYNC_DECODE)) {
-    result.DrawableRef()->WaitUntilComplete();
+    result.DrawableRef()->WaitUntilFinished();
   }
 
   return Move(result.DrawableRef());
@@ -612,7 +612,7 @@ RasterImage::GetFrameInternal(const IntSize& aSize,
     frameSurf = CopyFrame(aWhichFrame, aFlags);
   }
 
-  if (!frameRef->IsImageComplete()) {
+  if (!frameRef->IsFinished()) {
     return MakePair(DrawResult::INCOMPLETE, Move(frameSurf));
   }
 
@@ -1450,7 +1450,7 @@ RasterImage::DrawInternal(DrawableFrameRef&& aFrameRef,
 {
   gfxContextMatrixAutoSaveRestore saveMatrix(aContext);
   ImageRegion region(aRegion);
-  bool frameIsComplete = aFrameRef->IsImageComplete();
+  bool frameIsFinished = aFrameRef->IsFinished();
 
   // By now we may have a frame with the requested size. If not, we need to
   // adjust the drawing parameters accordingly.
@@ -1469,7 +1469,7 @@ RasterImage::DrawInternal(DrawableFrameRef&& aFrameRef,
     RecoverFromInvalidFrames(aSize, aFlags);
     return DrawResult::TEMPORARY_ERROR;
   }
-  if (!frameIsComplete) {
+  if (!frameIsFinished) {
     return DrawResult::INCOMPLETE;
   }
   if (couldRedecodeForBetterFrame) {
@@ -1528,7 +1528,7 @@ RasterImage::Draw(gfxContext* aContext,
   }
 
   bool shouldRecordTelemetry = !mDrawStartTime.IsNull() &&
-                               ref->IsImageComplete();
+                               ref->IsFinished();
 
   auto result = DrawInternal(Move(ref), aContext, aSize,
                              aRegion, aFilter, flags);

image/SurfaceCache.cpp

@@ -181,7 +181,7 @@ public:
 
   bool IsDecoded() const
   {
-    return !IsPlaceholder() && mSurface->IsImageComplete();
+    return !IsPlaceholder() && mSurface->IsFinished();
   }
 
   // A helper type used by SurfaceCacheImpl::CollectSizeOfSurfaces.

image/decoders/nsICODecoder.cpp

@@ -99,7 +99,7 @@ nsICODecoder::GetFinalStateFromContainedDecoder()
   mInvalidRect.UnionRect(mInvalidRect, mContainedDecoder->TakeInvalidRect());
   mCurrentFrame = mContainedDecoder->GetCurrentFrameRef();
 
-  MOZ_ASSERT(HasError() || !mCurrentFrame || mCurrentFrame->IsImageComplete());
+  MOZ_ASSERT(HasError() || !mCurrentFrame || mCurrentFrame->IsFinished());
 }
 
 // A BMP inside of an ICO has *2 height because of the AND mask

image/imgFrame.cpp

@@ -140,6 +140,7 @@ imgFrame::imgFrame()
   , mBlendMethod(BlendMethod::OVER)
   , mHasNoAlpha(false)
   , mAborted(false)
+  , mFinished(false)
   , mOptimizable(false)
   , mPalettedImageData(nullptr)
   , mPaletteDepth(0)
@@ -160,7 +161,8 @@ imgFrame::~imgFrame()
 {
 #ifdef DEBUG
   MonitorAutoLock lock(mMonitor);
-  MOZ_ASSERT(mAborted || IsImageCompleteInternal());
+  MOZ_ASSERT(mAborted || AreAllPixelsWritten());
+  MOZ_ASSERT(mAborted || mFinished);
 #endif
 
   free(mPalettedImageData);
@@ -321,7 +323,12 @@ imgFrame::InitWithDrawable(gfxDrawable* aDrawable,
 
   // If we reach this point, we should regard ourselves as complete.
   mDecoded = GetRect();
-  MOZ_ASSERT(IsImageComplete());
+  mFinished = true;
+
+#ifdef DEBUG
+  MonitorAutoLock lock(mMonitor);
+  MOZ_ASSERT(AreAllPixelsWritten());
+#endif
 
   return NS_OK;
 }
@@ -565,7 +572,7 @@ bool imgFrame::Draw(gfxContext* aContext, const ImageRegion& aRegion,
                       mOffset.x);
 
   bool doPadding = padding != nsIntMargin(0,0,0,0);
-  bool doPartialDecode = !IsImageCompleteInternal();
+  bool doPartialDecode = !AreAllPixelsWritten();
 
   if (mSinglePixel && !doPadding && !doPartialDecode) {
     if (mSinglePixelColor.a == 0.0) {
@@ -626,11 +633,6 @@ imgFrame::ImageUpdatedInternal(const nsIntRect& aUpdateRect)
   nsIntRect boundsRect(mOffset, mSize);
   mDecoded.IntersectRect(mDecoded, boundsRect);
 
-  // If the image is now complete, wake up anyone who's waiting.
-  if (IsImageCompleteInternal()) {
-    mMonitor.NotifyAll();
-  }
-
   return NS_OK;
 }
 
@@ -651,6 +653,10 @@ imgFrame::Finish(Opacity aFrameOpacity /* = Opacity::SOME_TRANSPARENCY */,
   mTimeout = aRawTimeout;
   mBlendMethod = aBlendMethod;
   ImageUpdatedInternal(GetRect());
+  mFinished = true;
+
+  // The image is now complete, wake up anyone who's waiting.
+  mMonitor.NotifyAll();
 }
 
 nsIntRect
@@ -834,8 +840,8 @@ imgFrame::UnlockImageData()
     return NS_ERROR_FAILURE;
   }
 
-  MOZ_ASSERT(mLockCount > 1 || IsImageCompleteInternal() || mAborted,
-             "Should have marked complete or aborted before unlocking");
+  MOZ_ASSERT(mLockCount > 1 || mFinished || mAborted,
+             "Should have Finish()'d or aborted before unlocking");
 
   // If we're about to become unlocked, we don't need to hold on to our data
   // surface anymore. (But we don't need to do anything for paletted images,
@@ -992,20 +998,20 @@ imgFrame::Abort()
 }
 
 bool
-imgFrame::IsImageComplete() const
+imgFrame::IsFinished() const
 {
   MonitorAutoLock lock(mMonitor);
-  return IsImageCompleteInternal();
+  return mFinished;
 }
 
 void
-imgFrame::WaitUntilComplete() const
+imgFrame::WaitUntilFinished() const
 {
   MonitorAutoLock lock(mMonitor);
 
   while (true) {
     // Return if we're aborted or complete.
-    if (mAborted || IsImageCompleteInternal()) {
+    if (mAborted || mFinished) {
       return;
     }
 
@@ -1015,7 +1021,7 @@ imgFrame::WaitUntilComplete() const
 }
 
 bool
-imgFrame::IsImageCompleteInternal() const
+imgFrame::AreAllPixelsWritten() const
 {
   mMonitor.AssertCurrentThreadOwns();
   return mDecoded.IsEqualInterior(nsIntRect(mOffset.x, mOffset.y,

image/imgFrame.h

@@ -212,7 +212,7 @@ public:
   /**
    * Returns true if this imgFrame is completely decoded.
    */
-  bool IsImageComplete() const;
+  bool IsFinished() const;
 
   /**
    * Blocks until this imgFrame is either completely decoded, or is marked as
@@ -222,7 +222,7 @@ public:
    * careful in your use of this method to avoid excessive main thread jank or
    * deadlock.
    */
-  void WaitUntilComplete() const;
+  void WaitUntilFinished() const;
 
   /**
    * Returns the number of bytes per pixel this imgFrame requires.  This is a
@@ -278,7 +278,7 @@ private: // methods
 
   void AssertImageDataLocked() const;
 
-  bool IsImageCompleteInternal() const;
+  bool AreAllPixelsWritten() const;
   nsresult ImageUpdatedInternal(const nsIntRect& aUpdateRect);
   void GetImageDataInternal(uint8_t** aData, uint32_t* length) const;
   uint32_t GetImageBytesPerRow() const;
@@ -342,6 +342,7 @@ private: // data
 
   bool mHasNoAlpha;
   bool mAborted;
+  bool mFinished;
   bool mOptimizable;