Make SkCodec support peek() and read()

src/codec/SkCodec.cpp

Index: src/codec/SkCodec.cpp
diff --git a/src/codec/SkCodec.cpp b/src/codec/SkCodec.cpp
index 11eb1f98479461b7307695ac49d44cde6f85cecb..e483c484eef6e7da5aba9f74dba114118cabfafe 100644
--- a/src/codec/SkCodec.cpp
+++ b/src/codec/SkCodec.cpp
@@ -20,7 +20,7 @@
 #include "SkWebpCodec.h"
 
 struct DecoderProc {
-    bool (*IsFormat)(SkStream*);
+    bool (*IsFormat)(const char*, size_t);
     SkCodec* (*NewFromStream)(SkStream*);
 };
 
@@ -42,23 +42,31 @@ SkCodec* SkCodec::NewFromStream(SkStream* stream,
     }
 
     SkAutoTDelete<SkStream> streamDeleter(stream);
-    
+
+    // 14 is enough to read all of the supported types.
+    size_t bytesRead = 14;
+    char buffer[bytesRead];
+    if (!stream->peek(buffer, bytesRead)) {
+        SkCodecPrintf("Could not peek!\n");
+        // It is possible the stream does not support peeking, but does support
+        // rewinding.
+        // It is also possible to have a complete image less than 14 bytes
+        // (e.g. a 1 x 1 wbmp), meaning peek() would fail, and read() may read
+        // less than 14 bytes.

[scroggo, 2015/11/30 20:02:30]

Some thoughts on the overall goal here:

We need to be able to somehow read the first few bytes, but then pass the entire
stream to the correct decoder. Originally we did this by calling rewind() on the
SkStream. This meant calling reset() on the Java InputStream, which may fail.
Worse, we have to call mark() on it first in order to have a place to "reset"
to. mark() takes an int parameter which basically says "buffer up to this size".
At one point, we called mark() on the passed in stream, with an arbitrary size
(one we thought was big enough). This could be a problem if someone else called
mark - we've overridden their mark, potentially with a smaller buffer.

Since we really only need to buffer the first X bytes, we solved this problem
with the SkFrontBufferedStream. It buffers the first X bytes (passed in by the
caller), allowing rewinding from within that range, but not after that (which is
never necessary).

We currently set X to 64, because the way SkWEBPImageDecoder was written, it
required reading 64 bytes to determine whether it was a webp (only 14 are
needed, as noted here), *and* because it has to read enough to know the size of
the image to allocate before starting the decode. (FWIW, SkWEBPImageDecoder
reads 64 bytes to determine this, but we've since learned that only 30 bytes are
needed.)

Regardless of the value of X, this seems backwards; BitmapFactory.cpp needs to
know how much SkImageDecoder needs to buffer in order to create its buffer.

The idea behind peek() is that BitmapFactory need not wrap its stream in an
SkFrontBufferedStream (using a buffer that it knows happens to be large enough).
Instead, SkCodec peeks at the bytes it knows it needs (to be fair, in this CL
I've sort of pushed the intimate knowledge just one level deeper; SkCodec
happens to know how many bytes each of its subclasses needs - I took this
approach because it seemed simpler, and I think it makes sense for SkCodec to
know about its subclasses).

peek() is easy on an SkMemoryStream, but for Android to take advantage of it it
also needs to be supported on JavaInputStreamAdaptor. This class already buffers
so that it can convert from Java bytes to a C buffer. My thought that we could
repurpose this buffer for peeking. This means we are limited to peeking the size
of the Java byte buffer. The byte buffer is provided by the caller, so in theory
it could be too small. In practice, the API (see
http://developer.android.com/reference/android/graphics/BitmapFactory.Options...)
recommends 16 KB (which is also the default), so this shouldn't be an issue.

The trickier part comes from very small images. You proposed a small wbmp, which
I agree is a possibility, and I could imagine there might be others. In these
cases, using read + rewind does not seem like a good fallback, since it means we
have to buffer anyway (defeating the purpose of peek()).

I could fix this by peeking inside the subclass - i.e. make IsWbmp (etc) take a
stream (like it/they do without this CL) and call peek() (this also eliminates
SkCodec's intimate knowledge). This eliminates the problem where peek failed
because the stream was smaller (since e.g. SkWbmpCodec will only peek as much as
it needs, although it will be messy, since it will need to peek a larger amount
each time to include what was already peeked plus new data). 

A downside is that if we want to continue to support reading + rewinding as a
fallback (which should no longer be a problem, since we've addressed the problem
of not having peeked enough), IsWbmp (etc) needs to report two things:
- did the stream match my format
- did it call read() (caller must rewind()) OR rewind() failed (caller must
abort)

Not terrible, but a little bit messier... (this was in fact how I started
writing this CL, but I switched to the current version which seemed cleaner).

I'll start working on that version. Let me know if you have any thoughts about
this.

[scroggo, 2015/12/01 22:33:49]

You've convinced me to do what I should've done all along - make peek() return a
size_t. That simplifies this solution. PTAL.

+        // Attempt to read() and pass the actual amount read to the decoder.
+        bytesRead = stream->read(buffer, bytesRead);
+        if (!stream->rewind()) {
+            return nullptr;
+        }
+    }
     SkAutoTDelete<SkCodec> codec(nullptr);
     // PNG is special, since we want to be able to supply an SkPngChunkReader.
     // But this code follows the same pattern as the loop.
-    const bool isPng = SkPngCodec::IsPng(stream);
-    if (!stream->rewind()) {
-        return NULL;
-    }
-    if (isPng) {
+    if (SkPngCodec::IsPng((const char*)buffer, bytesRead)) {
         codec.reset(SkPngCodec::NewFromStream(streamDeleter.detach(), chunkReader));
     } else {
         for (DecoderProc proc : gDecoderProcs) {
-            const bool correctFormat = proc.IsFormat(stream);
-            if (!stream->rewind()) {
-                return nullptr;
-            }
-            if (correctFormat) {
+            if (proc.IsFormat(buffer, bytesRead)) {
                 codec.reset(proc.NewFromStream(streamDeleter.detach()));
                 break;
             }