Move gif image decoder to SkCodec

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

Index: third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
diff --git a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
index b954a982e6fbc2decfe0c22117ab6573de011dc8..6a5c2b7163fb53abf039e90f2c5229a1a25f537c 100644
--- a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
@@ -26,9 +26,9 @@
 #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 {
 
@@ -36,247 +36,312 @@ 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_) {
+    SkCodec::Result stream_creation_result;
+    codec_.reset(SkCodec::NewFromStream(segment_stream_,
+                                        &stream_creation_result, nullptr));
+    switch (stream_creation_result) {
+      case SkCodec::kSuccess: {
+        // 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());
+        return;
+      }
+      case SkCodec::kIncompleteInput:
+        // |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;
+      default:
+        SetFailed();
+        return;
+    }
+  }
 }
 
 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_|).
-  //
-  // 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.
+  // packets sent back by the webserver) not always.
   //
-  // 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: {
+      // SkCodec returns 0 for both still images and animated images which only
+      // play once.
+      size_t frame_count = codec_->getFrameCount();
+      if (IsAllDataReceived() && frame_count == 1)

[scroggo_chromium, 2017/07/18 21:10:10]

nit: No need for this variable. This could be:

  if (IsAllDataReceived() && codec_->getFrameCount() == 1)

[cblume, 2017/07/19 23:27:05]

Done.

+        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();
+  segment_stream_ = nullptr;
+  codec_ = nullptr;

[scroggo_chromium, 2017/07/18 21:10:09]

nit: I prefer

  codec_.reset();

here. It's more obvious that it's deleting something. (At least to me.)

[cblume, 2017/07/19 23:27:05]

I hadn't considered this before. I like it. Done.

   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;
-      }
-    }
-  } 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;
+size_t GIFImageDecoder::ClearCacheExceptFrame(size_t index) {

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

Since we're overriding this method, please remove the comments on the baseclass
suggesting that it's virtual just for a test method and we should remove that:

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

[cblume, 2017/07/19 23:27:05]

Done.

+  // SkCodec attempts to report the earliest possible required frame, but it is
+  // possible that frame has been evicted, while a later frame (which could also
+  // be used as the required frame) is still cached. Try to preserve a frame
+  // that is still cached.
+  if (frame_buffer_cache_.size() <= 1)
+    return 0;
+
+  size_t index2 = kNotFound;
+  if (index < frame_buffer_cache_.size()) {
+    const ImageFrame& frame = frame_buffer_cache_[index];
+    if (!FrameStatusSufficientForSuccessors(index) ||
+        frame.GetDisposalMethod() == ImageFrame::kDisposeOverwritePrevious) {
+      // We need to preserve a previous frame

[scroggo_chromium, 2017/07/18 21:10:09]

This comment probably doesn't add much. (Tells a what, but not a why. And
ClearCacheExceptTwoFrames makes it clear we're preserving this one.)

[cblume, 2017/07/19 23:27:04]

Yeah, I agree. I think this comment is useless.

+      index2 = GetViableReferenceFrameIndex(index);
     }
   }
 
-  // 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();
+  return ClearCacheExceptTwoFrames(index, index2);
 }
 
-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);
+size_t GIFImageDecoder::DecodeFrameCount() {
+  if (!codec_)
+    return 0;

[scroggo_chromium, 2017/07/18 21:10:10]

In the old code, we returned the existing number of frames if we had failed.
Since SetFailed() deletes codec_, we'll never reach the below code if we've
failed.

I think we want something like:

  if (!codec_ || segment_stream_->IsCleared())
    return frame_buffer_cache_.size();

  return codec_->getFrameCount();

[cblume, 2017/07/19 23:27:05]

Done.

 
-  frame_buffer_cache_[frame_index].SetStatus(ImageFrame::kFrameComplete);
+  if (Failed() || segment_stream_->IsCleared())
+    return 0;
 
-  return true;
+  return codec_->getFrameCount();
 }
 
-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);
+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);

[scroggo_chromium, 2017/07/18 21:10:10]

This method returns a boolean. We should probably check its return value.

That said, looking at its implementation, it should only fail if index is out of
range, or if we haven't parsed enough data to know the stuff you're about to put
on the ImageFrame.

The only way that would currently happen is if it's the first frame (later
frames we don't report until they at least have this much data received).

I have posted a fix in codec here: https://skia-review.googlesource.com/c/24405/

[cblume, 2017/07/19 23:27:05]

Done.

+  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);
   }
-  ImageDecoder::ClearFrameBuffer(frame_index);
-}
+  frame.SetRequiredPreviousFrameIndex(required_previous_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));
+  ImageFrame::DisposalMethod disposal_method = ImageFrame::kDisposeNotSpecified;
+  switch (frame_info.fDisposalMethod) {
+    case SkCodecAnimation::DisposalMethod::kKeep:
+      disposal_method = ImageFrame::kDisposeKeep;
+      break;
+    case SkCodecAnimation::DisposalMethod::kRestoreBGColor:
+      disposal_method = ImageFrame::kDisposeOverwriteBgcolor;
+      break;
+    case SkCodecAnimation::DisposalMethod::kRestorePrevious:
+      disposal_method = ImageFrame::kDisposeOverwritePrevious;
+      break;
+  }
+  frame.SetDisposalMethod(disposal_method);
 }
 
 void GIFImageDecoder::Decode(size_t index) {
-  Parse(kGIFFrameCountQuery);
+  if (!codec_)
+    return;
 
-  if (Failed())
+  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());
+      bool oldAlpha = frame.HasAlpha();

[scroggo_chromium, 2017/07/18 21:10:09]

I know we've gone back and forth on this, but maybe add a comment about why you
have to do this? Something like:

// ZeroFillPixelData() sets HasAlpha to true, but there are other
// safeguards that ensure that a Partial ImageFrame doesn't claim to
// be opaque. Set it back to the intended state, which was set by
// InitializeNewFrame

That said, if we have to go through so much work to go against
ZeroFillPixelData's desires, why not just re-set it once the ImageFrame is
Complete? That seems like it would be simpler.

// Once complete...
FrameInfo frame_info;
codec_->getFrameInfo(index, &frame_info);
frame.SetHasAlpha(!SkAlphaTypeIsOpaque(frame_info.fAlphaType));

[cblume, 2017/07/19 23:27:05]

I'm pretty sure we tried SkAlphaTypeIsOpaque() before and ran into a problem.

I tested it out just now and it causes tests to fail. It looks like opaque
images are being marked as transparent. I can look into this more if you want
but I'm pretty sure I already did this.

[scroggo_chromium, 2017/07/20 13:59:52]

That doesn't make sense - SkAlphaTypeIsOpaque is how you computed this (look at
line 189).

That seems worth investigating.

If you leave the code as is, please add a comment explaining it.

[cblume, 2017/07/20 21:53:44]

Oh sorry, I think I misread. I thought you meant using the ImageFrame's
ImageInfo to find the alpha. It is updated separately from the ImageFrame's
has_alpha_ which is set during ZeroFillPixelData().

By the time we reach the Complete state (codec_->incrementalDecode() returning
kSuccess), something else has updated the alpha in the ImageFrame's ImageInfo.
So at that point, we can't use it.

But what you are suggesting is getting it from the codec again. This works. :)

+      frame.ZeroFillPixelData();
+      options.fZeroInitialized = SkCodec::kYes_ZeroInitialized;
+      frame.SetHasAlpha(oldAlpha);
+    } else {
+      size_t previous_frame_index = GetViableReferenceFrameIndex(index);
+      if (previous_frame_index == kNotFound)
+        previous_frame_index = required_previous_frame_index;
+
+      ImageFrame& previous_frame = frame_buffer_cache_[previous_frame_index];

[scroggo_chromium, 2017/07/18 21:10:10]

It seems like this could be a part of the prior if statement. We'll only need to
decode the prior frame if GetViableReferenceFrameIndex returned kNotFound,
right?

[cblume, 2017/07/19 23:27:05]

Done.

+      if (previous_frame.GetStatus() != ImageFrame::kFrameComplete)
+        Decode(previous_frame_index);
+
+      // We try to reuse |previous_frame| as starting state to avoid copying.
+      // If CanReusePreviousFrameBuffer returns false, we must copy the data
+      // since |previous_frame| is necessary to decode this or later frames.
+      // In that case copy the data instead.
+      if ((!CanReusePreviousFrameBuffer(index) ||
+           !frame.TakeBitmapDataIfWritable(&previous_frame)) &&
+          !frame.CopyBitmapData(previous_frame)) {
+        SetFailed();
+        return;
+      }
+      options.fPriorFrame = previous_frame_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)) {
-      SetFailed();
-      return;
+  if (frame.GetStatus() == ImageFrame::kFrameAllocated) {

[scroggo_chromium, 2017/07/18 21:10:10]

It took me a while to get on board with kFrameAllocated. I see why you added it,
but I kept thinking there might be a simpler way.

But as I look at TakeBitmapDataIfWritable and CopyBitmapData, I see that they
used to result in the frame having different states! Copy left it kFrameEmpty,
and Take left it kFrameComplete (neither of which seems correct). So now you've
unified them and they share the same Status :)

[cblume, 2017/07/19 23:27:05]

:)
I would be happy to adopt a simpler way if we know of one.
In my mind, the allocated status is the correct thing after Take & Copy. The
status could be partial, which just happens to have 0 updated rows. So it feels
like that might be the simpler way.

But really, that is only correct from the client's point of view. From the
internal point of view, we haven't even begun to partially decode. We can't
resume where we left off, for example.

+    SkCodec::Result start_incremental_decode_result =
+        codec_->startIncrementalDecode(image_info, frame.Bitmap().getPixels(),
+                                       frame.Bitmap().rowBytes(), &options);
+    switch (start_incremental_decode_result) {
+      case SkCodec::kSuccess:
+        break;
+      case SkCodec::kIncompleteInput:
+        return;
+      default:
+        SetFailed();
+        return;
     }
+    frame.SetStatus(ImageFrame::kFramePartial);
+  }

[scroggo_chromium, 2017/07/18 21:10:10]

nit: Add a newline after this? That way we separate the different stages in this
method:

if Empty -> do this
<new line> (already present)
if Allocated -> do this
<new line>
decode

[cblume, 2017/07/19 23:27:04]

Done.

+  int rows_decoded = 0;

[scroggo_chromium, 2017/07/18 21:10:10]

Now that we zero pixels ahead of time, this variable is not used. Please remove.

[cblume, 2017/07/19 23:27:05]

Done.

+  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::kIncompleteInput:
+      frame.SetPixelsChanged(true);
+      if (FrameIsCompleteAtIndex(index) || IsAllDataReceived()) {
+        SetFailed();
+        return;

[scroggo_chromium, 2017/07/18 21:10:10]

These returns feel arbitrary to me. If you left this one out, we'd fall through
to break, and there's nothing left after the switch statement. So I'd remove it.

[cblume, 2017/07/19 23:27:05]

This was actually intentional, but I'm happy to remove it.

My thinking was future changes. When someone comes and adds more code below, I
don't want them to have missed the cases which relied on there not being more
code. You'll notice the kSuccess case breaks, but the error case here
immediately returns and doesn't attempt to do any more work.

+      }
 
-    // If this returns false, we need more data to continue decoding.
-    if (!PostDecodeProcessing(*i))
       break;
+    default:
+      SetFailed();
+      return;

[scroggo_chromium, 2017/07/18 21:10:10]

I support putting either a break or a return here, but given that the method is
over, I'd push for break.

[cblume, 2017/07/19 23:27:05]

Done.

   }
-
-  // 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();
+bool GIFImageDecoder::CanReusePreviousFrameBuffer(size_t index) const {
+  DCHECK(index < frame_buffer_cache_.size());
+  return frame_buffer_cache_[index].GetDisposalMethod() !=
+         ImageFrame::kDisposeOverwritePrevious;
 }
 
-void GIFImageDecoder::OnInitFrameBuffer(size_t frame_index) {
-  current_buffer_saw_alpha_ = false;
-}
+size_t GIFImageDecoder::GetViableReferenceFrameIndex(
+    size_t dependent_index) const {
+  size_t required_previous_frame_index =
+      frame_buffer_cache_[dependent_index].RequiredPreviousFrameIndex();
+  // Any frame in the range [|required_previous_frame_index|, |dependent_index|)
+  // which has a disposal method other than kRestorePrevious can be provided as
+  // the prior frame to SkCodec.
+  //
+  // SkCodec sets SkCodec::FrameInfo::fRequiredFrame to the earliest frame which
+  // can be used. This might come up when several frames update the same
+  // subregion. If that same subregion is about to be overwritten, it doesn't
+  // matter which frame in that chain is provided.
+
+  DCHECK_LT(dependent_index, frame_buffer_cache_.size());
+  DCHECK(dependent_index);

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

I think this DCHECK might fire in the following situation:

- decode all frames of an animated image
- loop, and decode frame 0
- call ClearCacheExceptFrame(0)

ClearCacheExceptFrame checks for frame_buffer_cache_.size() <= 1, but not for
index == 0

[cblume, 2017/07/19 23:27:05]

I think you are right. I think we can drop it.

Frame 0 should have a previous frame of kNotFound, and thus shouldn't even enter
the loop below.

+
+  size_t previous_reference_frame_index = kNotFound;
+  if (required_previous_frame_index != kNotFound) {
+    for (size_t i = dependent_index - 1; i != required_previous_frame_index;

[scroggo_chromium, 2017/07/18 21:10:10]

Specify why we go in reverse?

[cblume, 2017/07/19 23:27:05]

Done.
I put in a fairly general comment rather than describing the implementation
details of SkCodec and whatnot.

+         i--) {
+      const ImageFrame& frame = frame_buffer_cache_[i];
+
+      if (frame.GetDisposalMethod() == ImageFrame::kDisposeOverwritePrevious)
+        continue;
+
+      if (frame.GetStatus() == ImageFrame::kFrameComplete) {
+        previous_reference_frame_index = i;
+        break;
+      }
+    }
+  }
 
-bool GIFImageDecoder::CanReusePreviousFrameBuffer(size_t frame_index) const {
-  DCHECK(frame_index < frame_buffer_cache_.size());
-  return frame_buffer_cache_[frame_index].GetDisposalMethod() !=
-         ImageFrame::kDisposeOverwritePrevious;
+  return previous_reference_frame_index;
 }
 
 }  // namespace blink