Move gif image decoder to SkCodec

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
  *    documentation and/or other materials provided with the distribution.
  *
  * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY
  * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
  * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
  * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE COMPUTER, INC. OR
  * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
  * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
  * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
  * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
  * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #include "platform/image-decoders/gif/GIFImageDecoder.h"
 
-#include "platform/image-decoders/gif/GIFImageReader.h"
+#include "base/logging.h"
 #include "wtf/NotFound.h"
 #include "wtf/PtrUtil.h"
 #include <limits>
 
 namespace blink {
 
 GIFImageDecoder::GIFImageDecoder(AlphaOption alphaOption,
                                  ColorSpaceOption colorOptions,
                                  sk_sp<SkColorSpace> targetColorSpace,
                                  size_t maxDecodedBytes)
     : ImageDecoder(alphaOption,
                    colorOptions,
                    std::move(targetColorSpace),
                    maxDecodedBytes),
-      m_repetitionCount(cAnimationLoopOnce) {}
+      m_currentBufferSawAlpha(false),
+      m_codec(),
+      m_segmentStream(new SegmentStream{}) {}

[scroggo_chromium, 2016/12/13 16:57:36]

Why not make m_segmentStream a full member?

Oh wait, m_codec is going to want to take ownership of it...

I think you need to do the following:

- Store a raw pointer to m_segmentStream
- In the destructor, delete m_segmentStream if !m_codec

A related issue - SkCodec::NewFromStream will delete m_segmentStream on failure,
so you'll need to recreate it if that call fails.

[cblume, 2016/12/14 09:16:56]

Your idea about a raw pointer makes sense to me.

If SkCodec::NewFromStream deletes the memory on failure, we lose any data passed
into it previously. This means the only safe way for me to create SkCodec would
be to first make a deep copy of my SegmentStream and its SegmentReader.

Would it be better for SkCodec::NewFromStream to not delete the memory on error?
It seems much easier from my end.

[scroggo_chromium, 2016/12/14 17:49:23]

NewFromStream deletes the object - not the memory. The memory still lives inside
the SegmentReader.
I do not see how that is necessary. The SegmentReader will stick around even if
the SegmentStream is deleted. I think you just need to do the following:

  if (!m_segmentStream) {
    m_segmentStream = new SegmentStream();
  }
  m_segmentStream->setReader(data, isAllDataReceived());
  if (!m_codec) {
    m_codec.reset(SkCodec::NewFromStream(m_segmentStream));
    if (!m_codec) {
      // NewFromStream deleted the stream
      m_segmentStream = nullptr;
    }
  }
Here's some history:

SkStream used to be ref-counted. Many classes in Skia were ref-counted, but we
have been removing it from classes where we don't think it makes sense. In the
case of SkStream, which is a stateful object, if multiple objects have a ref,
and are presumably reading from it, each object's reads are going to interfere
with the other. So we removed ref-counting from SkStream.

With SkCodec (and SkImageGenerator before it, which was a model for SkCodec), we
decided to delete the SkStream on failure because on success, the SkCodec is
expected to take ownership of the SkStream. Also "taking ownership" in the case
of failure prevents the client from having to do the following:

  SkStream* stream = new SkStream(...);
  std::unique_ptr<SkCodec> codec(SkCodec::NewFromStream(stream));
  if (!codec) {
    delete stream;
  }

Prior to integration with Chromium, we were not considering a stream that might
need to also receive more data, so there was no need to have a second owner. But
you're correct, this does make things trickier on your end.

I think the ultimate solution is to use a different abstraction of the input. I
have not figured out exactly what that looks like though...

[cblume, 2016/12/16 02:57:10]

Done.

 
 GIFImageDecoder::~GIFImageDecoder() {}
 
 void GIFImageDecoder::onSetData(SegmentReader* data) {
-  if (m_reader)
-    m_reader->setData(data);
+  // Add the segment to our stream
+  m_segmentStream->setReader(data, isAllDataReceived());
+
+  // If we don't have a SkCodec yet, create one from the stream
+  if (!m_codec) {
+    m_codec.reset(SkCodec::NewFromStream(m_segmentStream.get()));
+  }
 }
 
 int GIFImageDecoder::repetitionCount() const {
+  CHECK(m_codec);
+
   // This value can arrive at any point in the image data stream.  Most GIFs
   // in the wild declare it near the beginning of the file, so it usually is
   // set by the time we've decoded the size, but (depending on the GIF and the
-  // packets sent back by the webserver) not always.  If the reader hasn't
-  // seen a loop count yet, it will return cLoopCountNotSeen, in which case we
-  // should default to looping once (the initial value for
-  // |m_repetitionCount|).
+  // packets sent back by the webserver) not always.
   //
-  // There are some additional wrinkles here. First, ImageSource::clear()
-  // may destroy the reader, making the result from the reader _less_
-  // authoritative on future calls if the recreated reader hasn't seen the
-  // loop count.  We don't need to special-case this because in this case the
-  // new reader will once again return cLoopCountNotSeen, and we won't
-  // overwrite the cached correct value.
-  //
-  // Second, a GIF might never set a loop count at all, in which case we
-  // should continue to treat it as a "loop once" animation.  We don't need
-  // special code here either, because in this case we'll never change
-  // |m_repetitionCount| from its default value.
-  //
-  // Third, we use the same GIFImageReader for counting frames and we might
-  // see the loop count and then encounter a decoding error which happens
-  // later in the stream. It is also possible that no frames are in the
-  // stream. In these cases we should just loop once.
-  if (isAllDataReceived() && parseCompleted() && m_reader->imagesCount() == 1)
-    m_repetitionCount = cAnimationNone;
-  else if (failed() || (m_reader && (!m_reader->imagesCount())))
-    m_repetitionCount = cAnimationLoopOnce;
-  else if (m_reader && m_reader->loopCount() != cLoopCountNotSeen)
-    m_repetitionCount = m_reader->loopCount();
-  return m_repetitionCount;
+  // SkCodec will parse forward in the file if the repetition count has not been
+  // seen yet.
+
+  int repetitionCount = m_codec->getRepetitionCount();
+  switch (repetitionCount) {
+    case 0:
+      return cAnimationNone;
+    case SkCodec::kRepetitionCountInfinite:
+      return cAnimationLoopInfinite;
+    default:
+      return repetitionCount;
+  }
 }
 
 bool GIFImageDecoder::frameIsCompleteAtIndex(size_t index) const {
-  return m_reader && (index < m_reader->imagesCount()) &&
-         m_reader->frameContext(index)->isComplete();
+  CHECK(m_codec);
+
+  std::vector<SkCodec::FrameInfo> frameInfos = m_codec->getFrameInfo();
+
+  // If https://skia-review.googlesource.com/c/5635/ lands
+  if (index == 0) {
+    return (frameInfoes.size() >= 1 || m_segmentStream->isAtEnd);

[scroggo_chromium, 2016/12/13 16:57:36]

frameInfos*

m_segmentStream->isAtEnd()* - but doesn't the GIFImageDecoder know if all the
data was received? isAllDataReceived()?

And I think you want frameInfos.size() > 1, not >=. > means we've seen the
second frame, so the first frame must be complete.

Back to our discussion on those CLs - we'll be wrong between receiving the start
and the end of the second frame; during that time we'll incorrectly report that
frame 0 is incomplete.

[cblume, 2016/12/14 09:16:56]

Done.

Yeah, we'll be wrong until the end of the second frame.
If we land the CL where SkCodec exposes whether the frame is complete or not, I
can report this without error (the commented out section below).
If we instead land the CL where SkCodec always reports that frame 0 is complete,
I don't know a way to not report incorrectly. Do you have an idea?

[scroggo_chromium, 2016/12/14 17:49:23]

My hope was that we didn't actually need to know whether the first frame is
complete - at least without decoding. I brought up the reasons we want to know
whether the frame is complete here:

https://skia-review.googlesource.com/c/5635/#message-78219d862632f346d7a9ab90...

In most of those cases, I think we could actually go based on whether the frame
is decoded (rather than whether the frame was received), but that would require
some refactoring. I think https://skia-review.googlesource.com/c/5703/ is the
way to go here.

[cblume, 2016/12/16 02:57:10]

Done.

+    // Either We have found a second frame, which means frame 0 is complete,
+    // or we have all the data, the image could be a single-frame and complete.
+  }
+
+  return frameInfos.size() > index;
+
+  // If https://skia-review.googlesource.com/c/5703/ lands:
+  // if (frameInfos.size() < index) {
+  // // index out of bounds as far as we have parsed
+  // return false;
+  //}
+  //
+  // return frameInfos[index].fFullyReceived;
 }
 
 float GIFImageDecoder::frameDurationAtIndex(size_t index) const {
-  return (m_reader && (index < m_reader->imagesCount()) &&
-          m_reader->frameContext(index)->isHeaderDefined())
-             ? m_reader->frameContext(index)->delayTime()
-             : 0;
+  CHECK(m_codec);
+
+  std::vector<SkCodec::FrameInfo> frameInfos = m_codec->getFrameInfo();
+
+  if (frameInfos.size() < index) {
+    // index out of bounds as far as we have parsed
+    return 0;
+  }
+
+  return frameInfos[index].fDuration;
 }
 
-bool GIFImageDecoder::setFailed() {
-  m_reader.reset();
-  return ImageDecoder::setFailed();
-}
+void GIFImageDecoder::decodeSize() {
+  CHECK(m_codec);
 
-bool GIFImageDecoder::haveDecodedRow(size_t frameIndex,
-                                     GIFRow::const_iterator rowBegin,
-                                     size_t width,
-                                     size_t rowNumber,
-                                     unsigned repeatCount,
-                                     bool writeTransparentPixels) {
-  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().getTable()
-          : m_reader->globalColorMap().getTable();
-
-  if (colorTable.isEmpty())
-    return true;
-
-  GIFColorMap::Table::const_iterator colorTableIter = colorTable.begin();
-
-  // Initialize the frame if necessary.
-  ImageFrame& buffer = m_frameBufferCache[frameIndex];
-  if (!initFrameBuffer(frameIndex))
-    return false;
-
-  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;
-      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::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 (!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->getDisposalMethod() !=
-             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->getDisposalMethod() ==
-           ImageFrame::DisposeOverwriteBgcolor) &&
-          !prevBuffer->hasAlpha() &&
-          buffer.originalFrameRect().contains(prevBuffer->originalFrameRect()))
-        buffer.setHasAlpha(false);
-    }
-  }
-
-  return true;
-}
-
-void GIFImageDecoder::clearFrameBuffer(size_t frameIndex) {
-  if (m_reader &&
-      m_frameBufferCache[frameIndex].getStatus() == ImageFrame::FramePartial) {
-    // Reset the state of the partial frame in the reader so that the frame
-    // can be decoded again when requested.
-    m_reader->clearDecodeState(frameIndex);
-  }
-  ImageDecoder::clearFrameBuffer(frameIndex);
+  SkImageInfo imageInfo = m_codec->getInfo();
+  setSize(imageInfo->width(), imageInfo->height);
 }
 
 size_t GIFImageDecoder::decodeFrameCount() {
-  parse(GIFFrameCountQuery);
-  // If decoding fails, |m_reader| will have been destroyed.  Instead of
-  // returning 0 in this case, return the existing number of frames.  This way
-  // if we get halfway through the image before decoding fails, we won't
-  // suddenly start reporting that the image has zero frames.
-  return failed() ? m_frameBufferCache.size() : m_reader->imagesCount();
+  std::vector<SkCodec::FrameInfo> frameInfos = m_codec->getFrameInfo();
+  return frameInfos.size();
 }
 
 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->getDisposalMethod());
-  buffer->setRequiredPreviousFrameIndex(
-      findRequiredPreviousFrame(index, false));
+  CHECK(m_codec);
+
+  if (m_frameBufferCache.size() < index) {

[scroggo_chromium, 2016/12/13 16:57:36]

I think you want <=, here and elsewhere. (And the comment below may not be
necessary. Maybe it's just obvious to me, and not to someone else.)

Should this ever happen in this case, though? In the old code we do not make
this check. I think this only gets called when m_frameBufferCache is big enough.

[cblume, 2016/12/14 09:16:56]

Done.
I think you are right that it is not needed. I guess I was being paranoid.

+    // index out of bounds as far as we have parsed
+    return;
+  }
+
+  ImageFrame& frame = m_frameBufferCache[index];
+  std::vector<SkCodec::FrameInfo> frameInfos = m_codec->getFrameInfo();

[scroggo_chromium, 2016/12/13 16:57:36]

Since we call this so often, I think we should consider caching it. (One option
I like is to make SkCodec store it and return a reference. It knows whether it
needed to change anything, so when it parses it can only fill in the new ones.
Then you don't have to worry about it here.)

[cblume, 2016/12/14 09:16:56]

I like the idea of SkCodec having a method that returns a const reference.

+
+  frame.setOriginalFrameRect(size());
+  frame.setDuration(frameInfos[index].fDuration);
+  frame.setDisposalMethod(???);
+  // TODO: SkCodec doesn't expose the disposal method.

[scroggo_chromium, 2016/12/13 16:57:36]

These might just be awkward during the transition. I'm not convinced you need to
know the disposal method or original frame rect, since that will be handled
internally by SkCodec. But maybe I'm missing something?

[cblume, 2016/12/14 09:16:56]

I agree. I'm hoping this stuff isn't handled by the level above which calls into
the decoder.

My fear is that since these are here maybe they are.

I suppose even if it is, I can present each frame as being completely
independent and taking the whole area and having whatever disposal method. Then
I can let SkCodec handle the reality of it.

+  size_t requiredPreviousFrame = frameInfos[index].fRequiredFrame;
+  if (requiredPreviousFrame == SkCodec::kNone) {
+    requiredPreviousFrame = WTF::kNotFound;
+  }
+  frame.setRequiredPreviousFrameIndex(requiredPreviousFrame);
 }
 
 void GIFImageDecoder::decode(size_t index) {
-  parse(GIFFrameCountQuery);
+  CHECK(m_codec);
 
-  if (failed())
+  if (m_frameBufferCache.size() < index) {
+    // index out of bounds as far as we have parsed
     return;
+  }
 
   updateAggressivePurging(index);
 
-  Vector<size_t> framesToDecode = findFramesToDecode(index);
-  for (auto i = framesToDecode.rbegin(); i != framesToDecode.rend(); ++i) {
-    if (!m_reader->decode(*i)) {
-      setFailed();
-      return;
-    }
+  SkImageInfo imageInfo = m_codec->getInfo();
+  if (imageInfo.colorType() == kIndex8_SkColorType) {
+    // imageInfo's color type may be influenced by the untrusted image file.

[scroggo_chromium, 2016/12/13 16:57:36]

We report index8 if the GIF can support it (for the first frame). A GIF frame
only uses 256 colors directly, but if it blends with a prior frame, it may use
more. Since we only consider the first frame to potentially use index8, we do
not need to worry about prior frames, but we do need to worry about
transparency. If the frame is subset, and it does *not* have a transparent
pixel, we don't support index8, since we don't have anything to fill the
background with.

Here's the Skia code:
https://skia.googlesource.com/skia/+/36aa176d0/third_party/gif/SkGifImageRead...

(We could take this even further - if there is no transparent pixel, but the
color table is less than 256 colors, we could add an extra color to be
transparent. But we have not pursued that.)

All of this is just to say that it's fine for the codec to report index8. As a
client, you just need to request the color type that you want. In this case, you
want kN32:

  SkImageInfo imageInfo = m_codec->getInfo()->makeColorType(kN32_SkColorType);

[cblume, 2016/12/14 09:16:56]

Done.

+    // Index8 has special behavior when getting the pixels, which we do not yet
+    // handle
+    return;
+  }
 
-    // If this returns false, we need more data to continue decoding.
-    if (!postDecodeProcessing(*i))
-      break;
+  SkCodec::Options getPixelsOptions;
+  getPixelsOptions.fFrameIndex = index;
+  getPixelsOptions.fHasPriorFrame = false;
+
+  // TODO: On the second loop, can we avoid re-copying and decoding?
+  // Said another way, is there a way to know if we already populated this

[scroggo_chromium, 2016/12/13 16:57:36]

Check frame.getStatus()?

[cblume, 2016/12/14 09:16:56]

Hrmmm I seem to be able to check for empty, incomplete, and complete.
I can imagine a frame has been completely downloaded but not decoded. Or we
decoded it the first pass, purged the cache, and need to decode it again even
though it is complete.

So maybe I should be checking for empty?

But seeing as how this deals with completeness, it feels less like a check of
the cache.

[scroggo_chromium, 2016/12/14 17:49:23]

If i was purged, m_frameBufferCache[i] should be FrameEmpty. If we've started,
it should be FramePartial. (We currently set it to FramePartial in
initFrameBuffer [1], but you probably want to skip that method entirely.)

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...
I don't understand. If the cache (m_frameBufferCache[i]?) is FrameComplete,
isn't that how you know that it is complete?

[cblume, 2016/12/16 02:57:10]

I guess we have two types of "complete": completely received and completely
decoded.

For example, on the second pass through an animation the image is completely
downloaded but if we purged the cache then it is not completely decoded.

I guess I was just mixing the two up since we also have functions like
frameCompleteAtIndex().

You are right that this "complete" relates to the decode and it is what I need.

Maybe later I will rename those things to be more clear.

+  // frame?
+  ImageFrame& frame = m_frameBufferCache[index];
+  size_t requiredPreviousFrameIndex = frame.requiredPreviousFrameIndex();
+  if (requiredPreviousFrameIndex == WTF::kNotFound) {

[scroggo_chromium, 2016/12/13 16:57:36]

I think you mean != WTF::kNotFound?

[cblume, 2016/12/14 09:16:56]

Done.

+    getPixelsOptions.fHasPriorFrame = true;
+    ImageFrame& requiredPreviousFrame =
+        m_frameBufferCache[requiredPreviousFrameIndex];
+    frame.copyBitmapData(requiredPreviousFrame);

[scroggo_chromium, 2016/12/13 16:57:36]

Doesn't this happen in initFrameBuffer?

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...

[cblume, 2016/12/14 09:16:56]

Done.

[scroggo_chromium, 2016/12/14 17:49:23]

D'oh, and then I told you not to call that method...

[cblume, 2016/12/16 02:57:10]

I took another look again at initFrameBuffer.
It copies the bitmap data when we reply on the previous frame, fills with BG
color if that is the disposal method, sets the rect, and sets the frame to
partially complete.

Skia handles the disposal method and rect business. I think all I need to do is
copy the previous buffer (put copyBitmapData back in) and add partial/complete
flags.

Another thing initFrameBuffer() does is use some default color space for frames
with no prior dependency. I hadn't been paying attention to color space at all.
Maybe I should add something for that?

[scroggo_chromium, 2016/12/16 15:03:47]

I'd sync up with msarett@.

   }
 
+  SkCodec::Result getPixelsResult =

[scroggo_chromium, 2016/12/13 16:57:36]

You want to switch from getPixels() to startIncrementalDecode() and
incrementalDecode(). Those let you continue decoding where you left off.

[cblume, 2016/12/14 09:16:56]

That makes sense.

I'll have to research the behavior of Blink calling into the ImageDecoder to see
if it attempts an incremental code on the first frame but doesn't attempt any
decodes on other frames until they are complete.

Reading the comments in SkCodec I had a question:
https://cs.chromium.org/chromium/src/third_party/skia/include/codec/SkCodec.h...
Does it only decode complete rows? Can't have a partial row?

If it can decode a partial row then I think I need to initialize the SkBitmap
I'm using with either the background color or full alpha or something.

[scroggo_chromium, 2016/12/14 17:49:23]

As I understand it, Blink waits until a frame after the first is complete [1].
But it is fine to call SkCodec::incrementalDecode() on a complete image.

[1]
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...
It only decodes complete rows.
Chrome already fills the bitmap with transparent - again currently in
initFrameBuffer, though I think you'll want to handle it yourself. That said,
the out parameter from incrementalDecode() tells you how many rows were decoded,
so you could wait until you call the method, and only fill the remaining rows if
the return value is kIncompleteInput (and it's only necessary on the first
pass).

[cblume, 2016/12/16 02:57:10]

Done.

+      m_codec->getPixels(imageInfo, frame.getPixels(), frame.rowBytes(),
+                         &getPixelsOptions, nullptr, nullptr);
+  if (getPixelResult != kSuccess) {

[scroggo_chromium, 2016/12/13 16:57:36]

kIncomplete is also okay, so long as it's the latest frame.

[cblume, 2016/12/14 09:16:56]

Done.

+    setFailed();
+    return;
+  }
+
+  if (!postDecodeProcessing(index)) {
+    return;
+  }
+
+  // If we do not end up landing https://skia-review.googlesource.com/c/5635/
+  // ...
+
   // 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())
+  if (index >= m_frameBufferCache.size() - 1 && isAllDataReceived()) {
     setFailed();
-}
-
-void GIFImageDecoder::parse(GIFParseQuery query) {
-  if (failed())
-    return;
-
-  if (!m_reader) {
-    m_reader = makeUnique<GIFImageReader>(this);
-    m_reader->setData(m_data);
   }
-
-  if (!m_reader->parse(query))
-    setFailed();
-}
-
-void GIFImageDecoder::onInitFrameBuffer(size_t frameIndex) {
-  m_currentBufferSawAlpha = false;
 }
 
 bool GIFImageDecoder::canReusePreviousFrameBuffer(size_t frameIndex) const {
   DCHECK(frameIndex < m_frameBufferCache.size());
   return m_frameBufferCache[frameIndex].getDisposalMethod() !=
          ImageFrame::DisposeOverwritePrevious;
 }
 
 }  // namespace blink