Avoid too small reads to bufferMoreData()

src/codec/SkRawCodec.cpp

Index: src/codec/SkRawCodec.cpp
diff --git a/src/codec/SkRawCodec.cpp b/src/codec/SkRawCodec.cpp
index f400b19d89ed0b4eb6a6dba3afedc316d9bf8e5e..faba5d4f00dc9566841d4e2fdbd127e9cf105f3f 100644
--- a/src/codec/SkRawCodec.cpp
+++ b/src/codec/SkRawCodec.cpp
@@ -267,10 +267,13 @@ private:
             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);
@@ -438,9 +441,6 @@ private:
 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);