Bug 716140 - Don't call Set* methods directly on images from decoders; call them from the Decoder base class. r=seth

--HG--
extra : rebase_source : 9f7ec3cb8b8393053abbc872d706a6304a157d55

image/decoders/nsGIFDecoder2.cpp

@@ -112,11 +112,9 @@ nsGIFDecoder2::FinishInternal()
   if (!IsSizeDecode() && mGIFOpen) {
     if (mCurrentFrame == mGIFStruct.images_decoded)
       EndImageFrame();
-    PostDecodeDone();
+    PostDecodeDone(mGIFStruct.loop_count - 1);
     mGIFOpen = false;
   }
-
-  mImage.SetLoopCount(mGIFStruct.loop_count - 1);
 }
 
 // Push any new rows according to mCurrentPass/mLastFlushedPass and
@@ -208,9 +206,6 @@ nsresult nsGIFDecoder2::BeginImageFrame(uint16_t aDepth)
 
   memset(mImageData, 0, imageDataLength);
 
-  mImage.SetFrameDisposalMethod(mGIFStruct.images_decoded,
-                                mGIFStruct.disposal_method);
-
   // Tell the superclass we're starting a frame
   PostFrameStart();
 
@@ -232,6 +227,8 @@ nsresult nsGIFDecoder2::BeginImageFrame(uint16_t aDepth)
 //******************************************************************************
 void nsGIFDecoder2::EndImageFrame()
 {
+  RasterImage::FrameAlpha alpha = RasterImage::kFrameHasAlpha;
+
   // First flush all pending image data 
   if (!mGIFStruct.images_decoded) {
     // Only need to flush first frame
@@ -249,7 +246,7 @@ void nsGIFDecoder2::EndImageFrame()
     }
     // This transparency check is only valid for first frame
     if (mGIFStruct.is_transparent && !mSawTransparency) {
-      mImage.SetFrameHasNoAlpha(mGIFStruct.images_decoded);
+      alpha = RasterImage::kFrameOpaque;
     }
   }
   mCurrentRow = mLastFlushedRow = -1;
@@ -262,12 +259,6 @@ void nsGIFDecoder2::EndImageFrame()
       uint8_t *rowp = mImageData + ((mGIFStruct.height - mGIFStruct.rows_remaining) * mGIFStruct.width);
       memset(rowp, 0, mGIFStruct.rows_remaining * mGIFStruct.width);
     }
-
-    // We actually have the timeout information before we get the lzw encoded 
-    // image data, at least according to the spec, but we delay in setting the 
-    // timeout for the image until here to help ensure that we have the whole 
-    // image frame decoded before we go off and try to display another frame.
-    mImage.SetFrameTimeout(mGIFStruct.images_decoded, mGIFStruct.delay_time);
   }
 
   // Unconditionally increment images_decoded, because we unconditionally
@@ -277,7 +268,9 @@ void nsGIFDecoder2::EndImageFrame()
   mGIFStruct.images_decoded++;
 
   // Tell the superclass we finished a frame
-  PostFrameStop();
+  PostFrameStop(alpha,
+                RasterImage::FrameDisposalMethod(mGIFStruct.disposal_method),
+                mGIFStruct.delay_time);
 
   // Reset the transparent pixel
   if (mOldColor) {

image/decoders/nsPNGDecoder.cpp

@@ -54,6 +54,61 @@ GetPNGDecoderAccountingLog()
 #define HEIGHT_OFFSET (WIDTH_OFFSET + 4)
 #define BYTES_NEEDED_FOR_DIMENSIONS (HEIGHT_OFFSET + 4)
 
+struct AnimFrameInfo
+{
+  AnimFrameInfo()
+   : mDispose(RasterImage::kDisposeKeep)
+   , mBlend(RasterImage::kBlendOver)
+   , mTimeout(0)
+  {}
+
+#ifdef PNG_APNG_SUPPORTED
+  AnimFrameInfo(png_structp aPNG, png_infop aInfo)
+   : mDispose(RasterImage::kDisposeKeep)
+   , mBlend(RasterImage::kBlendOver)
+   , mTimeout(0)
+  {
+    png_uint_16 delay_num, delay_den;
+    /* delay, in seconds is delay_num/delay_den */
+    png_byte dispose_op;
+    png_byte blend_op;
+    delay_num = png_get_next_frame_delay_num(aPNG, aInfo);
+    delay_den = png_get_next_frame_delay_den(aPNG, aInfo);
+    dispose_op = png_get_next_frame_dispose_op(aPNG, aInfo);
+    blend_op = png_get_next_frame_blend_op(aPNG, aInfo);
+
+    if (delay_num == 0) {
+      mTimeout = 0; // SetFrameTimeout() will set to a minimum
+    } else {
+      if (delay_den == 0)
+        delay_den = 100; // so says the APNG spec
+
+      // Need to cast delay_num to float to have a proper division and
+      // the result to int to avoid compiler warning
+      mTimeout = static_cast<int32_t>(static_cast<double>(delay_num) * 1000 / delay_den);
+    }
+
+    if (dispose_op == PNG_DISPOSE_OP_PREVIOUS) {
+      mDispose = RasterImage::kDisposeRestorePrevious;
+    } else if (dispose_op == PNG_DISPOSE_OP_BACKGROUND) {
+      mDispose = RasterImage::kDisposeClear;
+    } else {
+      mDispose = RasterImage::kDisposeKeep;
+    }
+
+    if (blend_op == PNG_BLEND_OP_SOURCE) {
+      mBlend = RasterImage::kBlendSource;
+    } else {
+      mBlend = RasterImage::kBlendOver;
+    }
+  }
+#endif
+
+  RasterImage::FrameDisposalMethod mDispose;
+  RasterImage::FrameBlendMethod mBlend;
+  int32_t mTimeout;
+};
+
 // First 8 bytes of a PNG file
 const uint8_t 
 nsPNGDecoder::pngSignatureBytes[] = { 137, 80, 78, 71, 13, 10, 26, 10 };
@@ -108,11 +163,6 @@ void nsPNGDecoder::CreateFrame(png_uint_32 x_offset, png_uint_32 y_offset,
   // Tell the superclass we're starting a frame
   PostFrameStart();
 
-#ifdef PNG_APNG_SUPPORTED
-  if (png_get_valid(mPNG, mInfo, PNG_INFO_acTL))
-    SetAnimFrameInfo();
-#endif
-
   PR_LOG(GetPNGDecoderAccountingLog(), PR_LOG_DEBUG,
          ("PNGDecoderAccounting: nsPNGDecoder::CreateFrame -- created "
           "image frame with %dx%d pixels in container %p",
@@ -122,78 +172,34 @@ void nsPNGDecoder::CreateFrame(png_uint_32 x_offset, png_uint_32 y_offset,
   mFrameHasNoAlpha = true;
 }
 
-#ifdef PNG_APNG_SUPPORTED
-// set timeout and frame disposal method for the current frame
-void nsPNGDecoder::SetAnimFrameInfo()
-{
-  png_uint_16 delay_num, delay_den;
-  /* delay, in seconds is delay_num/delay_den */
-  png_byte dispose_op;
-  png_byte blend_op;
-  int32_t timeout; /* in milliseconds */
-
-  delay_num = png_get_next_frame_delay_num(mPNG, mInfo);
-  delay_den = png_get_next_frame_delay_den(mPNG, mInfo);
-  dispose_op = png_get_next_frame_dispose_op(mPNG, mInfo);
-  blend_op = png_get_next_frame_blend_op(mPNG, mInfo);
-
-  if (delay_num == 0) {
-    timeout = 0; // SetFrameTimeout() will set to a minimum
-  } else {
-    if (delay_den == 0)
-      delay_den = 100; // so says the APNG spec
-
-    // Need to cast delay_num to float to have a proper division and
-    // the result to int to avoid compiler warning
-    timeout = static_cast<int32_t>
-              (static_cast<double>(delay_num) * 1000 / delay_den);
-  }
-
-  uint32_t numFrames = GetFrameCount();
-
-  mImage.SetFrameTimeout(numFrames - 1, timeout);
-
-  if (dispose_op == PNG_DISPOSE_OP_PREVIOUS)
-      mImage.SetFrameDisposalMethod(numFrames - 1,
-                                    RasterImage::kDisposeRestorePrevious);
-  else if (dispose_op == PNG_DISPOSE_OP_BACKGROUND)
-      mImage.SetFrameDisposalMethod(numFrames - 1,
-                                    RasterImage::kDisposeClear);
-  else
-      mImage.SetFrameDisposalMethod(numFrames - 1,
-                                    RasterImage::kDisposeKeep);
-
-  if (blend_op == PNG_BLEND_OP_SOURCE)
-      mImage.SetFrameBlendMethod(numFrames - 1, RasterImage::kBlendSource);
-  /*else // 'over' is the default
-      mImage.SetFrameBlendMethod(numFrames - 1, RasterImage::kBlendOver); */
-}
-#endif
-
 // set timeout and frame disposal method for the current frame
 void nsPNGDecoder::EndImageFrame()
 {
   if (mFrameIsHidden)
     return;
 
-  uint32_t numFrames = 1;
+  RasterImage::FrameAlpha alpha;
+  if (mFrameHasNoAlpha)
+    alpha = RasterImage::kFrameOpaque;
+  else
+    alpha = RasterImage::kFrameHasAlpha;
+
+  AnimFrameInfo animInfo;
+
 #ifdef PNG_APNG_SUPPORTED
-  numFrames = GetFrameCount();
+  uint32_t numFrames = GetFrameCount();
 
   // We can't use mPNG->num_frames_read as it may be one ahead.
   if (numFrames > 1) {
-    // Tell the image renderer that the frame is complete
-    if (mFrameHasNoAlpha)
-      mImage.SetFrameHasNoAlpha(numFrames - 1);
-
-    // PNG is always non-premult
-    mImage.SetFrameAsNonPremult(numFrames - 1, true);
-
     PostInvalidation(mFrameRect);
   }
+
+  if (png_get_valid(mPNG, mInfo, PNG_INFO_acTL)) {
+    animInfo = AnimFrameInfo(mPNG, mInfo);
+  }
 #endif
 
-  PostFrameStop();
+  PostFrameStop(alpha, animInfo.mDispose, animInfo.mTimeout, animInfo.mBlend);
 }
 
 void
@@ -847,16 +853,17 @@ nsPNGDecoder::end_callback(png_structp png_ptr, png_infop info_ptr)
   // We shouldn't get here if we've hit an error
   NS_ABORT_IF_FALSE(!decoder->HasError(), "Finishing up PNG but hit error!");
 
+  int32_t loop_count = 0;
 #ifdef PNG_APNG_SUPPORTED
   if (png_get_valid(png_ptr, info_ptr, PNG_INFO_acTL)) {
     int32_t num_plays = png_get_num_plays(png_ptr, info_ptr);
-    decoder->mImage.SetLoopCount(num_plays - 1);
+    loop_count = num_plays - 1;
   }
 #endif
 
   // Send final notifications
   decoder->EndImageFrame();
-  decoder->PostDecodeDone();
+  decoder->PostDecodeDone(loop_count);
 }
 
 

image/decoders/nsPNGDecoder.h

@@ -35,8 +35,6 @@ public:
   void CreateFrame(png_uint_32 x_offset, png_uint_32 y_offset,
                    int32_t width, int32_t height,
                    gfxASurface::gfxImageFormat format);
-  void SetAnimFrameInfo();
-
   void EndImageFrame();
 
   // Check if PNG is valid ICO (32bpp RGBA)

image/src/Decoder.cpp

@@ -231,7 +231,10 @@ Decoder::PostFrameStart()
 }
 
 void
-Decoder::PostFrameStop()
+Decoder::PostFrameStop(RasterImage::FrameAlpha aFrameAlpha /* = RasterImage::kFrameHasAlpha */,
+                       RasterImage::FrameDisposalMethod aDisposalMethod /* = RasterImage::kDisposeKeep */,
+                       int32_t aTimeout /* = 0 */,
+                       RasterImage::FrameBlendMethod aBlendMethod /* = RasterImage::kBlendOver */)
 {
   // We should be mid-frame
   NS_ABORT_IF_FALSE(mInFrame, "Stopping frame when we didn't start one!");
@@ -239,6 +242,14 @@ Decoder::PostFrameStop()
   // Update our state
   mInFrame = false;
 
+  if (aFrameAlpha == RasterImage::kFrameOpaque) {
+    mImage.SetFrameHasNoAlpha(mFrameCount - 1);
+  }
+
+  mImage.SetFrameDisposalMethod(mFrameCount - 1, aDisposalMethod);
+  mImage.SetFrameTimeout(mFrameCount - 1, aTimeout);
+  mImage.SetFrameBlendMethod(mFrameCount - 1, aBlendMethod);
+
   // Flush any invalidations before we finish the frame
   FlushInvalidations();
 
@@ -263,7 +274,7 @@ Decoder::PostInvalidation(nsIntRect& aRect)
 }
 
 void
-Decoder::PostDecodeDone()
+Decoder::PostDecodeDone(int32_t aLoopCount /* = 0 */)
 {
   NS_ABORT_IF_FALSE(!IsSizeDecode(), "Can't be done with decoding with size decode!");
   NS_ABORT_IF_FALSE(!mInFrame, "Can't be done decoding if we're mid-frame!");
@@ -277,6 +288,8 @@ Decoder::PostDecodeDone()
     mImage.SetFrameAsNonPremult(i, isNonPremult);
   }
 
+  mImage.SetLoopCount(aLoopCount);
+
   // Notify
   mImage.DecodingComplete();
   if (mObserver) {

image/src/Decoder.h

@@ -150,10 +150,19 @@ protected:
   // the image of its size and sends notifications.
   void PostSize(int32_t aWidth, int32_t aHeight);
 
-  // Called by decoders when they begin/end a frame. Informs the image, sends
+  // Called by decoders when they begin a frame. Informs the image, sends
   // notifications, and does internal book-keeping.
   void PostFrameStart();
-  void PostFrameStop();
+
+  // Called by decoders when they end a frame. Informs the image, sends
+  // notifications, and does internal book-keeping.
+  // Specify whether this frame is opaque as an optimization.
+  // For animated images, specify the disposal, blend method and timeout for
+  // this frame.
+  void PostFrameStop(RasterImage::FrameAlpha aFrameAlpha = RasterImage::kFrameHasAlpha,
+                     RasterImage::FrameDisposalMethod aDisposalMethod = RasterImage::kDisposeKeep,
+                     int32_t aTimeout = 0,
+                     RasterImage::FrameBlendMethod aBlendMethod = RasterImage::kBlendOver);
 
   // Called by the decoders when they have a region to invalidate. We may not
   // actually pass these invalidations on right away.
@@ -164,7 +173,10 @@ protected:
   // the stream, or by us calling FinishInternal().
   //
   // May not be called mid-frame.
-  void PostDecodeDone();
+  //
+  // For animated images, specify the loop count. -1 means loop forever, 0
+  // means a single iteration, stopping on the last frame.
+  void PostDecodeDone(int32_t aLoopCount = 0);
 
   // Data errors are the fault of the source data, decoder errors are our fault
   void PostDataError();

image/src/RasterImage.h

@@ -270,7 +270,7 @@ public:
 
   // "Blend" method indicates how the current image is combined with the
   // previous image.
-  enum {
+  enum FrameBlendMethod {
     // All color components of the frame, including alpha, overwrite the current
     // contents of the frame's output buffer region
     kBlendSource =  0,
@@ -280,7 +280,7 @@ public:
     kBlendOver
   };
 
-  enum {
+  enum FrameDisposalMethod {
     kDisposeClearAll         = -1, // Clear the whole image, revealing
                                    // what was there before the gif displayed
     kDisposeNotSpecified,   // Leave frame, let new frame draw on top
@@ -289,7 +289,14 @@ public:
     kDisposeRestorePrevious // Restore the previous (composited) frame
   };
 
-  nsCString GetURIString() {
+  // A hint as to whether an individual frame is entirely opaque, or requires
+  // alpha blending.
+  enum FrameAlpha {
+    kFrameHasAlpha,
+    kFrameOpaque
+  };
+
+ nsCString GetURIString() {
     nsCString spec;
     if (GetURI()) {
       GetURI()->GetSpec(spec);