Gif: zero filling should use memset instead of setRGBA for every pixel
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 3 Jul 2013 13:54:45 +0000 (13:54 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 3 Jul 2013 13:54:45 +0000 (13:54 +0000)
https://bugs.webkit.org/show_bug.cgi?id=118350

Patch by Balazs Kelemen <b.kelemen@samsung.com> on 2013-07-03
Reviewed by Allan Sandfeld Jensen.

No new tests. Actually it is not covered by existing tests. Surprisingly we haven't got pixel
tests for animated images. Given that this patch is pretty trivial I don't think it's worth the
cost to start introducing such tests.
I added a manual test: animated-gif-dispose-background.html.

GIFImageDecoder::initializeFrameBuffer use a loop to fill a subrect with tranparent pixels.
This is extremely ineffecient. The use case for this code path is not frequent on the web
but it's still better to fix it.

* platform/image-decoders/ImageDecoder.cpp:
(WebCore::ImageFrame::zeroFillFrameRect):
* platform/image-decoders/ImageDecoder.h:
* platform/image-decoders/gif/GIFImageDecoder.cpp:
(WebCore::GIFImageDecoder::initFrameBuffer):
Fixed indentation in addition.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@152352 268f45cc-cd09-0410-ab3c-d52691b4dbfc

ManualTests/animated-gif-dispose-background.html [new file with mode: 0644]
ManualTests/resources/dispose-background.gif [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/platform/image-decoders/ImageDecoder.cpp
Source/WebCore/platform/image-decoders/ImageDecoder.h
Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp

diff --git a/ManualTests/animated-gif-dispose-background.html b/ManualTests/animated-gif-dispose-background.html
new file mode 100644 (file)
index 0000000..8f939d3
--- /dev/null
@@ -0,0 +1,6 @@
+<html>
+<body>
+    <p>Animated gif with background dispose method. Animation frames live behind their rects filled with transparent pixels.</p>
+    <img src="./resources/dispose-background.gif" />
+</body>
+</html>
diff --git a/ManualTests/resources/dispose-background.gif b/ManualTests/resources/dispose-background.gif
new file mode 100644 (file)
index 0000000..d685040
Binary files /dev/null and b/ManualTests/resources/dispose-background.gif differ
index ae9348c..cfe2759 100644 (file)
@@ -1,3 +1,26 @@
+2013-07-03  Balazs Kelemen  <b.kelemen@samsung.com>
+
+        Gif: zero filling should use memset instead of setRGBA for every pixel
+        https://bugs.webkit.org/show_bug.cgi?id=118350
+
+        Reviewed by Allan Sandfeld Jensen.
+
+        No new tests. Actually it is not covered by existing tests. Surprisingly we haven't got pixel
+        tests for animated images. Given that this patch is pretty trivial I don't think it's worth the
+        cost to start introducing such tests.
+        I added a manual test: animated-gif-dispose-background.html.
+
+        GIFImageDecoder::initializeFrameBuffer use a loop to fill a subrect with tranparent pixels.
+        This is extremely ineffecient. The use case for this code path is not frequent on the web
+        but it's still better to fix it.
+
+        * platform/image-decoders/ImageDecoder.cpp:
+        (WebCore::ImageFrame::zeroFillFrameRect):
+        * platform/image-decoders/ImageDecoder.h:
+        * platform/image-decoders/gif/GIFImageDecoder.cpp:
+        (WebCore::GIFImageDecoder::initFrameBuffer):
+        Fixed indentation in addition.
+
 2013-07-03  Przemyslaw Szymanski  <p.szymanski3@samsung.com>
 
         TextureUnit code optimization
index 741a9fe..2d4dff4 100644 (file)
@@ -178,6 +178,23 @@ void ImageFrame::zeroFillPixelData()
     m_hasAlpha = true;
 }
 
+void ImageFrame::zeroFillFrameRect(const IntRect& rect)
+{
+    ASSERT(IntRect(IntPoint(), m_size).contains(rect));
+
+    if (rect.isEmpty())
+        return;
+
+    size_t rectWidthInBytes = rect.width() * sizeof(PixelData);
+    PixelData* start = m_bytes + (rect.y() * width()) + rect.x();
+    for (int i = 0; i < rect.height(); ++i) {
+        memset(start, 0, rectWidthInBytes);
+        start += width();
+    }
+
+    setHasAlpha(true);
+}
+
 bool ImageFrame::copyBitmapData(const ImageFrame& other)
 {
     if (this == &other)
index 83b9289..eb97778 100644 (file)
@@ -77,6 +77,7 @@ namespace WebCore {
         // These do not touch other metadata, only the raw pixel data.
         void clearPixelData();
         void zeroFillPixelData();
+        void zeroFillFrameRect(const IntRect&);
 
         // Makes this frame have an independent copy of the provided image's
         // pixel data, so that modifications in one frame are not reflected in
index 55d0406..1695d67 100644 (file)
@@ -370,7 +370,7 @@ bool GIFImageDecoder::initFrameBuffer(unsigned frameIndex)
     int top = upperBoundScaledY(frameRect.y());
     int bottom = lowerBoundScaledY(frameRect.maxY(), top);
     buffer->setOriginalFrameRect(IntRect(left, top, right - left, bottom - top));
-    
+
     if (!frameIndex) {
         // This is the first frame, so we're not relying on any previous data.
         if (!buffer->setSize(scaledSize().width(), scaledSize().height()))
@@ -407,15 +407,10 @@ bool GIFImageDecoder::initFrameBuffer(unsigned frameIndex)
                 if (!buffer->setSize(bufferSize.width(), bufferSize.height()))
                     return setFailed();
             } else {
-              // Copy the whole previous buffer, then clear just its frame.
-              if (!buffer->copyBitmapData(*prevBuffer))
-                  return setFailed();
-              for (int y = prevRect.y(); y < prevRect.maxY(); ++y) {
-                  for (int x = prevRect.x(); x < prevRect.maxX(); ++x)
-                      buffer->setRGBA(x, y, 0, 0, 0, 0);
-              }
-              if ((prevRect.width() > 0) && (prevRect.height() > 0))
-                  buffer->setHasAlpha(true);
+                // Copy the whole previous buffer, then clear just its frame.
+                if (!buffer->copyBitmapData(*prevBuffer))
+                    return setFailed();
+                buffer->zeroFillFrameRect(prevRect);
             }
         }
     }