Report first GIF frame after knowing its meta data

Previously, we reported the first image as soon as it was available. As
a result, in crrev.com/2565323003, InitializeNewFrame might be called
before the metadata is known, meaning it would read the wrong metadata.

Instead of looking at the imagesCount(), SkGifCodec::NewFromStream looks
at frameContext(0), which may still exist even if it's not yet counted
in imagesCount().

Add a test that confirms the desired behavior.

Change-Id: Ib392721ecd2218ba0fcd35aaa64117c0ba3e4ea6
Reviewed-on: https://skia-review.googlesource.com/24405
Reviewed-by: Derek Sollenberger <djsollen@google.com>
Commit-Queue: Leon Scroggins <scroggo@google.com>

3 files changed, 30 insertions, 10 deletions

diff --git a/src/codec/SkGifCodec.cpp b/src/codec/SkGifCodec.cpp
index 9b3d4b5ed5..7b31a618ea 100644
--- a/src/codec/SkGifCodec.cpp
+++ b/src/codec/SkGifCodec.cpp
@@ -76,7 +76,8 @@ SkCodec* SkGifCodec::NewFromStream(SkStream* stream, Result* result) {
 
     // If no images are in the data, or the first header is not yet defined, we cannot
     // create a codec. In either case, the width and height are not yet known.
-    if (0 == reader->imagesCount() || !reader->frameContext(0)->isHeaderDefined()) {
+    auto* frame = reader->frameContext(0);
+    if (!frame || !frame->isHeaderDefined()) {
         *result = kInvalidInput;
         return nullptr;
     }
@@ -136,9 +137,7 @@ bool SkGifCodec::onGetFrameInfo(int i, SkCodec::FrameInfo* frameInfo) const {
     }
 
     const SkGIFFrameContext* frameContext = fReader->frameContext(i);
-    if (!frameContext->reachedStartOfData()) {
-        return false;
-    }
+    SkASSERT(frameContext->reachedStartOfData());
 
     if (frameInfo) {
         frameInfo->fDuration = frameContext->getDuration();
diff --git a/tests/GifTest.cpp b/tests/GifTest.cpp
index 7728d27dcb..5749df1be5 100644
--- a/tests/GifTest.cpp
+++ b/tests/GifTest.cpp
@@ -251,11 +251,34 @@ DEF_TEST(Gif_Sampled, r) {
 // If a GIF file is truncated before the header for the first image is defined,
 // we should not create an SkCodec.
 DEF_TEST(Codec_GifTruncated, r) {
-    SkString path = GetResourcePath("test640x479.gif");
-    sk_sp<SkData> data(SkData::MakeFromFileName(path.c_str()));
+    sk_sp<SkData> data(GetResourceAsData("test640x479.gif"));
+    if (!data) {
+        return;
+    }
 
     // This is right before the header for the first image.
     data = SkData::MakeSubset(data.get(), 0, 446);
     std::unique_ptr<SkCodec> codec(SkCodec::NewFromData(data));
     REPORTER_ASSERT(r, !codec);
 }
+
+DEF_TEST(Codec_GifTruncated2, r) {
+    sk_sp<SkData> data(GetResourceAsData("box.gif"));
+    if (!data) {
+        return;
+    }
+
+    // This is after the header, but before the color table.
+    data = SkData::MakeSubset(data.get(), 0, 23);
+    std::unique_ptr<SkCodec> codec(SkCodec::NewFromData(data));
+    if (!codec) {
+        ERRORF(r, "Failed to create codec with partial data");
+        return;
+    }
+
+    // Although we correctly created a codec, no frame is
+    // complete enough that it has its metadata. Returning 0
+    // ensures that Chromium will not try to create a frame
+    // too early.
+    REPORTER_ASSERT(r, codec->getFrameCount() == 0);
+}
diff --git a/third_party/gif/SkGifImageReader.h b/third_party/gif/SkGifImageReader.h
index 0b89b04818..0ba767ee89 100644
--- a/third_party/gif/SkGifImageReader.h
+++ b/third_party/gif/SkGifImageReader.h
@@ -321,11 +321,9 @@ public:
 
     int imagesCount() const
     {
-        // Report the first frame immediately, so the parser can stop when it
-        // sees the size on a SizeQuery.
         const size_t frames = m_frames.size();
-        if (frames <= 1) {
-            return static_cast<int>(frames);
+        if (!frames) {
+            return 0;
         }
 
         // This avoids counting an empty frame when the file is truncated (or