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..6f6fa050ac01cb5b564fc0b3d080940a9e15b29d 100644
--- a/src/codec/SkRawCodec.cpp
+++ b/src/codec/SkRawCodec.cpp
@@ -159,21 +159,64 @@ public:
 
 }  // 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() {}
+
+   /* 
+    * Gets the length of the stream. Denpending on the type of stream, this may require read to the

[scroggo, 2016/02/02 19:23:06]

depending*

reading*

[yujieqin, 2016/02/04 14:50:32]

Done.

+    * end of stream.

[scroggo, 2016/02/02 19:23:06]

the* stream

[yujieqin, 2016/02/04 14:50:31]

Done.

+    */
+   virtual uint64 getLength() = 0;
 
-    ~SkRawStream() override {}
+   virtual bool read(size_t offset, size_t length, void* data) = 0;

[scroggo, 2016/02/02 19:23:06]

nit: We typically put our output parameters first. I think we also typically
name this variable "buffer". So this should be:

virtual bool read(void* buffer, size_t offset, size_t length) = 0;

[yujieqin, 2016/02/04 14:50:31]

Done.

 
     /*
      * 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:
+    static SkRawStream* NewFromStream(SkStream* stream) {
+        return new SkRawBufferedStream(stream);
+    }
+
+    // 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(!fStream->hasPosition());
+    }
+
+    ~SkRawBufferedStream() override {}
+
+    uint64 getLength() override {
+        if (!this->bufferMoreData(kReadToEnd)) {  // read whole stream
+            ThrowReadFile();
+        }
+        return fStreamBuffer.bytesWritten();
+    }
+
+    bool read(size_t offset, size_t length, void* data) {
+        if (offset == 0 && length == 0) {

[scroggo, 2016/02/02 19:23:06]

Why is there a special case here? I can see why a length of 0 is an automatic
success, but why does it also check offset?

[yujieqin, 2016/02/04 14:50:31]

I think you are right. This should be a bug. :)

+            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,18 +287,110 @@ private:
     const size_t kReadToEnd = 0;
 };
 
+class SkRawSeekableStream : public SkRawStream {
+public:
+    static SkRawStream* NewFromStream(SkStream* stream) {
+        return new SkRawSeekableStream(stream);
+    }
+
+    // Will take the ownership of the stream.
+    explicit SkRawSeekableStream(SkStream* stream)
+        : fStream(stream)
+    {
+        // Only use SkRawSeekableStream when the stream is seekable.
+        SkASSERT(fStream->hasPosition());
+    }
+
+    ~SkRawSeekableStream() override {}
+
+    uint64 getLength() {
+        return fStream->getLength();

[scroggo, 2016/02/02 19:23:06]

Maybe we should also check hasLength() to determine to use the
SkRawSeekableStream, since there could be a stream which has a position but not
a length.

[yujieqin, 2016/02/04 14:50:31]

Done.

+    }
+
+
+    bool read(size_t offset, size_t length, void* data) {
+        if (offset == 0 && length == 0) {
+            return true;
+        }
+
+        size_t sum;
+        if (!safe_add_to_size_t(offset, length, &sum)) {
+            return false;
+        }
+
+        if (sum > fStream->getLength()) {

[scroggo, 2016/02/02 19:23:06]

I don't think this check is necessary - the below check should return false if
sum > length

[yujieqin, 2016/02/04 14:50:31]

Done.

+          return false;

[scroggo, 2016/02/02 19:23:06]

four space indent.

[yujieqin, 2016/02/04 14:50:31]

Done.

+        }
+        return fStream->seek(offset) && (fStream->read(data, length) == length);
+    }
+
+    SkMemoryStream* transferBuffer(size_t offset, size_t size) {
+        size_t sum;
+        if (!safe_add_to_size_t(offset, size, &sum)) {
+            return nullptr;
+        }
+
+        if (fStream->getMemoryBase()) {
+            SkAutoTUnref<SkData> data(SkData::NewWithCopy(
+                static_cast<const uint8_t*>(fStream->getMemoryBase()) + offset, size));

[scroggo, 2016/02/02 19:23:06]

This case should also support a partial jpeg file.

[yujieqin, 2016/02/04 14:50:31]

Done.

+            fStream.free();
+            return new SkMemoryStream(data);
+        } else {
+            SkAutoTUnref<SkData> data(SkData::NewUninitialized(size));
+            if (!fStream->seek(offset)) {
+                return nullptr;
+            }
+            // Don't check the returned size, because the following JPEG codec may be able to

[scroggo, 2016/02/02 19:23:06]

We do need to know the returned size, though, because otherwise the JPEG codec
will try to read uninitialized memory.

[yujieqin, 2016/02/04 14:50:31]

Done.

+            // process an incomplete file.
+            fStream->read(data->writable_data(), size);
+            return new SkMemoryStream(data);
+        }
+    }
+private:
+    SkAutoTDelete<SkStream> fStream;
+};
+
+class SkPiexStream : public ::piex::StreamInterface {
+public:
+    // Note that this call will take the ownership of 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(offset, length, static_cast<void*>(data)) ?
+            ::piex::Error::kOk : ::piex::Error::kFail;
+    }
+
+    // Release the fStream. One should abandon current object after this call.
+    SkRawStream* releaseRawStream() {
+        return fStream.release();
+    }
+
+private:
+    SkAutoTDelete<SkRawStream> fStream;
+};
+
 class SkDngStream : public dng_stream {
 public:
-    SkDngStream(SkRawStream* rawStream) : fRawStream(rawStream) {}
+    // Will NOT take the ownership of the stream, because the stream needs to be able to reuse.
+    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) ||

[scroggo, 2016/02/02 19:23:06]

Won't the implementation of fStream->read also make this check?

[yujieqin, 2016/02/04 14:50:31]

as explained for last patch set, this check is for type uint64-->size_t, the
other check of fStream->read is for type size_t-->size_t.

+            !fStream->read(static_cast<size_t>(offset), static_cast<size_t>(count), data)) {
+            ThrowReadFile();
+        }
     }
 
 private:
-    SkRawStream* fRawStream;
+    SkRawStream* fStream;
 };
 
 class SkDngImage {
@@ -436,13 +531,15 @@ private:
  * fallback to create SkRawCodec for DNG images.
  */
 SkCodec* SkRawCodec::NewFromStream(SkStream* stream) {
-    SkAutoTDelete<SkRawStream> rawStream(new SkRawStream(stream));
+    SkAutoTDelete<SkPiexStream> piexStream(new SkPiexStream(
+        stream->hasPosition() ?
+        SkRawSeekableStream::NewFromStream(stream) : SkRawBufferedStream::NewFromStream(stream)));

[scroggo, 2016/02/02 19:23:06]

I don't think these factory functions are necessary. The constructor is public,
and it always succeeds.

We tend to like factory functions when we need to do some work to make sure the
object is valid before returning it. Like with the SkDngImage, readNegative may
fail, in which case we don't want to return an invalid object.

[yujieqin, 2016/02/04 14:50:31]

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 +547,8 @@ 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);
+                piexStream->releaseRawStream()->transferBuffer(imageData.preview_offset,

[scroggo, 2016/02/02 19:23:06]

This is going to leak the raw stream. I think it makes more sense to just call a
method on piexStream, and let it take care of deleting the raw stream (which
will happen soon).

[yujieqin, 2016/02/04 14:50:31]

Done.

+                                                               imageData.preview_length);
             return memoryStream ? SkJpegCodec::NewFromStream(memoryStream) : nullptr;
 #else
             return nullptr;
@@ -460,7 +558,7 @@ SkCodec* SkRawCodec::NewFromStream(SkStream* stream) {
         }
     }
 
-    SkAutoTDelete<SkDngImage> dngImage(SkDngImage::NewFromStream(rawStream.release()));
+    SkAutoTDelete<SkDngImage> dngImage(SkDngImage::NewFromStream(piexStream->releaseRawStream()));
     if (!dngImage) {
         return nullptr;
     }