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

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

 /*
  * Copyright (C) 2006 Apple Computer, Inc.  All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 15 matching lines @@
 #include "config.h"
 #include "platform/image-decoders/gif/GIFImageDecoder.h"
 
 #include <limits>
 #include "platform/image-decoders/gif/GIFImageReader.h"
 #include "wtf/NotFound.h"
 #include "wtf/PassOwnPtr.h"
 
 namespace blink {
 
-GIFImageDecoder::GIFImageDecoder(AlphaOption alphaOption, GammaAndColorProfileOption colorOptions, size_t maxDecodedBytes)
+GIFImageDecoder::GIFImageDecoder(AlphaOption alphaOption, GammaAndColorProfileOption colorOptions, size_t maxDecodedBytes, ImageFrame::ColorType colorType)
     : ImageDecoder(alphaOption, colorOptions, maxDecodedBytes)
+    , phaveDecodedRow(&GIFImageDecoder::haveDecodedRowIndex8)

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

If the requested color type is N32, shouldn't this be haveDecodedRowN32?

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

Done. Removed the line since the initialization or m_haveDecodedRowFunction
happens later in initFrameBuffer (so before writing any rows).

     , m_repetitionCount(cAnimationLoopOnce)
+    , m_colorMode(colorType)
 {
 }
 
 GIFImageDecoder::~GIFImageDecoder()
 {
 }
 
 void GIFImageDecoder::onSetData(SharedBuffer* data)
 {
     if (m_reader)
@@ skipping 46 matching lines @@
         m_reader->frameContext(index)->isHeaderDefined()) ?
         m_reader->frameContext(index)->delayTime() : 0;
 }
 
 bool GIFImageDecoder::setFailed()
 {
     m_reader.clear();
     return ImageDecoder::setFailed();
 }
 
-bool GIFImageDecoder::haveDecodedRow(size_t frameIndex, GIFRow::const_iterator rowBegin, size_t width, size_t rowNumber, unsigned repeatCount, bool writeTransparentPixels)
+static inline const GIFColorMap& getColorMap(const GIFFrameContext& frameContext, const GIFImageReader& reader)
 {
-    const GIFFrameContext* frameContext = m_reader->frameContext(frameIndex);
+    return (frameContext.localColorMap().isDefined() ? frameContext.localColorMap() : reader.globalColorMap());
+}
+
+bool GIFImageDecoder::haveDecodedRowN32(const GIFFrameContext& frameContext, GIFRow::const_iterator rowBegin, size_t rowNumber, unsigned repeatCount, bool writeTransparentPixels)
+{
+    ASSERT(m_frameBufferCache[frameContext.frameId()].bitmap().colorType() == kN32_SkColorType);
+
     // The pixel data and coordinates supplied to us are relative to the frame's
     // origin within the entire image size, i.e.
     // (frameContext->xOffset, frameContext->yOffset). There is no guarantee
     // that width == (size().width() - frameContext->xOffset), so
     // we must ensure we don't run off the end of either the source data or the
     // row's X-coordinates.
-    const int xBegin = frameContext->xOffset();
-    const int yBegin = frameContext->yOffset() + rowNumber;
-    const int xEnd = std::min(static_cast<int>(frameContext->xOffset() + width), size().width());
-    const int yEnd = std::min(static_cast<int>(frameContext->yOffset() + rowNumber + repeatCount), size().height());
+    const size_t width = frameContext.width();
+    const int xBegin = frameContext.xOffset();
+    const int yBegin = frameContext.yOffset() + rowNumber;
+    const int xEnd = std::min(static_cast<int>(xBegin + width), size().width());
+    const int yEnd = std::min(static_cast<int>(yBegin + repeatCount), size().height());
     if (!width || (xBegin < 0) || (yBegin < 0) || (xEnd <= xBegin) || (yEnd <= yBegin))
         return true;
 
-    const GIFColorMap::Table& colorTable = frameContext->localColorMap().isDefined() ? frameContext->localColorMap().table() : m_reader->globalColorMap().table();
+    const GIFColorMap::Table& colorTable = getColorMap(frameContext, *m_reader).table();
 
     if (colorTable.isEmpty())
         return true;
 
     GIFColorMap::Table::const_iterator colorTableIter = colorTable.begin();
 
-    // Initialize the frame if necessary.
-    ImageFrame& buffer = m_frameBufferCache[frameIndex];
-    if ((buffer.status() == ImageFrame::FrameEmpty) && !initFrameBuffer(frameIndex))
-        return false;
-
-    const size_t transparentPixel = frameContext->transparentPixel();
+    ImageFrame& buffer = m_frameBufferCache[frameContext.frameId()];
+    const size_t transparentPixel = frameContext.transparentPixel();
     GIFRow::const_iterator rowEnd = rowBegin + (xEnd - xBegin);
     ImageFrame::PixelData* currentAddress = buffer.getAddr(xBegin, yBegin);
 
     // We may or may not need to write transparent pixels to the buffer.
     // If we're compositing against a previous image, it's wrong, and if
     // we're writing atop a cleared, fully transparent buffer, it's
     // unnecessary; but if we're decoding an interlaced gif and
     // displaying it "Haeberli"-style, we must write these for passes
     // beyond the first, or the initial passes will "show through" the
     // later ones.
@@ skipping 23 matching lines @@
     }
 
     // Tell the frame to copy the row data if need be.
     if (repeatCount > 1)
         buffer.copyRowNTimes(xBegin, xEnd, yBegin, yEnd);
 
     buffer.setPixelsChanged(true);
     return true;
 }
 
+bool GIFImageDecoder::haveDecodedRowIndex8(const GIFFrameContext& frameContext, GIFRow::const_iterator rowBegin, size_t rowNumber, unsigned repeatCount, bool writeTransparentPixels)

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

Is there a way to share more code here?

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

Done.

+{
+    ASSERT(m_frameBufferCache[frameContext.frameId()].bitmap().colorType() == kIndex_8_SkColorType);
+
+    // The pixel data and coordinates supplied to us are relative to the frame's
+    // origin within the entire image size, i.e.
+    // (frameContext.xOffset, frameContext.yOffset). There is no guarantee
+    // that width == (size().width() - frameContext.xOffset), so
+    // we must ensure we don't run off the end of either the source data or the
+    // row's X-coordinates.
+    const size_t width = frameContext.width();
+    const int xBegin = frameContext.xOffset();
+    const int yBegin = frameContext.yOffset() + rowNumber;
+    const int xEnd = std::min(static_cast<int>(frameContext.xOffset() + width), size().width());
+    const int yEnd = std::min(static_cast<int>(frameContext.yOffset() + rowNumber + repeatCount), size().height());
+    if (!width || (xBegin < 0) || (yBegin < 0) || (xEnd <= xBegin) || (yEnd <= yBegin))
+        return true;
+
+    const GIFColorMap::Table& colorTable = getColorMap(frameContext, *m_reader).table();
+
+    if (colorTable.isEmpty())
+        return true;
+
+
+    ImageFrame& buffer = m_frameBufferCache[frameContext.frameId()];
+    const size_t transparentPixel = frameContext.transparentPixel();
+    GIFRow::const_iterator rowEnd = rowBegin + (xEnd - xBegin);
+    ImageFrame::PixelData8* currentAddress = buffer.getAddr8(xBegin, yBegin);
+
+    bool opaque = true;
+    // writeTransparentPixels is writing without check, but calculates if there is transparency
+    // (m_currentBufferSawAlpha). If transparency was found in previous rows, no need to calculate here.
+    writeTransparentPixels = writeTransparentPixels || buffer.requiredPreviousFrameIndex() == kNotFound;
+    if (transparentPixel < colorTable.size() && !(writeTransparentPixels && m_currentBufferSawAlpha)) {
+        if (writeTransparentPixels) {
+            while (rowBegin != rowEnd) {
+                opaque = opaque && (*rowBegin ^ transparentPixel);
+                *currentAddress++ = *rowBegin++;
+            }
+        } else {
+            for (; rowBegin != rowEnd; ++rowBegin, ++currentAddress) {
+                size_t index = *rowBegin;
+                if (index == transparentPixel) {
+                    opaque = false;
+                } else {
+                    *currentAddress = index;
+                }
+            }
+        }
+    } else {
+        // No transparency to deal with.
+        if (xEnd - xBegin == size().width()) {
+            size_t* destination = (size_t*)currentAddress;
+            const size_t* source = (const size_t*)rowBegin;
+            const size_t count = (rowEnd - rowBegin + sizeof(size_t) - 1) / sizeof(size_t);
+            const size_t* sourceEnd = source + count;
+            while (source != sourceEnd)
+                *destination++ = *source++;
+        } else {
+            while (rowBegin != rowEnd)
+                *currentAddress++ = *rowBegin++;
+        }
+    }
+
+    if (!opaque)
+        m_currentBufferSawAlpha = true;
+
+    // Tell the frame to copy the row data if specified.
+    if (repeatCount > 1) {
+        const int rowBytes = (xEnd - xBegin) * sizeof(uint8_t);
+        const ImageFrame::PixelData8* const startAddr = buffer.getAddr8(xBegin, yBegin);
+        for (int destY = yBegin + 1; destY < yEnd; ++destY)
+            memcpy(buffer.getAddr8(xBegin, destY), startAddr, rowBytes);
+    }
+
+    buffer.setPixelsChanged(true);
+    return true;
+}
+
 bool GIFImageDecoder::parseCompleted() const
 {
     return m_reader && m_reader->parseCompleted();
 }
 
 bool GIFImageDecoder::frameComplete(size_t frameIndex)
 {
     // Initialize the frame if necessary.  Some GIFs insert do-nothing frames,
     // in which case we never reach haveDecodedRow() before getting here.
     ImageFrame& buffer = m_frameBufferCache[frameIndex];
     if ((buffer.status() == ImageFrame::FrameEmpty) && !initFrameBuffer(frameIndex))
         return false; // initFrameBuffer() has already called setFailed().
 
     buffer.setStatus(ImageFrame::FrameComplete);
 
     if (!m_currentBufferSawAlpha) {
         // The whole frame was non-transparent, so it's possible that the entire
         // resulting buffer was non-transparent, and we can setHasAlpha(false).
         if (buffer.originalFrameRect().contains(IntRect(IntPoint(), size()))) {
             buffer.setHasAlpha(false);
             buffer.setRequiredPreviousFrameIndex(kNotFound);
         } else if (buffer.requiredPreviousFrameIndex() != kNotFound) {
             // Tricky case.  This frame does not have alpha only if everywhere
             // outside its rect doesn't have alpha.  To know whether this is
             // true, we check the start state of the frame -- if it doesn't have
             // alpha, we're safe.
-            const ImageFrame* prevBuffer = &m_frameBufferCache[buffer.requiredPreviousFrameIndex()];
-            ASSERT(prevBuffer->disposalMethod() != ImageFrame::DisposeOverwritePrevious);
+            const ImageFrame* previousBuffer = &m_frameBufferCache[buffer.requiredPreviousFrameIndex()];

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

Why the name change?

This name change distracts from the logical changes that you are making. My
preference would be to leave the name unchanged.

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

Done.

+            ASSERT(previousBuffer->disposalMethod() != ImageFrame::DisposeOverwritePrevious);
 
             // Now, if we're at a DisposeNotSpecified or DisposeKeep frame, then
             // we can say we have no alpha if that frame had no alpha.  But
             // since in initFrameBuffer() we already copied that frame's alpha
             // state into the current frame's, we need do nothing at all here.
             //
             // The only remaining case is a DisposeOverwriteBgcolor frame.  If
             // it had no alpha, and its rect is contained in the current frame's
             // rect, we know the current frame has no alpha.
-            if ((prevBuffer->disposalMethod() == ImageFrame::DisposeOverwriteBgcolor) && !prevBuffer->hasAlpha() && buffer.originalFrameRect().contains(prevBuffer->originalFrameRect()))
+            if ((previousBuffer->disposalMethod() == ImageFrame::DisposeOverwriteBgcolor) && !previousBuffer->hasAlpha() && buffer.originalFrameRect().contains(previousBuffer->originalFrameRect()))
                 buffer.setHasAlpha(false);
         }
     }
 
     return true;
 }
 
 size_t GIFImageDecoder::clearCacheExceptFrame(size_t clearExceptFrame)
 {
     // We need to preserve frames such that:
@@ skipping 61 matching lines @@
         if (m_frameBufferCache[*i].status() != ImageFrame::FrameComplete)
             break;
     }
 
     // It is also a fatal error if all data is received and we have decoded all
     // frames available but the file is truncated.
     if (index >= m_frameBufferCache.size() - 1 && isAllDataReceived() && m_reader && !m_reader->parseCompleted())
         setFailed();
 }
 
+void GIFImageDecoder::setupHaveDecodedRowCallbacks(bool isIndex8)
+{
+    phaveDecodedRow = isIndex8
+        ? (&GIFImageDecoder::haveDecodedRowIndex8)
+        : (&GIFImageDecoder::haveDecodedRowN32);
+}
+
 void GIFImageDecoder::parse(GIFParseQuery query)
 {
     if (failed())
         return;
 
     if (!m_reader) {
         m_reader = adoptPtr(new GIFImageReader(this));
         m_reader->setData(m_data);
     }
 
     if (!m_reader->parse(query))
         setFailed();
 }
 
-bool GIFImageDecoder::initFrameBuffer(size_t frameIndex)
+bool GIFImageDecoder::initFrameBufferFromPreviousN32(ImageFrame* const buffer, const ImageFrame& previousBuffer)

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

I was a little surprised to see a const pointer. Are you worried the
implementation is going to change buffer to point to something else? I don't
think Blink does this elsewhere?

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

Done. Thanks. Method renamed to initFrameBufferFromPrevious since the same
functionality is required for Index8.

 {
+    // Preserve the last frame as the starting state for this frame.
+    if (!buffer->copyBitmapData(previousBuffer, ImageFrame::N32))
+        return false;
+
+    if (previousBuffer.disposalMethod() == ImageFrame::DisposeOverwriteBgcolor) {
+        // We want to clear the previous frame to transparent, without
+        // affecting pixels in the image outside of the frame.
+        const IntRect& prevRect = previousBuffer.originalFrameRect();
+        ASSERT(!prevRect.contains(IntRect(IntPoint(), size())));
+        buffer->zeroFillFrameRect(prevRect);
+    }
+    return true;
+}
+
+bool GIFImageDecoder::initFrameBufferN32(size_t frameIndex)
+{
+    setupHaveDecodedRowCallbacks(false);
+
     // Initialize the frame rect in our buffer.
     ImageFrame* const buffer = &m_frameBufferCache[frameIndex];
 
     size_t requiredPreviousFrameIndex = buffer->requiredPreviousFrameIndex();
     if (requiredPreviousFrameIndex == kNotFound) {
         // This frame doesn't rely on any previous data.
-        if (!buffer->setSize(size().width(), size().height()))
+        if (!buffer->setSize(size().width(), size().height(), getBackgroundColor(frameIndex)))
             return setFailed();
     } else {
-        const ImageFrame* prevBuffer = &m_frameBufferCache[requiredPreviousFrameIndex];
-        ASSERT(prevBuffer->status() == ImageFrame::FrameComplete);
-
-        // Preserve the last frame as the starting state for this frame.
-        if (!buffer->copyBitmapData(*prevBuffer))
+        const ImageFrame* previousBuffer = &m_frameBufferCache[requiredPreviousFrameIndex];

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

Again, I think the name change is distracting.

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

Done.

+        ASSERT(previousBuffer->status() == ImageFrame::FrameComplete);
+        if (!initFrameBufferFromPreviousN32(buffer, *previousBuffer))
             return setFailed();
-
-        if (prevBuffer->disposalMethod() == ImageFrame::DisposeOverwriteBgcolor) {
-            // We want to clear the previous frame to transparent, without
-            // affecting pixels in the image outside of the frame.
-            const IntRect& prevRect = prevBuffer->originalFrameRect();
-            ASSERT(!prevRect.contains(IntRect(IntPoint(), size())));
-            buffer->zeroFillFrameRect(prevRect);
-        }
     }
 
     // Update our status to be partially complete.
     buffer->setStatus(ImageFrame::FramePartial);
 
+    // Reset the alpha pixel tracker for this frame.
+    m_currentBufferSawAlpha = false;
+    return true;
+}
+
+bool GIFImageDecoder::isIndex8Applicable(const GIFFrameContext& frame, size_t requiredPreviousFrameIndex) const
+{
+    const GIFColorMap& colorMap = getColorMap(frame, *m_reader);
+    const bool useGlobalColorMap = frame.localColorMap().isDefined();
+
+    if (requiredPreviousFrameIndex == kNotFound) {
+        if (colorMap.getTableSize() == 256 && frame.transparentPixel() >= colorMap.getTableSize()
+            && (useGlobalColorMap && m_reader->backgroundIndex() >= colorMap.getTableSize())

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

Do you need the parentheses here?

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

Done.

+            && (!isAllDataReceived() || !frame.frameRect().contains(IntRect(IntPoint(), size())))) {
+            // Background is filled with transparency or with background color (opaque gifs).
+            // Return false when there is a need for transparency and no space in colormap for it;
+            // when colormap is opaque and full (256 entries) and in the same time:

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

nit: at* the same time

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

Done.

+            // - image is not fully received, so missing part is presented as transparent.
+            // - all frames are not fullscreen, i.e. they are decoded to sub rectangle of image rectangle.

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

This says "all frames", but this only checks a single frame, correct?

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

Done. Yes, the check is per frame, not for all - fixed the comment.

+            return false;
+        }
+        return true;
+    }
+
+    // If frame is sharing colormap with required previous frame, it is possible to keep using Index8.

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

nit:

If frame shares* ...

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

Done.

+    const GIFFrameContext& previousFrame(*(m_reader->frameContext(requiredPreviousFrameIndex)));
+    const GIFColorMap& previousColorMap = getColorMap(previousFrame, *m_reader);
+    return ((colorMap.getPosition() == previousColorMap.getPosition()) && (frame.transparentPixel() == previousFrame.transparentPixel()));
+}
+
+// Helper method for recomputing if frame decoding has no dependency to previous frames.

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

I find this sentence a little awkward. How about:

Helper method to check to see if a previous frame dependency can be removed.

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

Done, "to see " removed.

+void GIFImageDecoder::updateRequiredPreviousFrame(ImageFrame* buffer, const GIFFrameContext& frameContext)
+{
+    ASSERT(m_colorMode == ImageFrame::Index8);
+    if (buffer->requiredPreviousFrameIndex() == kNotFound)
+        return;
+
+    bool isFullScreen = frameContext.frameRect().contains(IntRect(IntPoint(), size()));
+
+    // If transparent pixel is outside color table, there is no transparent pixel.
+    // e.g. Color table in some of the GIFs is having 40 elements and transparentPixel set to 0xFF.

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

nit: "is having" -> "has"

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

Done.

+    bool opaque = frameContext.transparentPixel() >= getColorMap(frameContext, *m_reader).getTableSize();

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

nit: If isFullScreen is false, we do not need to check for opaque.

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

Done.

+
+    // If fullscreen and opaque, the frame is independent of previous frame.
+    if (isFullScreen && opaque)
+        buffer->setRequiredPreviousFrameIndex(kNotFound);
+}
+
+// Returns transparent index or background index.
+unsigned char GIFImageDecoder::getBackgroundIndex(const GIFFrameContext& frameContext) const
+{
+    const size_t background = frameContext.transparentPixel();
+    const GIFColorMap& colorMap = getColorMap(frameContext, *m_reader);
+    if (background < colorMap.getTableSize())
+        return background;
+
+    if (!frameContext.localColorMap().isDefined() && m_reader->backgroundIndex() < colorMap.getTableSize())
+        return m_reader->backgroundIndex();
+    return 0xFF;

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

This still concerns me. Used as an index, 0xFF will be an arbitrary color in the
color table.

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

Done.
There is check in isIndex8Applicable is not allowing it to come here, but
instead of hiding it under implicit short->char conversion that you mentioned
before, changed all to unsigned and there is assert in
ImageFrame::setSizeIndex8.

+}
+
+SkColor GIFImageDecoder::getBackgroundColor(size_t frameIndex) const
+{
+    const GIFFrameContext* frameContext = m_reader->frameContext(frameIndex);
+    unsigned char backgroundIndex = getBackgroundIndex(*frameContext);
+    if (frameContext->transparentPixel() == backgroundIndex)
+        return 0;

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

SK_ColorTRANSPARENT ? Or do we use 0 elsewhere?

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

Changed here to SK_ColorTRANSPARENT.
 
0 is/was in use in haveDecodedRow, but there it is about writing to PixelData
(typedef uint32_t), not to SkColor. 

+    const GIFColorMap::Table& table = getColorMap(*frameContext, *m_reader).table();
+    return (backgroundIndex < table.size() ? table[backgroundIndex]: 0);
+}
+
+bool GIFImageDecoder::initFrameBuffer(size_t frameIndex)
+{
+    ImageFrame* buffer = &m_frameBufferCache[frameIndex];
+    if (buffer->status() != ImageFrame::FrameEmpty) {
+        // If it is partial, decode to the same bitmap.
+        setupHaveDecodedRowCallbacks(buffer->getSkBitmap().colorType() == kIndex_8_SkColorType);

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

If it is partial, shouldn't we already be using the correct callback?

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

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.  

+        return true;
+    }
+
+    if (m_colorMode == ImageFrame::N32) {
+        return initFrameBufferN32(frameIndex);
+    }
+    const GIFFrameContext* frameContext = m_reader->frameContext(frameIndex);
+    updateRequiredPreviousFrame(buffer, *frameContext);
+    const size_t requiredPreviousFrameIndex = buffer->requiredPreviousFrameIndex();
+
+    bool useIndex8 = true;
+    if (requiredPreviousFrameIndex == kNotFound) {
+        if (isIndex8Applicable(*frameContext, requiredPreviousFrameIndex)) {
+            const unsigned char backgroundIndex = getBackgroundIndex(*frameContext);
+            if (!buffer->setSizeIndex8(size().width(), size().height(), getColorMap(*frameContext, *m_reader).table(),
+                backgroundIndex, backgroundIndex == frameContext->transparentPixel()))
+                return setFailed();

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

I don't see this addressed in the style guide, but I find this hard to read -
the code lines up with the condition. I think this looks better as:

if (reallyLongCondition(
    parameter))
{
    code;
}

or

if (reallyLongCondition(
        parameter))
    code;

Others may disagree though.

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

Not sure if I did the right thing, let's see if it improved.

+        } else {
+            return initFrameBufferN32(frameIndex);
+        }
+    } else {
+        const ImageFrame* previousBuffer = &m_frameBufferCache[requiredPreviousFrameIndex];
+        ASSERT(previousBuffer->status() == ImageFrame::FrameComplete);
+
+        // Copy the required completed frame as the starting state for this frame.
+        useIndex8 = previousBuffer->getSkBitmap().colorType() == kIndex_8_SkColorType;
+        if (useIndex8) {
+            if (isIndex8Applicable(*frameContext, requiredPreviousFrameIndex)) {
+                if (!buffer->copyBitmapData(*previousBuffer))
+                    return setFailed();
+                ASSERT(buffer->getSkBitmap().colorType() == kIndex_8_SkColorType);
+            } else {
+                useIndex8 = false;
+            }
+        }
+        // If needed to decode to N32, frame starts as copy of required frame (Index8/N32) to N32.
+        if (!useIndex8 && !initFrameBufferFromPreviousN32(buffer, *previousBuffer))
+            return setFailed();
+    }
+    setupHaveDecodedRowCallbacks(useIndex8);
+
+    // Update our status to be partially complete.
+    buffer->setStatus(ImageFrame::FramePartial);
+
     // Reset the alpha pixel tracker for this frame.
     m_currentBufferSawAlpha = false;
     return true;
 }
 
+bool GIFImageDecoder::canDecodeTo(size_t index, ImageFrame::ColorType outputType)
+{
+    if ((index >= frameCount()) || (m_colorMode == ImageFrame::N32) || failed())

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

if outputType == N32, shouldn't we always return true? It seems like you only
need to go back to see if index8 is supported.

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

No, the recent changes made this deterministic early on, from the constructor
before decoding. If requested mode for decoder (in constructor) was index8,
output for given frame can only be one, index8 or n32. Note that decode API is
like decode(), not like decode(colortype).
Anyway, there is no call canDecodeTo(N32) now in the code and would like to keep
it as it is.

+        return (outputType == m_colorMode);
+
+    // Go from one to previous until calculating if Index8 is supported.
+    size_t frameToDecode = index;
+    ImageFrame::ColorType calculatedOutput = ImageFrame::Index8;
+    while (frameToDecode != kNotFound) {
+        ImageFrame* buffer = &m_frameBufferCache[frameToDecode];
+        if (buffer->status() == ImageFrame::FrameComplete) {
+            // In this case, color type from complete frame is kept.
+            calculatedOutput = static_cast<ImageFrame::ColorType>(buffer->getSkBitmap().colorType());
+            break;
+        }
+        const GIFFrameContext* frameContext = m_reader->frameContext(frameToDecode);
+        updateRequiredPreviousFrame(buffer, *frameContext);
+        if (!isIndex8Applicable(*frameContext, buffer->requiredPreviousFrameIndex())) {
+            calculatedOutput = ImageFrame::N32;
+            break;
+        }
+        frameToDecode = buffer->requiredPreviousFrameIndex();
+    }
+    return (calculatedOutput == outputType);
+}
+
 } // namespace blink