Optimize the SkRawStream when the input is an asset stream

src/codec/SkRawCodec.cpp

Index: src/codec/SkRawCodec.cpp
diff --git a/src/codec/SkRawCodec.cpp b/src/codec/SkRawCodec.cpp
index da3c28c549ff617cc74df0effb9ff18177eca023..4e890fd953cd2ac92d16d66cde18873619bb79f6 100644
--- a/src/codec/SkRawCodec.cpp
+++ b/src/codec/SkRawCodec.cpp
@@ -157,23 +157,66 @@ public:
     }
 };
 
+bool IsStreamSeekable(const SkStream& stream) {

[scroggo, 2016/02/04 15:46:24]

In SkStream.h, we consider a stream "seekable" if hasPosition returns true. We
consider it an "asset" if hasLength also returns true. So I think it would be
better to name this something like

is_asset_stream(...)

(Also it is a static function so it should be named lowercase and camel_case.)

[yujieqin, 2016/02/05 08:53:21]

Done.

+    return stream.hasLength() && stream.hasPosition();
+}
+
 }  // namespace
 
-// Note: this class could throw exception if it is used as dng_stream.
-class SkRawStream : public ::piex::StreamInterface {
+class SkRawStream {
 public:
-    // Note that this call will take the ownership of stream.
-    explicit SkRawStream(SkStream* stream)
-        : fStream(stream), fWholeStreamRead(false) {}
+    virtual ~SkRawStream() {}
 
-    ~SkRawStream() override {}
+   /* 
+    * Gets the length of the stream. Depending on the type of stream, this may require reading to
+    * the end of the stream.
+    */
+   virtual uint64 getLength() = 0;
+
+   virtual bool read(void* data, size_t offset, size_t length) = 0;
 
     /*
      * Creates an SkMemoryStream from the offset with size.
      * Note: for performance reason, this function is destructive to the SkRawStream. One should
      *       abandon current object after the function call.
      */
-    SkMemoryStream* transferBuffer(size_t offset, size_t size) {
+   virtual SkMemoryStream* transferBuffer(size_t offset, size_t size) = 0;
+};
+
+class SkRawBufferedStream : public SkRawStream {
+public:
+    // Will take the ownership of the stream.
+    explicit SkRawBufferedStream(SkStream* stream)
+        : fStream(stream)
+        , fWholeStreamRead(false)
+    {
+        // Only use SkRawBufferedStream when the stream is not seekable.
+        SkASSERT(!IsStreamSeekable(*stream));
+    }
+
+    ~SkRawBufferedStream() override {}
+
+    uint64 getLength() override {
+        if (!this->bufferMoreData(kReadToEnd)) {  // read whole stream
+            ThrowReadFile();
+        }
+        return fStreamBuffer.bytesWritten();
+    }
+
+    bool read(void* data, size_t offset, size_t length) override {
+        if (length == 0) {
+            return true;
+        }
+
+        size_t sum;
+        if (!safe_add_to_size_t(offset, length, &sum)) {
+            return false;
+        }
+
+        return this->bufferMoreData(sum) && fStreamBuffer.read(data, offset, length);
+    }
+
+    SkMemoryStream* transferBuffer(size_t offset, size_t size) override {
         SkAutoTUnref<SkData> data(SkData::NewUninitialized(size));
         if (offset > fStreamBuffer.bytesWritten()) {
             // If the offset is not buffered, read from fStream directly and skip the buffering.
@@ -208,46 +251,6 @@ public:
         return new SkMemoryStream(data);
     }
 
-    // For PIEX
-    ::piex::Error GetData(const size_t offset, const size_t length,
-                          uint8* data) override {
-        if (offset == 0 && length == 0) {
-            return ::piex::Error::kOk;
-        }
-        size_t sum;
-        if (!safe_add_to_size_t(offset, length, &sum) || !this->bufferMoreData(sum)) {
-            return ::piex::Error::kFail;
-        }
-        if (!fStreamBuffer.read(data, offset, length)) {
-            return ::piex::Error::kFail;
-        }
-        return ::piex::Error::kOk;
-    }
-
-    // For dng_stream
-    uint64 getLength() {
-        if (!this->bufferMoreData(kReadToEnd)) {  // read whole stream
-            ThrowReadFile();
-        }
-        return fStreamBuffer.bytesWritten();
-    }
-
-    // For dng_stream
-    void read(void* data, uint32 count, uint64 offset) {
-        if (count == 0 && offset == 0) {
-            return;
-        }
-        size_t sum;
-        if (!safe_add_to_size_t(static_cast<uint64>(count), offset, &sum) ||
-            !this->bufferMoreData(sum)) {
-            ThrowReadFile();
-        }
-
-        if (!fStreamBuffer.read(data, static_cast<size_t>(offset), count)) {
-            ThrowReadFile();
-        }
-    }
-
 private:
     // Note: if the newSize == kReadToEnd (0), this function will read to the end of stream.
     bool bufferMoreData(size_t newSize) {
@@ -284,22 +287,114 @@ private:
     const size_t kReadToEnd = 0;
 };
 
+class SkRawSeekableStream : public SkRawStream {
+public:
+    // Will take the ownership of the stream.
+    explicit SkRawSeekableStream(SkStream* stream)
+        : fStream(stream)
+    {
+        // Only use SkRawSeekableStream when the stream is seekable.
+        SkASSERT(IsStreamSeekable(*stream));
+    }
+
+    ~SkRawSeekableStream() override {}
+
+    uint64 getLength() {
+        return fStream->getLength();
+    }
+
+
+    bool read(void* data, size_t offset, size_t length) override {
+        if (length == 0) {
+            return true;
+        }
+
+        size_t sum;
+        if (!safe_add_to_size_t(offset, length, &sum)) {
+            return false;
+        }
+
+        return fStream->seek(offset) && (fStream->read(data, length) == length);
+    }
+
+    SkMemoryStream* transferBuffer(size_t offset, size_t size) {
+        if (fStream->getLength() < offset) {
+            return nullptr;
+        }
+
+        size_t sum;
+        if (!safe_add_to_size_t(offset, size, &sum)) {
+            return nullptr;
+        }
+
+        // This will allow read less than the requested "size", because the JPEG codec wants to
+        // handle also a partial JPEG file.
+        const size_t bytesToRead = SkTMin(sum, fStream->getLength()) - offset;
+        if (bytesToRead == 0) {
+            return nullptr;
+        }
+
+        if (fStream->getMemoryBase()) {  // directly copy if getMemoryBase() is available.
+            SkAutoTUnref<SkData> data(SkData::NewWithCopy(
+                static_cast<const uint8_t*>(fStream->getMemoryBase()) + offset, bytesToRead));
+            fStream.free();
+            return new SkMemoryStream(data);
+        } else {
+            SkAutoTUnref<SkData> data(SkData::NewUninitialized(bytesToRead));
+            if (!fStream->seek(offset)) {
+                return nullptr;
+            }
+            if (bytesToRead != fStream->read(data->writable_data(), bytesToRead)) {

[scroggo, 2016/02/04 15:46:24]

This isn't the fix I intended. You changed the first case to support a partial
jpeg file, but you removed that from this case.

Instead, this should say something like:

const size_t bytesRead = fStream->read(...);
if (bytesRead < bytesToRead) {
    data.reset(SkData::NewSubset(data.get(), 0, bytesRead));
}

(We already do this in the SkRawBufferedStream.)

-----

Looking above, I realize that you already checked to make sure that bytesToRead
does not go beyond the length. So it would be a buggy stream that failed to read
all the way to the length. But I think it's still better to handle this case as
I described above.

[yujieqin, 2016/02/05 08:53:21]

Done.

+                return nullptr;
+            }
+            return new SkMemoryStream(data);
+        }
+    }
+private:
+    SkAutoTDelete<SkStream> fStream;
+};
+
+class SkPiexStream : public ::piex::StreamInterface {
+public:
+    // Will NOT take the ownership of the stream.
+    explicit SkPiexStream(SkRawStream* stream) : fStream(stream) {}
+
+    ~SkPiexStream() override {}
+
+    ::piex::Error GetData(const size_t offset, const size_t length,
+                          uint8* data) override {
+        return fStream->read(static_cast<void*>(data), offset, length) ?
+            ::piex::Error::kOk : ::piex::Error::kFail;
+    }
+
+private:
+    SkRawStream* fStream;
+};
+
 class SkDngStream : public dng_stream {
 public:
-    SkDngStream(SkRawStream* rawStream) : fRawStream(rawStream) {}
+    // Will NOT take the ownership of the stream.
+    SkDngStream(SkRawStream* stream) : fStream(stream) {}
+
+    ~SkDngStream() override {}
 
-    uint64 DoGetLength() override { return fRawStream->getLength(); }
+    uint64 DoGetLength() override { return fStream->getLength(); }
 
     void DoRead(void* data, uint32 count, uint64 offset) override {
-        fRawStream->read(data, count, offset);
+        size_t sum;
+        if (!safe_add_to_size_t(static_cast<uint64>(count), offset, &sum) ||
+            !fStream->read(data, static_cast<size_t>(offset), static_cast<size_t>(count))) {
+            ThrowReadFile();
+        }
     }
 
 private:
-    SkRawStream* fRawStream;
+    SkRawStream* fStream;
 };
 
 class SkDngImage {
 public:
+    // Will take the ownership of the stream.
     static SkDngImage* NewFromStream(SkRawStream* stream) {
         SkAutoTDelete<SkDngImage> dngImage(new SkDngImage(stream));
         if (!dngImage->readDng()) {
@@ -436,13 +531,21 @@ private:
  * fallback to create SkRawCodec for DNG images.
  */
 SkCodec* SkRawCodec::NewFromStream(SkStream* stream) {
-    SkAutoTDelete<SkRawStream> rawStream(new SkRawStream(stream));
+    SkAutoTDelete<SkRawStream> rawStream;
+    if (IsStreamSeekable(*stream)) {
+        rawStream.reset(new SkRawSeekableStream(stream));
+    } else {
+        rawStream.reset(new SkRawBufferedStream(stream));
+    }
+
+    // Does not take the ownership of rawStream.
+    SkAutoTDelete<SkPiexStream> piexStream(new SkPiexStream(rawStream.get()));

[scroggo, 2016/02/04 15:46:24]

No need to auto delete this. You can just put it on the stack:

SkPiexStream piexStream(rawStream.get());

[yujieqin, 2016/02/05 08:53:21]

Done.

     ::piex::PreviewImageData imageData;
     // FIXME: ::piex::GetPreviewImageData() calls GetData() frequently with small amounts,
     // resulting in many calls to bufferMoreData(). Could we make this more efficient by grouping
     // smaller requests together?
-    if (::piex::IsRaw(rawStream.get())) {
-        ::piex::Error error = ::piex::GetPreviewImageData(rawStream.get(), &imageData);
+    if (::piex::IsRaw(piexStream.get())) {
+        ::piex::Error error = ::piex::GetPreviewImageData(piexStream.get(), &imageData);
 
         if (error == ::piex::Error::kOk && imageData.preview_length > 0) {
 #if !defined(GOOGLE3)
@@ -450,7 +553,7 @@ SkCodec* SkRawCodec::NewFromStream(SkStream* stream) {
             // function call.
             // FIXME: one may avoid the copy of memoryStream and use the buffered rawStream.
             SkMemoryStream* memoryStream =
-                    rawStream->transferBuffer(imageData.preview_offset, imageData.preview_length);
+                rawStream->transferBuffer(imageData.preview_offset, imageData.preview_length);
             return memoryStream ? SkJpegCodec::NewFromStream(memoryStream) : nullptr;
 #else
             return nullptr;
@@ -459,7 +562,9 @@ SkCodec* SkRawCodec::NewFromStream(SkStream* stream) {
             return nullptr;
         }
     }
+    piexStream.free();
 
+    // Takes the ownership of the rawStream.
     SkAutoTDelete<SkDngImage> dngImage(SkDngImage::NewFromStream(rawStream.release()));
     if (!dngImage) {
         return nullptr;