GIF decoding to Index8, unit tests and misusing unit test as benchmark

third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.cpp

Index: third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.cpp
diff --git a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.cpp b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.cpp
index 4c6631dfe2b4ea139902f443fdf716cde83a9c68..707381c03fd8a04ea1b70aad9aa6865336f1f0bc 100644
--- a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.cpp
@@ -145,9 +145,8 @@ bool GIFLZWContext::outputRow(GIFRow::const_iterator rowBegin)
         return true;
 
     // CALLBACK: Let the client know we have decoded a row.
-    if (!m_client->haveDecodedRow(m_frameContext->frameId(), rowBegin, m_frameContext->width(),
-        drowStart, drowEnd - drowStart + 1, m_frameContext->progressiveDisplay() && m_frameContext->interlaced() && ipass > 1))
-        return false;
+    m_client->haveDecodedRow(*m_frameContext, rowBegin,
+        drowStart, drowEnd - drowStart + 1, m_frameContext->progressiveDisplay() && m_frameContext->interlaced() && ipass > 1);
 
     if (!m_frameContext->interlaced())
         irow++;
@@ -283,18 +282,19 @@ bool GIFLZWContext::doLZW(const unsigned char* block, size_t bytesInBlock)
 
             // Output as many rows as possible.
             GIFRow::iterator rowBegin = rowBuffer.begin();
-            for (; rowBegin + width <= rowIter; rowBegin += width) {
-                if (!outputRow(rowBegin))
-                    return false;
-                rowsRemaining--;
-                if (!rowsRemaining)
-                    return true;
-            }
+            if (rowBegin + width <= rowIter) {

[scroggo_chromium, 2016/04/29 19:48:15]

These changes (i.e. this if block) might be better (though it's not obvious to
me that they are), but they seem to be unrelated to the rest of the change. This
is already a very large change, which makes it hard to review. It would be
helpful to remove pieces like this that are orthogonal to the main change being
made.

+                for (; rowBegin + width <= rowIter; rowBegin += width) {
+                    if (!outputRow(rowBegin))
+                        return false;
+                    rowsRemaining--;
+                    if (!rowsRemaining)
+                        return true;
+                }
 
-            if (rowBegin != rowBuffer.begin()) {
                 // Move the remaining bytes to the beginning of the buffer.
                 const size_t bytesToCopy = rowIter - rowBegin;
-                memcpy(rowBuffer.begin(), rowBegin, bytesToCopy);
+                if (bytesToCopy)
+                    memcpy(rowBuffer.begin(), rowBegin, bytesToCopy);
                 rowIter = rowBuffer.begin() + bytesToCopy;
             }
         }
@@ -342,6 +342,11 @@ bool GIFFrameContext::decode(blink::FastSharedBufferReader* reader, blink::GIFIm
         m_currentLzwBlock = 0;
     }
 
+    if (m_lzwContext->hasRemainingRows()) {
+        if (!client->initFrameBuffer(m_frameId))

[scroggo_chromium, 2016/04/29 19:48:15]

Why is this needed now?

+            return false;
+    }
+
     // Some bad GIFs have extra blocks beyond the last row, which we don't want to decode.
     while (m_currentLzwBlock < m_lzwBlocks.size() && m_lzwContext->hasRemainingRows()) {
         size_t blockPosition = m_lzwBlocks[m_currentLzwBlock].blockPosition;
@@ -453,7 +458,7 @@ bool GIFImageReader::parseData(size_t dataPosition, size_t len, GIFImageDecoder:
         case GIFGlobalHeader: {
             const unsigned char* currentComponent =
                 reinterpret_cast<const unsigned char*>(
-                    reader.getConsecutiveData(currentComponentPosition, 5, readBuffer));
+                    reader.getConsecutiveData(currentComponentPosition, 6, readBuffer));
 
             // This is the height and width of the "screen" or frame into which
             // images are rendered. The individual images can be smaller than
@@ -465,6 +470,7 @@ bool GIFImageReader::parseData(size_t dataPosition, size_t len, GIFImageDecoder:
             m_screenHeight = GETINT16(currentComponent + 2);
 
             const size_t globalColorMapColors = 2 << (currentComponent[4] & 0x07);
+            m_backgroundIndex = currentComponent[5];

[scroggo_chromium, 2016/04/29 19:48:15]

It looks like we never looked at this color before. Were we doing something
wrong?

Ah, I think this relates to your comment in your doc that we should use the
background color for partially decoded independent frames. Is that correct?

Please separate behavior changes (i.e. we're doing something different that the
user can see) from performance changes.

 
             if ((currentComponent[4] & 0x80) && globalColorMapColors > 0) { /* global map */
                 m_globalColorMap.setTablePositionAndSize(dataPosition, globalColorMapColors);