Bug 905468 (Part 1) - Make imgStatusTracker state diffing a cleaner process. r=jdm, a=bajaj
authorSeth Fowler <seth@mozilla.com>
Wed, 28 Aug 2013 15:39:04 -0700
changeset 153911 21545bbe54a8d026b84773720048485aa5178ec7
parent 153910 669f7ea493b01e26789f9d56092cd57adfd0b087
child 153912 bfc9ae46d06ecbbb1b1061d03621b6fe41d936a8
push id2859
push userakeybl@mozilla.com
push dateMon, 16 Sep 2013 19:14:59 +0000
treeherdermozilla-beta@87d3c51cd2bf [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjdm, bajaj
bugs905468
milestone25.0a2
Bug 905468 (Part 1) - Make imgStatusTracker state diffing a cleaner process. r=jdm, a=bajaj
image/src/RasterImage.cpp
image/src/VectorImage.cpp
image/src/imgStatusTracker.cpp
image/src/imgStatusTracker.h
--- a/image/src/RasterImage.cpp
+++ b/image/src/RasterImage.cpp
@@ -2949,19 +2949,20 @@ RasterImage::FinishedSomeDecoding(eShutd
       // decoding routines have been finished.
       rv = image->ShutdownDecoder(aIntent);
       if (NS_FAILED(rv)) {
         image->DoError();
       }
     }
   }
 
-  imgStatusTracker::StatusDiff diff;
+  ImageStatusDiff diff;
   if (request) {
-    diff = image->mStatusTracker->CalculateAndApplyDifference(request->mStatusTracker);
+    diff = image->mStatusTracker->Difference(request->mStatusTracker);
+    image->mStatusTracker->ApplyDifference(diff);
   }
 
   {
     // Notifications can't go out with the decoding lock held.
     MutexAutoUnlock unlock(mDecodingMutex);
 
     // Then, tell the observers what happened in the decoder.
     // If we have no request, we have not yet created a decoder, but we still
--- a/image/src/VectorImage.cpp
+++ b/image/src/VectorImage.cpp
@@ -384,20 +384,22 @@ VectorImage::OnImageDataComplete(nsIRequ
   nsresult finalStatus = OnStopRequest(aRequest, aContext, aStatus);
 
   // Give precedence to Necko failure codes.
   if (NS_FAILED(aStatus))
     finalStatus = aStatus;
 
   // Actually fire OnStopRequest.
   if (mStatusTracker) {
+    // XXX(seth): Is this seriously the least insane way to do this?
     nsRefPtr<imgStatusTracker> clone = mStatusTracker->CloneForRecording();
     imgDecoderObserver* observer = clone->GetDecoderObserver();
     observer->OnStopRequest(aLastPart, finalStatus);
-    imgStatusTracker::StatusDiff diff = mStatusTracker->CalculateAndApplyDifference(clone);
+    ImageStatusDiff diff = mStatusTracker->Difference(clone);
+    mStatusTracker->ApplyDifference(diff);
     mStatusTracker->SyncNotifyDifference(diff);
   }
   return finalStatus;
 }
 
 nsresult
 VectorImage::OnImageDataAvailable(nsIRequest* aRequest,
                                   nsISupports* aContext,
@@ -848,17 +850,18 @@ VectorImage::OnStartRequest(nsIRequest* 
 
   // Sending StartDecode will block page load until the document's ready.  (We
   // unblock it by sending StopDecode in OnSVGDocumentLoaded or
   // OnSVGDocumentError.)
   if (mStatusTracker) {
     nsRefPtr<imgStatusTracker> clone = mStatusTracker->CloneForRecording();
     imgDecoderObserver* observer = clone->GetDecoderObserver();
     observer->OnStartDecode();
-    imgStatusTracker::StatusDiff diff = mStatusTracker->CalculateAndApplyDifference(clone);
+    ImageStatusDiff diff = mStatusTracker->Difference(clone);
+    mStatusTracker->ApplyDifference(diff);
     mStatusTracker->SyncNotifyDifference(diff);
   }
 
   // Create a listener to wait until the SVG document is fully loaded, which
   // will signal that this image is ready to render. Certain error conditions
   // will prevent us from ever getting this notification, so we also create a
   // listener that waits for parsing to complete and cancels the
   // SVGLoadEventListener if needed. The listeners are automatically attached
@@ -936,17 +939,18 @@ VectorImage::OnSVGDocumentLoaded()
     nsRefPtr<imgStatusTracker> clone = mStatusTracker->CloneForRecording();
     imgDecoderObserver* observer = clone->GetDecoderObserver();
 
     observer->OnStartContainer(); // Signal that width/height are available.
     observer->FrameChanged(&nsIntRect::GetMaxSizedIntRect());
     observer->OnStopFrame();
     observer->OnStopDecode(NS_OK); // Unblock page load.
 
-    imgStatusTracker::StatusDiff diff = mStatusTracker->CalculateAndApplyDifference(clone);
+    ImageStatusDiff diff = mStatusTracker->Difference(clone);
+    mStatusTracker->ApplyDifference(diff);
     mStatusTracker->SyncNotifyDifference(diff);
   }
 
   EvaluateAnimation();
 }
 
 void
 VectorImage::OnSVGDocumentError()
@@ -959,17 +963,18 @@ VectorImage::OnSVGDocumentError()
   mError = true;
 
   if (mStatusTracker) {
     nsRefPtr<imgStatusTracker> clone = mStatusTracker->CloneForRecording();
     imgDecoderObserver* observer = clone->GetDecoderObserver();
 
     // Unblock page load.
     observer->OnStopDecode(NS_ERROR_FAILURE);
-    imgStatusTracker::StatusDiff diff = mStatusTracker->CalculateAndApplyDifference(clone);
+    ImageStatusDiff diff = mStatusTracker->Difference(clone);
+    mStatusTracker->ApplyDifference(diff);
     mStatusTracker->SyncNotifyDifference(diff);
   }
 }
 
 //------------------------------------------------------------------------------
 // nsIStreamListener method
 
 //******************************************************************************
--- a/image/src/imgStatusTracker.cpp
+++ b/image/src/imgStatusTracker.cpp
@@ -514,91 +514,105 @@ imgStatusTracker::SyncNotifyState(nsTObs
     NOTIFY_IMAGE_OBSERVERS(OnStopDecode());
   }
 
   if (state & stateRequestStopped) {
     NOTIFY_IMAGE_OBSERVERS(OnStopRequest(hadLastPart));
   }
 }
 
-imgStatusTracker::StatusDiff
-imgStatusTracker::CalculateAndApplyDifference(imgStatusTracker* other)
+ImageStatusDiff
+imgStatusTracker::Difference(imgStatusTracker* aOther) const
 {
-  LOG_SCOPE(GetImgLog(), "imgStatusTracker::SyncAndCalculateDifference");
-
-  // We must not modify or notify for the start-load state, which happens from Necko callbacks.
-  uint32_t loadState = mState & stateRequestStarted;
-
-  StatusDiff diff;
-  diff.mDiffState = ~mState & other->mState & ~stateRequestStarted;
-  diff.mUnblockedOnload = mState & stateBlockingOnload && !(other->mState & stateBlockingOnload);
-  diff.mFoundError = (mImageStatus != imgIRequest::STATUS_ERROR) && (other->mImageStatus == imgIRequest::STATUS_ERROR);
-
-  // Now that we've calculated the difference in state, synchronize our state
-  // with the other tracker.
+  ImageStatusDiff diff;
+  diff.diffState = ~mState & aOther->mState & ~stateRequestStarted;
+  diff.diffImageStatus = ~mImageStatus & aOther->mImageStatus;
+  diff.unblockedOnload = mState & stateBlockingOnload && !(aOther->mState & stateBlockingOnload);
+  diff.unsetDecodeStarted = mImageStatus & imgIRequest::STATUS_DECODE_STARTED
+                         && !(aOther->mImageStatus & imgIRequest::STATUS_DECODE_STARTED);
+  diff.foundError = (mImageStatus != imgIRequest::STATUS_ERROR)
+                 && (aOther->mImageStatus == imgIRequest::STATUS_ERROR);
 
-  // First, actually synchronize our state.
-  mState |= diff.mDiffState | loadState;
-  if (diff.mUnblockedOnload) {
-    mState &= ~stateBlockingOnload;
-  }
+  MOZ_ASSERT(!mIsMultipart || aOther->mIsMultipart, "mIsMultipart should be monotonic");
+  diff.foundIsMultipart = !mIsMultipart && aOther->mIsMultipart;
+  MOZ_ASSERT(!mHadLastPart || aOther->mHadLastPart, "mHadLastPart should be monotonic");
+  diff.foundLastPart = !mHadLastPart && aOther->mHadLastPart;
 
-  mIsMultipart = other->mIsMultipart;
-  mHadLastPart = other->mHadLastPart;
-  mImageStatus |= other->mImageStatus;
-  mHasBeenDecoded = mHasBeenDecoded || other->mHasBeenDecoded;
-
-  // The error state is sticky and overrides all other bits.
-  if (mImageStatus & imgIRequest::STATUS_ERROR) {
-    mImageStatus = imgIRequest::STATUS_ERROR;
-  } else {
-    // Unset the bits that can get unset as part of the decoding process.
-    if (!(other->mImageStatus & imgIRequest::STATUS_DECODE_STARTED)) {
-      mImageStatus &= ~imgIRequest::STATUS_DECODE_STARTED;
-    }
-  }
+  diff.gotDecoded = !mHasBeenDecoded && aOther->mHasBeenDecoded;
 
   // Only record partial invalidations if we haven't been decoded before.
   // When images are re-decoded after discarding, we don't want to display
   // partially decoded versions to the user.
-  bool doInvalidations  = !mHasBeenDecoded
-                       || mImageStatus & imgIRequest::STATUS_ERROR
-                       || mImageStatus & imgIRequest::STATUS_DECODE_COMPLETE;
+  const uint32_t combinedStatus = mImageStatus | aOther->mImageStatus;
+  const bool doInvalidations  = !(mHasBeenDecoded || aOther->mHasBeenDecoded)
+                             || combinedStatus & imgIRequest::STATUS_ERROR
+                             || combinedStatus & imgIRequest::STATUS_DECODE_COMPLETE;
 
-  // Record the invalid rectangles and reset them for another go.
+  // Record and reset the invalid rectangle.
+  // XXX(seth): We shouldn't be resetting anything here; see bug 910441.
   if (doInvalidations) {
-    diff.mInvalidRect = mInvalidRect.Union(other->mInvalidRect);
-    other->mInvalidRect.SetEmpty();
-    mInvalidRect.SetEmpty();
+    diff.invalidRect = aOther->mInvalidRect;
+    aOther->mInvalidRect.SetEmpty();
   }
 
   return diff;
 }
 
 void
-imgStatusTracker::SyncNotifyDifference(imgStatusTracker::StatusDiff diff)
+imgStatusTracker::ApplyDifference(const ImageStatusDiff& aDiff)
+{
+  LOG_SCOPE(GetImgLog(), "imgStatusTracker::ApplyDifference");
+
+  // We must not modify or notify for the start-load state, which happens from Necko callbacks.
+  uint32_t loadState = mState & stateRequestStarted;
+
+  // Synchronize our state.
+  mState |= aDiff.diffState | loadState;
+  if (aDiff.unblockedOnload)
+    mState &= ~stateBlockingOnload;
+
+  mIsMultipart = mIsMultipart || aDiff.foundIsMultipart;
+  mHadLastPart = mHadLastPart || aDiff.foundLastPart;
+  mHasBeenDecoded = mHasBeenDecoded || aDiff.gotDecoded;
+
+  // Update the image status. There are some subtle points which are handled below.
+  mImageStatus |= aDiff.diffImageStatus;
+
+  // Unset bits which can get unset as part of the decoding process.
+  if (aDiff.unsetDecodeStarted)
+    mImageStatus &= ~imgIRequest::STATUS_DECODE_STARTED;
+
+  // The error state is sticky and overrides all other bits.
+  if (mImageStatus & imgIRequest::STATUS_ERROR)
+    mImageStatus = imgIRequest::STATUS_ERROR;
+}
+
+void
+imgStatusTracker::SyncNotifyDifference(const ImageStatusDiff& diff)
 {
   LOG_SCOPE(GetImgLog(), "imgStatusTracker::SyncNotifyDifference");
 
-  SyncNotifyState(mConsumers, !!mImage, diff.mDiffState, diff.mInvalidRect, mHadLastPart);
+  nsIntRect invalidRect = mInvalidRect.Union(diff.invalidRect);
+  mInvalidRect.SetEmpty();
+  SyncNotifyState(mConsumers, !!mImage, diff.diffState, invalidRect, mHadLastPart);
 
-  if (diff.mUnblockedOnload) {
+  if (diff.unblockedOnload) {
     nsTObserverArray<imgRequestProxy*>::ForwardIterator iter(mConsumers);
     while (iter.HasMore()) {
       // Hold on to a reference to this proxy, since notifying the state can
       // cause it to disappear.
       nsRefPtr<imgRequestProxy> proxy = iter.GetNext();
 
       if (!proxy->NotificationsDeferred()) {
         SendUnblockOnload(proxy);
       }
     }
   }
 
-  if (diff.mFoundError) {
+  if (diff.foundError) {
     FireFailureNotification();
   }
 }
 
 imgStatusTracker*
 imgStatusTracker::CloneForRecording()
 {
   imgStatusTracker* clone = new imgStatusTracker(*this);
--- a/image/src/imgStatusTracker.h
+++ b/image/src/imgStatusTracker.h
@@ -9,42 +9,97 @@
 
 class imgIContainer;
 class imgRequestProxy;
 class imgStatusNotifyRunnable;
 class imgRequestNotifyRunnable;
 class imgStatusTrackerObserver;
 class imgStatusTrackerNotifyingObserver;
 struct nsIntRect;
-namespace mozilla {
-namespace image {
-class Image;
-} // namespace image
-} // namespace mozilla
-
 
 #include "mozilla/RefPtr.h"
 #include "nsCOMPtr.h"
 #include "nsTObserverArray.h"
 #include "nsIRunnable.h"
 #include "nscore.h"
 #include "imgDecoderObserver.h"
 #include "nsISupportsImpl.h"
 
+namespace mozilla {
+namespace image {
+
+class Image;
+
+struct ImageStatusDiff
+{
+  ImageStatusDiff()
+    : invalidRect()
+    , diffState(0)
+    , diffImageStatus(0)
+    , unblockedOnload(false)
+    , unsetDecodeStarted(false)
+    , foundError(false)
+    , foundIsMultipart(false)
+    , foundLastPart(false)
+    , gotDecoded(false)
+  { }
+
+  static ImageStatusDiff NoChange() { return ImageStatusDiff(); }
+  bool IsNoChange() const { return *this == NoChange(); }
+
+  bool operator!=(const ImageStatusDiff& aOther) const { return !(*this == aOther); }
+  bool operator==(const ImageStatusDiff& aOther) const {
+    return aOther.invalidRect == invalidRect
+        && aOther.diffState == diffState
+        && aOther.diffImageStatus == diffImageStatus
+        && aOther.unblockedOnload == unblockedOnload
+        && aOther.unsetDecodeStarted == unsetDecodeStarted
+        && aOther.foundError == foundError
+        && aOther.foundIsMultipart == foundIsMultipart
+        && aOther.foundLastPart == foundLastPart
+        && aOther.gotDecoded == gotDecoded;
+  }
+
+  void Combine(const ImageStatusDiff& aOther) {
+    invalidRect = invalidRect.Union(aOther.invalidRect);
+    diffState |= aOther.diffState;
+    diffImageStatus |= aOther.diffImageStatus;
+    unblockedOnload = unblockedOnload || aOther.unblockedOnload;
+    unsetDecodeStarted = unsetDecodeStarted || aOther.unsetDecodeStarted;
+    foundError = foundError || aOther.foundError;
+    foundIsMultipart = foundIsMultipart || aOther.foundIsMultipart;
+    foundLastPart = foundLastPart || aOther.foundLastPart;
+    gotDecoded = gotDecoded || aOther.gotDecoded;
+  }
+
+  nsIntRect invalidRect;
+  uint32_t  diffState;
+  uint32_t  diffImageStatus;
+  bool      unblockedOnload    : 1;
+  bool      unsetDecodeStarted : 1;
+  bool      foundError         : 1;
+  bool      foundIsMultipart   : 1;
+  bool      foundLastPart      : 1;
+  bool      gotDecoded         : 1;
+};
+
 enum {
   stateRequestStarted    = 1u << 0,
   stateHasSize           = 1u << 1,
   stateDecodeStarted     = 1u << 2,
   stateDecodeStopped     = 1u << 3,
   stateFrameStopped      = 1u << 4,
   stateRequestStopped    = 1u << 5,
   stateBlockingOnload    = 1u << 6,
   stateImageIsAnimated   = 1u << 7
 };
 
+} // namespace image
+} // namespace mozilla
+
 /*
  * The image status tracker is a class that encapsulates all the loading and
  * decoding status about an Image, and makes it possible to send notifications
  * to imgRequestProxys, both synchronously (i.e., the status now) and
  * asynchronously (the status later).
  *
  * When a new proxy needs to be notified of the current state of an image, call
  * the Notify() method on this class with the relevant proxy as its argument,
@@ -191,31 +246,25 @@ public:
 
   // Weak pointer getters - no AddRefs.
   inline mozilla::image::Image* GetImage() const { return mImage; }
 
   inline imgDecoderObserver* GetDecoderObserver() { return mTrackerObserver.get(); }
 
   imgStatusTracker* CloneForRecording();
 
-  struct StatusDiff
-  {
-    uint32_t mDiffState;
-    bool mUnblockedOnload;
-    bool mFoundError;
-    nsIntRect mInvalidRect;
-  };
+  // Compute the difference between this status tracker and aOther.
+  mozilla::image::ImageStatusDiff Difference(imgStatusTracker* aOther) const;
 
-  // Calculate the difference between this and other, apply that difference to
-  // ourselves, and return it for passing to SyncNotifyDifference.
-  StatusDiff CalculateAndApplyDifference(imgStatusTracker* other);
+  // Update our state to incorporate the changes in aDiff.
+  void ApplyDifference(const mozilla::image::ImageStatusDiff& aDiff);
 
-  // Notify for the difference found in CalculateAndApplyDifference. No
-  // decoding locks may be held.
-  void SyncNotifyDifference(StatusDiff diff);
+  // Notify for the changes captured in an ImageStatusDiff. Because this may
+  // result in recursive notifications, no decoding locks may be held.
+  void SyncNotifyDifference(const mozilla::image::ImageStatusDiff& aDiff);
 
   nsIntRect GetInvalidRect() const { return mInvalidRect; }
 
 private:
   friend class imgStatusNotifyRunnable;
   friend class imgRequestNotifyRunnable;
   friend class imgStatusTrackerObserver;
   friend class imgStatusTrackerNotifyingObserver;