Use demux API in SkWebpCodec

src/codec/SkWebpCodec.cpp

 /*
  * Copyright 2015 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkCodecPriv.h"
 #include "SkWebpCodec.h"
 #include "SkTemplates.h"
@@ skipping 18 matching lines @@
     return bytesRead >= 14 && !memcmp(bytes, "RIFF", 4) && !memcmp(&bytes[8], "WEBPVP", 6);
 }
 
 // Parse headers of RIFF container, and check for valid Webp (VP8) content.
 // NOTE: This calls peek instead of read, since onGetPixels will need these
 // bytes again.
 // Returns an SkWebpCodec on success;
 SkCodec* SkWebpCodec::NewFromStream(SkStream* stream) {
     SkAutoTDelete<SkStream> streamDeleter(stream);
 
-    unsigned char buffer[WEBP_VP8_HEADER_SIZE];
-    SkASSERT(WEBP_VP8_HEADER_SIZE <= SkCodec::MinBufferedBytesNeeded());
+    // 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);
 
-    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())
+        // 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)) {
             return nullptr;
         }
         // now check that if we are 4-bytes per pixel, we also don't overflow
         if (sk_64_asS32(size) > (0x7FFFFFFF >> 2)) {
             return nullptr;
         }
     }
@@ skipping 24 matching lines @@
             break;
         case 2:
             // This is the lossless format (BGRA).
             color = SkEncodedInfo::kBGRA_Color;
             alpha = SkEncodedInfo::kUnpremul_Alpha;
             break;
         default:
             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
 // supports both byte orders for 8888.
 static bool webp_conversion_possible(const SkImageInfo& dst, const SkImageInfo& src) {
     if (!valid_alpha(dst.alphaType(), src.alphaType())) {
         return false;
     }
 
     switch (dst.colorType()) {
@@ skipping 16 matching lines @@
     dim.fHeight = SkTMax(1, SkScalarRoundToInt(desiredScale * dim.fHeight));
     return dim;
 }
 
 bool SkWebpCodec::onDimensionsSupported(const SkISize& dim) {
     const SkImageInfo& info = this->getInfo();
     return dim.width() >= 1 && dim.width() <= info.width()
             && dim.height() >= 1 && dim.height() <= info.height();
 }
 
-
 static WEBP_CSP_MODE webp_decode_mode(SkColorType ct, bool premultiply) {
     switch (ct) {
         case kBGRA_8888_SkColorType:
             return premultiply ? MODE_bgrA : MODE_BGRA;
         case kRGBA_8888_SkColorType:
             return premultiply ? MODE_rgbA : MODE_RGBA;
         case kRGB_565_SkColorType:
             return MODE_RGB_565;
         default:
             return MODE_LAST;
     }
 }
 
-// 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;
     }
 
     SkIRect dimensions  = SkIRect::MakeSize(this->getInfo().dimensions());
     if (!dimensions.contains(*desiredSubset)) {
         return false;
     }
 
@@ skipping 66 matching lines @@
         config.options.scaled_height = dstDimensions.height();
     }
 
     config.output.colorspace = webp_decode_mode(dstInfo.colorType(),
             dstInfo.alphaType() == kPremul_SkAlphaType);
     config.output.u.RGBA.rgba = (uint8_t*) dst;
     config.output.u.RGBA.stride = (int) rowBytes;
     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))
 {}