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 <limits>
-#include "platform/image-decoders/gif/GIFImageReader.h"
 #include "platform/wtf/NotFound.h"
 #include "platform/wtf/PtrUtil.h"
+#include "third_party/skia/include/core/SkImageInfo.h"
 
 namespace blink {
 
 GIFImageDecoder::GIFImageDecoder(AlphaOption alpha_option,
                                  const ColorBehavior& color_behavior,
                                  size_t max_decoded_bytes)
     : ImageDecoder(alpha_option, color_behavior, max_decoded_bytes),
-      repetition_count_(kAnimationLoopOnce) {}
-
-GIFImageDecoder::~GIFImageDecoder() {}
+      codec_(),
+      segment_stream_(nullptr) {}
+
+GIFImageDecoder::~GIFImageDecoder() {
+  if (!codec_) {
+    // if we did not create |codec_| and thus did not pass ownership to it
+    if (segment_stream_)
+      delete segment_stream_;
+  }
+}
 
 void GIFImageDecoder::OnSetData(SegmentReader* data) {
-  if (reader_)
-    reader_->SetData(data);
+  if (!data) {
+    if (segment_stream_)
+      segment_stream_->SetReader(nullptr);
+    return;
+  }
+
+  if (!segment_stream_)
+    segment_stream_ = new SegmentStream();
+
+  segment_stream_->SetReader(PassRefPtr<SegmentReader>(data));
+
+  if (!codec_) {
+    codec_.reset(SkCodec::NewFromStream(segment_stream_, nullptr));
+    if (!codec_) {
+      // |segment_stream_|'s ownership is passed into NewFromStream.
+      // It is deleted if NewFromStream fails.
+      // If NewFromStream fails, we set |segment_stream_| to null so
+      // we aren't pointing to reclaimed memory.
+      segment_stream_ = nullptr;
+      return;
+    }
+
+    // SkCodec::NewFromStream will read enough of the image to get the image
+    // size.
+    SkImageInfo image_info = codec_->getInfo();
+    SetSize(image_info.width(), image_info.height());
+  }
 }
 
 int GIFImageDecoder::RepetitionCount() const {
+  if (!codec_)
+    return kAnimationLoopOnce;
+
+  DCHECK(!Failed());
+
   // 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 kCLoopCountNotSeen, in which case we
-  // should default to looping once (the initial value for
-  // |repetition_count_|).
+  // 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 kCLoopCountNotSeen, 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
-  // |repetition_count_| 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() && reader_->ImagesCount() == 1)
-    repetition_count_ = kAnimationNone;
-  else if (Failed() || (reader_ && (!reader_->ImagesCount())))
-    repetition_count_ = kAnimationLoopOnce;
-  else if (reader_ && reader_->LoopCount() != kCLoopCountNotSeen)
-    repetition_count_ = reader_->LoopCount();
-  return repetition_count_;
+  // SkCodec will parse forward in the file if the repetition count has not been
+  // seen yet.
+  int repetition_count = codec_->getRepetitionCount();
+
+  switch (repetition_count) {
+    case 0: {
+      size_t frame_count = codec_->getFrameCount();
+      if (IsAllDataReceived() && frame_count == 1)
+        return kAnimationNone;
+
+      return kAnimationLoopOnce;
+    }
+    case SkCodec::kRepetitionCountInfinite:
+      return kAnimationLoopInfinite;
+    default:
+      return repetition_count;
+  }
 }
 
 bool GIFImageDecoder::FrameIsCompleteAtIndex(size_t index) const {
-  return reader_ && (index < reader_->ImagesCount()) &&
-         reader_->FrameContext(index)->IsComplete();
+  if (!codec_)
+    return false;
+
+  if (static_cast<size_t>(codec_->getFrameCount()) <= index)
+    return false;
+
+  SkCodec::FrameInfo frame_info;
+  codec_->getFrameInfo(index, &frame_info);
+  return frame_info.fFullyReceived;
 }
 
 float GIFImageDecoder::FrameDurationAtIndex(size_t index) const {
-  return (reader_ && (index < reader_->ImagesCount()) &&
-          reader_->FrameContext(index)->IsHeaderDefined())
-             ? reader_->FrameContext(index)->DelayTime()
-             : 0;
+  if (index < frame_buffer_cache_.size())
+    return frame_buffer_cache_[index].Duration();
+  return 0;
 }
 
 bool GIFImageDecoder::SetFailed() {
-  reader_.reset();
+  DCHECK(codec_);
+
+  segment_stream_ = nullptr;
+  codec_ = nullptr;
   return ImageDecoder::SetFailed();
 }
 
-bool GIFImageDecoder::HaveDecodedRow(size_t frame_index,
-                                     GIFRow::const_iterator row_begin,
-                                     size_t width,
-                                     size_t row_number,
-                                     unsigned repeat_count,
-                                     bool write_transparent_pixels) {
-  const GIFFrameContext* frame_context = reader_->FrameContext(frame_index);
-  // The pixel data and coordinates supplied to us are relative to the frame's
-  // origin within the entire image size, i.e.
-  // (frameC_context->xOffset, frame_context->yOffset). There is no guarantee
-  // that width == (size().width() - frame_context->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 x_begin = frame_context->XOffset();
-  const int y_begin = frame_context->YOffset() + row_number;
-  const int x_end = std::min(static_cast<int>(frame_context->XOffset() + width),
-                             Size().Width());
-  const int y_end = std::min(
-      static_cast<int>(frame_context->YOffset() + row_number + repeat_count),
-      Size().Height());
-  if (!width || (x_begin < 0) || (y_begin < 0) || (x_end <= x_begin) ||
-      (y_end <= y_begin))
-    return true;
-
-  const GIFColorMap::Table& color_table =
-      frame_context->LocalColorMap().IsDefined()
-          ? frame_context->LocalColorMap().GetTable()
-          : reader_->GlobalColorMap().GetTable();
-
-  if (color_table.IsEmpty())
-    return true;
-
-  GIFColorMap::Table::const_iterator color_table_iter = color_table.begin();
-
-  // Initialize the frame if necessary.
-  ImageFrame& buffer = frame_buffer_cache_[frame_index];
-  if (!InitFrameBuffer(frame_index))
-    return false;
-
-  const size_t transparent_pixel = frame_context->TransparentPixel();
-  GIFRow::const_iterator row_end = row_begin + (x_end - x_begin);
-  ImageFrame::PixelData* current_address = buffer.GetAddr(x_begin, y_begin);
-
-  // 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 |write_transparent_pixels|.
-  // 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 (write_transparent_pixels) {
-    for (; row_begin != row_end; ++row_begin, ++current_address) {
-      const size_t source_value = *row_begin;
-      if ((source_value != transparent_pixel) &&
-          (source_value < color_table.size())) {
-        *current_address = color_table_iter[source_value];
-      } else {
-        *current_address = 0;
-        current_buffer_saw_alpha_ = true;
+size_t GIFImageDecoder::DecodeFrameCount() {
+  if (!codec_)
+    return 0;
+
+  if (Failed() || segment_stream_->IsCleared())
+    return 0;
+
+  return codec_->getFrameCount();
+}
+
+void GIFImageDecoder::InitializeNewFrame(size_t index) {
+  DCHECK(codec_);
+
+  ImageFrame& frame = frame_buffer_cache_[index];
+  // SkCodec does not inform us if only a portion of the image was updated
+  // in the current frame. Because of this, rather than correctly filling in
+  // the frame rect, we set the frame rect to be the image's full size.
+  // The original frame rect is not used, anyway.
+  IntSize full_image_size = Size();
+  frame.SetOriginalFrameRect(IntRect(IntPoint(), full_image_size));
+
+  SkCodec::FrameInfo frame_info;
+  codec_->getFrameInfo(index, &frame_info);
+  frame.SetDuration(frame_info.fDuration);
+  frame.SetHasAlpha(!SkAlphaTypeIsOpaque(frame_info.fAlphaType));
+  size_t required_previous_frame_index;
+  if (frame_info.fRequiredFrame == SkCodec::kNone) {
+    required_previous_frame_index = WTF::kNotFound;
+  } else {
+    required_previous_frame_index =
+        static_cast<size_t>(frame_info.fRequiredFrame);
+  }
+  frame.SetRequiredPreviousFrameIndex(required_previous_frame_index);
+  // The disposal method is not required any more, but is left in place
+  // for the other image decoders that do not yet rely on SkCodec.
+  // For now, fill it with DisposeKeep.
+  frame.SetDisposalMethod(ImageFrame::kDisposeKeep);
+}
+
+void GIFImageDecoder::Decode(size_t index) {
+  if (!codec_)
+    return;
+
+  DCHECK(!Failed());
+
+  DCHECK_LT(index, frame_buffer_cache_.size());
+
+  if (segment_stream_->IsCleared())
+    return;
+
+  UpdateAggressivePurging(index);
+  SkImageInfo image_info = codec_->getInfo()
+                               .makeColorType(kN32_SkColorType)
+                               .makeColorSpace(ColorSpaceForSkImages());
+
+  SkCodec::Options options;
+  options.fFrameIndex = index;
+  options.fPriorFrame = SkCodec::kNone;
+  options.fZeroInitialized = SkCodec::kNo_ZeroInitialized;
+
+  ImageFrame& frame = frame_buffer_cache_[index];
+  if (frame.GetStatus() == ImageFrame::kFrameEmpty) {
+    size_t required_previous_frame_index = frame.RequiredPreviousFrameIndex();
+    if (required_previous_frame_index == WTF::kNotFound) {
+      frame.AllocatePixelData(Size().Width(), Size().Height(),
+                              ColorSpaceForSkImages());
+    } else {
+      ImageFrame& required_previous_frame =
+          frame_buffer_cache_[required_previous_frame_index];
+
+      if (required_previous_frame.GetStatus() != ImageFrame::kFrameComplete)
+        Decode(required_previous_frame_index);
+
+      // We try to reuse |required_previous_frame| as starting state to avoid
+      // copying. If CanReusePreviousFrameBuffer returns false, we must copy
+      // the data since |required_previous_frame| is necessary to decode this
+      // or later frames. In that case copy the data instead.
+      if ((!CanReusePreviousFrameBuffer(index) ||
+           !frame.TakeBitmapDataIfWritable(&required_previous_frame)) &&
+          !frame.CopyBitmapData(required_previous_frame)) {
+        SetFailed();
+        return;
       }
-    }
-  } else {
-    for (; row_begin != row_end; ++row_begin, ++current_address) {
-      const size_t source_value = *row_begin;
-      if ((source_value != transparent_pixel) &&
-          (source_value < color_table.size()))
-        *current_address = color_table_iter[source_value];
-      else
-        current_buffer_saw_alpha_ = true;
-    }
-  }
-
-  // Tell the frame to copy the row data if need be.
-  if (repeat_count > 1)
-    buffer.CopyRowNTimes(x_begin, x_end, y_begin, y_end);
-
-  buffer.SetPixelsChanged(true);
-  return true;
-}
-
-bool GIFImageDecoder::ParseCompleted() const {
-  return reader_ && reader_->ParseCompleted();
-}
-
-bool GIFImageDecoder::FrameComplete(size_t frame_index) {
-  // Initialize the frame if necessary.  Some GIFs insert do-nothing frames,
-  // in which case we never reach HaveDecodedRow() before getting here.
-  if (!InitFrameBuffer(frame_index))
-    return SetFailed();
-
-  if (!current_buffer_saw_alpha_)
-    CorrectAlphaWhenFrameBufferSawNoAlpha(frame_index);
-
-  frame_buffer_cache_[frame_index].SetStatus(ImageFrame::kFrameComplete);
-
-  return true;
-}
-
-void GIFImageDecoder::ClearFrameBuffer(size_t frame_index) {
-  if (reader_ && frame_buffer_cache_[frame_index].GetStatus() ==
-                     ImageFrame::kFramePartial) {
-    // Reset the state of the partial frame in the reader so that the frame
-    // can be decoded again when requested.
-    reader_->ClearDecodeState(frame_index);
-  }
-  ImageDecoder::ClearFrameBuffer(frame_index);
-}
-
-size_t GIFImageDecoder::DecodeFrameCount() {
-  Parse(kGIFFrameCountQuery);
-  // If decoding fails, |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() ? frame_buffer_cache_.size() : reader_->ImagesCount();
-}
-
-void GIFImageDecoder::InitializeNewFrame(size_t index) {
-  ImageFrame* buffer = &frame_buffer_cache_[index];
-  const GIFFrameContext* frame_context = reader_->FrameContext(index);
-  buffer->SetOriginalFrameRect(
-      Intersection(frame_context->FrameRect(), IntRect(IntPoint(), Size())));
-  buffer->SetDuration(frame_context->DelayTime());
-  buffer->SetDisposalMethod(frame_context->GetDisposalMethod());
-  buffer->SetRequiredPreviousFrameIndex(
-      FindRequiredPreviousFrame(index, false));
-}
-
-void GIFImageDecoder::Decode(size_t index) {
-  Parse(kGIFFrameCountQuery);
-
-  if (Failed())
-    return;
-
-  UpdateAggressivePurging(index);
-
-  Vector<size_t> frames_to_decode = FindFramesToDecode(index);
-  for (auto i = frames_to_decode.rbegin(); i != frames_to_decode.rend(); ++i) {
-    if (!reader_->Decode(*i)) {
+      options.fPriorFrame = SkCodec::kNone;

[cblume, 2017/07/07 22:11:58]

This is supposed to be set to the previous frame.

+    }
+  }
+
+  if (frame.GetStatus() == ImageFrame::kFrameAllocated) {
+    SkCodec::Result start_incremental_decode_result =
+        codec_->startIncrementalDecode(image_info, frame.Bitmap().getPixels(),
+                                       frame.Bitmap().rowBytes(), &options,
+                                       nullptr, nullptr);
+    switch (start_incremental_decode_result) {
+      case SkCodec::kSuccess:
+        break;
+      case SkCodec::kIncompleteInput:
+        return;
+      case SkCodec::kErrorInInput:
+        frame.ZeroFillPixelData();
+        frame.SetPixelsChanged(true);
+      // Fall through
+      default:
+        SetFailed();
+        return;
+    }
+    frame.SetStatus(ImageFrame::kFramePartial);
+  }
+  int rows_decoded = 0;
+  bool error_in_input = false;
+  SkCodec::Result incremental_decode_result =
+      codec_->incrementalDecode(&rows_decoded);
+  switch (incremental_decode_result) {
+    case SkCodec::kSuccess:
+      frame.SetPixelsChanged(true);
+      frame.SetStatus(ImageFrame::kFrameComplete);
+      PostDecodeProcessing(index);
+      break;
+    case SkCodec::kErrorInInput:
+      error_in_input = true;
+    // Fall through
+    case SkCodec::kIncompleteInput:
+      if (!index) {

[cblume, 2017/07/07 22:11:58]

If we encounter an error in frame 5, do we display the partial frame 5?

Currently, the only partial frame we display in this patch is the first frame.

[scroggo_chromium, 2017/07/10 17:45:57]

My guess would be no. Let's say frame 5 is overlayed onto frame 4. (5 is subset
and/or has transparency.) If we decode only the first half of 5, and then there
is an error, we'll end up with a frame that displays the top of 5 and the bottom
of 4.

That said, I don't see the code that would do anything to prevent showing such a
broken frame.
If a frame beyond the first is not believed to be complete, you are correct, we
will not attempt to decode/display it. But if there's an error in frame 5, I
don't know what prevents us from drawing it.

[cblume, 2017/07/11 00:23:37]

The if (!index) { will enforce that only the first frame gets zero filled.
(Note to self: I need to do this in the startIncrementalDecode handler as well)
Later, if (... error_in_input) { calls SetFailed();


I'm looking into the "prevent showing a broken frame" bit now...

Perhaps you are saying that even thought the index isn't 0 and the frame isn't
marked as complete, it will be displayed?

At this point, the frame should be marked as kFramePartial.

It seems like the existing ImageFrameGenerator didn't handle partial frames
differently [1]. And the DeferredImageDecoder would still try to create a
SkBitmap [2]. Both have special handling for if the frame is marked empty
though. Maybe I should make it as empty?

The existing Blink gif decoder would exit early when there was an error in
decoding, which left the frame marked as empty. This seems to be the right path.

One thing to note is at that point we have already taken or copied the previous
bitmap. This is part of the new design, currently. This is probably okay since
we likely would have purged that frame anyway.

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...
[2]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...
I think perhaps the frame being marked as empty prevents it (see my links /
description above).

[scroggo_chromium, 2017/07/11 17:41:28]

It doesn't really handle empty frames differently either. Notice that it will
return false if the frame is marked FrameEmpty, but is_decode_complete will also
be set to false (and then returned) if the frame is not FrameComplete (and
!all_data_received). The return value is just used to determine whether we can
free memory (by deleting the decoder, for a single frame image, or clearing
cache for a multi-frame image).
Agreed, DeferredImageDecoder handles Empty differently.
Are you sure? In the old code, the status gets bumped to FramePartial in
InitFrameBuffer. I think a test would be the best way to verify this.
And part of the old design, right? I don't think that has changed.
What do you mean?

[cblume, 2017/07/11 18:25:02]

Pretty sure.

It seems like before InitFrameBuffer gets called (in fact, before any decoding
happens) GIFImageDecoder::Decode() tries to parse [1]. If the parse itself
fails, it calls SetFailed() and then GIFImageDecoder::Decode() exits early
(before calling InitFrameBuffer) [2].

It seems like InitFrameBuffer is only called from two places: OutputRow and
FrameComplete.
Both are only called after the parse, in the reader->Decode() part of [1].

So it seems like an error in input would leave the frame marked as FrameEmpty,
which cues both ImageFrameGenerator and DeferredImageDecoder to use their
special treatment of FrameEmpty.


But I think you are right -- we can put this in a separate CL. I'll add the
tests there, too. For now, I think always zero initializing is the play.


[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...
[2]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...
I think this has changed.

As I described above, the old GIF decoder would exit early if it found a failure
in parsing. Specifically, it would exit before calling InitFrameBuffer().
InitFrameBuffer() is where we would take / copy the old frame [3].

Even though SkGifCodec parses at an earlier step (when the data is provided) and
the error is potentially encountered earlier, that error isn't reported until we
call startIncrementalDecode() or incrementalDecode().

The only way for the client to know there was an error in the input is after the
frame has been taken. But this is probably fine, as I describe below.

[3]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...
What I mean is suppose we steal frame 10 and then hit an error in decode. So
even though we stole the frame, we cannot display it. And we loop back to the
beginning of the animation. Upon hitting frame 10 again in the next animation
iteration, we now need to decode it again since the frame was stolen.

But we would have needed to decode it again anyway, since Blink purges after
every decode.


Note that this only matters if the frame was not displayed (IE when
fast-forwarding an animation to catch up). When a frame is displayed, it is
marked immutable and we can no longer steal it in the first place.

So basically, this is not a common situation and even when it does happen it
probably doesn't matter.

[scroggo_chromium, 2017/07/11 20:36:39]

Except that parsing isn't the only case where we can see an error. There can
also be an error in decoding. (i.e. in doLZW)
Again, ImageFrameGenerator doesn't treat Empty differently from Partial (besides
deciding whether to call SetHasAlpha).
Great. Sgtm.
Ah, I hadn't been considering an error during parse.

[cblume, 2017/07/12 21:22:41]

Ah, right. Good call.

I spoke to vmpstr@ to find where Blink makes the decision whether or not to
paint a frame that is partial for an animation.
It looks like it is here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...

It only seems to check if a frame is complete in an animation, meaning we can
leave it marked as partial and that is fine. It is more correct as well.
You're right.

[scroggo_chromium, 2017/07/13 12:31:06]

That code is misleading, though. Although the method is named
FrameIsCompleteAtIndex, it is really just checking to see whether the parse
marked it as complete. If there is an error in the frame, I don't think we
handle that case. Now that we zero all the memory ahead of time, it's not as big
of a deal, since we are not showing uninitialized memory, but if I'm right, we
might show part of the old frame and part of the new one. This behavior doesn't
change with your patch (now that it zeroes the memory first), though.

[scroggo_chromium, 2017/07/18 17:52:41]

Filed https://bugs.chromium.org/p/chromium/issues/detail?id=745819

+        bool oldAlpha = frame.HasAlpha();
+
+        IntRect remaining_rect = IntRect(IntPoint(), Size());
+        remaining_rect.SetY(rows_decoded);
+        remaining_rect.SetHeight(remaining_rect.Height() - rows_decoded);
+        frame.ZeroFillFrameRect(remaining_rect);
+        // ZeroFillFrameRect() resets the alpha to true.
+        // We want to preserve the alpha. So we need to set it back.
+        SkCodec::FrameInfo frame_info;
+        codec_->getFrameInfo(index, &frame_info);

[cblume, 2017/07/07 22:11:58]

I think these 2 lines don't do anything and should be removed.

It is a leftover from when the code was:
SkCodec::FrameInfo frame_info;
codec_->getFrameInfo(index, &frame_info);
frame.SetHasAlpha(!SkAlphaTypeIsOpaque(frame_info.fAlphaType));

I'm going to remove it.

+
+        frame.SetHasAlpha(oldAlpha);
+      }
+
+      frame.SetPixelsChanged(true);
+
+      if (FrameIsCompleteAtIndex(index) || IsAllDataReceived() ||
+          error_in_input) {
+        SetFailed();
+        return;
+      }
+
+      break;
+    default:
       SetFailed();
       return;
-    }
-
-    // If this returns false, we need more data to continue decoding.
-    if (!PostDecodeProcessing(*i))
-      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 >= frame_buffer_cache_.size() - 1 && IsAllDataReceived() &&
-      reader_ && !reader_->ParseCompleted())
-    SetFailed();
-}
-
-void GIFImageDecoder::Parse(GIFParseQuery query) {
-  if (Failed())
-    return;
-
-  if (!reader_) {
-    reader_ = WTF::MakeUnique<GIFImageReader>(this);
-    reader_->SetData(data_);
-  }
-
-  if (!reader_->Parse(query))
-    SetFailed();
-}
-
-void GIFImageDecoder::OnInitFrameBuffer(size_t frame_index) {
-  current_buffer_saw_alpha_ = false;
-}
-
-bool GIFImageDecoder::CanReusePreviousFrameBuffer(size_t frame_index) const {
-  DCHECK(frame_index < frame_buffer_cache_.size());
-  return frame_buffer_cache_[frame_index].GetDisposalMethod() !=
-         ImageFrame::kDisposeOverwritePrevious;
+  }
+}
+
+bool GIFImageDecoder::CanReusePreviousFrameBuffer(size_t index) const {
+  DCHECK(index < frame_buffer_cache_.size());
+
+  // If the current frame and the next frame depend on the same frame, we cannot
+  // reuse the old frame. We must preserve it for the next frame.
+  //
+  // However, if the current and next frame depend on different frames then we
+  // know the current frame is the last one to use the frame it depends on. That
+  // means the current frame can reuse the previous frame buffer.
+  //
+  // If we do not have information about the next frame yet, we cannot assume it
+  // is safe to reuse the previous frame buffer.
+
+  if (index + 1 >= frame_buffer_cache_.size())
+    return false;
+
+  const ImageFrame& frame = frame_buffer_cache_[index];
+  size_t required_frame_index = frame.RequiredPreviousFrameIndex();
+
+  const ImageFrame& next_frame = frame_buffer_cache_[index + 1];
+  size_t next_required_frame_index = next_frame.RequiredPreviousFrameIndex();
+
+  return required_frame_index != next_required_frame_index;
 }
 
 }  // namespace blink