Add SkStream::readIntoSkData()

Add SkStream::readIntoSkData()

[epoger, 2013-02-08 19:29:27]

https://codereview.appspot.com/7300071/diff/1/include/core/SkStream.h
File include/core/SkStream.h (right):

https://codereview.appspot.com/7300071/diff/1/include/core/SkStream.h#newcode99
include/core/SkStream.h:99: *  Create a new SkData from the stream contents.
This balances the call
Is there some new documentation I should put here?

I think it's unfortunate that this method reads the length out of the stream--it
would be more generally useful if it didn't--but I don't think we have much
appetite for breaking any callers by changing it now...

https://codereview.appspot.com/7300071/diff/1/src/core/SkStream.cpp
File src/core/SkStream.cpp (right):

https://codereview.appspot.com/7300071/diff/1/src/core/SkStream.cpp#newcode110
src/core/SkStream.cpp:110: return SkData::NewFromMalloc(bufStart,
bytesReadSoFar);
So the SkData object will own a buffer that is maxBytes long, even though only
bytesReadSoFar bytes of it will be in use.  I think that's OK, right?

https://codereview.appspot.com/7300071/diff/1/src/core/SkStream.cpp#newcode115
src/core/SkStream.cpp:115: return this->readIntoSkData(size);
I haven't manually tested to see whether readData() still works, but all the
unit tests pass, and the code seems straightforward enough...

[caryclark1, 2013-02-08 19:45:28]

https://codereview.appspot.com/7300071/diff/1/src/core/SkStream.cpp
File src/core/SkStream.cpp (right):

https://codereview.appspot.com/7300071/diff/1/src/core/SkStream.cpp#newcode109
src/core/SkStream.cpp:109: }
a-b  // b+= c raises a red flag for me. What about:

size_t bytesRemaining = maxBytes;
while (bytesRemaining <= maxBytes) {
   size_t bytesReadThisTime = this->read(bufPtr, bytesRemaining);
   ...
   bytesRemaining -=  bytesReadThisTime;
}

or better:

do {
...
} while (bytesRemaining < maxBytes);

[epoger, 2013-02-08 19:55:45]

https://codereview.appspot.com/7300071/diff/1/src/core/SkStream.cpp
File src/core/SkStream.cpp (right):

https://codereview.appspot.com/7300071/diff/1/src/core/SkStream.cpp#newcode109
src/core/SkStream.cpp:109: }
I'd be curious to know what the big red flag is about exactly (performance?
opportunity for errors to sneak in?).  At any rate, does this look better?

(My main concern is still whether we want to add this to common code at all; it
just seems a shame for everyone to reinvent this wheel.)

On 2013/02/08 19:45:28, caryclark1 wrote:
> a-b  // b+= c raises a red flag for me. What about:
> 
> size_t bytesRemaining = maxBytes;
> while (bytesRemaining <= maxBytes) {
>    size_t bytesReadThisTime = this->read(bufPtr, bytesRemaining);
>    ...
>    bytesRemaining -=  bytesReadThisTime;
> }
> 
> or better:
> 
> do {
> ...
> } while (bytesRemaining < maxBytes);
>

[reed1, 2013-02-08 19:57:19]

Not sure about the read-until logic here. Maybe its correct, but none of the
other methods or callers use SkStream in this fashion (afaik).

We have getLength() already, which returns the total size of the stream. Do we
need this max-bytes parameter at all? What is the current use-case that is
prompting us to add a new public API?

[epoger, 2013-02-08 20:08:12]

On 2013/02/08 19:57:19, reed1 wrote:
> Not sure about the read-until logic here. Maybe its correct, but none of the
> other methods or callers use SkStream in this fashion (afaik).
> 
> We have getLength() already, which returns the total size of the stream. Do we
> need this max-bytes parameter at all? What is the current use-case that is
> prompting us to add a new public API?

The use case is my desire to read an entire file into a single contiguous
buffer.  We've had an email thread going about that, and the tenor seems to be
"everyone who needs to do that writes something like this".  So I figure it
would be good to put that routine somewhere for common use.

I don't so much care where that common place would be... I'm open to
suggestions.

In terms of implementation, the root of the problem is: it's not clear (to me)
whether SkStream::read() guarantees that it will read as many bytes as possible
before returning.  The documentation doesn't say, and I could certainly imagine
cases where an implementation would return partial results for its own
convenience.

So, it's not clear to me that the existing SkStream::readData() would always
read the entire blob the caller wanted (like if the 32-bit length field said 10
megabytes, and read() only returned a megabyte at a time).

[epoger, 2013-02-08 20:09:43]

> In terms of implementation, the root of the problem is: it's not clear (to me)
> whether SkStream::read() guarantees that it will read as many bytes as
possible
> before returning.

If we are willing to state that all implementations of SkStream::read() already
read as many bytes as possible from the stream, then the implementation of
ReadEntireFileIntoSkData would be much simpler, and perhaps we could put it in
utils somewhere?

[epoger, 2013-02-08 20:12:58]

https://codereview.appspot.com/7300071/diff/2002/src/core/SkStream.cpp
File src/core/SkStream.cpp (left):

https://codereview.appspot.com/7300071/diff/2002/src/core/SkStream.cpp#oldcod...
src/core/SkStream.cpp:103: return SkData::NewFromMalloc(buffer, size);
P.S. Mike: This looks like an uninitialized memory error waiting to happen.  If
size=10 but only 8 bytes were read (for whatever reason), the last 2 bytes in
the underlying buffer will be uninitialized.

(Except for whatever initialization sk_malloc_throw() may provide)

[caryclark1, 2013-02-08 20:39:29]

https://codereview.appspot.com/7300071/diff/1/src/core/SkStream.cpp
File src/core/SkStream.cpp (right):

https://codereview.appspot.com/7300071/diff/1/src/core/SkStream.cpp#newcode109
src/core/SkStream.cpp:109: }
On 2013/02/08 19:55:45, epoger wrote:
> I'd be curious to know what the big red flag is about exactly (performance?
> opportunity for errors to sneak in?).  
Clarity.
>At any rate, does this look better?
I guess it's all a moot point until the question is answered about if/when the
loop is actually required.
> 
> (My main concern is still whether we want to add this to common code at all;
it
> just seems a shame for everyone to reinvent this wheel.)
> 
> On 2013/02/08 19:45:28, caryclark1 wrote:
> > a-b  // b+= c raises a red flag for me. What about:
> > 
> > size_t bytesRemaining = maxBytes;
> > while (bytesRemaining <= maxBytes) {
> >    size_t bytesReadThisTime = this->read(bufPtr, bytesRemaining);
> >    ...
> >    bytesRemaining -=  bytesReadThisTime;
> > }
> > 
> > or better:
> > 
> > do {
> > ...
> > } while (bytesRemaining < maxBytes);
> > 
>

[reed1, 2013-03-15 20:28:49]

I don't think any of our streams support this async model (correct me if I'm
wrong).

stream->read(buffer, len)

will *always* block until either the request is met, or it fails (returning some
value less than len). In all of the other call-sites, we don't loop, and will
just report an error if the first call to read() returned fewer than expected
bytes.