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 141 matching lines @@
         // and supposed to be sufficient for all valid DNG images.
         if (size > 300 * 1024 * 1024) {  // 300 MB
             ThrowMemoryFull();
         }
         return dng_memory_allocator::Allocate(size);
     }
 };
 
 }  // 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. 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;
+
+   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.
             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));
@@ skipping 14 matching lines @@
                     if (!safe_add_to_size_t(alreadyBuffered, bytesRead, &newSize)) {
                         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;
-    }
-
-    // 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) {
         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());
@@ skipping 16 matching lines @@
     }
 
     SkAutoTDelete<SkStream> fStream;
     bool fWholeStreamRead;
 
     SkDynamicMemoryWStream fStreamBuffer;
 
     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) {}
 
-    uint64 DoGetLength() override { return fRawStream->getLength(); }
+    ~SkDngStream() override {}
+
+    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 {
 public:
     static SkDngImage* NewFromStream(SkRawStream* stream) {
         SkAutoTDelete<SkDngImage> dngImage(new SkDngImage(stream));
         if (!dngImage->readDng()) {
             return nullptr;
         }
 
@@ skipping 120 matching lines @@
     bool fIsScalable;
     bool fIsXtransImage;
 };
 
 /*
  * Tries to handle the image with PIEX. If PIEX returns kOk and finds the preview image, create a
  * SkJpegCodec. If PIEX returns kFail, then the file is invalid, return nullptr. In other cases,
  * 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)
             // transferBuffer() is destructive to the rawStream. Abandon the rawStream after this
             // 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;
 #endif
         } else if (error == ::piex::Error::kFail) {
             return nullptr;
         }
     }
 
-    SkAutoTDelete<SkDngImage> dngImage(SkDngImage::NewFromStream(rawStream.release()));
+    SkAutoTDelete<SkDngImage> dngImage(SkDngImage::NewFromStream(piexStream->releaseRawStream()));
     if (!dngImage) {
         return nullptr;
     }
 
     return new SkRawCodec(dngImage.release());
 }
 
 SkCodec::Result SkRawCodec::onGetPixels(const SkImageInfo& requestedInfo, void* dst,
                                         size_t dstRowBytes, const Options& options,
                                         SkPMColor ctable[], int* ctableCount,
@@ skipping 85 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) {}