Changeset 44573 in webkit

Timestamp:

Jun 10, 2009 10:46:51 AM (15 years ago)

Author:

pkasting@chromium.org

Message:

2009-06-10 Peter Kasting <​pkasting@google.com>

Reviewed by Eric Seidel.

​https://bugs.webkit.org/show_bug.cgi?id=25709 part five
Rewrite users of ImageDecoder.h to use "safe" API calls, Skia side.
This tweaks the RGBA32Buffer interfaces to be implementable by Cairo as
well and modifies Skia's usage of those interfaces accordingly. Once a
similar change lands on the Cairo side, the Skia and Cairo decoders
should be identical except for the implementation of RGBA32Buffer.

• platform/graphics/skia/ImageSourceSkia.cpp: (WebCore::ImageSource::createFrameAtIndex): Use new RGBA32Buffer::asNewNativeImage() function.
• platform/image-decoders/skia/BMPImageReader.cpp: (WebCore::BMPImageReader::processNonRLEData): Use new RGBA32Buffer::zeroFill() function.
• platform/image-decoders/skia/GIFImageDecoder.cpp: (WebCore::GIFImageDecoder::haveDecodedRow): Rewrite to use RGBA32Buffer::setRGBA(x, y, ...) calls as well as other RGBA32Buffer interface calls instead of knowing the underlying types inside the buffer; also try and add clarity.
• platform/image-decoders/skia/ImageDecoder.h: Removed bitmap(), width(), and height(). (WebCore::RGBA32Buffer::zeroFill): Added. (WebCore::RGBA32Buffer::copyBitmapData): Avoid using bitmap(), which is going away. (WebCore::RGBA32Buffer::copyRowNTimes): Added. (WebCore::RGBA32Buffer::setSize): Use new zeroFill() function. (WebCore::RGBA32Buffer::asNewNativeImage): Added. (WebCore::RGBA32Buffer::setRGBA): Condensed two versions into one that takes coordinates instead of a raw pointer.

Location:

trunk/WebCore

Files:

• ChangeLog (modified) (1 diff)
• platform/graphics/skia/ImageSourceSkia.cpp (modified) (1 diff)
• platform/image-decoders/skia/BMPImageReader.cpp (modified) (1 diff)
• platform/image-decoders/skia/GIFImageDecoder.cpp (modified) (3 diffs)
• platform/image-decoders/skia/ImageDecoder.h (modified) (5 diffs)

trunk/WebCore/ChangeLog

@@ +1 @@
+2009-06-10  Peter Kasting  <pkasting@google.com>
+
+        Reviewed by Eric Seidel.
+
+        https://bugs.webkit.org/show_bug.cgi?id=25709 part five
+        Rewrite users of ImageDecoder.h to use "safe" API calls, Skia side.
+        This tweaks the RGBA32Buffer interfaces to be implementable by Cairo as
+        well and modifies Skia's usage of those interfaces accordingly.  Once a
+        similar change lands on the Cairo side, the Skia and Cairo decoders
+        should be identical except for the implementation of RGBA32Buffer.
+
+        * platform/graphics/skia/ImageSourceSkia.cpp:
+        (WebCore::ImageSource::createFrameAtIndex): Use new RGBA32Buffer::asNewNativeImage() function.
+        * platform/image-decoders/skia/BMPImageReader.cpp:
+        (WebCore::BMPImageReader::processNonRLEData): Use new RGBA32Buffer::zeroFill() function.
+        * platform/image-decoders/skia/GIFImageDecoder.cpp:
+        (WebCore::GIFImageDecoder::haveDecodedRow): Rewrite to use RGBA32Buffer::setRGBA(x, y, ...) calls as well as other RGBA32Buffer interface calls instead of knowing the underlying types inside the buffer; also try and add clarity.
+        * platform/image-decoders/skia/ImageDecoder.h: Removed bitmap(), width(), and height().
+        (WebCore::RGBA32Buffer::zeroFill): Added.
+        (WebCore::RGBA32Buffer::copyBitmapData): Avoid using bitmap(), which is going away.
+        (WebCore::RGBA32Buffer::copyRowNTimes): Added.
+        (WebCore::RGBA32Buffer::setSize): Use new zeroFill() function.
+        (WebCore::RGBA32Buffer::asNewNativeImage): Added.
+        (WebCore::RGBA32Buffer::setRGBA): Condensed two versions into one that takes coordinates instead of a raw pointer.
+
 2009-06-10  Xan Lopez  <xlopez@igalia.com>
 

trunk/WebCore/platform/graphics/skia/ImageSourceSkia.cpp

@@ -186 +186 @@
 
     // Copy the bitmap.  The pixel data is refcounted internally by SkBitmap, so
-    // this doesn't cost much.  This pointer will be owned by the BitmapImage
-    // and freed in FrameData::clear().
-    return new NativeImageSkia(buffer->bitmap());
+    // this doesn't cost much.  
+    return buffer->asNewNativeImage();
 }
 

trunk/WebCore/platform/image-decoders/skia/BMPImageReader.cpp

@@ -804 +804 @@
                     m_seenNonZeroAlphaPixel = true;
                     if (m_seenZeroAlphaPixel) {
-                        // The eraseARGB() call here also sets "hasAlpha" true.
-                        m_frameBufferCache.first().bitmap().eraseARGB(0, 0, 0,
-                                                                      0);
+                        m_frameBufferCache.first().zeroFill();
                         m_seenZeroAlphaPixel = false;
                     } else if (alpha != 255)

trunk/WebCore/platform/image-decoders/skia/GIFImageDecoder.cpp

@@ -341 +341 @@
 
 void GIFImageDecoder::haveDecodedRow(unsigned frameIndex,
-                                     unsigned char* rowBuffer,   // Pointer to single scanline temporary buffer
+                                     unsigned char* rowBuffer,
                                      unsigned char* rowEnd,
-                                     unsigned rowNumber,  // The row index
-                                     unsigned repeatCount,  // How many times to repeat the row
+                                     unsigned rowNumber,
+                                     unsigned repeatCount,
                                      bool writeTransparentPixels)
 {
-    // Initialize the frame if necessary.
-    RGBA32Buffer& buffer = m_frameBufferCache[frameIndex];
-    if ((buffer.status() == RGBA32Buffer::FrameEmpty) && !initFrameBuffer(frameIndex))
-        return;
-
-    // Do nothing for bogus data.
-    if (rowBuffer == 0 || static_cast<int>(m_reader->frameYOffset() + rowNumber) >= size().height())
-        return;
-
+    // The pixel data and coordinates supplied to us are relative to the frame's
+    // origin within the entire image size, i.e.
+    // (m_reader->frameXOffset(), m_reader->frameYOffset()).
+    int x = m_reader->frameXOffset();
+    const int y = m_reader->frameYOffset() + rowNumber;
+
+    // Sanity-check the arguments.
+    if ((rowBuffer == 0) || (y >= size().height()))
+        return;
+
+    // Get the colormap.
     unsigned colorMapSize;
     unsigned char* colorMap;
@@ -362 +364 @@
         return;
 
-    // The buffers that we draw are the entire image's width and height, so a final output frame is
-    // width * height RGBA32 values in size.
-    //
-    // A single GIF frame, however, can be smaller than the entire image, i.e., it can represent some sub-rectangle
-    // within the overall image.  The rows we are decoding are within this
-    // sub-rectangle.  This means that if the GIF frame's sub-rectangle is (x,y,w,h) then row 0 is really row
-    // y, and each row goes from x to x+w.
-    unsigned dstPos = (m_reader->frameYOffset() + rowNumber) * size().width() + m_reader->frameXOffset();
-    unsigned* dst = buffer.bitmap().getAddr32(0, 0) + dstPos;
-    unsigned* dstEnd = dst + size().width() - m_reader->frameXOffset();
-    unsigned* currDst = dst;
-    unsigned char* currentRowByte = rowBuffer;
-
-    while (currentRowByte != rowEnd && currDst < dstEnd) {
-        if ((!m_reader->isTransparent() || *currentRowByte != m_reader->transparentPixel()) && *currentRowByte < colorMapSize) {
-            unsigned colorIndex = *currentRowByte * 3;
-            unsigned red = colorMap[colorIndex];
-            unsigned green = colorMap[colorIndex + 1];
-            unsigned blue = colorMap[colorIndex + 2];
-            RGBA32Buffer::setRGBA(currDst, red, green, blue, 255);
+    // Initialize the frame if necessary.
+    RGBA32Buffer& buffer = m_frameBufferCache[frameIndex];
+    if ((buffer.status() == RGBA32Buffer::FrameEmpty) && !initFrameBuffer(frameIndex))
+        return;
+
+    // Write one row's worth of data into the frame.  There is no guarantee that
+    // (rowEnd - rowBuffer) == (size().width() - m_reader->frameXOffset()), so
+    // we must ensure we don't run off the end of either the source data or the
+    // row's X-coordinates.
+    for (unsigned char* sourceAddr = rowBuffer; (sourceAddr != rowEnd) && (x < size().width()); ++sourceAddr, ++x) {
+        const unsigned char sourceValue = *sourceAddr;
+        if ((!m_reader->isTransparent() || (sourceValue != m_reader->transparentPixel())) && (sourceValue < colorMapSize)) {
+            const size_t colorIndex = static_cast<size_t>(sourceValue) * 3;
+            buffer.setRGBA(x, y, colorMap[colorIndex], colorMap[colorIndex + 1], colorMap[colorIndex + 2], 255);
         } else {
             m_currentBufferSawAlpha = true;
@@ -392 +388 @@
             // later ones.
             if (writeTransparentPixels)
-                RGBA32Buffer::setRGBA(currDst, 0, 0, 0, 0);
-        }
-        currDst++;
-        currentRowByte++;
-    }
-
-    if (repeatCount > 1) {
-        // Copy the row |repeatCount|-1 times.
-        unsigned num = currDst - dst;
-        unsigned dataSize = num * sizeof(unsigned);
-        unsigned width = size().width();
-        unsigned* end = buffer.bitmap().getAddr32(0, 0) + width * size().height();
-        currDst = dst + width;
-        for (unsigned i = 1; i < repeatCount; i++) {
-            if (currDst + num > end) // Protect against a buffer overrun from a bogus repeatCount.
-                break;
-            memcpy(currDst, dst, dataSize);
-            currDst += width;
-        }
-    }
+                buffer.setRGBA(x, y, 0, 0, 0, 0);
+        }
+    }
+
+    // Tell the frame to copy the row data if need be.
+    if (repeatCount > 1)
+        buffer.copyRowNTimes(m_reader->frameXOffset(), x, y, std::min(y + static_cast<int>(repeatCount), size().height()));
 }
 

trunk/WebCore/platform/image-decoders/skia/ImageDecoder.h

@@ -106 +106 @@
         }
 
+        void zeroFill()
+        {
+            m_bitmap.eraseARGB(0, 0, 0, 0);
+        }
+
         // This function creates a new copy of the image data in |other|, so the
         // two images can be modified independently.
@@ -114 +119 @@
 
             m_bitmap.reset();
-            const NativeImageSkia& otherBitmap = other.bitmap();
+            const NativeImageSkia& otherBitmap = other.m_bitmap;
             otherBitmap.copyTo(&m_bitmap, otherBitmap.config());
         }
 
-        NativeImageSkia& bitmap() { return m_bitmap; }
-        const NativeImageSkia& bitmap() const { return m_bitmap; }
+        // This function copies [(startX, startY), (endX, startY)) to the same
+        // X-coordinates on each subsequent row up to but not including endY.
+        //
+        // NOTE: This function does not sanity-check its arguments!  Callers
+        // MUST not pass invalid values or this will corrupt memory.
+        void copyRowNTimes(int startX, int endX, int startY, int endY)
+        {
+            ASSERT(startX < m_bitmap.width());
+            ASSERT(endX <= m_bitmap.width());
+            ASSERT(startY < m_bitmap.height());
+            ASSERT(endY <= m_bitmap.height());
+            const int rowBytes = (endX - startX) * sizeof uint32_t;
+            const uint32_t* const startAddr = m_bitmap.getAddr32(startX, startY);
+            for (int destY = startY + 1; destY < endY; ++destY)
+                memcpy(m_bitmap.getAddr32(startX, destY), startAddr, rowBytes);
+        }
 
         // Must be called before any pixels are written. Will return true on
@@ -135 +154 @@
             }
 
-            // Clear the image.
-            m_bitmap.eraseARGB(0, 0, 0, 0);
+            // Zero the image.
+            zeroFill();
 
             return true;
         }
 
-        int width() const { return m_bitmap.width(); }
-        int height() const { return m_bitmap.height(); }
+        // To be used by ImageSource::createFrameAtIndex().  Returns a pointer
+        // to the underlying native image data.  This pointer will be owned by
+        // the BitmapImage and freed in FrameData::clear().
+        NativeImagePtr asNewNativeImage()
+        {
+            return new NativeImageSkia(m_bitmap);
+        }
 
         const IntRect& rect() const { return m_rect; }
@@ -161 +185 @@
         void setHasAlpha(bool alpha) { m_bitmap.setIsOpaque(!alpha); }
 
-        static void setRGBA(uint32_t* dest, uint8_t r, uint8_t g, uint8_t b, uint8_t a)
-        {
+        void setRGBA(int x, int y, uint8_t r, uint8_t g, uint8_t b, uint8_t a)
+        {
+            uint32_t* const dest = m_bitmap.getAddr32(x, y);
+
             // We store this data pre-multiplied.
             if (a == 0)
@@ -177 +203 @@
         }
 
-        void setRGBA(int x, int y, uint8_t r, uint8_t g, uint8_t b, uint8_t a)
-        {
-            setRGBA(m_bitmap.getAddr32(x, y), r, g, b, a);
-        }
-
     private:
         NativeImageSkia m_bitmap;