Stop building SkCodec

Bug: 768878

Building SkCodec seems to have caused a paint regression on a webpage
without any images. This leads us to suspect "some minor compiler
optimization tickling". Stop building it to confirm. Two CLs rely on
SkCodec:

"Use SkCodec internally in GIFImageDecoder"
4fed3346. This introduced building
SkCodec.

"Enable Skia's SkImageGenerator implementation"
f5eb27c2. This used SkCodec to fix
crbug.com/758459, but that seems to have been fixed in another way.

In addition, this corrects some formatting in the old code (as
commanded by presubmit), and makes some other minor changes (no more
PassRefPtr, FrameDurationAtIndex now returns a TimeDelta).

Change-Id: Ic2bdd87740da0232c9c07e27eed6049efc26d76c
Reviewed-on: https://chromium-review.googlesource.com/718918
Commit-Queue: Leon Scroggins <scroggo@chromium.org>
Reviewed-by: Chris Blume <cblume@chromium.org>
Reviewed-by: Fredrik Söderquist <fs@opera.com>
Reviewed-by: Leon Scroggins <scroggo@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509570}

skia/BUILD.gn

@@ -45,10 +45,6 @@ config("skia_config") {
     "//third_party/skia/third_party/vulkan",
   ]
 
-  if (!is_ios) {
-    include_dirs += [ "//third_party/skia/include/codec" ]
-  }
-
   defines = skia_for_chromium_defines
   defines += [
     "SK_HAS_PNG_LIBRARY",
@@ -286,39 +282,16 @@ component("skia") {
     "//third_party/skia/src/images/SkPngEncoder.cpp",
     "//third_party/skia/src/images/SkWebpEncoder.cpp",
     "//third_party/skia/src/ports/SkGlobalInitialization_default.cpp",
+    "//third_party/skia/src/ports/SkImageGenerator_none.cpp",
     "//third_party/skia/src/ports/SkOSFile_stdio.cpp",
     "//third_party/skia/src/sfnt/SkOTTable_name.cpp",
     "//third_party/skia/src/sfnt/SkOTUtils.cpp",
   ]
   if (!is_ios) {
     sources += [
-      "//third_party/skia/src/codec/SkBmpBaseCodec.cpp",
-      "//third_party/skia/src/codec/SkBmpCodec.cpp",
-      "//third_party/skia/src/codec/SkBmpMaskCodec.cpp",
-      "//third_party/skia/src/codec/SkBmpRLECodec.cpp",
-      "//third_party/skia/src/codec/SkBmpStandardCodec.cpp",
-      "//third_party/skia/src/codec/SkCodec.cpp",
-      "//third_party/skia/src/codec/SkCodecImageGenerator.cpp",
-      "//third_party/skia/src/codec/SkGifCodec.cpp",
-      "//third_party/skia/src/codec/SkIcoCodec.cpp",
-      "//third_party/skia/src/codec/SkJpegCodec.cpp",
-      "//third_party/skia/src/codec/SkJpegDecoderMgr.cpp",
-      "//third_party/skia/src/codec/SkJpegUtility.cpp",
-      "//third_party/skia/src/codec/SkMaskSwizzler.cpp",
-      "//third_party/skia/src/codec/SkMasks.cpp",
-      "//third_party/skia/src/codec/SkPngCodec.cpp",
-      "//third_party/skia/src/codec/SkSampler.cpp",
-      "//third_party/skia/src/codec/SkStreamBuffer.cpp",
-      "//third_party/skia/src/codec/SkSwizzler.cpp",
-      "//third_party/skia/src/codec/SkWbmpCodec.cpp",
-      "//third_party/skia/src/codec/SkWebpCodec.cpp",
       "//third_party/skia/src/images/SkJPEGWriteUtility.cpp",
       "//third_party/skia/src/images/SkJpegEncoder.cpp",
-      "//third_party/skia/src/ports/SkImageGenerator_skia.cpp",
-      "//third_party/skia/third_party/gif/SkGifImageReader.cpp",
     ]
-  } else {
-    sources += [ "//third_party/skia/src/ports/SkImageGenerator_none.cpp" ]
   }
 
   # This and skia_opts are really the same conceptual target so share headers.

third_party/WebKit/Source/platform/BUILD.gn

@@ -1178,14 +1178,14 @@ jumbo_component("platform") {
     "image-decoders/ImageFrame.h",
     "image-decoders/SegmentReader.cpp",
     "image-decoders/SegmentReader.h",
-    "image-decoders/SegmentStream.cpp",
-    "image-decoders/SegmentStream.h",
     "image-decoders/bmp/BMPImageDecoder.cpp",
     "image-decoders/bmp/BMPImageDecoder.h",
     "image-decoders/bmp/BMPImageReader.cpp",
     "image-decoders/bmp/BMPImageReader.h",
     "image-decoders/gif/GIFImageDecoder.cpp",
     "image-decoders/gif/GIFImageDecoder.h",
+    "image-decoders/gif/GIFImageReader.cpp",
+    "image-decoders/gif/GIFImageReader.h",
     "image-decoders/ico/ICOImageDecoder.cpp",
     "image-decoders/ico/ICOImageDecoder.h",
     "image-decoders/jpeg/JPEGImageDecoder.cpp",
@@ -1859,7 +1859,6 @@ jumbo_source_set("blink_platform_unittests_sources") {
     "image-decoders/ImageDecoderTest.cpp",
     "image-decoders/ImageDecoderTestHelpers.cpp",
     "image-decoders/ImageDecoderTestHelpers.h",
-    "image-decoders/SegmentStreamTest.cpp",
     "image-decoders/bmp/BMPImageDecoderTest.cpp",
     "image-decoders/gif/GIFImageDecoderTest.cpp",
     "image-decoders/ico/ICOImageDecoderTest.cpp",

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

@@ -253,6 +253,15 @@ class PLATFORM_EXPORT ImageDecoder {
   // Callers may pass WTF::kNotFound to clear all frames.
   // Note: If |frame_buffer_cache_| contains only one frame, it won't be
   // cleared. Returns the number of bytes of frame data actually cleared.
+  //
+  // This is a virtual method because MockImageDecoder needs to override it in
+  // order to run the test ImageFrameGeneratorTest::ClearMultiFrameDecode.
+  //
+  // @TODO  Let MockImageDecoder override ImageFrame::ClearFrameBuffer instead,
+  //        so this method can be made non-virtual. It is used in the test
+  //        ImageFrameGeneratorTest::ClearMultiFrameDecode. The test needs to
+  //        be modified since two frames may be kept in cache, instead of
+  //        always just one, with this ClearCacheExceptFrame implementation.
   virtual size_t ClearCacheExceptFrame(size_t);
 
   // If the image has a cursor hot-spot, stores it in the argument
@@ -392,9 +401,7 @@ class PLATFORM_EXPORT ImageDecoder {
   // |index| is smaller than |frame_buffer_cache_|.size().
   virtual bool FrameStatusSufficientForSuccessors(size_t index) {
     DCHECK(index < frame_buffer_cache_.size());
-    ImageFrame::Status frame_status = frame_buffer_cache_[index].GetStatus();
-    return frame_status == ImageFrame::kFramePartial ||
-           frame_status == ImageFrame::kFrameComplete;
+    return frame_buffer_cache_[index].GetStatus() != ImageFrame::kFrameEmpty;
   }
 
  private:

third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp

@@ -82,12 +82,8 @@ bool ImageFrame::CopyBitmapData(const ImageFrame& other) {
   has_alpha_ = other.has_alpha_;
   bitmap_.reset();
   SkImageInfo info = other.bitmap_.info();
-  if (!bitmap_.tryAllocPixels(info)) {
-    return false;
-  }
-
-  status_ = kFrameAllocated;
-  return other.bitmap_.readPixels(info, bitmap_.getPixels(), bitmap_.rowBytes(),
+  return bitmap_.tryAllocPixels(info) &&
+         other.bitmap_.readPixels(info, bitmap_.getPixels(), bitmap_.rowBytes(),
                                   0, 0);
 }
 
@@ -102,7 +98,6 @@ bool ImageFrame::TakeBitmapDataIfWritable(ImageFrame* other) {
   bitmap_.reset();
   bitmap_.swap(other->bitmap_);
   other->status_ = kFrameEmpty;
-  status_ = kFrameAllocated;
   return true;
 }
 
@@ -116,11 +111,7 @@ bool ImageFrame::AllocatePixelData(int new_width,
       new_width, new_height,
       premultiply_alpha_ ? kPremul_SkAlphaType : kUnpremul_SkAlphaType,
       std::move(color_space)));
-  bool allocated = bitmap_.tryAllocPixels(allocator_);
-  if (allocated)
-    status_ = kFrameAllocated;
-
-  return allocated;
+  return bitmap_.tryAllocPixels(allocator_);
 }
 
 bool ImageFrame::HasAlpha() const {

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

@@ -46,7 +46,7 @@ class PLATFORM_EXPORT ImageFrame final {
   DISALLOW_NEW_EXCEPT_PLACEMENT_NEW();
 
  public:
-  enum Status { kFrameEmpty, kFrameAllocated, kFramePartial, kFrameComplete };
+  enum Status { kFrameEmpty, kFramePartial, kFrameComplete };
   enum DisposalMethod {
     // If you change the numeric values of these, make sure you audit
     // all users, as some users may cast raw values to/from these

third_party/WebKit/Source/platform/image-decoders/SegmentStream.cpp

-// Copyright (c) 2017 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "platform/image-decoders/SegmentStream.h"
-#include <utility>
-#include "platform/image-decoders/SegmentReader.h"
-
-namespace blink {
-
-SegmentStream::SegmentStream() = default;
-
-SegmentStream::SegmentStream(SegmentStream&& rhs)
-    : reader_(std::move(rhs.reader_)), position_(rhs.position_) {}
-
-SegmentStream& SegmentStream::operator=(SegmentStream&& rhs) {
-  reader_ = std::move(rhs.reader_);
-  position_ = rhs.position_;
-
-  return *this;
-}
-
-SegmentStream::~SegmentStream() = default;
-
-void SegmentStream::SetReader(WTF::RefPtr<SegmentReader> reader) {
-  reader_ = std::move(reader);
-}
-
-bool SegmentStream::IsCleared() const {
-  return !reader_ || position_ > reader_->size();
-}
-
-size_t SegmentStream::read(void* buffer, size_t size) {
-  DCHECK(!IsCleared());
-
-  size = std::min(size, reader_->size() - position_);
-
-  size_t bytes_advanced = 0;
-  if (!buffer) {  // skipping, not reading
-    bytes_advanced = size;
-  } else {
-    bytes_advanced = peek(buffer, size);
-  }
-
-  position_ += bytes_advanced;
-
-  return bytes_advanced;
-}
-
-size_t SegmentStream::peek(void* buffer, size_t size) const {
-  DCHECK(!IsCleared());
-
-  size = std::min(size, reader_->size() - position_);
-
-  size_t total_bytes_peeked = 0;
-  char* buffer_as_char_ptr = reinterpret_cast<char*>(buffer);
-  while (size) {
-    const char* segment = nullptr;
-    size_t bytes_peeked =
-        reader_->GetSomeData(segment, position_ + total_bytes_peeked);
-    if (!bytes_peeked)
-      break;
-    if (bytes_peeked > size)
-      bytes_peeked = size;
-
-    memcpy(buffer_as_char_ptr, segment, bytes_peeked);
-    buffer_as_char_ptr += bytes_peeked;
-    size -= bytes_peeked;
-    total_bytes_peeked += bytes_peeked;
-  }
-
-  return total_bytes_peeked;
-}
-
-bool SegmentStream::isAtEnd() const {
-  return !reader_ || position_ >= reader_->size();
-}
-
-bool SegmentStream::rewind() {
-  position_ = 0;
-  return true;
-}
-
-bool SegmentStream::seek(size_t position) {
-  position_ = position;
-  return true;
-}
-
-bool SegmentStream::move(long offset) {
-  DCHECK_GT(offset, 0);
-  position_ += offset;
-  return true;
-}
-
-size_t SegmentStream::getLength() const {
-  if (reader_)
-    return reader_->size();
-
-  return 0;
-}
-
-}  // namespace blink

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

-// Copyright (c) 2017 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef SegmentStream_h
-#define SegmentStream_h
-
-#include <algorithm>
-#include "platform/PlatformExport.h"
-#include "platform/wtf/RefPtr.h"
-#include "third_party/skia/include/core/SkStream.h"
-
-namespace blink {
-
-class SegmentReader;
-
-class PLATFORM_EXPORT SegmentStream : public SkStream {
- public:
-  SegmentStream();
-  SegmentStream(const SegmentStream&) = delete;
-  SegmentStream& operator=(const SegmentStream&) = delete;
-  SegmentStream(SegmentStream&&);
-  SegmentStream& operator=(SegmentStream&&);
-
-  ~SegmentStream() override;
-
-  void SetReader(WTF::RefPtr<SegmentReader>);
-  // If a buffer has shrunk beyond the point we have read, it has been cleared.
-  // This allows clients to be aware of when data suddenly disappears.
-  bool IsCleared() const;
-
-  // From SkStream:
-  size_t read(void* buffer, size_t) override;
-  size_t peek(void* buffer, size_t) const override;
-  bool isAtEnd() const override;
-  bool rewind() override;
-  bool hasPosition() const override { return true; }
-  size_t getPosition() const override { return position_; }
-  bool seek(size_t position) override;
-  bool move(long offset) override;
-  bool hasLength() const override { return true; }
-  size_t getLength() const override;
-
- private:
-  WTF::RefPtr<SegmentReader> reader_;
-  size_t position_ = 0;
-};
-
-}  // namespace blink
-
-#endif

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

@@ -29,13 +29,13 @@
 #include <memory>
 #include "platform/image-decoders/ImageDecoder.h"
 #include "platform/wtf/Noncopyable.h"
-#include "platform/wtf/RefPtr.h"
 #include "platform/wtf/Time.h"
-#include "third_party/skia/include/codec/SkCodec.h"
 
 namespace blink {
 
-class SegmentStream;
+class GIFImageReader;
+
+using GIFRow = Vector<unsigned char>;
 
 // This class decodes the GIF image format.
 class PLATFORM_EXPORT GIFImageDecoder final : public ImageDecoder {
@@ -45,43 +45,56 @@ class PLATFORM_EXPORT GIFImageDecoder final : public ImageDecoder {
   GIFImageDecoder(AlphaOption, const ColorBehavior&, size_t max_decoded_bytes);
   ~GIFImageDecoder() override;
 
+  enum GIFParseQuery { kGIFSizeQuery, kGIFFrameCountQuery };
+
   // ImageDecoder:
   String FilenameExtension() const override { return "gif"; }
   void OnSetData(SegmentReader* data) override;
   int RepetitionCount() const override;
   bool FrameIsReceivedAtIndex(size_t) const override;
   TimeDelta FrameDurationAtIndex(size_t) const override;
-  // CAUTION: SetFailed() deletes |codec_|. Be careful to avoid
-  // accessing deleted memory.
+  // CAUTION: SetFailed() deletes |reader_|.  Be careful to avoid
+  // accessing deleted memory, especially when calling this from inside
+  // GIFImageReader!
   bool SetFailed() override;
 
-  size_t ClearCacheExceptFrame(size_t) override;
+  // Callbacks from the GIF reader.
+  bool HaveDecodedRow(size_t frame_index,
+                      GIFRow::const_iterator row_begin,
+                      size_t width,
+                      size_t row_number,
+                      unsigned repeat_count,
+                      bool write_transparent_pixels);
+  bool FrameComplete(size_t frame_index);
+
+  // For testing.
+  bool ParseCompleted() const;
 
  private:
   // ImageDecoder:
-  void DecodeSize() override {}
+  void ClearFrameBuffer(size_t frame_index) override;
+  virtual void DecodeSize() { Parse(kGIFSizeQuery); }
   size_t DecodeFrameCount() override;
   void InitializeNewFrame(size_t) override;
   void Decode(size_t) override;
+
+  // Parses as much as is needed to answer the query, ignoring bitmap
+  // data. If parsing fails, sets the "decode failure" flag.
+  void Parse(GIFParseQuery);
+
+  // Reset the alpha tracker for this frame. Before calling this method, the
+  // caller must verify that the frame exists.
+  void OnInitFrameBuffer(size_t) override;
+
   // When the disposal method of the frame is DisposeOverWritePrevious, the
-  // next frame will use a previous frame's buffer as its starting state, so
+  // next frame will use the previous frame's buffer as its starting state, so
   // we can't take over the data in that case. Before calling this method, the
   // caller must verify that the frame exists.
   bool CanReusePreviousFrameBuffer(size_t) const override;
 
-  // When a frame depends on a previous frame's content, there is a list of
-  // candidate reference frames. This function will find a previous frame from
-  // that list which satisfies the requirements of being a reference frame
-  // (kFrameComplete, not kDisposeOverwritePrevious).
-  // If no frame is found, it returns kNotFound.
-  size_t GetViableReferenceFrameIndex(size_t) const;
-
-  std::unique_ptr<SkCodec> codec_;
-  // |codec_| owns the SegmentStream, but we need access to it to append more
-  // data as it arrives.
-  SegmentStream* segment_stream_;
-  mutable int repetition_count_ = kAnimationLoopOnce;
-  int prior_frame_;
+  bool current_buffer_saw_alpha_;
+  mutable int repetition_count_;
+  std::unique_ptr<GIFImageReader> reader_;
 };
 
 }  // namespace blink

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

@@ -58,8 +58,16 @@ void TestRepetitionCount(const char* dir,
   RefPtr<SharedBuffer> data = ReadFile(dir, file);
   ASSERT_TRUE(data.get());
   decoder->SetData(data.get(), true);
+  EXPECT_EQ(kAnimationLoopOnce,
+            decoder->RepetitionCount());  // Default value before decode.
 
-  EXPECT_EQ(expected_repetition_count, decoder->RepetitionCount());
+  for (size_t i = 0; i < decoder->FrameCount(); ++i) {
+    ImageFrame* frame = decoder->DecodeFrameBufferAtIndex(i);
+    EXPECT_EQ(ImageFrame::kFrameComplete, frame->GetStatus());
+  }
+
+  EXPECT_EQ(expected_repetition_count,
+            decoder->RepetitionCount());  // Expected value after decode.
 }
 
 }  // anonymous namespace
@@ -70,6 +78,7 @@ TEST(GIFImageDecoderTest, decodeTwoFrames) {
   RefPtr<SharedBuffer> data = ReadFile(kLayoutTestResourcesDir, "animated.gif");
   ASSERT_TRUE(data.get());
   decoder->SetData(data.get(), true);
+  EXPECT_EQ(kAnimationLoopOnce, decoder->RepetitionCount());
 
   ImageFrame* frame = decoder->DecodeFrameBufferAtIndex(0);
   uint32_t generation_id0 = frame->Bitmap().getGenerationID();
@@ -94,6 +103,10 @@ TEST(GIFImageDecoderTest, parseAndDecode) {
   RefPtr<SharedBuffer> data = ReadFile(kLayoutTestResourcesDir, "animated.gif");
   ASSERT_TRUE(data.get());
   decoder->SetData(data.get(), true);
+  EXPECT_EQ(kAnimationLoopOnce, decoder->RepetitionCount());
+
+  // This call will parse the entire file.
+  EXPECT_EQ(2u, decoder->FrameCount());
 
   ImageFrame* frame = decoder->DecodeFrameBufferAtIndex(0);
   EXPECT_EQ(ImageFrame::kFrameComplete, frame->GetStatus());
@@ -307,13 +320,10 @@ TEST(GIFImageDecoderTest, invalidDisposalMethod) {
 
   EXPECT_EQ(2u, decoder->FrameCount());
   // Disposal method 4 is converted to ImageFrame::DisposeOverwritePrevious.
-  // This is because some specs say method 3 is "overwrite previous", while
-  // others say setting the third bit (i.e. method 4) is.
   EXPECT_EQ(ImageFrame::kDisposeOverwritePrevious,
             decoder->DecodeFrameBufferAtIndex(0)->GetDisposalMethod());
-  // Unknown disposal methods (5 in this case) are converted to
-  // ImageFrame::DisposeKeep.
-  EXPECT_EQ(ImageFrame::kDisposeKeep,
+  // Disposal method 5 is ignored.
+  EXPECT_EQ(ImageFrame::kDisposeNotSpecified,
             decoder->DecodeFrameBufferAtIndex(1)->GetDisposalMethod());
 }
 
@@ -380,11 +390,11 @@ TEST(GIFImageDecoderTest, bitmapAlphaType) {
   ImageFrame* premul_frame = premul_decoder->DecodeFrameBufferAtIndex(0);
   EXPECT_TRUE(premul_frame &&
               premul_frame->GetStatus() != ImageFrame::kFrameComplete);
-  EXPECT_EQ(kPremul_SkAlphaType, premul_frame->Bitmap().alphaType());
+  EXPECT_EQ(premul_frame->Bitmap().alphaType(), kPremul_SkAlphaType);
   ImageFrame* unpremul_frame = unpremul_decoder->DecodeFrameBufferAtIndex(0);
   EXPECT_TRUE(unpremul_frame &&
               unpremul_frame->GetStatus() != ImageFrame::kFrameComplete);
-  EXPECT_EQ(kUnpremul_SkAlphaType, unpremul_frame->Bitmap().alphaType());
+  EXPECT_EQ(unpremul_frame->Bitmap().alphaType(), kUnpremul_SkAlphaType);
 
   // Fully decoded frame => the frame alpha type is known (opaque).
   premul_decoder->SetData(full_data_buffer.get(), true);
@@ -394,11 +404,11 @@ TEST(GIFImageDecoderTest, bitmapAlphaType) {
   premul_frame = premul_decoder->DecodeFrameBufferAtIndex(0);
   EXPECT_TRUE(premul_frame &&
               premul_frame->GetStatus() == ImageFrame::kFrameComplete);
-  EXPECT_EQ(kOpaque_SkAlphaType, premul_frame->Bitmap().alphaType());
+  EXPECT_EQ(premul_frame->Bitmap().alphaType(), kOpaque_SkAlphaType);
   unpremul_frame = unpremul_decoder->DecodeFrameBufferAtIndex(0);
   EXPECT_TRUE(unpremul_frame &&
               unpremul_frame->GetStatus() == ImageFrame::kFrameComplete);
-  EXPECT_EQ(kOpaque_SkAlphaType, unpremul_frame->Bitmap().alphaType());
+  EXPECT_EQ(unpremul_frame->Bitmap().alphaType(), kOpaque_SkAlphaType);
 }
 
 namespace {