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 ae9348c09f18c3569633a4cd4c4ad5ade86dd6f0..cfe2759c1aa2185003d098b8ca7d68b0c2c3f5f1 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 741a9fe982a149baa2361ea329432b5164b5b7d2..2d4dff44899bc1098847f93c99ba534e40cecee0 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 83b92890d6877c9ffbd489553ac5b66ec9d3964c..eb97778e3b4e1b3f5da14f42b5fef606df0347a3 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 55d0406617d867d7cb0dd1c75e4eecf76adb716e..1695d67502ea5b93b79db593300f42caaf432d51 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);
             }
         }
     }