Bug 1079653 (Part 3) - Make decoders track whether they need to flush data after getting a new frame. r=tn
authorSeth Fowler <seth@mozilla.com>
Tue, 18 Nov 2014 12:06:27 -0800
changeset 240640 a0af85989c9cfaedd3250bb9d9c438c2b05e6d92
parent 240639 dfdcfd8f603844256f9470fd8ff467bff9f56f30
child 240641 dff90d1d4b3c435b559cc6a6320690d16a44fe5a
push id4311
push userraliiev@mozilla.com
push dateMon, 12 Jan 2015 19:37:41 +0000
treeherdermozilla-beta@150c9fed433b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstn
bugs1079653
milestone36.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 1079653 (Part 3) - Make decoders track whether they need to flush data after getting a new frame. r=tn
image/decoders/nsJPEGDecoder.cpp
image/src/Decoder.cpp
image/src/Decoder.h
image/src/RasterImage.cpp
image/src/RasterImage.h
--- a/image/decoders/nsJPEGDecoder.cpp
+++ b/image/decoders/nsJPEGDecoder.cpp
@@ -176,30 +176,21 @@ nsJPEGDecoder::InitInternal()
   // Record app markers for ICC data
   for (uint32_t m = 0; m < 16; m++)
     jpeg_save_markers(&mInfo, JPEG_APP0 + m, 0xFFFF);
 }
 
 void
 nsJPEGDecoder::FinishInternal()
 {
-  // If we're not in any sort of error case, flush the decoder.
-  //
-  // XXXbholley - It seems wrong that this should be necessary, but at the
-  // moment I'm just folding the contents of Flush() into Close() so that
-  // we can get rid of it.
-  //
-  // XXX(seth): It'd be great to get rid of this. For now, we treat this as a
-  // write to a synchronous decoder, which means that this must be called only
-  // on the main thread. (That's asserted in Decoder::Finish and
-  // Decoder::FinishSharedDecoder.)
+  // If we're not in any sort of error case, force our state to JPEG_DONE.
   if ((mState != JPEG_DONE && mState != JPEG_SINK_NON_JPEG_TRAILER) &&
       (mState != JPEG_ERROR) &&
       !IsSizeDecode()) {
-    this->Write(nullptr, 0, DECODE_SYNC);
+    mState = JPEG_DONE;
   }
 }
 
 void
 nsJPEGDecoder::WriteInternal(const char* aBuffer, uint32_t aCount,
                              DecodeStrategy)
 {
   mSegment = (const JOCTET*)aBuffer;
--- a/image/src/Decoder.cpp
+++ b/image/src/Decoder.cpp
@@ -23,16 +23,17 @@ Decoder::Decoder(RasterImage &aImage)
   , mChunkCount(0)
   , mDecodeFlags(0)
   , mBytesDecoded(0)
   , mDecodeDone(false)
   , mDataError(false)
   , mFrameCount(0)
   , mFailCode(NS_OK)
   , mNeedsNewFrame(false)
+  , mNeedsToFlushData(false)
   , mInitialized(false)
   , mSizeDecode(false)
   , mInFrame(false)
   , mIsAnimated(false)
 { }
 
 Decoder::~Decoder()
 {
@@ -103,16 +104,22 @@ Decoder::Write(const char* aBuffer, uint
 
   // Begin recording telemetry data.
   TimeStamp start = TimeStamp::Now();
   mChunkCount++;
 
   // Keep track of the total number of bytes written.
   mBytesDecoded += aCount;
 
+  // If we're flushing data, clear the flag.
+  if (aBuffer == nullptr && aCount == 0) {
+    MOZ_ASSERT(mNeedsToFlushData, "Flushing when we don't need to");
+    mNeedsToFlushData = false;
+  }
+
   // If a data error occured, just ignore future data.
   if (HasDataError())
     return;
 
   if (IsSizeDecode() && HasSize()) {
     // More data came in since we found the size. We have nothing to do here.
     return;
   }
@@ -249,16 +256,22 @@ Decoder::AllocateFrame()
   } else if (NS_FAILED(rv)) {
     PostDataError();
   }
 
   // Mark ourselves as not needing another frame before talking to anyone else
   // so they can tell us if they need yet another.
   mNeedsNewFrame = false;
 
+  // If we've received any data at all, we may have pending data that needs to
+  // be flushed now that we have a frame to decode into.
+  if (mBytesDecoded > 0) {
+    mNeedsToFlushData = true;
+  }
+
   return rv;
 }
 
 void
 Decoder::SetSizeOnImage()
 {
   MOZ_ASSERT(mImageMetadata.HasSize(), "Should have size");
   MOZ_ASSERT(mImageMetadata.HasOrientation(), "Should have orientation");
--- a/image/src/Decoder.h
+++ b/image/src/Decoder.h
@@ -38,16 +38,21 @@ public:
    */
   void InitSharedDecoder(uint8_t* imageData, uint32_t imageDataLength,
                          uint32_t* colormap, uint32_t colormapSize,
                          imgFrame* currentFrame);
 
   /**
    * Writes data to the decoder.
    *
+   * If aBuffer is null and aCount is 0, Write() flushes any buffered data to
+   * the decoder. Data is buffered if the decoder wasn't able to completely
+   * decode it because it needed a new frame.  If it's necessary to flush data,
+   * NeedsToFlushData() will return true.
+   *
    * @param aBuffer buffer containing the data to be written
    * @param aCount the number of bytes to write
    *
    * Any errors are reported by setting the appropriate state on the decoder.
    *
    * Notifications Sent: TODO
    */
   void Write(const char* aBuffer, uint32_t aCount, DecodeStrategy aStrategy);
@@ -167,16 +172,21 @@ public:
   // will be called again with nullptr and 0 as arguments.
   void NeedNewFrame(uint32_t frameNum, uint32_t x_offset, uint32_t y_offset,
                     uint32_t width, uint32_t height,
                     gfx::SurfaceFormat format,
                     uint8_t palette_depth = 0);
 
   virtual bool NeedsNewFrame() const { return mNeedsNewFrame; }
 
+  // Returns true if we may have stored data that we need to flush now that we
+  // have a new frame to decode into. Callers can use Write() to actually
+  // flush the data; see the documentation for that method.
+  bool NeedsToFlushData() const { return mNeedsToFlushData; }
+
   // Try to allocate a frame as described in mNewFrameData and return the
   // status code from that attempt. Clears mNewFrameData.
   virtual nsresult AllocateFrame();
 
   already_AddRefed<imgFrame> GetCurrentFrame() const
   {
     nsRefPtr<imgFrame> frame = mCurrentFrame;
     return frame.forget();
@@ -289,16 +299,17 @@ private:
     uint32_t mOffsetY;
     uint32_t mWidth;
     uint32_t mHeight;
     gfx::SurfaceFormat mFormat;
     uint8_t mPaletteDepth;
   };
   NewFrameData mNewFrameData;
   bool mNeedsNewFrame;
+  bool mNeedsToFlushData;
   bool mInitialized;
   bool mSizeDecode;
   bool mInFrame;
   bool mIsAnimated;
 };
 
 } // namespace image
 } // namespace mozilla
--- a/image/src/RasterImage.cpp
+++ b/image/src/RasterImage.cpp
@@ -2325,21 +2325,19 @@ RasterImage::SyncDecode()
       // with the new flags. If we can't discard then there isn't
       // anything we can do.
       if (!CanForciblyDiscardAndRedecode())
         return NS_ERROR_NOT_AVAILABLE;
       ForceDiscard();
     }
   }
 
-  // If we're currently waiting on a new frame for this image, we have to create
-  // it now.
+  // If we're currently waiting on a new frame for this image, create it now.
   if (mDecoder && mDecoder->NeedsNewFrame()) {
     mDecoder->AllocateFrame();
-    mDecodeRequest->mAllocatedNewFrame = true;
   }
 
   // If we don't have a decoder, create one
   if (!mDecoder) {
     rv = InitDecoder(/* aDoSizeDecode = */ false);
     CONTAINER_ENSURE_SUCCESS(rv);
   }
 
@@ -2695,18 +2693,17 @@ RasterImage::DecodeSomeData(size_t aMaxB
 {
   MOZ_ASSERT(mDecoder, "Should have a decoder");
 
   mDecodingMonitor.AssertCurrentThreadIn();
 
   // First, if we've just been called because we allocated a frame on the main
   // thread, let the decoder deal with the data it set aside at that time by
   // passing it a null buffer.
-  if (mDecodeRequest->mAllocatedNewFrame) {
-    mDecodeRequest->mAllocatedNewFrame = false;
+  if (mDecoder->NeedsToFlushData()) {
     nsresult rv = WriteToDecoder(nullptr, 0, aStrategy);
     if (NS_FAILED(rv) || mDecoder->NeedsNewFrame()) {
       return rv;
     }
   }
 
   // If we have nothing else to decode, return.
   if (mDecoder->BytesDecoded() == mSourceData.Length()) {
@@ -2742,18 +2739,17 @@ RasterImage::IsDecodeFinished()
     }
   } else if (mDecoder->GetDecodeDone()) {
     return true;
   }
 
   // If the decoder returned because it needed a new frame and we haven't
   // written to it since then, the decoder may be storing data that it hasn't
   // decoded yet.
-  if (mDecoder->NeedsNewFrame() ||
-      (mDecodeRequest && mDecodeRequest->mAllocatedNewFrame)) {
+  if (mDecoder->NeedsNewFrame() || mDecoder->NeedsToFlushData()) {
     return false;
   }
 
   // Otherwise, if we have all the source data and wrote all the source data,
   // we're done.
   //
   // (NB - This can be the case even for non-erroneous images because
   // Decoder::GetDecodeDone() might not return true until after we call
@@ -3350,19 +3346,17 @@ RasterImage::DecodePool::DecodeSomeOfIma
   // example, a synchronous decode request came while the worker was pending).
   if (!aImg->mDecoder || aImg->mDecoded)
     return NS_OK;
 
   // If we're doing synchronous decodes, and we're waiting on a new frame for
   // this image, get it now.
   if (aStrategy == DECODE_SYNC && aImg->mDecoder->NeedsNewFrame()) {
     MOZ_ASSERT(NS_IsMainThread());
-
     aImg->mDecoder->AllocateFrame();
-    aImg->mDecodeRequest->mAllocatedNewFrame = true;
   }
 
   // If we're not synchronous, we can't allocate a frame right now.
   else if (aImg->mDecoder->NeedsNewFrame()) {
     return NS_OK;
   }
 
   nsRefPtr<Decoder> decoderKungFuDeathGrip = aImg->mDecoder;
@@ -3393,17 +3387,17 @@ RasterImage::DecodePool::DecodeSomeOfIma
   //  * we run out of time.
   // We also try to decode at least one "chunk" if we've allocated a new frame,
   // even if we have no more data to send to the decoder.
   while ((aImg->mSourceData.Length() > aImg->mDecoder->BytesDecoded() &&
           bytesToDecode > 0 &&
           !aImg->IsDecodeFinished() &&
           !(aDecodeType == DECODE_TYPE_UNTIL_SIZE && aImg->mHasSize) &&
           !aImg->mDecoder->NeedsNewFrame()) ||
-         (aImg->mDecodeRequest && aImg->mDecodeRequest->mAllocatedNewFrame)) {
+         aImg->mDecoder->NeedsToFlushData()) {
     uint32_t chunkSize = std::min(bytesToDecode, maxBytes);
     nsresult rv = aImg->DecodeSomeData(chunkSize, aStrategy);
     if (NS_FAILED(rv)) {
       aImg->DoError();
       return rv;
     }
 
     bytesToDecode -= chunkSize;
@@ -3459,17 +3453,16 @@ RasterImage::FrameNeededWorker::Run()
 {
   ReentrantMonitorAutoEnter lock(mImage->mDecodingMonitor);
   nsresult rv = NS_OK;
 
   // If we got a synchronous decode in the mean time, we don't need to do
   // anything.
   if (mImage->mDecoder && mImage->mDecoder->NeedsNewFrame()) {
     rv = mImage->mDecoder->AllocateFrame();
-    mImage->mDecodeRequest->mAllocatedNewFrame = true;
   }
 
   if (NS_SUCCEEDED(rv) && mImage->mDecoder) {
     // By definition, we're not done decoding, so enqueue us for more decoding.
     DecodePool::Singleton()->RequestDecode(mImage);
   }
 
   return NS_OK;
--- a/image/src/RasterImage.h
+++ b/image/src/RasterImage.h
@@ -315,36 +315,31 @@ private:
    * Each RasterImage has a pointer to one or zero heap-allocated
    * DecodeRequests.
    */
   struct DecodeRequest
   {
     explicit DecodeRequest(RasterImage* aImage)
       : mImage(aImage)
       , mRequestStatus(REQUEST_INACTIVE)
-      , mAllocatedNewFrame(false)
     { }
 
     NS_INLINE_DECL_THREADSAFE_REFCOUNTING(DecodeRequest)
 
     RasterImage* mImage;
 
     enum DecodeRequestStatus
     {
       REQUEST_INACTIVE,
       REQUEST_PENDING,
       REQUEST_ACTIVE,
       REQUEST_WORK_DONE,
       REQUEST_STOPPED
     } mRequestStatus;
 
-    /* True if a new frame has been allocated, but DecodeSomeData hasn't yet
-     * been called to flush data to it */
-    bool mAllocatedNewFrame;
-
   private:
     ~DecodeRequest() {}
   };
 
   /*
    * DecodePool is a singleton class we use when decoding large images.
    *
    * When we wish to decode an image larger than