Avoid too small reads to bufferMoreData()

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 249 matching lines @@
             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);
+        // Try to read at least 256 bytes to avoid to many small reads.

[adaubert, 2016/01/28 14:20:25]

FYI: In Snapseed we usually read 8192, same applies for the default value of the
BufferedInputStream.java.

[yujieqin, 2016/01/28 14:27:44]

I don't have preference here. msarett@ & scroggo@, which size do you prefer for
Skia?

[msarett, 2016/01/28 16:23:44]

I guess how much we need really depends on the image...  The case I mentioned in
https://codereview.chromium.org/1520403003 calls bufferMoreData over 200 times
to increase from 108kb to 112kb.  8192 covers this (plus some extra) and 256
seems a little small here.

I would lean toward the larger size.  I don't think an extra 8 kb will make much
difference considering it is much smaller than the stream size.  But I would
defer your judgment on what is the best balance for RAW.

[scroggo, 2016/01/28 16:49:58]

As Matt suggested, the ideal number will depend on the data. Let's assume that
the common case is a valid image, meaning we'll eventually read the whole
stream. So the larger size we use here, the fewer times we make these small
copies. On the other hand, if we read the whole stream early on (when does that
typically happen?), this doesn't really matter since we will already have the
whole thing buffered.

The only danger here would be if ended up buffering too much during an attempt
to get the size, resulting in overflowing the Java buffer, so the caller can no
longer rewind to do a full decode. Note that it's only a problem if reading the
exact amount requested did *not* overflow the Java buffer.

In general, though, this only matters if it helps performance, so it'd be nice
to know what difference it makes.

[msarett, 2016/01/28 17:00:35]

In the case that Piex can find a preview, I think we may not need to read the
entire stream?
+1 for knowing the performance impact (if any).  Though I would support this
change even if we can't see a performance impact.

If you want to use Skia's CodecBench, you can run:
ninja -C out/Release
out/Release/nanobench --images <path to image or images> --match <some part of
the filename(s)>

Or even better, performance tests on Android:
BUILDTYPE=Release ./platform_tools/android/bin/android_ninja [-d device_id]
nanobench
./platform_tools/android/bin/android_run_skia --release nanobench  -images <>
--match <>

+        const size_t minSizeToRead = 256;

[scroggo, 2016/01/28 16:49:57]

nit: This is effectively a constant, which we would typically write as

kMinSizeToRead

[yujieqin, 2016/02/01 16:25:19]

Done.

+        const size_t sizeMustRead = newSize - fStreamBuffer.bytesWritten();

[scroggo, 2016/01/28 16:49:58]

nit: These names don't distinguish themselves very well to me. Maybe this should
be named "sizeRequested"?

[yujieqin, 2016/02/01 16:25:19]

Done.

+        const size_t sizeToRead = SkTMax(minSizeToRead, sizeMustRead);
+        SkAutoSTMalloc<minSizeToRead, uint8> tempBuffer(sizeToRead);
         const size_t bytesRead = fStream->read(tempBuffer.get(), sizeToRead);
-        if (bytesRead != sizeToRead) {
+        if (bytesRead < sizeMustRead) {
             return false;
         }
         return fStreamBuffer.write(tempBuffer.get(), bytesRead);
     }
 
     SkAutoTDelete<SkStream> fStream;
     bool fWholeStreamRead;
 
     SkDynamicMemoryWStream fStreamBuffer;
 
@@ skipping 147 matching lines @@
 };
 
 /*
  * 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));
     ::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 (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);
@@ skipping 105 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) {}