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

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

 /* -*- Mode: C; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* ***** BEGIN LICENSE BLOCK *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
  *
  * The contents of this file are subject to the Mozilla Public License Version
  * 1.1 (the "License"); you may not use this file except in compliance with
  * the License. You may obtain a copy of the License at
  * http://www.mozilla.org/MPL/
  *
  * Software distributed under the License is distributed on an "AS IS" basis,
@@ skipping 127 matching lines @@
 
         if ((unsigned)drowEnd >= m_frameContext->height())
             drowEnd = m_frameContext->height() - 1;
     }
 
     // Protect against too much image data.
     if ((unsigned)drowStart >= m_frameContext->height())
         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++;
     else {
         do {
             switch (ipass) {
             case 1:
                 irow += 8;
                 if (irow >= m_frameContext->height()) {
                     ipass++;
@@ skipping 115 matching lines @@
                 if (!(avail & codemask) && avail < MAX_DICTIONARY_ENTRIES) {
                     ++codesize;
                     codemask += avail;
                 }
             }
             oldcode = tempCode;
             rowIter += codeLength;
 
             // 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;
             }
         }
     }
     return true;
 }
 
 void GIFColorMap::buildTable(blink::FastSharedBufferReader* reader)
 {
     if (!m_isDefined || !m_table.isEmpty())
@@ skipping 27 matching lines @@
 
         m_lzwContext = adoptPtr(new GIFLZWContext(client, this));
         if (!m_lzwContext->prepareToDecode()) {
             m_lzwContext.clear();
             return false;
         }
 
         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;
         size_t blockSize = m_lzwBlocks[m_currentLzwBlock].blockSize;
         if (blockPosition + blockSize > reader->size())
             return false;
 
         while (blockSize) {
             const char* segment = 0;
             size_t segmentLength = reader->getSomeData(segment, blockPosition);
@@ skipping 91 matching lines @@
                 m_version = 87;
             else
                 return false;
             GETN(7, GIFGlobalHeader);
             break;
         }
 
         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
             // the screen size and located with an origin anywhere within the
             // screen.
             // Note that we don't inform the client of the size yet, as it might
             // change after we read the first frame's image header.
             m_screenWidth = GETINT16(currentComponent);
             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);
                 GETN(BYTES_PER_COLORMAP_ENTRY * globalColorMapColors, GIFGlobalColormap);
                 break;
             }
 
             GETN(1, GIFImageStart);
             break;
         }
@@ skipping 362 matching lines @@
     rowIter = rowBuffer.begin();
     rowsRemaining = m_frameContext->height();
 
     // Clearing the whole suffix table lets us be more tolerant of bad data.
     for (int i = 0; i < clearCode; ++i) {
         suffix[i] = i;
         suffixLength[i] = 1;
     }
     return true;
 }