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 f400b19d89ed0b4eb6a6dba3afedc316d9bf8e5e..c5430197a2b75b56f4e6f1c8a1642d3a0e1fe19a 100644
--- a/src/codec/SkRawCodec.cpp
+++ b/src/codec/SkRawCodec.cpp
@@ -164,7 +164,9 @@ class SkRawStream : public ::piex::StreamInterface {
 public:
     // Note that this call will take the ownership of stream.
     explicit SkRawStream(SkStream* stream)
-        : fStream(stream), fWholeStreamRead(false) {}
+        : fStream(stream)
+        , fWholeStreamRead(false)
+        , fIsMemoryStream(fStream->getMemoryBase() != nullptr) {}

[adaubert, 2016/01/28 11:54:22]

Would it be better to use a dynamic_cast? E.g.

fIsMemoryStream(dynamic_cast<SkMemoryStream*>(fStream) != nullptr)

[yujieqin, 2016/01/28 12:25:14]

error: ‘dynamic_cast’ not permitted with -fno-rtti

 
     ~SkRawStream() override {}
 
@@ -174,6 +176,16 @@ public:
      *       abandon current object after the function call.
      */
     SkMemoryStream* transferBuffer(size_t offset, size_t size) {
+        if (fIsMemoryStream) {
+            size_t sum;
+            if (!safe_add_to_size_t(offset, size, &sum) ||
+                (sum > fStream->getLength()) ||

[msarett, 2016/01/28 17:25:40]

I don't think we necessarily want to fail if (sum > fStream->getLength()).

SkJpegCodec is written to handle incomplete images when possible.  I believe in
the non-optimized case below, we will still "succeed" even if we can't read the
entire size.

[yujieqin, 2016/02/01 12:59:47]

Done

+                !fStream->seek(offset)) {
+                return nullptr;
+            }
+            return static_cast<SkMemoryStream*>(fStream.release());

[adaubert, 2016/01/28 11:54:22]

dynamic_cast?

[yujieqin, 2016/01/28 12:25:14]

error: ‘dynamic_cast’ not permitted with -fno-rtti

[scroggo, 2016/01/28 18:53:44]

This isn't safe. Another implementation can implement getMemoryBase as well (in
fact, a few others do). We don't really have a good way to do what you want to
do though. I think what you really want to do is grab the underlying SkData
directly. We have a TODO to add a virtual method to do that, so I put a patch up
at crrev.com/1642943002 which adds it.

With SkStream::getData(), all of this can be much more efficient. I would
recommend (assuming my other CL lands) having a separate implementation for the
raw stream which uses the SkData directly. It never needs to do any buffering,
and can just return its length/copy. You could also make it wrap an
SkMemoryStream, if that's clearer.

Once you've done that, transferBuffer becomes even simpler. The implementation
here is unchanged, but for your other implementation, you just copy. 

[yujieqin, 2016/02/01 12:59:47]

I tried to change to use the peekData() based on your CL. PTAL. 

BTW, I did not make a additional SkRawStream for the seekable case for now, I
just use if(fIsSeekable) to control the workflow. But we can surely think about
what you suggested if you see there is more benefits.

+        }
+
         SkAutoTUnref<SkData> data(SkData::NewUninitialized(size));
         if (offset > fStreamBuffer.bytesWritten()) {
             // If the offset is not buffered, read from fStream directly and skip the buffering.
@@ -211,46 +223,58 @@ public:
     // 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;
+        return read(offset, length, static_cast<void*>(data)) ?
+            ::piex::Error::kOk : ::piex::Error::kFail;
     }
 
     // For dng_stream
     uint64 getLength() {
-        if (!this->bufferMoreData(kReadToEnd)) {  // read whole stream
-            ThrowReadFile();
+        if (fIsMemoryStream) {
+            return fStream->getLength();
+        } else {
+            if (!this->bufferMoreData(kReadToEnd)) {  // read whole stream
+                ThrowReadFile();
+            }
+            return fStreamBuffer.bytesWritten();
         }
-        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) ||

[msarett, 2016/01/28 17:25:40]

Is this check unnecessary?

It looks like we will check again in the other read() function.

[yujieqin, 2016/02/01 12:59:47]

Actually I think it is necessary. Because the special static_cast.

Here we check the safe_add_to_size_t<uint64>, later in the other read, we check
safe_add_to_size_t<size_t>. So we may miss some issue due to the
static_cast<size_t>(count).

[msarett, 2016/02/01 16:23:43]

Acknowledged.

-            !this->bufferMoreData(sum)) {
+            !read(static_cast<size_t>(offset), static_cast<size_t>(count), data)) {
             ThrowReadFile();
         }
+    }
 
-        if (!fStreamBuffer.read(data, static_cast<size_t>(offset), count)) {
-            ThrowReadFile();
+private:
+    bool read(size_t offset, size_t length, void* data) {

[msarett, 2016/01/28 17:25:40]

I find it confusing to have two functions both named "read".

[yujieqin, 2016/02/01 12:59:47]

changed the other read to dngRead().

+        if (offset == 0 && length == 0) {
+            return true;
+        }
+
+        size_t sum;
+        if (!safe_add_to_size_t(offset, length, &sum)) {
+            return false;
+        }
+
+        if (fIsMemoryStream) {
+            if (sum > fStream->getLength()) {
+              return false;
+            }
+
+            return fStream->seek(offset) &&
+                (fStream->read(data, length) == length);
+        } else {
+            return this->bufferMoreData(sum) && fStreamBuffer.read(data, offset, length);
         }
     }
 
-private:
     // Note: if the newSize == kReadToEnd (0), this function will read to the end of stream.
     bool bufferMoreData(size_t newSize) {
+        SkASSERT(!fIsMemoryStream);
+
         if (newSize == kReadToEnd) {
             if (fWholeStreamRead) {  // already read-to-end.
                 return true;
@@ -278,6 +302,7 @@ private:
 
     SkAutoTDelete<SkStream> fStream;
     bool fWholeStreamRead;
+    bool fIsMemoryStream;
 
     SkDynamicMemoryWStream fStreamBuffer;