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..b2eea188f7c53e6ed2c42fa30b8b63defb7df32f 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,324 @@ 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_,

[vmpstr, 2017/07/17 21:28:40]

It seems kind of awkward that a codec_ takes ownership of the stream, but
doesn't (publicly) expose the stream as a getter, meaning we need to play around
with raw memory...

[cblume, 2017/07/18 10:23:49]

I agree.

One option would be a shared ownership. I'm not a fan of shared ownership if it
is avoidable.
Another option is a getter, like you said.

scroggo@ Can we add this to SkCodec?

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

SkStream was originally meant to be something you couldn't add more data to
later, which is why it doesn't allow shared ownership. I've been thinking about
some other type of input for SkCodec, where it would make more sense to have
some kind of shared ownership...
See https://skia-review.googlesource.com/c/24402/

+                                        &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:
+        segment_stream_ = nullptr;

[vmpstr, 2017/07/17 21:28:40]

nit: SetFailed will do this?

[cblume, 2017/07/18 10:23:49]

Done.

+        SetFailed();

[vmpstr, 2017/07/17 21:28:40]

SetFailed DCHECKs that codec_ exists, but it's not too clear to me that that's
true in all cases, since we might have got a nullptr from the codec, no?

[cblume, 2017/07/18 10:23:49]

You are right.
The call to SetFailed() still needs to happen (and the call further up, to
ImageDecoder::SetFailed()).

I think the DCHECK should go.

+        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.
+  // packets sent back by the webserver) not always.
   //
-  // 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: {

[vmpstr, 2017/07/17 21:28:40]

Please leave a comment about the fact that SkCodec will report 0 for both static
images and images that need to only play once

[cblume, 2017/07/18 10:23:49]

Done.

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

[scroggo_chromium, 2017/07/17 20:07:56]

First off, I'll acknowledge that this is intended to be temporary behavior,
since you plan to move caching into the compositor. Further, I know that this is
in line with how GIF, WEBP and APNG work today.

That said, this method seems much more complicated than necessary. If there is
only one frame, we do nothing, which is fine. It only gets interesting when we
get to the second frame.

The first loop checks whether FrameStatusSufficientForSuccessors, which means
true if the Frame is FramePartial or FrameComplete. Will that ever return false?

ClearCacheExceptFrame is called in two ways:
- clear all the frames
- clear all except for the one we just decoded

Now, relying on this fact is arguably spaghetti code, but is there any other
sensible way to call it?

The first case bypasses the loops and does the right thing, so let's just look
at the second case (i.e. index != kNotFound). In that case, I argue that the
Frame in question will always be complete (unless there was an error, in which
case we don't bother decoding future frames, so keeping around the cache is
moot).
- The client only ever asks for frames that have received all their data.
Relying on this might again be spaghetti code, but...
- GIF is currently the only multi-frame decoder that even reports that a frame
exists if it is not yet complete (besides the first frame). It seems logical to
bring it in line with APNG and WEBP, which only report latter frames if they're
complete. Then it's safe to assume that |index| is complete (or there was an
error, or it's the first frame so there's nothing to do).

The interesting bit is if the frame is kDisposeOverwritePrevious. In that case,
we really want to also save the frame that |index+1| depends on, but we're
finding the frame that |index| depends on. It seems like we could check to see
if |index+1| has been received yet, and if it has, look at whatever frame it
depends on. That is a change in behavior, but it seems weird to mostly rewrite
(with a couple meaningful differences) the method while preserving some of its
oddities.

[cblume, 2017/07/18 10:23:49]

I may agree. I kept it as similar as possible to the existing code to minimize
friction of landing this patch.
That depends on the client behavior.
If we assume the current client behavior which calls
ClearCacheExceptFrame(frame_we_just_decoded) then it will never return false.

But the client is free to pass in whatever index they want.
For example, ClearCacheExceptFrame(some_frame_we_just_cleared);

In an effort to not rely on behavior at a distance, I wouldn't recommend
changing that.
I really like this idea.

But I believe it belongs in the refactor phase.
My goal here is to make as few changes as possible to ease SkCodec adoption.
Even small behavior changes like no longer correcting opaque frames has added
months to this.

 
You're completely correct about this.
If |index| is marked as kDisposeOverwritePrevious then we need some prior frame.
And rather than guess which prior frame, we could just ask SkCodec. If we have
that information, it is silly that we don't use it.

Like the issue above, the effects of this change are visible outside of
GIFImageDecoder. They are behavior changes.
Changing this would be an improvement (a big one!) if the incorrect frame is
guessed.
I would LOVE to fix this. But since the same code would need the same approvals
and buy-in, there is an advantage to landing SkCodec without this improvement
and then adding it right after.

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

Makes sense.
That seems okay... Maybe add a TODO, bug, or at least a note? (Perhaps the
better fix is not fix this exact situation until we move caching out of
ImageDecoder entirely? I thought about suggesting that ClearCacheExceptFrame be
called *by* Decode, which would match the current behavior (mostly) and we
wouldn't need to worry about behavior at a distance...)
Filed crbug.com/745820
Add a TODO, note, bug?

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

Yeah, I think this will just disappear once the cache is moved out of the
ImageDecoder.
I filed https://crbug.com/745967 (specifically, for the situation below) which
should cover this.

I suspect the path would be:
- land this
- land a patch for this new bug, since it should be quick & easy (but a behavior
change)
- later, move the cache out of the ImageDecoder (at which point we can use the
cleaned up code)
Filed https://crbug.com/745967

+  // SkCodec marks a required previous frame as the first frame in
+  // [required_ptrevious_frame, index). Other Blink image decoders instead would

[scroggo_chromium, 2017/07/17 20:07:56]

required_previous_frame (no 't')

[cblume, 2017/07/18 10:23:49]

Done.

+  // have marked the last index in that range.

[scroggo_chromium, 2017/07/17 20:07:56]

This comment is confusing:
- It doesn't really describe what the range is
- Other Blink image decoders don't do as much as SkCodec to find the earliest
required frame, but they do not necessarily select the most recent frame (see
FindRequiredPreviousFrame). That said, after decoding, they do correct to
potentially end up with an earlier required frame.

I think you want to say something more like:
"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."

[cblume, 2017/07/18 10:23:49]

Done.

   //
-  // 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;
+  // When we clear the cache, we preserve a previous frame if the frame at

[scroggo_chromium, 2017/07/17 20:07:56]

This tells what, but not why. I think it can be eliminated.

[cblume, 2017/07/18 10:23:49]

Done.

+  // |index| is kDisposePrevious.
+  //
+  // This means SkCodec-based decoders must override ClearCacheExceptFrame() to

[scroggo_chromium, 2017/07/17 20:07:56]

This line seems redundant - this is inside the overriding version of
ClearCacheExceptFrame. Obviously we needed to override it or this code wouldn't
be here.

[cblume, 2017/07/18 10:23:49]

Done.

I wanted to illustrate that it was really for SkCodec and not for gif.

After a second SkCodec-based decoder lands I want to refactor out common code
like this.

+  // not keep the marked required previous frame in the case of
+  // kDisposePrevious. They should instead keep the last frame in the range,

[scroggo_chromium, 2017/07/17 20:07:56]

Don't necessarily need the last frame in the range. We just want to preserve the
frame that is still cached (if any). I think this is covered by my proposed
comment on line 148.

[cblume, 2017/07/18 10:23:49]

I removed this comment and went with what you put on line 148.

+  // like other Blink decoders do.
+  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) ||

[scroggo_chromium, 2017/07/17 20:07:56]

Since this is only called here (i.e. not by the base class), you could have left
the base class version of this method alone and called a private method.

(Are you thinking that other SkCodec based ImageDecoders will want to share the
same version of FrameStatusSufficientForSuccessors, so we might as well update
it?)

[cblume, 2017/07/18 10:23:49]

The base class version needed to be updated.
It only checked for != Empty.
But it also needs to consider != Allocated now (which this patch added).

Other decoders happened to not ever set to Allocated, so it happened to work.
But it wasn't really saying what it needed.

In fact, "!= Empty" wasn't previously saying what it needed, either. It needed
== Complete || == Partial.
There were only the 3 values, so it happened to cover the same thing. But it
wasn't future-proof. If any other status was added, it would also cause a
similar situation.

So really, the base version was updated to just be more accurate / descriptive /
not rely on distant code being just-so. It is less fragile now.

+        frame.GetDisposalMethod() == ImageFrame::kDisposeOverwritePrevious) {
+      // We need to preserve a previous frame
+      index2 = GetPreviousReferenceFrame(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;
-}
+  while (index2 < frame_buffer_cache_.size() &&
+         !FrameStatusSufficientForSuccessors(index2)) {

[scroggo_chromium, 2017/07/17 20:07:56]

This call makes sense in theory, although I suspect in practice if the required
frame isn't there we'll be going back to the beginning of the image.

[cblume, 2017/07/18 10:23:49]

I think you are right.

I think the call to GetPreviousReferenceFrame() (now renamed) in the previous
loop takes care of everything here. I don't think we need this second loop.

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

Now that I look at it more closely, if nothing in the range was sufficient,
GetPreviousReferenceFrame returns kNotFound. So the second loop wouldn't do
anything.

But if we're not relying on the fact that ClearCacheExceptFrame is always called
after decoding the frame that is passed in as a parameter, it is theoretically
possible that the frame that |index| depends on is not there, but the frame it
depends on is.

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

Right. I actually removed the second loop. The second loop made sense before I
created GetPreviousReferenceFrame(). That function essentially contained the
second loop.
I think it is fair that if the client provides an index to a cleared frame we
effectively don't save anything. We're really preserving the second frame
secretly to speed up future decodes. But the API itself sounds like it purges
all but 1 frame, not all but 2. If the frame the client attempted to preserve is
already purged, I think it is okay that we do not preserve its reference frame.

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

Not quite - GetPreviousReferenceFrame() goes back to the reported required
frame, and no further. If no frames in that range are Complete, it returns
kNotFound.

The old second loop would see if the required frame was cached, and if not, it
would return its required frame, potentially going back to the beginning of the
image. In practice, with our knowledge of how this method is called, we know
that that is not fruitful, because we would have already cleared those frames.
sgtm.

+    index2 = GetPreviousReferenceFrame(index2);
+  }
 
-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;
 
-  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);
+  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);
+  frame.SetStatus(ImageFrame::kFrameEmpty);

[scroggo_chromium, 2017/07/17 20:07:56]

Why is this line necessary? Doesn't frame start off as kFrameEmpty?

[cblume, 2017/07/18 10:23:49]

Done.

 }
 
 void GIFImageDecoder::Decode(size_t index) {
-  Parse(kGIFFrameCountQuery);
+  if (!codec_)
+    return;
+
+  DCHECK(!Failed());
 
-  if (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();
+      frame.ZeroFillPixelData();
+      options.fZeroInitialized = SkCodec::kYes_ZeroInitialized;
+      frame.SetHasAlpha(oldAlpha);
+    } else {
+      size_t previous_frame_index = GetPreviousReferenceFrame(index);
+      if (previous_frame_index == kNotFound)
+        previous_frame_index = required_previous_frame_index;
+
+      ImageFrame& previous_frame = frame_buffer_cache_[previous_frame_index];
+      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) {
+    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);
+  }
+  int rows_decoded = 0;
+  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;
+      }
 
-    // If this returns false, we need more data to continue decoding.
-    if (!PostDecodeProcessing(*i))
       break;
+    default:
+      SetFailed();
+      return;
   }
-
-  // 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::GetPreviousReferenceFrame(size_t index) const {
+  // Any frame in the range [required_previous_frame_index, index) which

[scroggo_chromium, 2017/07/17 20:07:56]

nit: required_previous_frame_index hasn't been used yet, but maybe it's clear
that you mean

frame_buffer_cache_[index].RequiredPreviousFrameIndex()

(It's certainly more concise!)

[cblume, 2017/07/18 10:23:49]

I moved the declaration of required_previous_Frame_index above the comment.

At first, I worried that it would be awkward. But I think it actually makes the
code (and this comment) a bit easier to digest.

+  // has a disposal method other than kRestorePrevious can be provided as
+  // the prior frame to SkCodec.
+  //
+  // This is because 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(index, frame_buffer_cache_.size());
+  DCHECK(index);
+
+  size_t previous_reference_frame_index = kNotFound;
+  size_t required_previous_frame_index =
+      frame_buffer_cache_[index].RequiredPreviousFrameIndex();
+  if (required_previous_frame_index != kNotFound) {
+    for (size_t i = index - 1; i != required_previous_frame_index; 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