Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23717055:
Add a buffered SkStream class. (Closed)
|
Created:
7 years, 3 months ago by scroggo Modified:
7 years, 2 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionAdd a buffered SkStream class.
This is used by Android to buffer an input stream which may not
otherwise be able to rewind.
Add a test for the new class.
R=bungeman@google.com, mtklein@google.com, reed@google.com
Committed: https://code.google.com/p/skia/source/detail?r=11488
Patch Set 1 #
Total comments: 16
Patch Set 2 : Respond to comments #
Total comments: 2
Patch Set 3 : Comments, check for NULL dst #
Total comments: 7
Patch Set 4 : SkBufferedStream -> SkFrontBufferedStream #
Total comments: 1
Patch Set 5 : Free buffer when done with it. #
Total comments: 2
Patch Set 6 : Rename test #
Total comments: 10
Patch Set 7 : Respond to comments. #Patch Set 8 : Refactor read() #
Total comments: 4
Patch Set 9 : Respond to comments #Patch Set 10 : Fix a bug #
Total comments: 1
Messages
Total messages: 19 (0 generated)
Test seems to be missing from the patchset? https://codereview.chromium.org/23717055/diff/1/include/utils/SkBufferedStream.h File include/utils/SkBufferedStream.h (right): https://codereview.chromium.org/23717055/diff/1/include/utils/SkBufferedStrea... include/utils/SkBufferedStream.h:42: SkStream* fStream; SkAutoTUnref? https://codereview.chromium.org/23717055/diff/1/include/utils/SkBufferedStrea... include/utils/SkBufferedStream.h:46: SkAutoMalloc fBuffer; Maybe use an SkAutoTMalloc<char> or <uint8_t> here? Just eyeing the double reinterpret_cast. https://codereview.chromium.org/23717055/diff/1/src/utils/SkBufferedStream.cpp File src/utils/SkBufferedStream.cpp (right): https://codereview.chromium.org/23717055/diff/1/src/utils/SkBufferedStream.cp... src/utils/SkBufferedStream.cpp:30: // Even if the underlying stream is at the end, this stream has been Can you sketch out the invariants we're maintaining between fOffset, fBufferedSoFar and fBufferSize somewhere in here? Ideally as a bool validate() method? Am I getting it right that it should be 0 <= fOffset <= fBufferedSoFar < fBufferSize? https://codereview.chromium.org/23717055/diff/1/src/utils/SkBufferedStream.cp... src/utils/SkBufferedStream.cpp:41: if (fOffset <= fBufferSize) { Isn't this off by one? When fOffset == fBufferSize, we're outside the buffer? https://codereview.chromium.org/23717055/diff/1/src/utils/SkBufferedStream.cp... src/utils/SkBufferedStream.cpp:49: SkASSERT(fStream != NULL); I guess the more SkASSERTs the better, but these seem really, obviously impossible to fail, given the SkBufferedStream creation API. https://codereview.chromium.org/23717055/diff/1/src/utils/SkBufferedStream.cp... src/utils/SkBufferedStream.cpp:59: static void add_to_void_ptr(void** ptr, size_t bytes) { Makes me think we should be using a char* everywhere. It's not just a void* now, it's a void* with 1-byte alignment, so we might as well just use a char* or uint8_t*. https://codereview.chromium.org/23717055/diff/1/src/utils/SkBufferedStream.cp... src/utils/SkBufferedStream.cpp:66: size_t SkBufferedStream::read(void* dst, size_t size) { This is a pretty long/complex method. I'm sort of glazing over half way through trying to keep track of the state and invariants. Can you break it up into smaller steps? https://codereview.chromium.org/23717055/diff/1/src/utils/SkBufferedStream.cp... src/utils/SkBufferedStream.cpp:75: memcpy(dst, this->getBufferAtOffset(), bytesToCopy); This is just a suggestion. Writing the API here as this->bufferAt(fOffset) isn't any longer, but makes the call a little less context dependent.
> Test seems to be missing from the patchset? Yes... Included in the next patch set. I've addressed some of your comments. The main things left are to make the read function clearer and add invariants, if they are helpful. https://codereview.chromium.org/23717055/diff/1/include/utils/SkBufferedStream.h File include/utils/SkBufferedStream.h (right): https://codereview.chromium.org/23717055/diff/1/include/utils/SkBufferedStrea... include/utils/SkBufferedStream.h:42: SkStream* fStream; On 2013/09/17 19:18:32, mtklein wrote: > SkAutoTUnref? Done. https://codereview.chromium.org/23717055/diff/1/include/utils/SkBufferedStrea... include/utils/SkBufferedStream.h:46: SkAutoMalloc fBuffer; On 2013/09/17 19:18:32, mtklein wrote: > Maybe use an SkAutoTMalloc<char> or <uint8_t> here? Just eyeing the double > reinterpret_cast. Done. https://codereview.chromium.org/23717055/diff/1/src/utils/SkBufferedStream.cpp File src/utils/SkBufferedStream.cpp (right): https://codereview.chromium.org/23717055/diff/1/src/utils/SkBufferedStream.cp... src/utils/SkBufferedStream.cpp:30: // Even if the underlying stream is at the end, this stream has been On 2013/09/17 19:18:32, mtklein wrote: > Can you sketch out the invariants we're maintaining between fOffset, > fBufferedSoFar and fBufferSize somewhere in here? Ideally as a bool validate() > method? > > Am I getting it right that it should be 0 <= fOffset <= fBufferedSoFar < > fBufferSize? Almost. It should be: 0 <= fBufferedSoFar <= fBufferSize. fOffset can be anywhere. Since these are size_ts, I think the compiler may complain about me comparing against zero, leaving a not very interesting test. I'll see if I can add some more comments for clarity. https://codereview.chromium.org/23717055/diff/1/src/utils/SkBufferedStream.cp... src/utils/SkBufferedStream.cpp:41: if (fOffset <= fBufferSize) { On 2013/09/17 19:18:32, mtklein wrote: > Isn't this off by one? When fOffset == fBufferSize, we're outside the buffer? No, fOffset is the number of bytes that were 'read', so if fOffset == fBufferSize, that means we have read the exact number of bytes that can be buffered (and therefore rewound). https://codereview.chromium.org/23717055/diff/1/src/utils/SkBufferedStream.cp... src/utils/SkBufferedStream.cpp:49: SkASSERT(fStream != NULL); On 2013/09/17 19:18:32, mtklein wrote: > I guess the more SkASSERTs the better, but these seem really, obviously > impossible to fail, given the SkBufferedStream creation API. They're impossible to fail, at least until maintenance happens. Hopefully someone modifying this file would understand that it is expected to be non-NULL. I am not opposed to removing these asserts. https://codereview.chromium.org/23717055/diff/1/src/utils/SkBufferedStream.cp... src/utils/SkBufferedStream.cpp:59: static void add_to_void_ptr(void** ptr, size_t bytes) { On 2013/09/17 19:18:32, mtklein wrote: > Makes me think we should be using a char* everywhere. It's not just a void* > now, it's a void* with 1-byte alignment, so we might as well just use a char* or > uint8_t*. Done. https://codereview.chromium.org/23717055/diff/1/src/utils/SkBufferedStream.cp... src/utils/SkBufferedStream.cpp:66: size_t SkBufferedStream::read(void* dst, size_t size) { On 2013/09/17 19:18:32, mtklein wrote: > This is a pretty long/complex method. I'm sort of glazing over half way through > trying to keep track of the state and invariants. Can you break it up into > smaller steps? Next piece to chew off.... https://codereview.chromium.org/23717055/diff/1/src/utils/SkBufferedStream.cp... src/utils/SkBufferedStream.cpp:75: memcpy(dst, this->getBufferAtOffset(), bytesToCopy); On 2013/09/17 19:18:32, mtklein wrote: > This is just a suggestion. Writing the API here as this->bufferAt(fOffset) > isn't any longer, but makes the call a little less context dependent. In this case, I only ever want the buffer at fOffset, and I am only changing fOffset after the call. I understand the concern about context, but I also tend to think it's safer as is.
https://codereview.chromium.org/23717055/diff/2001/include/utils/SkBufferedSt... File include/utils/SkBufferedStream.h (right): https://codereview.chromium.org/23717055/diff/2001/include/utils/SkBufferedSt... include/utils/SkBufferedStream.h:11: class SkBufferedStream : public SkStreamRewindable { We might consider just adding the static Create to SkStream itself, and hiding *all* of this detail inside the imp. e.g. class SkStream ... { ... static SkStream* CreateBufferedStream(SkStream* other, size_t ...); ... };
Latest patch checks for NULL dst, which can happen in skip(), and adds tests for skipping. https://codereview.chromium.org/23717055/diff/2001/include/utils/SkBufferedSt... File include/utils/SkBufferedStream.h (right): https://codereview.chromium.org/23717055/diff/2001/include/utils/SkBufferedSt... include/utils/SkBufferedStream.h:11: class SkBufferedStream : public SkStreamRewindable { On 2013/09/17 20:23:02, reed1 wrote: > We might consider just adding the static Create to SkStream itself, and hiding > *all* of this detail inside the imp. > > e.g. > > class SkStream ... { > ... > static SkStream* CreateBufferedStream(SkStream* other, size_t ...); > ... > }; I was leaving it in utils since I think only Android needs it, but if we want it in core that is fine with me too.
https://codereview.chromium.org/23717055/diff/13001/include/utils/SkBufferedS... File include/utils/SkBufferedStream.h (right): https://codereview.chromium.org/23717055/diff/13001/include/utils/SkBufferedS... include/utils/SkBufferedStream.h:11: class SkBufferedStream : public SkStreamRewindable { So I'm a bit iffy about the name, simply because this buffers the start of the stream and doesn't circular buffer the last bufferSize bytes read (like Java's BufferedInputStream). https://codereview.chromium.org/23717055/diff/13001/src/utils/SkBufferedStrea... File src/utils/SkBufferedStream.cpp (right): https://codereview.chromium.org/23717055/diff/13001/src/utils/SkBufferedStrea... src/utils/SkBufferedStream.cpp:14: return SkNEW_ARGS(SkBufferedStream, (stream, bufferSize)); One nice thing about this Create maybe being a virtual on SkStream itself is that if a stream is already rewindable or better then it could just return itself, avoiding any need to create any buffer and make the extra copy. However, I'd hate to just keep throwing things onto SkStream. If you look back at https://codereview.chromium.org/15298009/diff/15001/include/core/SkStream.h#n... I had proposed an 'Optimizer' for this sort of thing, so that double dispatch could be used to optimize these sorts of things instead of cluttering up the SkStream interface. https://codereview.chromium.org/23717055/diff/13001/src/utils/SkBufferedStrea... src/utils/SkBufferedStream.cpp:40: return false; My reaction was "returning NULL in here!? ...oh, that is in some sense an error". This should be fine since the contract is that you won't rewind after reading more than the buffer will hold. https://codereview.chromium.org/23717055/diff/13001/src/utils/SkBufferedStrea... src/utils/SkBufferedStream.cpp:129: SkASSERT(fBufferSize == fBufferedSoFar && fOffset >= fBufferSize); It seems like once you get here you can release the memory for fBuffer? Does it do any good at all to do so early? If someone were to read more than the buffer size right off, it looks like we would copy into the buffer and the dst, then copy the remainder into the dst. If the final offset is going to be past the end of the buffer at the beginning, does it make sense to just drop the buffer and just do the read? This may not work because we don't know for sure that we're going to satisfy the entire read request (we may bail atEnd), but we may or may not want the buffer to be 'cancel-able' in the sense that once we're done with the seeking about at the beginning and we're ready to rip through, we don't keep buffering when we know we're not going to need it. This may be completely premature optimization stuff, but I figured I'd put it here since I thought about it.
New patch up for review. https://codereview.chromium.org/23717055/diff/13001/include/utils/SkBufferedS... File include/utils/SkBufferedStream.h (right): https://codereview.chromium.org/23717055/diff/13001/include/utils/SkBufferedS... include/utils/SkBufferedStream.h:11: class SkBufferedStream : public SkStreamRewindable { On 2013/09/17 22:19:11, bungeman1 wrote: > So I'm a bit iffy about the name, simply because this buffers the start of the > stream and doesn't circular buffer the last bufferSize bytes read (like Java's > BufferedInputStream). Good point. How about SkFrontBufferedStream? https://codereview.chromium.org/23717055/diff/13001/src/utils/SkBufferedStrea... File src/utils/SkBufferedStream.cpp (right): https://codereview.chromium.org/23717055/diff/13001/src/utils/SkBufferedStrea... src/utils/SkBufferedStream.cpp:14: return SkNEW_ARGS(SkBufferedStream, (stream, bufferSize)); On 2013/09/17 22:19:11, bungeman1 wrote: > One nice thing about this Create maybe being a virtual on SkStream itself is > that if a stream is already rewindable or better then it could just return > itself, avoiding any need to create any buffer and make the extra copy. However, > I'd hate to just keep throwing things onto SkStream. If you look back at > https://codereview.chromium.org/15298009/diff/15001/include/core/SkStream.h#n... > I had proposed an 'Optimizer' for this sort of thing, so that double dispatch > could be used to optimize these sorts of things instead of cluttering up the > SkStream interface. As Ben and I discussed in person, in my sole use case, I really have a Java InputStream, and I don't know whether the source can be rewound, so this might be a bit of overkill. I think that there's no need to put this on SkStream, since it's a very specialized use case. https://codereview.chromium.org/23717055/diff/13001/src/utils/SkBufferedStrea... src/utils/SkBufferedStream.cpp:129: SkASSERT(fBufferSize == fBufferedSoFar && fOffset >= fBufferSize); On 2013/09/17 22:19:11, bungeman1 wrote: > It seems like once you get here you can release the memory for fBuffer? Almost. The stream could be at the end, in which case this read would not go beyond the end of the buffer. > Does it do any good at all to do so early? Sure - then the memory could be reused. I actually considered making this a templated class, in which case we could avoid the extra allocation, but would not be able to do this early free. > > If someone were to read more than the buffer size right off, it looks like we > would copy into the buffer and the dst, then copy the remainder into the dst. If > the final offset is going to be past the end of the buffer at the beginning, > does it make sense to just drop the buffer and just do the read? This may not > work because we don't know for sure that we're going to satisfy the entire read > request (we may bail atEnd), but we may or may not want the buffer to be > 'cancel-able' in the sense that once we're done with the seeking about at the > beginning and we're ready to rip through, we don't keep buffering when we know > we're not going to need it. This may be completely premature optimization stuff, > but I figured I'd put it here since I thought about it. Good point. Maybe this was implied, but if the caller read more than the buffer size after having buffered some, we'd run into the same situation. If we do want to preserve the ability to rewind if the stream was shorter than the buffer, we'd need to special case that as well. At this point I think I' tend to think that's a premature optimization at the cost of some more complicated code. https://codereview.chromium.org/23717055/diff/19001/include/utils/SkFrontBuff... File include/utils/SkFrontBufferedStream.h (right): https://codereview.chromium.org/23717055/diff/19001/include/utils/SkFrontBuff... include/utils/SkFrontBufferedStream.h:19: class SkFrontBufferedStream : public SkStreamRewindable { Patch set 4 ONLY changes the name of SkBufferedStream to SkFrontBufferedStream (and adds the comment above the class definition), so that the changes to code can be more easily seen.
lgtm with the test name changed https://codereview.chromium.org/23717055/diff/22001/gyp/tests.gyp File gyp/tests.gyp (right): https://codereview.chromium.org/23717055/diff/22001/gyp/tests.gyp#newcode36 gyp/tests.gyp:36: '../tests/BufferedStreamTest.cpp', FrontBufferedStreamTest?
https://codereview.chromium.org/23717055/diff/22001/gyp/tests.gyp File gyp/tests.gyp (right): https://codereview.chromium.org/23717055/diff/22001/gyp/tests.gyp#newcode36 gyp/tests.gyp:36: '../tests/BufferedStreamTest.cpp', On 2013/09/18 15:35:03, bungeman1 wrote: > FrontBufferedStreamTest? Done.
https://codereview.chromium.org/23717055/diff/32001/include/utils/SkFrontBuff... File include/utils/SkFrontBufferedStream.h (right): https://codereview.chromium.org/23717055/diff/32001/include/utils/SkFrontBuff... include/utils/SkFrontBufferedStream.h:29: static SkFrontBufferedStream* Create(SkStream* stream, size_t bufferSize); Why does this return SkFontBufferedStream, instead of SkStreamRewindable or SkStream? Do we need to 'expose' this new subclass to the caller?
Little things and some ideas about how to break up read(). https://codereview.chromium.org/23717055/diff/32001/include/utils/SkFrontBuff... File include/utils/SkFrontBufferedStream.h (right): https://codereview.chromium.org/23717055/diff/32001/include/utils/SkFrontBuff... include/utils/SkFrontBufferedStream.h:65: return fBuffer.get() + fOffset; Now that you're using SkAutoTMalloc<char>, I think you can write this as return fBuffer + fOffset. Might want to just inline that? https://codereview.chromium.org/23717055/diff/32001/src/utils/SkFrontBuffered... File src/utils/SkFrontBufferedStream.cpp (right): https://codereview.chromium.org/23717055/diff/32001/src/utils/SkFrontBuffered... src/utils/SkFrontBufferedStream.cpp:57: // First, read any data that was previously buffered. I think this wants to be three method calls, one to read from the buffer, one to read into both the buffer and dst, and one to read straight from fStream into dst. It'd help me understanding this if we can separate out the interesting meat of how to read the bytes in three different ways from the bookkeeping of keeping fOffset, dst, and size in sync as we go along and maybe return early. What would this code look like if we don't have any early returns? They're sort of blowing through my keeping-track-of-paths-through-the-code budget. Can we just have some form that always drops through to a single return at the end? Altogether, I'm thinking it'd read something like const size_t start = fOffset; if (fOffset < fBufferedSoFar) { const size_t read = readFromBuffer(dst, size); update fOffset, dst, size; } if (fBufferedSoFar < fBufferSize) { const size_t read = bufferAndWriteTo(dst, size); update fOffset, dst, size; } const size_t read = readDirectlyFromStream(dst, size); update fOffset; return fOffset-start; https://codereview.chromium.org/23717055/diff/32001/tests/FrontBufferedStream... File tests/FrontBufferedStreamTest.cpp (right): https://codereview.chromium.org/23717055/diff/32001/tests/FrontBufferedStream... tests/FrontBufferedStreamTest.cpp:32: const char gAbcs[] = "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz"; line wraps https://codereview.chromium.org/23717055/diff/32001/tests/FrontBufferedStream... tests/FrontBufferedStreamTest.cpp:42: test_read(reporter, bufferedStream, gAbcs, bufferSize >> 1); Just a nit: bufferSize / 2 here may be a little more intuitive for future readers. Similarly below.
Little things and some ideas about how to break up read(). https://codereview.chromium.org/23717055/diff/32001/include/utils/SkFrontBuff... File include/utils/SkFrontBufferedStream.h (right): https://codereview.chromium.org/23717055/diff/32001/include/utils/SkFrontBuff... include/utils/SkFrontBufferedStream.h:65: return fBuffer.get() + fOffset; Now that you're using SkAutoTMalloc<char>, I think you can write this as return fBuffer + fOffset. Might want to just inline that? https://codereview.chromium.org/23717055/diff/32001/src/utils/SkFrontBuffered... File src/utils/SkFrontBufferedStream.cpp (right): https://codereview.chromium.org/23717055/diff/32001/src/utils/SkFrontBuffered... src/utils/SkFrontBufferedStream.cpp:57: // First, read any data that was previously buffered. I think this wants to be three method calls, one to read from the buffer, one to read into both the buffer and dst, and one to read straight from fStream into dst. It'd help me understanding this if we can separate out the interesting meat of how to read the bytes in three different ways from the bookkeeping of keeping fOffset, dst, and size in sync as we go along and maybe return early. What would this code look like if we don't have any early returns? They're sort of blowing through my keeping-track-of-paths-through-the-code budget. Can we just have some form that always drops through to a single return at the end? Altogether, I'm thinking it'd read something like const size_t start = fOffset; if (fOffset < fBufferedSoFar) { const size_t read = readFromBuffer(dst, size); update fOffset, dst, size; } if (fBufferedSoFar < fBufferSize) { const size_t read = bufferAndWriteTo(dst, size); update fOffset, dst, size; } const size_t read = readDirectlyFromStream(dst, size); update fOffset; return fOffset-start; https://codereview.chromium.org/23717055/diff/32001/tests/FrontBufferedStream... File tests/FrontBufferedStreamTest.cpp (right): https://codereview.chromium.org/23717055/diff/32001/tests/FrontBufferedStream... tests/FrontBufferedStreamTest.cpp:32: const char gAbcs[] = "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz"; line wraps https://codereview.chromium.org/23717055/diff/32001/tests/FrontBufferedStream... tests/FrontBufferedStreamTest.cpp:42: test_read(reporter, bufferedStream, gAbcs, bufferSize >> 1); Just a nit: bufferSize / 2 here may be a little more intuitive for future readers. Similarly below.
https://codereview.chromium.org/23717055/diff/32001/include/utils/SkFrontBuff... File include/utils/SkFrontBufferedStream.h (right): https://codereview.chromium.org/23717055/diff/32001/include/utils/SkFrontBuff... include/utils/SkFrontBufferedStream.h:29: static SkFrontBufferedStream* Create(SkStream* stream, size_t bufferSize); On 2013/09/18 18:25:54, reed1 wrote: > Why does this return SkFontBufferedStream, instead of SkStreamRewindable or > SkStream? Do we need to 'expose' this new subclass to the caller? We do not. Changed to an SkStreamRewindable. https://codereview.chromium.org/23717055/diff/32001/include/utils/SkFrontBuff... include/utils/SkFrontBufferedStream.h:65: return fBuffer.get() + fOffset; On 2013/09/19 18:56:53, mtklein wrote: > Now that you're using SkAutoTMalloc<char>, I think you can write this as return > fBuffer + fOffset. Might want to just inline that? Done. https://codereview.chromium.org/23717055/diff/32001/src/utils/SkFrontBuffered... File src/utils/SkFrontBufferedStream.cpp (right): https://codereview.chromium.org/23717055/diff/32001/src/utils/SkFrontBuffered... src/utils/SkFrontBufferedStream.cpp:57: // First, read any data that was previously buffered. On 2013/09/19 18:56:53, mtklein wrote: > I think this wants to be three method calls, one to read from the buffer, one to > read into both the buffer and dst, and one to read straight from fStream into > dst. It'd help me understanding this if we can separate out the interesting > meat of how to read the bytes in three different ways from the bookkeeping of > keeping fOffset, dst, and size in sync as we go along and maybe return early. > > What would this code look like if we don't have any early returns? They're sort > of blowing through my keeping-track-of-paths-through-the-code budget. Can we > just have some form that always drops through to a single return at the end? > > Altogether, I'm thinking it'd read something like > > const size_t start = fOffset; > if (fOffset < fBufferedSoFar) { > const size_t read = readFromBuffer(dst, size); > update fOffset, dst, size; > } > if (fBufferedSoFar < fBufferSize) { > const size_t read = bufferAndWriteTo(dst, size); > update fOffset, dst, size; > } > const size_t read = readDirectlyFromStream(dst, size); > update fOffset; > return fOffset-start; FWIW, I think there are two camps for whether to return early. When writing this code, I thought it was clearer to return early (it also means we can skip over some more checks). I went ahead and tried writing it without the early returns - do you find this one to be more clear? I've mostly followed your suggestion for the three functions, only I've updated member variables in the member functions. https://codereview.chromium.org/23717055/diff/32001/tests/FrontBufferedStream... File tests/FrontBufferedStreamTest.cpp (right): https://codereview.chromium.org/23717055/diff/32001/tests/FrontBufferedStream... tests/FrontBufferedStreamTest.cpp:32: const char gAbcs[] = "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz"; On 2013/09/19 18:56:53, mtklein wrote: > line wraps Done. https://codereview.chromium.org/23717055/diff/32001/tests/FrontBufferedStream... tests/FrontBufferedStreamTest.cpp:42: test_read(reporter, bufferedStream, gAbcs, bufferSize >> 1); On 2013/09/19 18:56:53, mtklein wrote: > Just a nit: bufferSize / 2 here may be a little more intuitive for future > readers. Similarly below. Done.
Thanks Leon. This is exactly what I was thinking. Very much LGTM. https://codereview.chromium.org/23717055/diff/48001/include/utils/SkFrontBuff... File include/utils/SkFrontBufferedStream.h (right): https://codereview.chromium.org/23717055/diff/48001/include/utils/SkFrontBuff... include/utils/SkFrontBufferedStream.h:59: SkFrontBufferedStream(); Seems unneeded given the existence of another private constructor right after this? https://codereview.chromium.org/23717055/diff/48001/src/utils/SkFrontBuffered... File src/utils/SkFrontBufferedStream.cpp (right): https://codereview.chromium.org/23717055/diff/48001/src/utils/SkFrontBuffered... src/utils/SkFrontBufferedStream.cpp:101: sk_free(fBuffer.detach()); Just curious, any reason not to do fBuffer.reset(0) instead?
https://codereview.chromium.org/23717055/diff/48001/include/utils/SkFrontBuff... File include/utils/SkFrontBufferedStream.h (right): https://codereview.chromium.org/23717055/diff/48001/include/utils/SkFrontBuff... include/utils/SkFrontBufferedStream.h:59: SkFrontBufferedStream(); On 2013/09/26 15:36:21, mtklein wrote: > Seems unneeded given the existence of another private constructor right after > this? Done. https://codereview.chromium.org/23717055/diff/48001/src/utils/SkFrontBuffered... File src/utils/SkFrontBufferedStream.cpp (right): https://codereview.chromium.org/23717055/diff/48001/src/utils/SkFrontBuffered... src/utils/SkFrontBufferedStream.cpp:101: sk_free(fBuffer.detach()); On 2013/09/26 15:36:21, mtklein wrote: > Just curious, any reason not to do fBuffer.reset(0) instead? No. In fact, reset might be clearer. Changed. https://codereview.chromium.org/23717055/diff/63001/src/utils/SkFrontBuffered... File src/utils/SkFrontBufferedStream.cpp (right): https://codereview.chromium.org/23717055/diff/63001/src/utils/SkFrontBuffered... src/utils/SkFrontBufferedStream.cpp:100: if (bytesReadDirectly > 0) { In patch set 9 I accidentally removed this line, which broke the intended behavior that an SkFrontBufferedStream which has buffered exactly the entire stream can be rewound. It also passed my tests, unfortunately, so I added a test to catch that case.
On 2013/09/26 17:26:03, scroggo wrote: > https://codereview.chromium.org/23717055/diff/48001/include/utils/SkFrontBuff... > File include/utils/SkFrontBufferedStream.h (right): > > https://codereview.chromium.org/23717055/diff/48001/include/utils/SkFrontBuff... > include/utils/SkFrontBufferedStream.h:59: SkFrontBufferedStream(); > On 2013/09/26 15:36:21, mtklein wrote: > > Seems unneeded given the existence of another private constructor right after > > this? > > Done. > > https://codereview.chromium.org/23717055/diff/48001/src/utils/SkFrontBuffered... > File src/utils/SkFrontBufferedStream.cpp (right): > > https://codereview.chromium.org/23717055/diff/48001/src/utils/SkFrontBuffered... > src/utils/SkFrontBufferedStream.cpp:101: sk_free(fBuffer.detach()); > On 2013/09/26 15:36:21, mtklein wrote: > > Just curious, any reason not to do fBuffer.reset(0) instead? > > No. In fact, reset might be clearer. Changed. > > https://codereview.chromium.org/23717055/diff/63001/src/utils/SkFrontBuffered... > File src/utils/SkFrontBufferedStream.cpp (right): > > https://codereview.chromium.org/23717055/diff/63001/src/utils/SkFrontBuffered... > src/utils/SkFrontBufferedStream.cpp:100: if (bytesReadDirectly > 0) { > In patch set 9 I accidentally removed this line, which broke the intended > behavior that an SkFrontBufferedStream which has buffered exactly the entire > stream can be rewound. It also passed my tests, unfortunately, so I added a test > to catch that case. reed@, I addressed the concern you mentioned. Any others? (need LGTM for API change)
lgtm -- pending consensus by ben and mike
Message was sent while issue was closed.
Committed patchset #10 manually as r11488 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
