Bug 1247152 (Part 2) - Remove even more code from the GIF decoder. r=edwin
authorSeth Fowler <mark.seth.fowler@gmail.com>
Wed, 09 Mar 2016 15:39:02 -0800
changeset 287579 cb965dc05ec7d56b64581b6bbdb70357f6435052
parent 287578 a1c6dad1153682c6c9fcafe85157bc2b9e02b419
child 287580 2bb06f221cfee6f6ccc8c98f770b004c6197d459
push id73237
push usermfowler@mozilla.com
push dateWed, 09 Mar 2016 23:39:39 +0000
treeherdermozilla-inbound@cb965dc05ec7 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersedwin
bugs1247152
milestone48.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 1247152 (Part 2) - Remove even more code from the GIF decoder. r=edwin
image/decoders/GIF2.h
image/decoders/nsGIFDecoder2.cpp
image/decoders/nsGIFDecoder2.h
--- a/image/decoders/GIF2.h
+++ b/image/decoders/GIF2.h
@@ -24,17 +24,16 @@ enum { GIF_APPLICATION_EXTENSION_LABEL =
 
 // List of possible parsing states
 typedef enum {
     gif_type,
     gif_global_header,
     gif_global_colormap,
     gif_image_start,
     gif_image_header,
-    gif_image_header_continue,
     gif_image_colormap,
     gif_lzw_start,
     gif_lzw,
     gif_sub_block,
     gif_extension,
     gif_control_extension,
     gif_consume_block,
     gif_skip_block,
@@ -65,38 +64,33 @@ typedef struct gif_struct {
     int count;                  // Remaining # bytes in sub-block
     int bits;                   // Number of unread bits in "datum"
     int32_t datum;              // 32-bit input buffer
 
     // Output state machine
     int64_t pixels_remaining;  // Pixels remaining to be output.
 
     // Parameters for image frame currently being decoded
-    unsigned x_offset, y_offset; // With respect to "screen" origin
-    unsigned height, width;
-    unsigned clamped_height;    // Size of the frame rectangle clamped to the
-    unsigned clamped_width;     //  global logical size after x_ and y_offset.
     int tpixel;                 // Index of transparent pixel
     int32_t disposal_method;    // Restore to background, leave in place, etc.
     uint32_t* local_colormap;   // Per-image colormap
     int local_colormap_size;    // Size of local colormap array.
     uint32_t delay_time;        // Display time, in milliseconds,
                                 // for this image in a multi-image GIF
 
     // Global (multi-image) state
     int version;                // Either 89 for GIF89 or 87 for GIF87
-    unsigned screen_width;      // Logical screen width & height
-    unsigned screen_height;
+    int32_t screen_width;       // Logical screen width & height
+    int32_t screen_height;
     uint32_t global_colormap_depth; // Depth of global colormap array
     int images_decoded;         // Counts images for multi-part GIFs
     int loop_count;             // Netscape specific extension block to control
                                 // the number of animation loops a GIF
                                 // renders.
 
-    bool interlaced;            // TRUE, if scanlines arrive interlaced order
     bool is_transparent;        // TRUE, if tpixel is valid
 
     uint16_t  prefix[MAX_BITS];            // LZW decoding tables
     uint8_t*  hold;                        // Accumulation buffer
     uint32_t  global_colormap[MAX_COLORS]; // Default colormap if local not
                                            //   supplied
     uint8_t   suffix[MAX_BITS];            // LZW decoding tables
     uint8_t   stack[MAX_BITS];             // Base of LZW decoder stack
--- a/image/decoders/nsGIFDecoder2.cpp
+++ b/image/decoders/nsGIFDecoder2.cpp
@@ -180,58 +180,57 @@ nsGIFDecoder2::ClampToImageRect(const In
     visibleFrameRect.MoveTo(0, 0);
   }
 
   return visibleFrameRect;
 }
 
 //******************************************************************************
 nsresult
-nsGIFDecoder2::BeginImageFrame(uint16_t aDepth)
+nsGIFDecoder2::BeginImageFrame(const IntRect& aFrameRect,
+                               uint16_t aDepth,
+                               bool aIsInterlaced)
 {
   MOZ_ASSERT(HasSize());
 
-  IntRect frameRect(mGIFStruct.x_offset, mGIFStruct.y_offset,
-                    mGIFStruct.width, mGIFStruct.height);
-
-  bool hasTransparency = CheckForTransparency(frameRect);
+  bool hasTransparency = CheckForTransparency(aFrameRect);
   gfx::SurfaceFormat format = hasTransparency ? SurfaceFormat::B8G8R8A8
                                               : SurfaceFormat::B8G8R8X8;
 
   // Make sure there's no animation if we're downscaling.
   MOZ_ASSERT_IF(mDownscaler, !GetImageMetadata().HasAnimation());
 
-  SurfacePipeFlags pipeFlags = mGIFStruct.interlaced
+  SurfacePipeFlags pipeFlags = aIsInterlaced
                              ? SurfacePipeFlags::DEINTERLACE
                              : SurfacePipeFlags();
 
   Maybe<SurfacePipe> pipe;
   if (mGIFStruct.images_decoded == 0) {
     // This is the first frame. We may be downscaling, so compute the target
     // size and target frame rect.
     IntSize targetSize = mDownscaler ? mDownscaler->TargetSize()
                                      : GetSize();
     IntRect targetFrameRect = mDownscaler ? IntRect(IntPoint(), targetSize)
-                                          : frameRect;
+                                          : aFrameRect;
 
     // The first frame may be displayed progressively.
     pipeFlags |= SurfacePipeFlags::PROGRESSIVE_DISPLAY;
 
     // The first frame is always decoded into an RGB surface.
     pipe =
       SurfacePipeFactory::CreateSurfacePipe(this, mGIFStruct.images_decoded,
                                             GetSize(), targetSize,
                                             targetFrameRect, format, pipeFlags);
   } else {
     // This is an animation frame (and not the first). To minimize the memory
     // usage of animations, the image data is stored in paletted form.
     MOZ_ASSERT(!mDownscaler);
     pipe =
       SurfacePipeFactory::CreatePalettedSurfacePipe(this, mGIFStruct.images_decoded,
-                                                    GetSize(), frameRect, format,
+                                                    GetSize(), aFrameRect, format,
                                                     aDepth, pipeFlags);
   }
 
   mCurrentFrameIndex = mGIFStruct.images_decoded;
 
   if (!pipe) {
     mPipe = SurfacePipe();
     return NS_ERROR_FAILURE;
@@ -244,36 +243,20 @@ nsGIFDecoder2::BeginImageFrame(uint16_t 
 
 //******************************************************************************
 void
 nsGIFDecoder2::EndImageFrame()
 {
   Opacity opacity = Opacity::SOME_TRANSPARENCY;
 
   // First flush all pending image data
-  if (!mGIFStruct.images_decoded) {
+  if (mGIFStruct.images_decoded == 0) {
     // Only need to flush first frame
     FlushImageData();
 
-    // If the first frame is smaller in height than the entire image, send an
-    // invalidation for the area it does not have data for.
-    // This will clear the remaining bits of the placeholder. (Bug 37589)
-    const uint32_t realFrameHeight = mGIFStruct.height + mGIFStruct.y_offset;
-    if (realFrameHeight < mGIFStruct.screen_height) {
-      if (mDownscaler) {
-        IntRect targetRect = IntRect(IntPoint(), mDownscaler->TargetSize());
-        PostInvalidation(IntRect(IntPoint(), GetSize()), Some(targetRect));
-      } else {
-        nsIntRect r(0, realFrameHeight,
-                    mGIFStruct.screen_width,
-                    mGIFStruct.screen_height - realFrameHeight);
-        PostInvalidation(r);
-      }
-    }
-
     // The first frame was preallocated with alpha; if it wasn't transparent, we
     // should fix that. We can also mark it opaque unconditionally if we didn't
     // actually see any transparent pixels - this test is only valid for the
     // first frame.
     if (!mGIFStruct.is_transparent && !mSawTransparency) {
       opacity = Opacity::FULLY_OPAQUE;
     }
   }
@@ -867,139 +850,96 @@ nsGIFDecoder2::WriteInternal(const char*
 
         if (mDownscaler) {
           MOZ_ASSERT_UNREACHABLE("Doing downscale-during-decode "
                                  "for an animated image?");
           mDownscaler.reset();
         }
       }
 
+      IntRect frameRect;
+
       // Get image offsets, with respect to the screen origin
-      mGIFStruct.x_offset = GETINT16(q);
-      mGIFStruct.y_offset = GETINT16(q + 2);
+      frameRect.x = GETINT16(q);
+      frameRect.y = GETINT16(q + 2);
 
       // Get image width and height.
-      mGIFStruct.width  = GETINT16(q + 4);
-      mGIFStruct.height = GETINT16(q + 6);
+      frameRect.width  = GETINT16(q + 4);
+      frameRect.height = GETINT16(q + 6);
 
       if (!mGIFStruct.images_decoded) {
         // Work around broken GIF files where the logical screen
         // size has weird width or height.  We assume that GIF87a
         // files don't contain animations.
-        if ((mGIFStruct.screen_height < mGIFStruct.height) ||
-            (mGIFStruct.screen_width < mGIFStruct.width) ||
+        if ((mGIFStruct.screen_height < frameRect.height) ||
+            (mGIFStruct.screen_width < frameRect.width) ||
             (mGIFStruct.version == 87)) {
-          mGIFStruct.screen_height = mGIFStruct.height;
-          mGIFStruct.screen_width = mGIFStruct.width;
-          mGIFStruct.x_offset = 0;
-          mGIFStruct.y_offset = 0;
+          mGIFStruct.screen_height = frameRect.height;
+          mGIFStruct.screen_width = frameRect.width;
+          frameRect.MoveTo(0, 0);
         }
         // Create the image container with the right size.
         BeginGIF();
         if (HasError()) {
           // Setting the size led to an error.
           mGIFStruct.state = gif_error;
           return;
         }
 
         // If we were doing a metadata decode, we're done.
         if (IsMetadataDecode()) {
-          IntRect frameRect(mGIFStruct.x_offset, mGIFStruct.y_offset,
-                            mGIFStruct.width, mGIFStruct.height);
           CheckForTransparency(frameRect);
           return;
         }
       }
 
       // Work around more broken GIF files that have zero image width or height
-      if (!mGIFStruct.height || !mGIFStruct.width) {
-        mGIFStruct.height = mGIFStruct.screen_height;
-        mGIFStruct.width = mGIFStruct.screen_width;
-        if (!mGIFStruct.height || !mGIFStruct.width) {
+      if (!frameRect.height || !frameRect.width) {
+        frameRect.height = mGIFStruct.screen_height;
+        frameRect.width = mGIFStruct.screen_width;
+        if (!frameRect.height || !frameRect.width) {
           mGIFStruct.state = gif_error;
           break;
         }
       }
 
-      // Hack around GIFs with frame rects outside the given screen bounds.
-      IntRect clampedRect =
-        ClampToImageRect(IntRect(mGIFStruct.x_offset, mGIFStruct.y_offset,
-                                 mGIFStruct.width, mGIFStruct.height));
-      if (clampedRect.IsEmpty()) {
-        // XXX Bug 1227546 - Maybe we should treat this as valid?
-        mGIFStruct.state = gif_error;
-        break;
-      }
-      mGIFStruct.clamped_width = clampedRect.width;
-      mGIFStruct.clamped_height = clampedRect.height;
-
-      MOZ_ASSERT(mGIFStruct.clamped_width <= mGIFStruct.width);
-      MOZ_ASSERT(mGIFStruct.clamped_height <= mGIFStruct.height);
-
       // Depth of colors is determined by colormap
       // (q[8] & 0x80) indicates local colormap
       // bits per pixel is (q[8]&0x07 + 1) when local colormap is set
       uint32_t depth = mGIFStruct.global_colormap_depth;
       if (q[8] & 0x80) {
         depth = (q[8]&0x07) + 1;
       }
       uint32_t realDepth = depth;
       while (mGIFStruct.tpixel >= (1 << realDepth) && (realDepth < 8)) {
         realDepth++;
       }
+
       // Mask to limit the color values within the colormap
       mColorMask = 0xFF >> (8 - realDepth);
 
-      if (NS_FAILED(BeginImageFrame(realDepth))) {
+      // Determine if this frame is interlaced or not.
+      const bool isInterlaced = q[8] & 0x40;
+
+      if (NS_FAILED(BeginImageFrame(frameRect, realDepth, isInterlaced))) {
         mGIFStruct.state = gif_error;
         return;
       }
-      MOZ_FALLTHROUGH; // to continue decoding header.
-    }
 
-    case gif_image_header_continue: {
       // While decoders can reuse frames, we unconditionally increment
       // mGIFStruct.images_decoded when we're done with a frame, so we both can
       // and need to zero out the colormap and image data after every new frame.
       memset(mImageData, 0, mImageDataLength);
       if (mColormap) {
         memset(mColormap, 0, mColormapSize);
       }
 
-      if (!mGIFStruct.images_decoded) {
-        // Send a onetime invalidation for the first frame if it has a y-axis
-        // offset. Otherwise, the area may never be refreshed and the
-        // placeholder will remain on the screen. (Bug 37589)
-        if (mGIFStruct.y_offset > 0) {
-          if (mDownscaler) {
-            IntRect targetRect = IntRect(IntPoint(), mDownscaler->TargetSize());
-            PostInvalidation(IntRect(IntPoint(), GetSize()), Some(targetRect));
-          } else {
-            nsIntRect r(0, 0, mGIFStruct.screen_width, mGIFStruct.y_offset);
-            PostInvalidation(r);
-          }
-        }
-      }
+      // Clear state from last image
+      mGIFStruct.pixels_remaining = frameRect.width * frameRect.height;
 
-      mGIFStruct.interlaced = bool(q[8] & 0x40);
-
-      // Clear state from last image
-      mGIFStruct.pixels_remaining = mGIFStruct.width * mGIFStruct.height;
-
-      // Depth of colors is determined by colormap
-      // (q[8] & 0x80) indicates local colormap
-      // bits per pixel is (q[8]&0x07 + 1) when local colormap is set
-      uint32_t depth = mGIFStruct.global_colormap_depth;
-      if (q[8] & 0x80) {
-        depth = (q[8]&0x07) + 1;
-      }
-      uint32_t realDepth = depth;
-      while (mGIFStruct.tpixel >= (1 << realDepth) && (realDepth < 8)) {
-        realDepth++;
-      }
       // has a local colormap?
       if (q[8] & 0x80) {
         mGIFStruct.local_colormap_size = 1 << depth;
         if (!mGIFStruct.images_decoded) {
           // First frame has local colormap, allocate space for it
           // as the image frame doesn't have its own palette
           mColormapSize = sizeof(uint32_t) << realDepth;
           if (!mGIFStruct.local_colormap) {
--- a/image/decoders/nsGIFDecoder2.h
+++ b/image/decoders/nsGIFDecoder2.h
@@ -28,21 +28,35 @@ public:
   virtual Telemetry::ID SpeedHistogram() override;
 
 private:
   friend class DecoderFactory;
 
   // Decoders should only be instantiated via DecoderFactory.
   explicit nsGIFDecoder2(RasterImage* aImage);
 
-  // These functions will be called when the decoder has a decoded row,
-  // frame size information, etc.
+  /// Called when we begin decoding the image.
   void      BeginGIF();
-  nsresult  BeginImageFrame(uint16_t aDepth);
+
+  /**
+   * Called when we begin decoding a frame.
+   *
+   * @param aFrameRect The region of the image that contains data. The region
+   *                   outside this rect is transparent.
+   * @param aDepth The palette depth of this frame.
+   * @param aIsInterlaced If true, this frame is an interlaced frame.
+   */
+  nsresult  BeginImageFrame(const gfx::IntRect& aFrameRect,
+                            uint16_t aDepth,
+                            bool aIsInterlaced);
+
+  /// Called when we finish decoding a frame.
   void      EndImageFrame();
+
+  /// Called when we finish decoding the entire image.
   void      FlushImageData();
 
   nsresult  GifWrite(const uint8_t* buf, uint32_t numbytes);
 
   /// Transforms a palette index into a pixel.
   template <typename PixelSize> PixelSize
   ColormapIndexToPixel(uint8_t aIndex);