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 39ce3346c3a9347f17284c59b19bf5515b5b7f8c..ac20d124991535ade645a3ca3c2b58b553a54672 100644
--- a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageReader.cpp
@@ -146,7 +146,7 @@ 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(),
+    if (!m_client->haveDecodedRow(*m_frameContext, rowBegin,
         drowStart, drowEnd - drowStart + 1, m_frameContext->progressiveDisplay() && m_frameContext->interlaced() && ipass > 1))
         return false;
 
@@ -205,7 +205,7 @@ bool GIFLZWContext::doLZW(const unsigned char* block, size_t bytesInBlock)
     if (rowIter == rowBuffer.end())
         return true;
 
-    for (const unsigned char* ch = block; bytesInBlock-- > 0; ch++) {
+    for (const unsigned char* ch = block; bytesInBlock-- > 0; ++ch) {

[scroggo_chromium, 2016/01/06 21:50:41]

This seems like an unnecessary change, and at least unrelated.

[aleksandar.stojiljkovic, 2016/01/18 13:58:50]

Done.

         // Feed the next byte into the decoder's 32-bit input buffer.
         datum += ((int) *ch) << bits;
         bits += 8;
@@ -284,18 +284,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) {
+                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;
             }
         }
@@ -343,6 +344,11 @@ bool GIFFrameContext::decode(blink::FastSharedBufferReader* reader, blink::GIFIm
         m_currentLzwBlock = 0;
     }
 
+    if (m_lzwContext->hasRemainingRows()) {
+        if (!client->initFrameBuffer(m_frameId))

[scroggo_chromium, 2016/01/06 21:50:41]

This function (GIFFrameContext::decode) may be called multiple times for the
same frame (if not all of the data is available), resulting in calling
initFrameBuffer multiple times. Is that intentional? The existing callers check
first to see if it's empty.

[aleksandar.stojiljkovic, 2016/01/18 13:58:50]

Acknowledged.
It is intentional.
initFrameBuffer doesnt do much if frame is not empty - just sets proper
callbacks.

About setting callbacks for non empty frame, there is an explanation in
initFrameBuffer(), copying here:
e.g. if partial frame is N32, and previous required frame was index8 (and got
completely decoded and then removed from cache), then when again decoding
previous required frame, decoding callbacks would be set to index8 and here it
is important to set them for this frame.

Previously, (existing callers) it was called on every line, that's where
checking before calling made sense, now it is called on the decode call for the
frame...
 

+            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;
@@ -454,7 +460,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
@@ -466,6 +472,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];
 
             if ((currentComponent[4] & 0x80) && globalColorMapColors > 0) { /* global map */
                 m_globalColorMap.setTablePositionAndSize(dataPosition, globalColorMapColors);