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 25a00e19e67b15afea346b667c28500d2a13b27d..aba962f7479162295dae40c5ee94a7f4f9767330 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,252 @@ GIFImageDecoder::GIFImageDecoder(AlphaOption alpha_option,
                                  const ColorBehavior& color_behavior,
                                  size_t max_decoded_bytes)
     : ImageDecoder(alpha_option, color_behavior, max_decoded_bytes),
-      repetition_count_(kCAnimationLoopOnce) {}
-
-GIFImageDecoder::~GIFImageDecoder() {}
+      codec_(),
+      segment_stream_(nullptr),
+      frame_infos_() {}
+
+GIFImageDecoder::~GIFImageDecoder() {
+  if (!codec_) {
+    // if we did not create m_codec and thus did not pass ownership to it

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

This still references "m_codec" (from before the name change).

[cblume, 2017/04/20 03:42:27]

Done.

+    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, false);
+    return;
+  }
+
+  if (!segment_stream_)
+    segment_stream_ = new SegmentStream();
+
+  segment_stream_->SetReader(data, IsAllDataReceived());
+
+  // If we don't have a SkCodec yet, create one from the stream
+  if (!codec_) {
+    SkCodec* codec = SkCodec::NewFromStream(segment_stream_);

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

Why do you have this local variable? Instead, you could say

codec_.reset(SkCodec::NewFromStream(...));

if (!codec_) {
  segment_stream_ = nullptr;
  return;
}

[cblume, 2017/04/20 03:42:26]

Done.

+    if (codec) {
+      codec_.reset(codec);
+    } else {
+      // segment_stream_'s ownership is passed. It is deleted if SkCodec
+      // creation fails. In this case, release our reference so we can create a

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

I interpret "release our reference" as "decrement the refcount", which is not
what is going on here.

Instead, maybe this should say something like:

If NewFromStream failed, it deleted segment_stream_. Stop pointing to reclaimed
memory.

[cblume, 2017/04/20 03:42:26]

Done.

+      // new SegmentStream later.
+      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 kCAnimationLoopOnce;
+
   // 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|).
-  //
-  // 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.
+  // 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
-  // |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() && reader_->imagesCount() == 1)
-    repetition_count_ = kCAnimationNone;
-  else if (Failed() || (reader_ && (!reader_->imagesCount())))
-    repetition_count_ = kCAnimationLoopOnce;
-  else if (reader_ && reader_->loopCount() != cLoopCountNotSeen)
-    repetition_count_ = reader_->loopCount();
-  return repetition_count_;
-}
+  // SkCodec will parse forward in the file if the repetition count has not been
+  // seen yet.
 
-bool GIFImageDecoder::FrameIsCompleteAtIndex(size_t index) const {
-  return reader_ && (index < reader_->imagesCount()) &&
-         reader_->frameContext(index)->isComplete();
-}
-
-float GIFImageDecoder::FrameDurationAtIndex(size_t index) const {
-  return (reader_ && (index < reader_->imagesCount()) &&
-          reader_->frameContext(index)->isHeaderDefined())
-             ? reader_->frameContext(index)->delayTime()
-             : 0;
-}
-
-bool GIFImageDecoder::SetFailed() {
-  reader_.reset();
-  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.
-  // (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 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 |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 (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;
-    }
+  if (IsAllDataReceived() && frame_infos_.size() == 1)

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

Could this be a problem if we have not parsed since all data was received?

e.g.
1. Receive the first frame (IsAllDataReceived == false)
2. Parse available data (frame_infos_.size() == 1)
3. Receive the rest of the data (including more frames)
4. Call this method

Then might we return kCAnimationNone, when the image actually is animated?

[cblume, 2017/04/20 03:42:26]

I think I follow.
But doesn't SkCodec parse the info when it is provided? So step 3 would imply
we've parsed further?

If not, I need to make sure we've parsed all we have when this function is
called. That would mean calling one of SkCodec's functions which forces the
parse, right?

[scroggo_chromium, 2017/04/21 20:09:31]

SkCodec will parse it when you call getRepetitionCount(), down below. I *think*
GetFrameCount() will always be called before RepetitionCount() in practice, but
it seems fragile to require it.
Yes.

[cblume, 2017/04/21 23:47:38]

Okay, I follow. I rearranged the function to call getRepetitionCount() earlier
and use it instead of frame_infos_.size() (which gets updated in a separate
function). This makes things less fragile.

+    return kCAnimationNone;
+  if (Failed())
+    return kCAnimationLoopOnce;
+
+  int repetition_count = codec_->getRepetitionCount();
+  switch (repetition_count) {
+    case 0:
+      return kCAnimationLoopOnce;
+    case SkCodec::kRepetitionCountInfinite:
+      return kCAnimationLoopInfinite;
+    default:
+      return repetition_count;
   }
-
-  // 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);
+bool GIFImageDecoder::FrameIsCompleteAtIndex(size_t index) const {
+  if (!codec_)
+    return false;
 
-  frame_buffer_cache_[frame_index].SetStatus(ImageFrame::kFrameComplete);
+  if (frame_infos_.size() <= index)
+    return false;
 
-  return true;
+  return frame_infos_[index].fFullyReceived;
 }
 
-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);
+float GIFImageDecoder::FrameDurationAtIndex(size_t index) const {
+  if (index < frame_buffer_cache_.size())
+    return frame_buffer_cache_[index].Duration();
+  return 0;
 }
 
 size_t GIFImageDecoder::DecodeFrameCount() {
-  Parse(kGIFFrameCountQuery);
-  // 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() ? frame_buffer_cache_.size() : reader_->imagesCount();
+  if (!codec_)
+    return 0;
+
+  if (!Failed() && !(segment_stream_ && segment_stream_->IsCleared()))

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

This seems to say that if segment_stream_ is null, we will call getFrameInfo. I
would expect that we would not, but in that case I think codec_ will also be
null?

[cblume, 2017/04/20 03:42:27]

Oops. The ! should have been in front of the IsCleared(). I accidentally clumped
the null check in with it.
Thank you for catching this.

However, segment_stream_ can be null when codec_ is non-null. That is what
SegmentStream::IsCleared() is all about.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...

[scroggo_chromium, 2017/04/21 20:09:31]

I don't think so. If the SegmentReader is set to null, you don't set
segment_stream_ to null, you just call setReader(null).

(segment_reader_ looks to only be null if you don't have a codec or if you've
already failed. In both cases you won't reach this check.)

[cblume, 2017/04/21 23:47:38]

Ah. I misunderstood what you were saying.
You are right, segment_stream_ will not be null in that case.
segment_stream_.reader_ will be.

But still, my mistake was putting the ! on the outside of the (). It needed to
be in front of the IsCleared(). I've fixed this.

[scroggo_chromium, 2017/04/24 15:06:53]

I agree that that was wrong. But I still don't think you need to check for a
null segment_stream_, since it will not be null if we are not in the Failed
state.

[cblume, 2017/04/24 18:22:33]

Done.

+    frame_infos_ = codec_->getFrameInfo();
+  return frame_infos_.size();
 }
 
 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));
+  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.
+  IntSize full_image_size = Size();
+  frame.SetOriginalFrameRect(IntRect(IntPoint(), full_image_size));
+  frame.SetDuration(frame_infos_[index].fDuration);
+  frame.SetHasAlpha(frame_infos_[index].fAlphaType == kPremul_SkAlphaType ||

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

Alternatively, this could be

frame.SetHasAlpha(!SkAlphaTypeIsOpaque(frame_infos_[index].fAlphaType));

[cblume, 2017/04/20 03:42:26]

Doing that would include kUnknown_SkAlphaType.
I'm willing to do this if you prefer it. I avoided it because I was unsure if it
might (now or in the future) be possible for SkGifDecoder to return
kUnknown_SkAlphaType.

I guess it might not be possible. The gif format stores the duration, the
disposal method, and the transparency in the graphics control extension. So we
couldn't fill in the other values while still not knowing the transparency.

Even going byte-by-byte, the disposal method is stored in the same byte. The
duration are in the following 2 bytes. So if we have the duration, we must
already have the transparency information.

Okay, yeah. I'll use SkAlphaTypeIsOpaque(). :)

[scroggo_chromium, 2017/04/21 20:09:31]

Unknown is really just a bogus value to use in e.g. a bitmap that has been
reset().

[cblume, 2017/04/21 23:47:38]

Done.

+                    frame_infos_[index].fAlphaType == kUnpremul_SkAlphaType);
+  size_t required_previous_frame_index = frame_infos_[index].fRequiredFrame;
+  if (required_previous_frame_index == SkCodec::kNone)
+    required_previous_frame_index = WTF::kNotFound;
+  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) {
-  Parse(kGIFFrameCountQuery);
-
   if (Failed())
     return;
 
-  UpdateAggressivePurging(index);
+  if (!codec_)
+    return;
 
-  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)) {
+  if (segment_stream_ && segment_stream_->IsCleared())

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

Again, I think if codec_ is non-null, segment_stream_ will also be non-null.

[cblume, 2017/04/21 23:47:38]

Done.

+    return;
+
+  if (frame_buffer_cache_.size() <= index) {
+    // It is a fatal error if all data is received and we have decoded all
+    // frames available but the file is truncated.
+    if (IsAllDataReceived())
       SetFailed();
-      return;
+
+    return;
+  }
+
+  UpdateAggressivePurging(index);
+  SkImageInfo image_info = codec_->getInfo().makeColorType(kN32_SkColorType);
+
+  SkCodec::Options options;
+  options.fFrameIndex = index;
+  options.fHasPriorFrame = false;
+  options.fZeroInitialized = SkCodec::kYes_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());
+      frame.SetStatus(ImageFrame::kFrameAllocated);

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

I see you added this new state, but it does not look like you ever check for it.
I assume if startIncrementalDecode returns kIncompleteInput, we'll need to call
it again on the next attempt to decode. Right now it looks like we only call it
if the status is kFrameEmpty?

[cblume, 2017/04/20 03:42:26]

You are right.
It looks like I accidentally nested this inside the else branch of the
if(kFrameEmpty)

What I want to happen is:
if(kFrameEmpty)
  call AllocatePixelData() only once
  set to kFrameAllocated

if(kFrameAllocated)
  startIncrementalDecode()

That should let me only allocate once but call startIncrementalDecode() as many
times as needed until we get at least partial.

+      frame.SetHasAlpha(frame_infos_[index].fAlphaType == kPremul_SkAlphaType ||

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

Isn't this taken care of by InitializeNewFrame?

[cblume, 2017/04/20 03:42:27]

Not really.
The call to frame.AllocatePixelData() above essentially resets the alpha to
true.
So after calling that, we have to reset it back to what it *actually* is.

I have a separate CR for this which I'll return to once I get more time:
https://codereview.chromium.org/2762643004/

[scroggo_chromium, 2017/04/21 20:09:31]

When you say it "essentially resets the alpha to true", what do you mean? The
ImageFrame's alpha (has_alpha_) or the SkBitmap's alpha (fInfo.alphaType())?

AllocatePixelData does not appear to modify has_alpha_[1]; it just sets the
alphaType on the SkBitmap, and it always assume there is alpha, since we may not
have decoded the full image.

SetHasAlpha changes has_alpha_ (but I think in this case, you're setting it to
the same value it already was). Then it will ComputeAlphaType and set that to
the SkBitmap, but again, I think it will set it to the same value. It only makes
it opaque if the Status is Complete.

So I don't think this line is necessary.

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...
It looks like that one affects ZeroFillPixelData.

[cblume, 2017/04/21 23:47:38]

Correct.
AllocatePixelData() does not modify has_alpha_. It does modify the SkBitmap's
alpha, though.
This odd mismatch is being addressed in a separate CL:
https://codereview.chromium.org/2762643004/diff/40001/third_party/WebKit/Sour...
But for now, I'm doing what the existing behavior is and what the other decoders
do: explicitly call SetHasAlpha() after calling AllocatePixelData().
For example:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/image...

[scroggo_chromium, 2017/04/24 15:06:53]

FWIW, the line you point to in WEBP is *also* after ZeroFillPixelData, which
*does* affect has_alpha_. That line is also unnecessary, IMO, since it doesn't
change any behavior.

Likewise, this line doesn't change any behavior, but it does add more noise to
this CL. If you think it's necessary to be consistent with the other decoders
(which already are not consistent), I would recommend landing your other change
first to simplify this one.

[cblume, 2017/04/24 18:22:33]

Oh, right. So I also have another CL to separate zeroing from setting the alpha.
But that hasn't landed yet either.
https://codereview.chromium.org/2762643004/

I'll get rid of this .setHasAlpha(). We can put it back when those CLs land and
make it necessary. But you are right -- until then, it is noise.

Be advised that frame.ZeroFillFrameRect() calls .SetHasAlpha(true); so I need to
set it back right after.

+                        frame_infos_[index].fAlphaType ==
+                            kUnpremul_SkAlphaType);
+    } 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;
+      }
+      options.fHasPriorFrame = true;
     }
+    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;
+      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.SetHasAlpha(frame_infos_[index].fAlphaType == kPremul_SkAlphaType ||

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

Again, I think this was handled by InitializeNewFrame?

Maybe you reset it because of the SetHasAlpha(true) call below? Isn't that
unnecessary because we always treat a partial image as having alpha?

[cblume, 2017/04/20 03:42:26]

I reset it because of the SetHasAlpha(true) below, correct.
My thought was a partial image has alpha.
But once it becomes a full image, it may no longer have alpha.

Should the image continue to have alpha even if the palette says there is none
only because it was partially decoded for a bit?

[scroggo_chromium, 2017/04/21 20:09:31]

Yes, that is the way we behave today, and I do not see any reason to change
that. (And certainly not in this larger CL.)

[cblume, 2017/04/21 23:47:38]

I'm not entirely sure I follow.

Inside InitializeNewFrame() we set the frame alpha to what it should be.
We then have to change it to .SetHasAlpha(true) (no matter what it was before)
if we get a partial decode.
But once the frame is done being decoded, we then have to set it back.

I could remove the .SetHasAlpha() call inside InitializeNewFrame() since it is
redundant. Is that what you mean?


Oh, I think you mean that once a frame had a partial decode and became
.SetHasAlpha(true), we leave it that way. We don't set it back to what it
claimed to be. Is that what you mean? If so, I'll remove this line. I agree,
let's not change that behavior if this isn't how it worked before.

[scroggo_chromium, 2017/04/24 15:06:53]

Yes.
I disagree. ImageFrame already handles this for you. I propose that if the frame
is reported to be opaque, you leave has_alpha_ as false. If the image is
incomplete, the SkBitmap will continue to have a non-opaque alpha type.

The only difference will be that ImageFrame::HasAlpha() will return false, but
no clients call that while the frame is incomplete.
No.
No, this is also not what I mean.

[cblume, 2017/04/24 18:22:33]

Okay. The way I understand what you are saying is there will deliberately be a
difference between has_alpha_ and the SkBitmap's alpha.
has_alpha_ will be what the image reports. It does not change during a partial
decode.
SkBitmap's alpha starts as true but ImageFrame::SetStatus() updates it once it
hits FrameComplete.

I see now. Done.

+                        frame_infos_[index].fAlphaType ==
+                            kUnpremul_SkAlphaType);
+      frame.SetPixelsChanged(true);
+      frame.SetStatus(ImageFrame::kFrameComplete);
+      PostDecodeProcessing(index);
+      break;
+    case SkCodec::kIncompleteInput:
+      if (FrameIsCompleteAtIndex(index) || IsAllDataReceived()) {
+        SetFailed();
+        return;
+      }
+      {
+        IntRect remaining_rect = frame.OriginalFrameRect();
+        remaining_rect.SetY(rows_decoded);
+        remaining_rect.SetHeight(remaining_rect.Height() - rows_decoded);
+        frame.ZeroFillFrameRect(remaining_rect);
+        frame.SetHasAlpha(true);
+      }
 
-    // If this returns false, we need more data to continue decoding.
-    if (!PostDecodeProcessing(*i))
+      frame.SetPixelsChanged(true);
       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;
+bool GIFImageDecoder::CanReusePreviousFrameBuffer(size_t index) const {
+  DCHECK(index < frame_buffer_cache_.size());
 
-  if (!reader_) {
-    reader_ = WTF::MakeUnique<GIFImageReader>(this);
-    reader_->setData(data_);
-  }
+  // 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

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

I think I broke this with https://skia-review.googlesource.com/c/9810/.

What you really want to know is the disposal method for frame |index|, which I
(currently) do not provide. This seems like a decent way to determine it, but in
the above CL, I use the frame rectangles to determine dependency. So we could
get in the following situation:

Frame A is Keep
Frame B depends on A, is DisposePrevious
Frame C is independent, because its rectangle covers A's, and it has no
transparency. C is also DisposePrevious
Frame D depends on A

CanReusePreviousFrameBuffer(B) will determine that it can reuse A (since C does
not depend on it), but we'll need A again for D.

Maybe the right solution here is to make SkCodec report the disposal method.

[scroggo_chromium, 2017/04/18 16:18:26]

https://skia-review.googlesource.com/c/13722/ adds the method to SkCodec.

[cblume, 2017/04/20 03:42:27]

I see what you're saying.
I think you might be right.

I think it is wise to keep frame C marked independent. But I cannot use this to
know that frame A is no longer needed and can be safely purged if I'm under
memory pressure. I'll need some other indicator.

I think exposing disposal method is the best way to do it. The only time I'll
know a frame is done is when it is no longer referenced by DisposePrevious. So
if I encounter a frame that is Keep, DisposeBackground, or Unspecified then I
know that frame is no longer used for the rest of the image.

+  // 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 (!reader_->parse(query))
-    SetFailed();
-}
+  if (index + 1 >= frame_buffer_cache_.size())
+    return false;
 
-void GIFImageDecoder::OnInitFrameBuffer(size_t frame_index) {
-  current_buffer_saw_alpha_ = 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();
 
-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 required_frame_index != next_required_frame_index;
 }
 
 }  // namespace blink