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 128 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(),
+    if (!m_client->haveDecodedRow(*m_frameContext, rowBegin,
         drowStart, drowEnd - drowStart + 1, m_frameContext->progressiveDisplay() && m_frameContext->interlaced() && ipass > 1))
         return false;
 
     if (!m_frameContext->interlaced())
         irow++;
     else {
         do {
             switch (ipass) {
             case 1:
                 irow += 8;
@@ skipping 38 matching lines @@
 // Perform Lempel-Ziv-Welch decoding.
 // Returns true if decoding was successful. In this case the block will have been completely consumed and/or rowsRemaining will be 0.
 // Otherwise, decoding failed; returns false in this case, which will always cause the GIFImageReader to set the "decode failed" flag.
 bool GIFLZWContext::doLZW(const unsigned char* block, size_t bytesInBlock)
 {
     const size_t width = m_frameContext->width();
 
     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;
 
         // Check for underflow of decoder's 32-bit input buffer.
         while (bits >= codesize) {
             // Get the leading variable-length symbol from the data stream.
             int code = datum & codemask;
             datum >>= codesize;
             bits -= codesize;
@@ skipping 58 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) {
+                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/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;
         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];
 
             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;
 }