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 17 matching lines @@
 
 #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)
     : ImageDecoder(alphaOption, colorOptions, maxDecodedBytes)
+    , phaveDecodedRow(&GIFImageDecoder::haveDecodedRowIndex8)
     , m_repetitionCount(cAnimationLoopOnce)
+    , m_colorMode(ImageFrame::Index8)
+    , m_forceN32Decoding(false)
 {
 }
 
 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)
+bool GIFImageDecoder::haveDecodedRowN32(size_t frameIndex, GIFRow::const_iterator rowBegin, size_t width, size_t rowNumber, unsigned repeatCount, bool writeTransparentPixels)
 {
+    ASSERT(m_colorMode == ImageFrame::N32 || m_forceN32Decoding);

[scroggo_chromium, 2015/12/03 21:47:21]

It's weird to me that m_colorMode might *not* be N32, while m_forceN32Decoding
is true.

[aleksandar.stojiljkovic, 2015/12/04 00:07:35]

Yes. m_colorMode holds information about what was requested by client. force is
set when switching from index8 to rgba for some frames.
Especially for testing, index8 decoder always set to index8, n32 decoder always
set to n32 and then compare frame by frame pixel by pixel. For index8 some of
frames are index8 some later are n32. 

[aleksandar.stojiljkovic, 2015/12/07 19:24:22]

Done.

     const GIFFrameContext* frameContext = m_reader->frameContext(frameIndex);
     // 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());
     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();
 
     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;
+    if (buffer.status() == ImageFrame::FrameEmpty) {
+        if (!initFrameBuffer(frameIndex))
+            return false;
+        // Tricky part here - decoding first row inits frame, and recognized that the frame doesnt require

[scroggo_chromium, 2015/12/03 21:47:22]

This is tricky. Maybe it would be better for initFrameBuffer to report that it
needed to switch?

[aleksandar.stojiljkovic, 2015/12/04 01:27:27]

I actually plan to move init out of looping through rows - it can be called from
client before calling GIFImageDecoder::haveDecodedRowN32 x height times. Again,
I thought that changing as less as possible would help review but not sure
anymore.

[aleksandar.stojiljkovic, 2015/12/07 19:24:22]

done (removed), init happens before outputting rows.

[scroggo_chromium, 2015/12/08 22:24:51]

Yeah, I think that has to be a judgment call. Changing less can make sense to
confirm that an idea is good (so you don't keep working down a path that will
need to change), but it also makes it hard to imagine the end result.

+        // forced N32 decoding (. initFrameBuffer sets proper haveDecodedRow callback, but for this
+        // first row, we need manually to redirect.
+        // It would be much cleaner to move initFrameBuffer before calling haveDecodedRow, but the
+        // intention here is not to change original code path.
+        if (m_colorMode == ImageFrame::Index8 && !m_forceN32Decoding)

[scroggo_chromium, 2015/12/03 21:47:22]

IIUC, if this is true, initFrameBuffer called initFrameBufferIndex8, so it is
already set up to decode the first row?

[aleksandar.stojiljkovic, 2015/12/04 01:27:27]

yes, for every row check if initialized then initializes and then return to row
processing - I'll change it to initialize, go through rows since you had already
went through this.

[aleksandar.stojiljkovic, 2015/12/07 19:24:21]

Done.

+            return haveDecodedRowIndex8(frameIndex, rowBegin, width, rowNumber, repeatCount, writeTransparentPixels);
+    }
 
     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.
     //
     // The loops below are almost identical. One writes a transparent pixel
     // and one doesn't based on the value of |writeTransparentPixels|.
     // The condition check is taken out of the loop to enhance performance.
     // This optimization reduces decoding time by about 15% for a 3MB image.
     if (writeTransparentPixels) {
         for (; rowBegin != rowEnd; ++rowBegin, ++currentAddress) {
             const size_t sourceValue = *rowBegin;
+            // FIXME - potential optimization: extend colorTable to 256 elems, write 0

[scroggo_chromium, 2015/12/03 21:47:21]

So this optimization is when the colortable is less than 256 elems only?

[aleksandar.stojiljkovic, 2015/12/04 01:27:27]

no; i meant that for colortables of e.g. 32 elements we could allways create 256
elements colortable and change this code bellow (remove *currentAddress =
colorTableIter[sourceValue])
i.e. replace branching by always doing *currentAddress =
colorTableIter[sourceValue]) (colortable would be extended to max sourceValue so
no need to check and transparent entries wuld be in place in the colorTable).

But, then I measured the benefit on different platforms and didn't see
significant improvement trend. Will remove the comment.

[aleksandar.stojiljkovic, 2015/12/07 19:24:22]

Done.

+            // in colortable on index transparentPixel so there would be no need to to this
+            // branching bellow. See index8 implementation.
+            // FIXME optimization: do separate branching for when there is no transparentPixel
+            // (value outside colorTable.size())
             if ((sourceValue != transparentPixel) && (sourceValue < colorTable.size())) {
                 *currentAddress = colorTableIter[sourceValue];
             } else {
                 *currentAddress = 0;
                 m_currentBufferSawAlpha = true;
             }
         }
     } else {
         for (; rowBegin != rowEnd; ++rowBegin, ++currentAddress) {
             const size_t sourceValue = *rowBegin;
             if ((sourceValue != transparentPixel) && (sourceValue < colorTable.size()))
                 *currentAddress = colorTableIter[sourceValue];
             else
                 m_currentBufferSawAlpha = true;
         }
     }
 
     // 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(size_t frameIndex, GIFRow::const_iterator rowBegin, size_t width, size_t rowNumber, unsigned repeatCount, bool writeTransparentPixels)

[scroggo_chromium, 2015/12/03 21:47:21]

It looks like this is largely the same as haveDecodedRowN32. It would be nice to
share more code here.

[aleksandar.stojiljkovic, 2015/12/04 01:27:27]

Agree, duplication for purpose that reviewer easier see that N32 decoding is
kept the same should be highlighted.

+{
+    ASSERT(m_colorMode == ImageFrame::Index8 && !m_forceN32Decoding);
+    const GIFFrameContext* frameContext = m_reader->frameContext(frameIndex);
+    // 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());
+    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();
+
+    if (colorTable.isEmpty())
+        return true;
+
+
+    // Initialize the frame if necessary.
+    ImageFrame& buffer = m_frameBufferCache[frameIndex];
+    if (buffer.status() == ImageFrame::FrameEmpty) {
+        if (!initFrameBufferIndex8(frameIndex))
+            return false;
+        // Tricky part here - decoding first row inits frame, and recognised that the frame requires
+        // N32 decoding. initFrameBuffer sets proper haveDecodedRow callback, but for this
+        // first row, we need manually to redirect.
+        // It would be much cleaner to move initFrameBuffer before calling haveDecodedRow, but the
+        // intention here is not to change original code path.
+        if (m_forceN32Decoding)
+            return haveDecodedRowN32(frameIndex, rowBegin, width, rowNumber, repeatCount, writeTransparentPixels);
+    }
+
+    const size_t transparentPixel = frameContext->transparentPixel();
+    GIFRow::const_iterator rowEnd = rowBegin + (xEnd - xBegin);
+    ImageFrame::PixelData8* currentAddress = buffer.getAddr8(xBegin, yBegin);
+
+    bool opaque = true;
+    if (transparentPixel < colorTable.size()) {
+        // writeTransparentPixels is writing without check, suitable for parallel uint32_t write later.
+        writeTransparentPixels = writeTransparentPixels || buffer.requiredPreviousFrameIndex() == kNotFound;
+        if (writeTransparentPixels) {
+            for (; rowBegin != rowEnd;) {
+                opaque = opaque && (*rowBegin ^ transparentPixel);
+                *currentAddress++ = *rowBegin++;
+            }
+        } else {
+            for (; rowBegin != rowEnd; ++rowBegin, ++currentAddress) {
+                size_t index = *rowBegin;
+                if (index == transparentPixel) {
+                    opaque = false;

[scroggo_chromium, 2015/12/03 21:47:22]

Why do we not want to put index into currentAddress here? (Is it because we
zero-filled?)

[aleksandar.stojiljkovic, 2015/12/04 01:27:26]

It is faster this way (at least for examples I used in test)
In Patch #3 I had it like this:
            opaque = opaque && (*rowBegin ^ transparentPixel);
            *currentAddress++ = *rowBegin++;

It is prefilled to transparent index in buffer->setSizeIndex8(size().width(),
size().height(), colorTable, 
     frameContext->transparentPixel()).

+                } else {
+                    *currentAddress = index;
+                }
+            }
+        }
+    } else {
+        // No transparency to deal with.
+        for (; rowBegin != rowEnd;)
+            *currentAddress++ = *rowBegin++;
+    }
+
+    m_currentBufferSawAlpha = !opaque;
+
+    // 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.
+    // FIXME do nothing frames for Index8 are probably copy of previous frame.

[scroggo_chromium, 2015/12/03 21:47:21]

What does this mean, and what is the FIXME? (i.e. what do you need to
fix/change?)

[aleksandar.stojiljkovic, 2015/12/04 01:27:27]

All fine now, to be removed.

[aleksandar.stojiljkovic, 2015/12/07 19:24:22]

Done.

     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()))) {
@@ skipping 59 matching lines @@
 void GIFImageDecoder::initializeNewFrame(size_t index)
 {
     ImageFrame* buffer = &m_frameBufferCache[index];
     const GIFFrameContext* frameContext = m_reader->frameContext(index);
     buffer->setOriginalFrameRect(intersection(frameContext->frameRect(), IntRect(IntPoint(), size())));
     buffer->setDuration(frameContext->delayTime());
     buffer->setDisposalMethod(frameContext->disposalMethod());
     buffer->setRequiredPreviousFrameIndex(findRequiredPreviousFrame(index, false));
 }
 
 void GIFImageDecoder::decode(size_t index)

[scroggo_chromium, 2015/12/03 21:47:21]

So IIUC, decodeTo (which you added) lets the caller choose, but decode (the
existing one) will just keep decoding to what was already selected?

[aleksandar.stojiljkovic, 2015/12/04 01:27:27]

decodeto wraps decode - decode() is not called directly but only from decodeto.
I was wrong here - by trying to keep the change minimal made it more complicated
for review.

[aleksandar.stojiljkovic, 2015/12/07 19:24:22]

Done - removed decodeTo method. Decoding output set in constructor only.

 {
+    phaveDecodedRow = (m_colorMode == ImageFrame::Index8 && !m_forceN32Decoding)
+        ? (&GIFImageDecoder::haveDecodedRowIndex8)
+        : (&GIFImageDecoder::haveDecodedRowN32);
     parse(GIFFrameCountQuery);
 
     if (failed())
         return;
 
     Vector<size_t> framesToDecode;
     size_t frameToDecode = index;
     do {
         framesToDecode.append(frameToDecode);
         frameToDecode = m_frameBufferCache[frameToDecode].requiredPreviousFrameIndex();
     } while (frameToDecode != kNotFound && m_frameBufferCache[frameToDecode].status() != ImageFrame::FrameComplete);
 
     for (auto i = framesToDecode.rbegin(); i != framesToDecode.rend(); ++i) {
+        bool notSuitableForIndex8(m_forceN32Decoding);
         if (!m_reader->decode(*i)) {

[scroggo_chromium, 2015/12/03 21:47:21]

So decode() may now fail for another reason (wrong color type)?

[aleksandar.stojiljkovic, 2015/12/04 01:27:27]

it could fail for previous reasons (setFailed() called), and if requesting color
switch (changed m_forceN32Decoding).
That is why getInfo() - related to previous discussion about virtual
SkImageGenerator::getInfo() will have to advance through all previous required
frames - to decode full frame and getPixels would just return it.

[aleksandar.stojiljkovic, 2015/12/22 15:19:55]

Done. This code removed.

-            setFailed();
-            return;
+            // Decoder asks to switch color mode as Index8 is not applicable. Try to decode using N32.
+            // This happens only when transparent pixels showing previous frames or offsets are detected
+            // and table is changed.
+            if (notSuitableForIndex8 != m_forceN32Decoding) {

[scroggo_chromium, 2015/12/03 21:47:21]

So GIFImageReader::decode modified m_forceN32Decoding? Maybe it would be better
to have it modify a local variable? (i.e. return a value, or modify an output
parameter)

[aleksandar.stojiljkovic, 2015/12/04 01:27:27]

Acknowledged.

[aleksandar.stojiljkovic, 2015/12/04 01:27:27]

This relates to that init inside haveDecodedRow and then switch index8 first row
-> init (detects it is rgba and init rgba) -> continue first row and all other
rows in this frame with rgba. 

[aleksandar.stojiljkovic, 2015/12/22 15:19:55]

Done. Simplified and removed m_forceN32Decoding. 

+                ASSERT(m_colorMode == ImageFrame::Index8);
+                clearFrameBuffer(*i);
+                // N32 decoding would continue until there is non dependent Index8 frame detected, see initFrameBuffer().
+                phaveDecodedRow = &GIFImageDecoder::haveDecodedRowN32;
+                if (!m_reader->decode(*i)) {
+                    setFailed();
+                    return;
+                }
+            } else {
+                setFailed();
+                return;
+            }
         }
 
         // We need more data to continue decoding.
         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::decodeTo(size_t index, ImageFrame::ColorType outputColor)
+{
+    ASSERT(outputColor == ImageFrame::N32 || outputColor == ImageFrame::Index8);
+    ImageFrame::ColorType c = m_colorMode;
+    m_colorMode = outputColor;
+    decode(index);
+    m_colorMode = c;
+}
+
+bool GIFImageDecoder::canDecodeTo(size_t index, ImageFrame::ColorType outputType)
+{
+    return ((outputType == ImageFrame::N32)
+        || (outputType == ImageFrame::Index8 && !m_forceN32Decoding));
+}
+
+void GIFImageDecoder::setForceN32Decoding(bool value)
+{
+    ASSERT(m_colorMode == ImageFrame::Index8);
+    m_forceN32Decoding = value;
+    phaveDecodedRow = (!value && m_colorMode == ImageFrame::Index8)
+        ? (&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)
 {
+    if (m_colorMode == ImageFrame::Index8) {
+        // Ended up here, since previous frame didn't support Index8 decoding.
+        // Try to switch back to Index( decoding.
+        return initFrameBufferIndex8(frameIndex);
+    }
     // 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()))
             return setFailed();
     } else {
         const ImageFrame* prevBuffer = &m_frameBufferCache[requiredPreviousFrameIndex];
@@ skipping 13 matching lines @@
     }
 
     // Update our status to be partially complete.
     buffer->setStatus(ImageFrame::FramePartial);
 
     // Reset the alpha pixel tracker for this frame.
     m_currentBufferSawAlpha = false;
     return true;
 }
 
+static bool framesUseSameColorTable(const GIFFrameContext *frame1, const GIFFrameContext *frame2)

[scroggo_chromium, 2015/12/03 21:47:22]

I think these should be const references?

[aleksandar.stojiljkovic, 2015/12/04 01:27:27]

Acknowledged.

[aleksandar.stojiljkovic, 2015/12/07 19:24:22]

Done.

+{
+    if (frame1->localColorMap().isDefined() ^ frame2->localColorMap().isDefined())
+        return false;
+    if (!frame1->localColorMap().isDefined()) {
+        // They both use global ColorMap.

[scroggo_chromium, 2015/12/03 21:47:22]

ASSERT(!frame2->localColorMap().isDefined())?

[aleksandar.stojiljkovic, 2015/12/04 01:27:27]

previous ^ should be resolving it, but it is complicated to read.

[aleksandar.stojiljkovic, 2015/12/07 19:24:22]

Done.

+        return (frame1->transparentPixel() == frame1->transparentPixel());
+    }
+    if (frame1->localColorMap().getPosition() == frame2->localColorMap().getPosition()) {

[scroggo_chromium, 2015/12/03 21:47:21]

This block of code is tough to follow. I think some local variables might make
it easier to follow; something like:

auto cmap1 = frame1->localColorMap();
auto cmap2 = frame2->localColorMap();

if (cmap1.getPosition() != cmap2.getPosition())
  return false;

auto trans1 = frame1->transparentPixel();
auto trans2 = frame2->transparentPixel();

return trans1 == trans2 || (trans1 > cmap1.table().size() && trans2 >=
cmap2.table().size());

[aleksandar.stojiljkovic, 2015/12/07 19:24:22]

Done.

+        if (frame1->transparentPixel() == frame2->transparentPixel()
+            || (frame1->transparentPixel() >= frame1->localColorMap().table().size()
+                && frame2->transparentPixel() >= frame2->localColorMap().table().size()))
+            return true;
+    }
+    return false;
+}
+
+bool GIFImageDecoder::initFrameBufferIndex8(size_t frameIndex)
+{
+    ASSERT(m_colorMode == ImageFrame::Index8);
+    // Initialize the frame rect in our buffer.
+    ImageFrame* buffer = &m_frameBufferCache[frameIndex];
+
+    const GIFFrameContext* frameContext = m_reader->frameContext(frameIndex);
+    bool isFullScreen = frameContext->frameRect().contains(IntRect(IntPoint(), size()));
+    const GIFColorMap::Table& colorTable = frameContext->localColorMap().isDefined()
+        ? frameContext->localColorMap().table() : m_reader->globalColorMap().table();
+
+    // Checking frameContext->transparentPixel() != kNotFound is not precise, since
+    // it also needs to include colortable information - if transparent pixels is outside colortable
+    // there is no transparent pixel.
+    // e.g. Colortable in some of the GIFs is having 40 elements andtransparentPixel set to 0xFF.
+    // This is changed here only to affect Index8 and the intention is to keep the N32 code unchanged.

[scroggo_chromium, 2015/12/03 21:47:21]

Are you saying that updating required previous frame index is only done in
Index8? Would N32 benefit from it?

[aleksandar.stojiljkovic, 2015/12/04 01:27:27]

Originally didn't want to change it, tried to deliver another patch for existing
code. Measurements replacing if() with unconditional writes didn't prove as
beneficial for performances - I still have the patch but in some cases it is
faster sometimes slower and didn't like it.

+    bool opaque = frameContext->transparentPixel() >= colorTable.size();
+
+    // The same check - if fullscreen and opaque, then previus is not required to copy - happens in
+    // frameComplete  -move it earlier so it affects also current run (not only starting from next loop).
+    if (isFullScreen && opaque)
+        buffer->setRequiredPreviousFrameIndex(kNotFound);
+
+    size_t requiredPreviousFrameIndex = buffer->requiredPreviousFrameIndex();
+
+    if (requiredPreviousFrameIndex == kNotFound) {
+        // This frame doesn't rely on any previous data.
+        // Reset not suitable and table changed information as this is full new frame.

[scroggo_chromium, 2015/12/03 21:47:21]

What does "Reset not suitable" mean?

[aleksandar.stojiljkovic, 2015/12/04 01:27:27]

m_forceN32Decoding can be reset to false, and resume with Index8 if originally
requested by client.

[aleksandar.stojiljkovic, 2015/12/07 19:24:22]

Done.

+        // It is OK to switch back from N32 to Index8 if it was originally requested.
+        setForceN32Decoding(false);
+        phaveDecodedRow = &GIFImageDecoder::haveDecodedRowIndex8;
+        if (!buffer->setSizeIndex8(size().width(), size().height(), colorTable, frameContext->transparentPixel()))

[aleksandar.stojiljkovic, 2015/12/04 01:27:27]

This call is prefilling it so that all pixels have transparentPixel index.

+            return setFailed();
+    } else {
+        const ImageFrame* prevBuffer = &m_frameBufferCache[requiredPreviousFrameIndex];
+        ASSERT(prevBuffer->status() == ImageFrame::FrameComplete);
+        ASSERT(!isFullScreen || !opaque);
+
+        // Preserve the last frame as the starting state for this frame.
+        // If in Index8 mode, and colorTable is not changed nor setForceN32Decoding set,
+        // preserve frame same way as for N32.
+        bool useN32 = true;
+        if (!m_forceN32Decoding) {

[scroggo_chromium, 2015/12/03 21:47:21]

Wait, should we reach this method (or get this far) if m_forceN32Decoding is
true?

[aleksandar.stojiljkovic, 2015/12/07 19:24:22]

I think it looks more logical now - initFrameBufferIndex8 is called to
initialize all frames, except for testing mode when decoder is constructed to do
N32. So, N32 is only for testing.
Back to question: yes, m_forceN32Decoding is used on Index8 mode to mark that
last decoded frame was forced to N32 (althrough m_colorMode is Index8).

[aleksandar.stojiljkovic, 2015/12/22 15:19:55]

Done. Simplified and removed m_forceN32Decoding. 

+            // If table is changed and in the same time, there is transparency or following frame
+            // is subrect, cannot do Index8. Otherwise, with the same table, no problem to continue
+            // with Index8 copy of dependent frame as starting point for this frame.
+            if (framesUseSameColorTable(frameContext, m_reader->frameContext(requiredPreviousFrameIndex))) {
+                useN32 = false;
+                if (!buffer->copyBitmapData(*prevBuffer))
+                    return setFailed();
+                ASSERT(buffer->getSkBitmap().colorType() == kIndex_8_SkColorType);
+            } else {
+                setForceN32Decoding(true);
+                useN32 = true;
+            }
+        }
+        // For unsupported Index8 and N32, next frame starts as previous frame copy to N32.
+        if (useN32 && !buffer->copyBitmapData(*prevBuffer, ImageFrame::N32))
+            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;
+}
 } // namespace blink