Fix stream decoding Webp from unrewindable stream Previous CL [1] changed SkWebpCodec to defer copying the stream data by owning the fStream. Comment suggested that deleting the stream was optional [2] but this actually was necessary to indicate to SkCodec that we didn't want to try to call rewind() [3]. This CL introduces a protected helper function rewindStream(), which returns true to indicate that we can rewind the fStream, but SkWebp's override of onRewind doesn't call this function since we have the SkData copy. Adds test case the reproduces this bug in just skia (bug was found in android). Tries to decode multiple frames of webp with unrewindable stream. [1] https://skia-review.googlesource.com/c/skia/+/989596 [2] https://source.chromium.org/chromium/_/skia/skia/+/c2f00d09af9834259ba596a8cc7c0f92c6ba148e:src/codec/SkWebpCodec.cpp;drc=a49594026be7fb586b18700e3b39a4cd2642ce7b;l=77 [3] https://source.chromium.org/chromium/chromium/src/+/main:third_party/skia/src/codec/SkCodec.cpp;drc=d3ff0fc4ed435f672bd7492f7b6e3063bac2dab2;l=329 Bug: 421351189 Change-Id: I274ea5055da2d0843457ff850f6d1071df0716f3 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1013978 Reviewed-by: Florin Malita <fmalita@google.com> Commit-Queue: Daniel Dilan <danieldilan@google.com>

diff --git a/include/codec/SkCodec.h b/include/codec/SkCodec.h
index dc5f202..fd34cd7 100644
--- a/include/codec/SkCodec.h
+++ b/include/codec/SkCodec.h

@@ -904,11 +904,20 @@
     [[nodiscard]] bool rewindIfNeeded();
 
     /**
+     *  Called by onRewind(), attempts to rewind fStream.
+     */
+    bool rewindStream();
+
+    /**
      *  Called by rewindIfNeeded, if the stream needed to be rewound.
      *
-     *  Subclasses should do any set up needed after a rewind.
+     *  Subclasses should call rewindStream() if they own one, and then
+     *  do any set up needed after.
      */
     virtual bool onRewind() {
+        if (!this->rewindStream()) {
+            return false;
+        }
         return true;
     }
 

diff --git a/src/codec/SkBmpCodec.cpp b/src/codec/SkBmpCodec.cpp
index 134020c..0ac4ab8 100644
--- a/src/codec/SkBmpCodec.cpp
+++ b/src/codec/SkBmpCodec.cpp

@@ -628,6 +628,9 @@
 {}
 
 bool SkBmpCodec::onRewind() {
+    if (!this->rewindStream()) {
+        return false;
+    }
     return SkBmpCodec::ReadHeader(this->stream(), this->inIco(), nullptr) == kSuccess;
 }
 

diff --git a/src/codec/SkCodec.cpp b/src/codec/SkCodec.cpp
index c71e49f..a4f1528 100644
--- a/src/codec/SkCodec.cpp
+++ b/src/codec/SkCodec.cpp

@@ -312,6 +312,15 @@
     }
 }
 
+bool SkCodec::rewindStream() {
+    // Some codecs do not have a stream.  They may hold onto their own data or another codec.
+    // They must handle rewinding themselves.
+    if (fStream && !fStream->rewind()) {
+        return false;
+    }
+    return true;
+}
+
 bool SkCodec::rewindIfNeeded() {
     // Store the value of fNeedsRewind so we can update it. Next read will
     // require a rewind.
@@ -326,12 +335,6 @@
     // startIncrementalDecode will need to be called before incrementalDecode.
     fStartedIncrementalDecode = false;
 
-    // Some codecs do not have a stream.  They may hold onto their own data or another codec.
-    // They must handle rewinding themselves.
-    if (fStream && !fStream->rewind()) {
-        return false;
-    }
-
     return this->onRewind();
 }
 

diff --git a/src/codec/SkHeifCodec.cpp b/src/codec/SkHeifCodec.cpp
index f8eea19..f605521 100644
--- a/src/codec/SkHeifCodec.cpp
+++ b/src/codec/SkHeifCodec.cpp

@@ -514,6 +514,9 @@
 }
 
 bool SkHeifCodec::onRewind() {
+    if (!this->rewindStream()) {
+        return false;
+    }
     fSwizzler.reset(nullptr);
     fSwizzleSrcRow = nullptr;
     fColorXformSrcRow = nullptr;

diff --git a/src/codec/SkJpegCodec.cpp b/src/codec/SkJpegCodec.cpp
index 8eb311e..6dae48c 100644
--- a/src/codec/SkJpegCodec.cpp
+++ b/src/codec/SkJpegCodec.cpp

@@ -267,6 +267,9 @@
 }
 
 bool SkJpegCodec::onRewind() {
+    if (!this->rewindStream()) {
+        return false;
+    }
     JpegDecoderMgr* decoderMgr = nullptr;
     if (kSuccess != ReadHeader(this->stream(), nullptr, &decoderMgr, nullptr)) {
         return fDecoderMgr->returnFalse("onRewind");

diff --git a/src/codec/SkJpegxlCodec.cpp b/src/codec/SkJpegxlCodec.cpp
index adc62f8..4918cc8 100644
--- a/src/codec/SkJpegxlCodec.cpp
+++ b/src/codec/SkJpegxlCodec.cpp

@@ -289,6 +289,9 @@
 }
 
 bool SkJpegxlCodec::onRewind() {
+    if (!this->rewindStream()) {
+        return false;
+    }
     JxlDecoderRewind(fCodec->fDecoder.get());
     return true;
 }

diff --git a/src/codec/SkPngCodec.cpp b/src/codec/SkPngCodec.cpp
index 98f355d..9211d0c 100644
--- a/src/codec/SkPngCodec.cpp
+++ b/src/codec/SkPngCodec.cpp

@@ -977,6 +977,9 @@
 }
 
 bool SkPngCodec::onRewind() {
+    if (!this->rewindStream()) {
+        return false;
+    }
     // This sets fPng_ptr and fInfo_ptr to nullptr. If read_header
     // succeeds, they will be repopulated, and if it fails, they will
     // remain nullptr. Any future accesses to fPng_ptr and fInfo_ptr will

diff --git a/src/codec/SkWbmpCodec.cpp b/src/codec/SkWbmpCodec.cpp
index 2678407..6fb2469 100644
--- a/src/codec/SkWbmpCodec.cpp
+++ b/src/codec/SkWbmpCodec.cpp

@@ -97,6 +97,9 @@
 }
 
 bool SkWbmpCodec::onRewind() {
+    if (!this->rewindStream()) {
+        return false;
+    }
     return read_header(this->stream(), nullptr);
 }
 

diff --git a/src/codec/SkWebpCodec.cpp b/src/codec/SkWebpCodec.cpp
index ca2ec05..3d33182 100644
--- a/src/codec/SkWebpCodec.cpp
+++ b/src/codec/SkWebpCodec.cpp

@@ -419,6 +419,12 @@
     return (flags & ANIMATION_FLAG) != 0 ? IsAnimated::kYes : IsAnimated::kNo;
 }
 
+bool SkWebpCodec::onRewind() {
+    // We may have a stream, but we never need to rewind it because we hold our own
+    // SkData copy.
+    return true;
+}
+
 int SkWebpCodec::onGetFrameCount() {
     auto flags = WebPDemuxGetI(fDemux.get(), WEBP_FF_FORMAT_FLAGS);
     if (!(flags & ANIMATION_FLAG)) {

diff --git a/src/codec/SkWebpCodec.h b/src/codec/SkWebpCodec.h
index df1f230..edb2274 100644
--- a/src/codec/SkWebpCodec.h
+++ b/src/codec/SkWebpCodec.h

@@ -47,6 +47,7 @@
     bool onGetFrameInfo(int, FrameInfo*) const override;
     int onGetRepetitionCount() override;
     IsAnimated onIsAnimated() override;
+    bool onRewind() override;
 
     const SkFrameHolder* getFrameHolder() const override {
         return &fFrameHolder;

diff --git a/tests/CodecTest.cpp b/tests/CodecTest.cpp
index 5034662..c99543f 100644
--- a/tests/CodecTest.cpp
+++ b/tests/CodecTest.cpp

@@ -2512,3 +2512,29 @@
         }
     }
 }
+
+DEF_TEST(Codec_webp_animated_image_rewind, r) {
+    // stoplight.webp is an animated image.
+    constexpr char path[] = "images/stoplight.webp";
+    sk_sp<SkData> data(GetResourceAsData(path));
+    if (!data) {
+        ERRORF(r, "Could not create data for: %s", path);
+    }
+    auto codec = SkCodec::MakeFromStream(std::make_unique<NonseekableStream>(std::move(data)),
+                                         nullptr, nullptr,
+                                         SkCodec::SelectionPolicy::kPreferStillImage);
+    SkCodec::Options options;
+    options.fFrameIndex = 0;
+    SkBitmap bm;
+    bm.allocPixels(codec->getInfo());
+    SkCodec::Result res = codec->getPixels(codec->getInfo(), bm.getAddr(0,0), bm.rowBytes(),
+                                           &options);
+    REPORTER_ASSERT(r, res == SkCodec::Result::kSuccess);
+
+    // For a non-rewindable stream, reading the next frame from an animated image should
+    // still succeed.
+    options.fPriorFrame = 0;
+    options.fFrameIndex = 1;
+    res = codec->getPixels(codec->getInfo(), bm.getAddr(0,0), bm.rowBytes(), &options);
+    REPORTER_ASSERT(r, res == SkCodec::Result::kSuccess);
+}