Use demux API in SkWebpCodec

src/codec/SkWebpCodec.cpp

Index: src/codec/SkWebpCodec.cpp
diff --git a/src/codec/SkWebpCodec.cpp b/src/codec/SkWebpCodec.cpp
index c28d077bb329826750ebca4bb92ac2f005e2ac02..77af3dce39d29112bb1884ae93b1742a1f14bf6f 100644
--- a/src/codec/SkWebpCodec.cpp
+++ b/src/codec/SkWebpCodec.cpp
@@ -36,24 +36,61 @@ bool SkWebpCodec::IsWebp(const void* buf, size_t bytesRead) {
 SkCodec* SkWebpCodec::NewFromStream(SkStream* stream) {
     SkAutoTDelete<SkStream> streamDeleter(stream);
 
-    unsigned char buffer[WEBP_VP8_HEADER_SIZE];
-    SkASSERT(WEBP_VP8_HEADER_SIZE <= SkCodec::MinBufferedBytesNeeded());
-
-    const size_t bytesPeeked = stream->peek(buffer, WEBP_VP8_HEADER_SIZE);
-    if (bytesPeeked != WEBP_VP8_HEADER_SIZE) {
-        // Use read + rewind as a backup
-        if (stream->read(buffer, WEBP_VP8_HEADER_SIZE) != WEBP_VP8_HEADER_SIZE
-            || !stream->rewind())
+    // Webp demux needs a contiguous data buffer.
+    sk_sp<SkData> data = nullptr;
+    if (stream->getMemoryBase()) {

[scroggo, 2016/09/07 14:20:51]

I think we have some common code to handle this case that comes up a lot. Maybe
in SkStreamPriv or something like that.

Maybe you can't use that because you sometimes want to delete the stream? 

[msarett, 2016/09/07 16:47:10]

Ahh yes.  The common code always performs a copy, so I'll keep the
getMemoryBase() path.

+        data = SkData::MakeWithoutCopy(stream->getMemoryBase(), stream->getLength());

[scroggo, 2016/09/07 14:20:51]

Maybe add a comment here that this is safe because we hang onto the stream?

[msarett, 2016/09/07 16:47:10]

SGTM

+    } else if (stream->hasLength()) {
+        size_t size = stream->getLength();
+        data = SkData::MakeUninitialized(size);
+        stream->read(data->writable_data(), size);
+
+        // Go ahead and delete the stream, we don't need it anymore.
+        streamDeleter.reset(nullptr);
+    } else {
+        SkDynamicMemoryWStream writeStream;
+        static constexpr size_t kChunkSize = 8192; // Arbitrary
+        uint8_t buffer[kChunkSize];
+        size_t bytesRead;
+        do {
+            bytesRead = stream->read(buffer, kChunkSize);
+            writeStream.write(buffer, bytesRead);
+        } while (bytesRead == kChunkSize);
+        data.reset(writeStream.copyToData());
+
+        streamDeleter.reset(nullptr);
+    }
+
+    WebPData webpData = { data->bytes(), data->size() };
+    SkAutoTCallVProc<WebPDemuxer, WebPDemuxDelete> demux(WebPDemuxPartial(&webpData, nullptr));

[scroggo, 2016/09/07 14:20:51]

I think this is OK (in fact I think it's what chrome does), but it's a little
weird that the demux will outlive the webpData (though it needs those pointers
to remain valid!)

[msarett, 2016/09/07 16:47:10]

Agree that this is weird...  Don't think it's nicer to preserve our copy of the
WebPData though.

I've added a comment.

+    if (nullptr == demux) {
+        return nullptr;
+    }
+
+    WebPChunkIterator chunkIterator;
+    SkAutoTCallVProc<WebPChunkIterator, WebPDemuxReleaseChunkIterator> autoCI(&chunkIterator);
+    sk_sp<SkColorSpace> colorSpace = nullptr;
+    if (WebPDemuxGetChunk(demux, "ICCP", 1, &chunkIterator)) {
+        colorSpace = SkColorSpace::NewICC(chunkIterator.chunk.bytes, chunkIterator.chunk.size);
+    }
+
+    if (!colorSpace) {
+        colorSpace = SkColorSpace::NewNamed(SkColorSpace::kSRGB_Named);
+    }
+
+    WebPIterator frame;
+    SkAutoTCallVProc<WebPIterator, WebPDemuxReleaseIterator> autoFrame(&frame);
+    if (!WebPDemuxGetFrame(demux, 1, &frame)) {

[scroggo, 2016/09/07 14:20:51]

I forget, does this decode the frame? I guess not, since you haven't set the
output yet. This just gets info?

[msarett, 2016/09/07 16:47:10]

Yes, this won't decode the image - just sets us up with pointers to the first
frame.  And then we parse the header to get the features.  Though the WebpDemux
has already parsed the header of all the frame... it just doesn't expose all of
the information we want.

I think this redundancy is fine while our decoder is still in flux (need to add
incremental support, animation) - but we may want to request an API change at
some point (once we know what we really want).  I've put some comments in the
code.

         return nullptr;
     }
 
     WebPBitstreamFeatures features;
-    VP8StatusCode status = WebPGetFeatures(buffer, WEBP_VP8_HEADER_SIZE, &features);
+    VP8StatusCode status = WebPGetFeatures(frame.fragment.bytes, frame.fragment.size, &features);
     if (VP8_STATUS_OK != status) {
-        return nullptr; // Invalid WebP file.
+        return nullptr;
     }
 
-    // sanity check for image size that's about to be decoded.
+    // Sanity check for image size that's about to be decoded.
     {
         const int64_t size = sk_64_mul(features.width, features.height);
         if (!sk_64_isS32(size)) {
@@ -98,30 +135,9 @@ SkCodec* SkWebpCodec::NewFromStream(SkStream* stream) {
             return nullptr;
     }
 
-    // FIXME (msarett):
-    // Temporary strategy for getting ICC profiles from webps.  Once the incremental decoding
-    // API lands, we will use the WebPDemuxer to manage the entire decode.
-    sk_sp<SkColorSpace> colorSpace = nullptr;
-    const void* memory = stream->getMemoryBase();
-    if (memory) {
-        WebPData data = { (const uint8_t*) memory, stream->getLength() };
-        WebPDemuxState state;
-        SkAutoTCallVProc<WebPDemuxer, WebPDemuxDelete> demux(WebPDemuxPartial(&data, &state));
-
-        WebPChunkIterator chunkIterator;
-        SkAutoTCallVProc<WebPChunkIterator, WebPDemuxReleaseChunkIterator> autoCI(&chunkIterator);
-        if (demux && WebPDemuxGetChunk(demux, "ICCP", 1, &chunkIterator)) {
-            colorSpace = SkColorSpace::NewICC(chunkIterator.chunk.bytes, chunkIterator.chunk.size);
-        }
-    }
-
-    if (!colorSpace) {
-        colorSpace = SkColorSpace::NewNamed(SkColorSpace::kSRGB_Named);
-    }
-
     SkEncodedInfo info = SkEncodedInfo::Make(color, alpha, 8);
-    return new SkWebpCodec(features.width, features.height, info, colorSpace,
-                           streamDeleter.release());
+    return new SkWebpCodec(features.width, features.height, info, std::move(colorSpace),
+                           streamDeleter.release(), demux.release(), std::move(data));

[scroggo, 2016/09/07 14:20:51]

Sometimes streamDeleter will hold null. I forgot, is that OK? I.e it doesn't
cause problems for rewinding or anything?

[msarett, 2016/09/07 16:47:10]

I hadn't thought about this, but now that I look closer, it will be fine.

There's no need to do anything to rewind SkWebpCodec, and we are already doing
nothing.  I've modified a comment in SkCodec.cpp.

 }
 
 // This version is slightly different from SkCodecPriv's version of conversion_possible. It
@@ -158,7 +174,6 @@ bool SkWebpCodec::onDimensionsSupported(const SkISize& dim) {
             && dim.height() >= 1 && dim.height() <= info.height();
 }
 
-
 static WEBP_CSP_MODE webp_decode_mode(SkColorType ct, bool premultiply) {
     switch (ct) {
         case kBGRA_8888_SkColorType:
@@ -172,11 +187,6 @@ static WEBP_CSP_MODE webp_decode_mode(SkColorType ct, bool premultiply) {
     }
 }
 
-// The WebP decoding API allows us to incrementally pass chunks of bytes as we receive them to the
-// decoder with WebPIAppend. In order to do so, we need to read chunks from the SkStream. This size
-// is arbitrary.
-static const size_t BUFFER_SIZE = 4096;
-
 bool SkWebpCodec::onGetValidSubset(SkIRect* desiredSubset) const {
     if (!desiredSubset) {
         return false;
@@ -263,33 +273,31 @@ SkCodec::Result SkWebpCodec::onGetPixels(const SkImageInfo& dstInfo, void* dst,
     config.output.u.RGBA.size = dstInfo.getSafeSize(rowBytes);
     config.output.is_external_memory = 1;
 
+    WebPIterator frame;
+    SkAutoTCallVProc<WebPIterator, WebPDemuxReleaseIterator> autoFrame(&frame);
+    // If this succeeded in NewFromStream(), it should succeed again here.
+    SkAssertResult(WebPDemuxGetFrame(fDemux, 1, &frame));
+
     SkAutoTCallVProc<WebPIDecoder, WebPIDelete> idec(WebPIDecode(nullptr, 0, &config));
     if (!idec) {
         return kInvalidInput;
     }
 
-    SkAutoTMalloc<uint8_t> storage(BUFFER_SIZE);
-    uint8_t* buffer = storage.get();
-    while (true) {
-        const size_t bytesRead = stream()->read(buffer, BUFFER_SIZE);
-        if (0 == bytesRead) {
-            WebPIDecGetRGB(idec, rowsDecoded, NULL, NULL, NULL);
+    switch (WebPIUpdate(idec, frame.fragment.bytes, frame.fragment.size)) {

[msarett, 2016/09/06 23:47:11]

It's kind of ugly to create the WebPIDecoder with no data and then immediately
update it...

But I can't see that WebPIDecode() gives us back the status in any way.

[scroggo, 2016/09/07 14:20:51]

It's the same as we did before though, right?

[msarett, 2016/09/07 16:47:10]

Yes, true.  Before it seemed to make a little more sense because we were
appending data in a loop (rather than just once).

I think this is fine though.

+        case VP8_STATUS_OK:
+            return kSuccess;
+        case VP8_STATUS_SUSPENDED:
+            WebPIDecGetRGB(idec, rowsDecoded, nullptr, nullptr, nullptr);
             return kIncompleteInput;
-        }
-
-        switch (WebPIAppend(idec, buffer, bytesRead)) {
-            case VP8_STATUS_OK:
-                return kSuccess;
-            case VP8_STATUS_SUSPENDED:
-                // Break out of the switch statement. Continue the loop.
-                break;
-            default:
-                return kInvalidInput;
-        }
+        default:
+            return kInvalidInput;
     }
 }
 
 SkWebpCodec::SkWebpCodec(int width, int height, const SkEncodedInfo& info,
-                         sk_sp<SkColorSpace> colorSpace, SkStream* stream)
-    : INHERITED(width, height, info, stream, colorSpace)
+                         sk_sp<SkColorSpace> colorSpace, SkStream* stream, WebPDemuxer* demux,
+                         sk_sp<SkData> data)
+    : INHERITED(width, height, info, stream, std::move(colorSpace))
+    , fDemux(demux)
+    , fData(std::move(data))
 {}