Move gif image decoder to SkCodec

third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h

 /*
  * Copyright (C) 2006 Apple Computer, Inc.  All rights reserved.
  * Copyright (C) Research In Motion Limited 2009-2010. 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
@@ skipping 368 matching lines @@
   // This method indicates whether the provided frame has enough data to decode
   // successive frames that depend on it. It is used by clearCacheExceptFrame
   // to determine which frame to keep in cache when the indicated frame is not
   // yet sufficiently decoded.
   //
   // The default condition is that the frame status needs to be FramePartial or
   // FrameComplete, since the data of previous frames is copied in
   // initFrameBuffer() before setting the status to FramePartial. For WebP,
   // however, the status needs to be FrameComplete since the complete buffer is
   // used to do alpha blending in WEBPImageDecoder::applyPostProcessing().
+  // And gif is also treated differently, since it relies on SkCodec which

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

IIUC, previously, GIF and APNG required at least kFramePartial, and WEBP
required kFrameComplete. With this change, GIF matches WEBP, and APNG is the
only one to use the default.

What is different about using SkCodec that requires the frame to be complete
now? Or is it because with SkCodec the Status could now be kFrameAlloc?

Can you write a test for this?

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

Your understanding is correct.
The way I understand it (which might be wrong) is that if I want to decode frame
X+1 (which relies on frame X), I will want to keep frame X around so we don't
decode from the beginning to get it back. That means I need the complete frame X
when using SkCodec.

However, the part I might not understand correctly is why gif didn't previously
require the full frame. I'm not sure why a partial frame would be sufficient to
decode future frames.

I think we actually don't want to test this. It is a protected method. I believe
we want to test public interfaces.

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

It will help if you take a look at the two callsites, in ClearCacheExceptFrame.
If the client wants to clear all frames except X, we don't quite honor it, with
the thinking that the client will probably want to decode X+1 in the future. If
this method returns false for X, we'll *also* keep around the frame that X
depends on.

The default is to compare the status against empty. If X is empty, clearly
keeping it in the cache will not help us decode X+1, so we hang on to the frame
that it depended on.

Partial is enough to decode any frame that depends on X, since we've already
copied the frame that X depends on in order to decode X.

I suspect you really want to make sure we're at least in the FramePartial
Status. If we've only allocated, then we haven't yet done the copy, so we need
to hang on to the frame X depends on in order to finish decoding X. (I just
looked back at the new Decode method. I've added a comment there, too, but now I
think you want to be in at least the FrameAllocated Status. Though you don't
change to that state when you copy the required frame, I believe that was your
intent. In which case the default implementation of this method should work
fine.)
In general, yes, we want to test public interfaces, but you're changing a
behavior. How do we know it's for the better?

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

Okay, I think I follow now.

So I understood that when we clear frames, we might keep the current frame + the
frame it depends on. I now understand that is because we may not be done
decoding this frame. In my mind, we were done decoding this frame but maybe the
disposal method is DisposePrevious. I guess it can be both cases. I just didn't
think of the other.

Anyway, I also now see how if we are at least partial, we know then we know the
frame this depends on is both complete and still in cache.


What threw me is gif (the format, not my patch) requires the required frame to
be complete. Essentially, we might need to continue decoding frame |index|
before it is actually sufficient for successors. It may actually not be
sufficient yet. BUT we are promising that it *can* be sufficient (eventually) so
long as we are partially decoded.



So now that I understand that, 1.) I think I can just keep the old behavior, and
2.) I think this function should be renamed in a separate CL. :)

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

I'm not sure that's the case. If this frame is partial, and *not*
DisposePrevious, what we're saying is that if the following frame depends on
this one, then this frame is decoded enough that there is no need to redecode
its required frame in order to decode the following one. At least in theory, we
could have evicted the frame that this one depends on (so its status is now
Empty), and that does not affect the outcome here.
Agreed.
Yeah, I can see the confusion. Maybe it's clearer if we flip the return value
and call it something like

  NeedRequiredFrameToDecodeSuccessors

and return status == kFrameEmpty in the default version.

+  // requires the complete required previous frame.
   //
   // Before calling this, verify that frame |index| exists by checking that
   // |index| is smaller than |m_frameBufferCache|.size().
   virtual bool FrameStatusSufficientForSuccessors(size_t index) {
     DCHECK(index < frame_buffer_cache_.size());
     return frame_buffer_cache_[index].GetStatus() != ImageFrame::kFrameEmpty;
   }
 
  private:
   enum class SniffResult { JPEG, PNG, GIF, WEBP, ICO, BMP, kInvalid };
@@ skipping 25 matching lines @@
   bool has_histogrammed_color_space_ = false;
 
   sk_sp<SkColorSpace> embedded_color_space_ = nullptr;
   bool source_to_target_color_transform_needs_update_ = false;
   std::unique_ptr<SkColorSpaceXform> source_to_target_color_transform_;
 };
 
 }  // namespace blink
 
 #endif