Optimize the SkRawStream when the input is an asset stream

src/codec/SkRawCodec.cpp

 /*
  * Copyright 2016 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkCodec.h"
 #include "SkCodecPriv.h"
 #include "SkColorPriv.h"
@@ skipping 146 matching lines @@
     }
 };
 
 }  // namespace
 
 // Note: this class could throw exception if it is used as dng_stream.
 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 {}
 
     /*
      * 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) {
+        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.
             const size_t skipLength = offset - fStreamBuffer.bytesWritten();
             if (fStream->skip(skipLength) != skipLength) {
                 return nullptr;
             }
             const size_t bytesRead = fStream->read(data->writable_data(), size);
             if (bytesRead < size) {
                 data.reset(SkData::NewSubset(data.get(), 0, bytesRead));
             }
         } else {
             const size_t alreadyBuffered = SkTMin(fStreamBuffer.bytesWritten() - offset, size);
             if (alreadyBuffered > 0 &&
                 !fStreamBuffer.read(data->writable_data(), offset, alreadyBuffered)) {
                 return nullptr;
             }
 
             const size_t remaining = size - alreadyBuffered;
             if (remaining) {
                 auto* dst = static_cast<uint8_t*>(data->writable_data()) + alreadyBuffered;
                 const size_t bytesRead = fStream->read(dst, remaining);
                 size_t newSize;
                 if (bytesRead < remaining) {
                     if (!safe_add_to_size_t(alreadyBuffered, bytesRead, &newSize)) {

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

I realize that this is unrelated to this change, but is this check necessary?

It seems to me that newSize must be smaller than the input size.

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

PTAL

                         return nullptr;
                     }
                     data.reset(SkData::NewSubset(data.get(), 0, newSize));
                 }
             }
         }
         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;
+        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)) {
-            ThrowReadFile();
-        }
-
-        if (!fStreamBuffer.read(data, static_cast<size_t>(offset), count)) {
+            !read(static_cast<size_t>(offset), static_cast<size_t>(count), data)) {
             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);
+        }
+    }
+
     // 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;
             }
 
             // TODO: optimize for the special case when the input is SkMemoryStream.
             return SkStreamCopy(&fStreamBuffer, fStream.get());
         }
 
         if (newSize <= fStreamBuffer.bytesWritten()) {  // already buffered to newSize
             return true;
         }
         if (fWholeStreamRead) {  // newSize is larger than the whole stream.
             return false;
         }
 
         const size_t sizeToRead = newSize - fStreamBuffer.bytesWritten();
         SkAutoTMalloc<uint8> tempBuffer(sizeToRead);
         const size_t bytesRead = fStream->read(tempBuffer.get(), sizeToRead);
         if (bytesRead != sizeToRead) {
             return false;
         }
         return fStreamBuffer.write(tempBuffer.get(), bytesRead);
     }
 
     SkAutoTDelete<SkStream> fStream;
     bool fWholeStreamRead;
+    bool fIsMemoryStream;
 
     SkDynamicMemoryWStream fStreamBuffer;
 
     const size_t kReadToEnd = 0;
 };
 
 class SkDngStream : public dng_stream {
 public:
     SkDngStream(SkRawStream* rawStream) : fRawStream(rawStream) {}
 
@@ skipping 268 matching lines @@
     SkISize sizeFloor = this->onGetScaledDimensions(1.f / std::floor(fullShortEdge / shortEdge));
     SkISize sizeCeil = this->onGetScaledDimensions(1.f / std::ceil(fullShortEdge / shortEdge));
     return sizeFloor == dim || sizeCeil == dim;
 }
 
 SkRawCodec::~SkRawCodec() {}
 
 SkRawCodec::SkRawCodec(SkDngImage* dngImage)
     : INHERITED(dngImage->getImageInfo(), nullptr)
     , fDngImage(dngImage) {}