Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1645963002:
Optimize the SkRawStream when the input is an asset stream (Closed)
DescriptionOptimize the SkRawStream when the input is an asset stream
BUG=b/26841494
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1645963002
Committed: https://skia.googlesource.com/skia/+/9c7a8a464894436fc71a15b5419e818905226cdf
Patch Set 1 #
Total comments: 15
Patch Set 2 : Use peekData() instead of getBaseMemory() #
Total comments: 2
Patch Set 3 : Refactor the SkRawStream logic #Patch Set 4 : Handle the case when getBaseMemory() == nullptr #
Total comments: 24
Patch Set 5 : Address comments and add test #
Total comments: 10
Patch Set 6 : Rebase #Patch Set 7 : Address comments #Patch Set 8 : Add missing override #
Messages
Total messages: 33 (10 generated)
Description was changed from ========== Optimize the SkRawStream when the input is SkMemoryStream BUG=skia: ========== to ========== Optimize the SkRawStream when the input is SkMemoryStream BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Optimize the SkRawStream when the input is SkMemoryStream BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Optimize the SkRawStream when the input is SkMemoryStream BUG=26841494 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Hi scroggo & msarett, I try to improve the TODO in SkRawCodec about the special handling of SkMemoryStream (see https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s...). Do you folks maybe have some ideas about how can I exercise this code path? (The input SkStream is actually a SkMemoryStream)
https://codereview.chromium.org/1645963002/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1645963002/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:169: , fIsMemoryStream(fStream->getMemoryBase() != nullptr) {} Would it be better to use a dynamic_cast? E.g. fIsMemoryStream(dynamic_cast<SkMemoryStream*>(fStream) != nullptr) https://codereview.chromium.org/1645963002/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:186: return static_cast<SkMemoryStream*>(fStream.release()); dynamic_cast?
https://codereview.chromium.org/1645963002/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1645963002/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:169: , fIsMemoryStream(fStream->getMemoryBase() != nullptr) {} On 2016/01/28 11:54:22, adaubert wrote: > Would it be better to use a dynamic_cast? E.g. > > fIsMemoryStream(dynamic_cast<SkMemoryStream*>(fStream) != nullptr) error: ‘dynamic_cast’ not permitted with -fno-rtti https://codereview.chromium.org/1645963002/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:186: return static_cast<SkMemoryStream*>(fStream.release()); On 2016/01/28 11:54:22, adaubert wrote: > dynamic_cast? error: ‘dynamic_cast’ not permitted with -fno-rtti
On 2016/01/28 11:45:32, yujieqin wrote: > Hi scroggo & msarett, > > I try to improve the TODO in SkRawCodec about the special handling of > SkMemoryStream (see > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s...). > Do you folks maybe have some ideas about how can I exercise this code path? (The > input SkStream is actually a SkMemoryStream) Oh, sorry, my mistake. The DM tool only triggers the new code path for SkMemoryStream. How can we test the existing code path for general SkStream (e.g. used by BitmapFactory)?
yujieqin@google.com changed reviewers: + msarett@google.com, scroggo@google.com
I like the optimizations to read(). I'm a little concerned about the changes to transferBuffer(). Supposing that the original RAW stream is about 30 MB and the embedded JPEG stream is about 3 MB, the SkMemoryStream optimizations will cause us to hold onto the full RAW stream during the JPEG decode. Maybe this isn't so bad, given that the stream will still be deleted after the decode? Can we demonstrate a performance improvement by removing the copy (to offset the extra memory use)? Sorry to ask, I know I'm the one who made this suggestion originally :). As far as testing, given that we now treat SkMemoryStreams and other Streams differently, I think we should add a test case to DM. Maybe we can create a test class that extends SkMemoryStream but returns null on getMemoryBase()? What do you think Leon? https://codereview.chromium.org/1645963002/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1645963002/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:182: (sum > fStream->getLength()) || I don't think we necessarily want to fail if (sum > fStream->getLength()). SkJpegCodec is written to handle incomplete images when possible. I believe in the non-optimized case below, we will still "succeed" even if we can't read the entire size. https://codereview.chromium.org/1645963002/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:213: if (!safe_add_to_size_t(alreadyBuffered, bytesRead, &newSize)) { I realize that this is unrelated to this change, but is this check necessary? It seems to me that newSize must be smaller than the input size. https://codereview.chromium.org/1645963002/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:245: if (!safe_add_to_size_t(static_cast<uint64>(count), offset, &sum) || Is this check unnecessary? It looks like we will check again in the other read() function. https://codereview.chromium.org/1645963002/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:252: bool read(size_t offset, size_t length, void* data) { I find it confusing to have two functions both named "read".
I think this idea is a reasonable optimization to make, although we won't see a benefit on Android where we never call this with an SkData. (Today - for subset decodes we always copy to an SkData first, and I think it would be possible to add subset decoding, since I think the multithreaded patch (which I still haven't really looked at, sorry!) splits up based on tile. We can also use an SkData from a file in some cases.) It might make more sense to special case for streams that are seekable, which will mean that we don't need to buffer. (See BitmapFactory.cpp; I think at least some of those are seekable, but I'm not sure whether they'll get used for RAW codec. e.g. There's an asset stream that is seekable, but that would typically be used for UX elements, rather than pictures from the camera/web.) On 2016/01/28 17:25:40, msarett wrote: > I like the optimizations to read(). > > I'm a little concerned about the changes to transferBuffer(). Supposing that > the original RAW stream is about 30 MB and the embedded JPEG stream is about 3 > MB, the SkMemoryStream optimizations will cause us to hold onto the full RAW > stream during the JPEG decode. Maybe this isn't so bad, given that the stream > will still be deleted after the decode? Can we demonstrate a performance > improvement by removing the copy (to offset the extra memory use)? Sorry to > ask, I know I'm the one who made this suggestion originally :). > > As far as testing, given that we now treat SkMemoryStreams and other Streams > differently, I think we should add a test case to DM. Maybe we can create a > test class that extends SkMemoryStream but returns null on getMemoryBase()? > What do you think Leon? Or we can use an SkFILEStream directly. I think there are cases where we end up with an SkMemoryStream behind your back, but that might be simpler. https://codereview.chromium.org/1645963002/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1645963002/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:186: return static_cast<SkMemoryStream*>(fStream.release()); On 2016/01/28 12:25:14, yujieqin wrote: > On 2016/01/28 11:54:22, adaubert wrote: > > dynamic_cast? > > error: ‘dynamic_cast’ not permitted with -fno-rtti This isn't safe. Another implementation can implement getMemoryBase as well (in fact, a few others do). We don't really have a good way to do what you want to do though. I think what you really want to do is grab the underlying SkData directly. We have a TODO to add a virtual method to do that, so I put a patch up at crrev.com/1642943002 which adds it. With SkStream::getData(), all of this can be much more efficient. I would recommend (assuming my other CL lands) having a separate implementation for the raw stream which uses the SkData directly. It never needs to do any buffering, and can just return its length/copy. You could also make it wrap an SkMemoryStream, if that's clearer. Once you've done that, transferBuffer becomes even simpler. The implementation here is unchanged, but for your other implementation, you just copy.
"It might make more sense to special case for streams that are seekable, which will mean that we don't need to buffer. (See BitmapFactory.cpp; I think at least some of those are seekable, but I'm not sure whether they'll get used for RAW codec. e.g. There's an asset stream that is seekable, but that would typically be used for UX elements, rather than pictures from the camera/web.)" FWIW: nativeDecodeAsset() is seekable nativeDecodeByteArray() is an SkMemoryStream nativeDecodeFileDescriptor() is not seekable, but it looks like maybe it could be if we rewrote the implementation to use SkFILEStream? Looks like we convert an SkFILEStream into a non-seekable stream. https://cs.corp.google.com/#android/frameworks/base/core/jni/android/graphics... I'm guessing that Java clients will mostly call decodeFile() or decodeFileDescriptor()? decodeFile() calls nativeDecodeStream() which we can't do much for. But decodeFileDescriptor() calls nativeDecodeFileDescriptor() when the actual file is seekable. Is it possible to optimize the decodeFileDescriptor() and encourage clients to use this API?
Description was changed from ========== Optimize the SkRawStream when the input is SkMemoryStream BUG=26841494 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Optimize the SkRawStream when the input is SkMemoryStream Depend on crrev.com/1642943002 BUG=26841494 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
https://codereview.chromium.org/1645963002/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1645963002/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:182: (sum > fStream->getLength()) || On 2016/01/28 17:25:40, msarett wrote: > I don't think we necessarily want to fail if (sum > fStream->getLength()). > > SkJpegCodec is written to handle incomplete images when possible. I believe in > the non-optimized case below, we will still "succeed" even if we can't read the > entire size. Done https://codereview.chromium.org/1645963002/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:186: return static_cast<SkMemoryStream*>(fStream.release()); On 2016/01/28 18:53:44, scroggo wrote: > On 2016/01/28 12:25:14, yujieqin wrote: > > On 2016/01/28 11:54:22, adaubert wrote: > > > dynamic_cast? > > > > error: ‘dynamic_cast’ not permitted with -fno-rtti > > This isn't safe. Another implementation can implement getMemoryBase as well (in > fact, a few others do). We don't really have a good way to do what you want to > do though. I think what you really want to do is grab the underlying SkData > directly. We have a TODO to add a virtual method to do that, so I put a patch up > at crrev.com/1642943002 which adds it. > > With SkStream::getData(), all of this can be much more efficient. I would > recommend (assuming my other CL lands) having a separate implementation for the > raw stream which uses the SkData directly. It never needs to do any buffering, > and can just return its length/copy. You could also make it wrap an > SkMemoryStream, if that's clearer. > > Once you've done that, transferBuffer becomes even simpler. The implementation > here is unchanged, but for your other implementation, you just copy. I tried to change to use the peekData() based on your CL. PTAL. BTW, I did not make a additional SkRawStream for the seekable case for now, I just use if(fIsSeekable) to control the workflow. But we can surely think about what you suggested if you see there is more benefits. https://codereview.chromium.org/1645963002/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:245: if (!safe_add_to_size_t(static_cast<uint64>(count), offset, &sum) || On 2016/01/28 17:25:40, msarett wrote: > Is this check unnecessary? > > It looks like we will check again in the other read() function. Actually I think it is necessary. Because the special static_cast. Here we check the safe_add_to_size_t<uint64>, later in the other read, we check safe_add_to_size_t<size_t>. So we may miss some issue due to the static_cast<size_t>(count). https://codereview.chromium.org/1645963002/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:252: bool read(size_t offset, size_t length, void* data) { On 2016/01/28 17:25:40, msarett wrote: > I find it confusing to have two functions both named "read". changed the other read to dngRead().
> I'm a little concerned about the changes to transferBuffer(). Supposing that > the original RAW stream is about 30 MB and the embedded JPEG stream is about 3 > MB, the SkMemoryStream optimizations will cause us to hold onto the full RAW > stream during the JPEG decode. Maybe this isn't so bad, given that the stream > will still be deleted after the decode? Can we demonstrate a performance > improvement by removing the copy (to offset the extra memory use)? Sorry to > ask, I know I'm the one who made this suggestion originally :). > I change the transferBuffer() to just copy the JPEG data and free the original stream before create the JPEG codec. And I can not see really performance differences by running the nanobench on my workstation.
Can we answer these questions? (1) What BitmapFactory APIs are used/important for RAW decodes? (2) What is the type of the SkStream that is used in (1)? (3) Can we optimize SkRawCodec for the type of streams discussed in (2)? (4) Can we change BitmapFactory.cpp to use a more versatile type of stream (in the event that it would improve RAW decodes)? (Answer to this might be no, though I think there might be a little flexibility in the FileDescriptor case) https://codereview.chromium.org/1645963002/diff/1/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1645963002/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:213: if (!safe_add_to_size_t(alreadyBuffered, bytesRead, &newSize)) { On 2016/01/28 17:25:40, msarett wrote: > I realize that this is unrelated to this change, but is this check necessary? > > It seems to me that newSize must be smaller than the input size. PTAL https://codereview.chromium.org/1645963002/diff/1/src/codec/SkRawCodec.cpp#ne... src/codec/SkRawCodec.cpp:245: if (!safe_add_to_size_t(static_cast<uint64>(count), offset, &sum) || On 2016/02/01 12:59:47, yujieqin wrote: > On 2016/01/28 17:25:40, msarett wrote: > > Is this check unnecessary? > > > > It looks like we will check again in the other read() function. > > Actually I think it is necessary. Because the special static_cast. > > Here we check the safe_add_to_size_t<uint64>, later in the other read, we check > safe_add_to_size_t<size_t>. So we may miss some issue due to the > static_cast<size_t>(count). Acknowledged. https://codereview.chromium.org/1645963002/diff/20001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1645963002/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:169: , fIsSeekable(fStream->peekData() != nullptr) {} I think maybe this is a mix of two suggestions. I believe that "hasPosition()" is a good way to check if the stream is seekable. "peekData()" is maybe a good way to get the memory base? Though I can't find peekData() in anywhere in Skia right now so I'm a little confused. Regardless, it looks like this CL still optimizes for a memory base.
On 2016/02/01 16:23:43, msarett wrote: > Can we answer these questions? > > (1) What BitmapFactory APIs are used/important for RAW decodes? At least for Photos, judds@ told me the following: We use one of two things: 1. For local, and some http requests (usually images we load from outside the app), we use a BufferedInputStream that wraps an InputStream with a buffer that grows up to 5mb. The wrapping occurs here: https://cs.corp.google.com/#piper///depot/google3/third_party/java_src/androi... The buffer size occurs here: https://cs.corp.google.com/#piper///depot/google3/third_party/java_src/androi... 2. For remote content, we use a custom InputStream that wraps a ByteBuffer. The InputStream is here: https://cs.corp.google.com/#piper///depot/google3/third_party/java_src/androi... And wrapped here: https://cs.corp.google.com/#piper///depot/google3/third_party/java_src/androi... > > (2) What is the type of the SkStream that is used in (1)? Ignoring SkFrontBufferedStream, which just wraps for rewinding the beginning (and if/when we add peek to the JavaInputStream is unneeded for some cases), I see the following: - JavaInputStream - SkFILEStream - This one we wrap to make sure we don't rewind past the beginning, but it is seekable and we could wrap with a seekable wrapper stream - AssetStreamAdaptor, which is not seekable but could be - this is only for Assets, though, which I'm guessing won't typically be used for raw - SkMemoryStream (from decodeByteArray) - (also looks to be called from decodeBitmap, which does not look like it is ever called) https://codereview.chromium.org/1645963002/diff/20001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1645963002/diff/20001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:169: , fIsSeekable(fStream->peekData() != nullptr) {} On 2016/02/01 16:23:43, msarett wrote: > I think maybe this is a mix of two suggestions. > > I believe that "hasPosition()" is a good way to check if the stream is seekable. That is correct. > > "peekData()" is maybe a good way to get the memory base? Though I can't find > peekData() in anywhere in Skia right now so I'm a little confused. Not yet checked in, but it's under review at crrev.com/1642943002 > > Regardless, it looks like this CL still optimizes for a memory base.
"- SkFILEStream - This one we wrap to make sure we don't rewind past the beginning, but it is seekable and we could wrap with a seekable wrapper stream" Ahh thanks. I was confused by the fact that we wrap this in a FrontBufferedStream. So we could potentially see benefits here if we wrap with a seekable stream? Would that require a change to BitmapFactory.cpp? "- SkMemoryStream (from decodeByteArray)" And we could see benefits here related to memory streams. But IIUC neither of these APIs is used by Photos?
On 2016/02/01 16:54:23, msarett wrote: > "- SkFILEStream > - This one we wrap to make sure we don't rewind past the beginning, but it is > seekable and we could wrap with a seekable wrapper stream" > > Ahh thanks. I was confused by the fact that we wrap this in > a FrontBufferedStream. So we could potentially see benefits > here if we wrap with a seekable stream? Would that require a change to > BitmapFactory.cpp? Yes, but it wouldn't be complicated. We just need some sort of wrapper that rewinds to the start point (instead of to the beginning). > > "- SkMemoryStream (from decodeByteArray)" > > And we could see benefits here related to memory streams. > > > But IIUC neither of these APIs is used by Photos? That is my understanding. But others might. (And maybe Photos could perhaps benefit if the file stream was optimized? I'm not sure whether it would ever make sense for them to use a file stream.)
On 2016/02/01 17:12:18, scroggo wrote: > On 2016/02/01 16:54:23, msarett wrote: > > "- SkFILEStream > > - This one we wrap to make sure we don't rewind past the beginning, but it > is > > seekable and we could wrap with a seekable wrapper stream" > > > > Ahh thanks. I was confused by the fact that we wrap this in > > a FrontBufferedStream. So we could potentially see benefits > > here if we wrap with a seekable stream? Would that require a change to > > BitmapFactory.cpp? > > Yes, but it wouldn't be complicated. We just need some sort of wrapper that > rewinds to the start point (instead of to the beginning). > > > > > "- SkMemoryStream (from decodeByteArray)" > > > > And we could see benefits here related to memory streams. > > > > > > But IIUC neither of these APIs is used by Photos? > > That is my understanding. But others might. (And maybe Photos could perhaps > benefit if the file stream was optimized? I'm not sure whether it would ever > make sense for them to use a file stream.) After some discussion on peekData, I don't think it's really necessary. Really you want to know if the stream is seekable (hasPosition, as Matt pointed out). This will be very efficient for SkMemoryStream, and work for other types of streams that are seekable (where it will also remove the need to buffer). Since the methods will now be so different (basically all your read methods will branch on fIsSeekable, and bufferMoreData isn't needed for one case), I really think it makes more sense to have two versions of the stream. When it's time to transferBuffer, you can call getMemoryBase then, and copy from it if non-null.
Description was changed from ========== Optimize the SkRawStream when the input is SkMemoryStream Depend on crrev.com/1642943002 BUG=26841494 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Optimize the SkRawStream when the input is SkMemoryStream BUG=26841494 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
On 2016/02/01 18:26:26, scroggo wrote: > On 2016/02/01 17:12:18, scroggo wrote: > > On 2016/02/01 16:54:23, msarett wrote: > > > "- SkFILEStream > > > - This one we wrap to make sure we don't rewind past the beginning, but it > > is > > > seekable and we could wrap with a seekable wrapper stream" > > > > > > Ahh thanks. I was confused by the fact that we wrap this in > > > a FrontBufferedStream. So we could potentially see benefits > > > here if we wrap with a seekable stream? Would that require a change to > > > BitmapFactory.cpp? > > > > Yes, but it wouldn't be complicated. We just need some sort of wrapper that > > rewinds to the start point (instead of to the beginning). > > > > > > > > "- SkMemoryStream (from decodeByteArray)" > > > > > > And we could see benefits here related to memory streams. > > > > > > > > > But IIUC neither of these APIs is used by Photos? > > > > That is my understanding. But others might. (And maybe Photos could perhaps > > benefit if the file stream was optimized? I'm not sure whether it would ever > > make sense for them to use a file stream.) > > After some discussion on peekData, I don't think it's really necessary. Really > you want to know if the stream is seekable (hasPosition, as Matt pointed out). > This will be very efficient for SkMemoryStream, and work for other types of > streams that are seekable (where it will also remove the need to buffer). Since > the methods will now be so different (basically all your read methods will > branch on fIsSeekable, and bufferMoreData isn't needed for one case), I really > think it makes more sense to have two versions of the stream. > > When it's time to transferBuffer, you can call getMemoryBase then, and copy from > it if non-null. Refactor a bit, PTAL. :)
We should probably add tests for some of this stuff. In particular, I'm concerned about transferBuffer, which has a lot of different cases that we may not be testing. https://codereview.chromium.org/1645963002/diff/60001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1645963002/diff/60001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:167: * Gets the length of the stream. Denpending on the type of stream, this may require read to the depending* reading* https://codereview.chromium.org/1645963002/diff/60001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:168: * end of stream. the* stream https://codereview.chromium.org/1645963002/diff/60001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:172: virtual bool read(size_t offset, size_t length, void* data) = 0; nit: We typically put our output parameters first. I think we also typically name this variable "buffer". So this should be: virtual bool read(void* buffer, size_t offset, size_t length) = 0; https://codereview.chromium.org/1645963002/diff/60001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:207: if (offset == 0 && length == 0) { Why is there a special case here? I can see why a length of 0 is an automatic success, but why does it also check offset? https://codereview.chromium.org/1645963002/diff/60001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:307: return fStream->getLength(); Maybe we should also check hasLength() to determine to use the SkRawSeekableStream, since there could be a stream which has a position but not a length. https://codereview.chromium.org/1645963002/diff/60001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:321: if (sum > fStream->getLength()) { I don't think this check is necessary - the below check should return false if sum > length https://codereview.chromium.org/1645963002/diff/60001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:322: return false; four space indent. https://codereview.chromium.org/1645963002/diff/60001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:335: static_cast<const uint8_t*>(fStream->getMemoryBase()) + offset, size)); This case should also support a partial jpeg file. https://codereview.chromium.org/1645963002/diff/60001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:343: // Don't check the returned size, because the following JPEG codec may be able to We do need to know the returned size, though, because otherwise the JPEG codec will try to read uninitialized memory. https://codereview.chromium.org/1645963002/diff/60001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:386: if (!safe_add_to_size_t(static_cast<uint64>(count), offset, &sum) || Won't the implementation of fStream->read also make this check? https://codereview.chromium.org/1645963002/diff/60001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:536: SkRawSeekableStream::NewFromStream(stream) : SkRawBufferedStream::NewFromStream(stream))); I don't think these factory functions are necessary. The constructor is public, and it always succeeds. We tend to like factory functions when we need to do some work to make sure the object is valid before returning it. Like with the SkDngImage, readNegative may fail, in which case we don't want to return an invalid object. https://codereview.chromium.org/1645963002/diff/60001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:550: piexStream->releaseRawStream()->transferBuffer(imageData.preview_offset, This is going to leak the raw stream. I think it makes more sense to just call a method on piexStream, and let it take care of deleting the raw stream (which will happen soon).
I also added some tests. https://codereview.chromium.org/1645963002/diff/60001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1645963002/diff/60001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:167: * Gets the length of the stream. Denpending on the type of stream, this may require read to the On 2016/02/02 19:23:06, scroggo wrote: > depending* > > reading* Done. https://codereview.chromium.org/1645963002/diff/60001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:168: * end of stream. On 2016/02/02 19:23:06, scroggo wrote: > the* stream Done. https://codereview.chromium.org/1645963002/diff/60001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:172: virtual bool read(size_t offset, size_t length, void* data) = 0; On 2016/02/02 19:23:06, scroggo wrote: > nit: We typically put our output parameters first. I think we also typically > name this variable "buffer". So this should be: > > virtual bool read(void* buffer, size_t offset, size_t length) = 0; Done. https://codereview.chromium.org/1645963002/diff/60001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:207: if (offset == 0 && length == 0) { On 2016/02/02 19:23:06, scroggo wrote: > Why is there a special case here? I can see why a length of 0 is an automatic > success, but why does it also check offset? I think you are right. This should be a bug. :) https://codereview.chromium.org/1645963002/diff/60001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:307: return fStream->getLength(); On 2016/02/02 19:23:06, scroggo wrote: > Maybe we should also check hasLength() to determine to use the > SkRawSeekableStream, since there could be a stream which has a position but not > a length. Done. https://codereview.chromium.org/1645963002/diff/60001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:321: if (sum > fStream->getLength()) { On 2016/02/02 19:23:06, scroggo wrote: > I don't think this check is necessary - the below check should return false if > sum > length Done. https://codereview.chromium.org/1645963002/diff/60001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:322: return false; On 2016/02/02 19:23:06, scroggo wrote: > four space indent. Done. https://codereview.chromium.org/1645963002/diff/60001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:335: static_cast<const uint8_t*>(fStream->getMemoryBase()) + offset, size)); On 2016/02/02 19:23:06, scroggo wrote: > This case should also support a partial jpeg file. Done. https://codereview.chromium.org/1645963002/diff/60001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:343: // Don't check the returned size, because the following JPEG codec may be able to On 2016/02/02 19:23:06, scroggo wrote: > We do need to know the returned size, though, because otherwise the JPEG codec > will try to read uninitialized memory. Done. https://codereview.chromium.org/1645963002/diff/60001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:386: if (!safe_add_to_size_t(static_cast<uint64>(count), offset, &sum) || On 2016/02/02 19:23:06, scroggo wrote: > Won't the implementation of fStream->read also make this check? as explained for last patch set, this check is for type uint64-->size_t, the other check of fStream->read is for type size_t-->size_t. https://codereview.chromium.org/1645963002/diff/60001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:536: SkRawSeekableStream::NewFromStream(stream) : SkRawBufferedStream::NewFromStream(stream))); On 2016/02/02 19:23:06, scroggo wrote: > I don't think these factory functions are necessary. The constructor is public, > and it always succeeds. > > We tend to like factory functions when we need to do some work to make sure the > object is valid before returning it. Like with the SkDngImage, readNegative may > fail, in which case we don't want to return an invalid object. Done. https://codereview.chromium.org/1645963002/diff/60001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:550: piexStream->releaseRawStream()->transferBuffer(imageData.preview_offset, On 2016/02/02 19:23:06, scroggo wrote: > This is going to leak the raw stream. I think it makes more sense to just call a > method on piexStream, and let it take care of deleting the raw stream (which > will happen soon). Done.
One question, when I try to run: out/Debug/dm --match Codec for executing the test on Linux, it seems like the SK_CODEC_DECODES_RAW is not defined, do you know if it is expected behavior?
From the commit message: > BUG=26841494 This actually makes a link to https://code.google.com/p/chromium/issues/detail?id=26841494. I don't think there's a way to make it link to an Android bug, but I usually write: BUG=b/26841494 That way it doesn't automatically create a bogus link, and you can more easily use it to go to the actual webpage. (An alternative is to create a Skia bug, and link to by using BUG=skia:###, but I think it's fine to use Android bugs.) On 2016/02/04 14:52:35, yujieqin wrote: > One question, when I try to run: > out/Debug/dm --match Codec > for executing the test on Linux, it seems like the SK_CODEC_DECODES_RAW is not > defined, do you know if it is expected behavior? I'm guessing you'll need to rebase. crrev.com/1612113002 made it so that SK_CODEC_DECODES_RAW is defined everywhere, so we can use it in tests. https://codereview.chromium.org/1645963002/diff/80001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1645963002/diff/80001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:160: bool IsStreamSeekable(const SkStream& stream) { In SkStream.h, we consider a stream "seekable" if hasPosition returns true. We consider it an "asset" if hasLength also returns true. So I think it would be better to name this something like is_asset_stream(...) (Also it is a static function so it should be named lowercase and camel_case.) https://codereview.chromium.org/1645963002/diff/80001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:347: if (bytesToRead != fStream->read(data->writable_data(), bytesToRead)) { This isn't the fix I intended. You changed the first case to support a partial jpeg file, but you removed that from this case. Instead, this should say something like: const size_t bytesRead = fStream->read(...); if (bytesRead < bytesToRead) { data.reset(SkData::NewSubset(data.get(), 0, bytesRead)); } (We already do this in the SkRawBufferedStream.) ----- Looking above, I realize that you already checked to make sure that bytesToRead does not go beyond the length. So it would be a buggy stream that failed to read all the way to the length. But I think it's still better to handle this case as I described above. https://codereview.chromium.org/1645963002/diff/80001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:542: SkAutoTDelete<SkPiexStream> piexStream(new SkPiexStream(rawStream.get())); No need to auto delete this. You can just put it on the stack: SkPiexStream piexStream(rawStream.get()); https://codereview.chromium.org/1645963002/diff/80001/tests/CodexTest.cpp File tests/CodexTest.cpp (right): https://codereview.chromium.org/1645963002/diff/80001/tests/CodexTest.cpp#new... tests/CodexTest.cpp:899: // Stream that is not seekable (!hasPotions() or !hasLength()) hasPosition()* https://codereview.chromium.org/1645963002/diff/80001/tests/CodexTest.cpp#new... tests/CodexTest.cpp:922: return false; Should this return fStream.isAtEnd()? (I'm guessing that it does not matter, but it seems odd.)
Description was changed from ========== Optimize the SkRawStream when the input is SkMemoryStream BUG=26841494 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Optimize the SkRawStream when the input is SkMemoryStream BUG=b/26841494 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Rebased. :) https://codereview.chromium.org/1645963002/diff/80001/src/codec/SkRawCodec.cpp File src/codec/SkRawCodec.cpp (right): https://codereview.chromium.org/1645963002/diff/80001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:160: bool IsStreamSeekable(const SkStream& stream) { On 2016/02/04 15:46:24, scroggo wrote: > In SkStream.h, we consider a stream "seekable" if hasPosition returns true. We > consider it an "asset" if hasLength also returns true. So I think it would be > better to name this something like > > is_asset_stream(...) > > (Also it is a static function so it should be named lowercase and camel_case.) Done. https://codereview.chromium.org/1645963002/diff/80001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:347: if (bytesToRead != fStream->read(data->writable_data(), bytesToRead)) { On 2016/02/04 15:46:24, scroggo wrote: > This isn't the fix I intended. You changed the first case to support a partial > jpeg file, but you removed that from this case. > > Instead, this should say something like: > > const size_t bytesRead = fStream->read(...); > if (bytesRead < bytesToRead) { > data.reset(SkData::NewSubset(data.get(), 0, bytesRead)); > } > > (We already do this in the SkRawBufferedStream.) > > ----- > > Looking above, I realize that you already checked to make sure that bytesToRead > does not go beyond the length. So it would be a buggy stream that failed to read > all the way to the length. But I think it's still better to handle this case as > I described above. Done. https://codereview.chromium.org/1645963002/diff/80001/src/codec/SkRawCodec.cp... src/codec/SkRawCodec.cpp:542: SkAutoTDelete<SkPiexStream> piexStream(new SkPiexStream(rawStream.get())); On 2016/02/04 15:46:24, scroggo wrote: > No need to auto delete this. You can just put it on the stack: > > SkPiexStream piexStream(rawStream.get()); Done. https://codereview.chromium.org/1645963002/diff/80001/tests/CodexTest.cpp File tests/CodexTest.cpp (right): https://codereview.chromium.org/1645963002/diff/80001/tests/CodexTest.cpp#new... tests/CodexTest.cpp:899: // Stream that is not seekable (!hasPotions() or !hasLength()) On 2016/02/04 15:46:24, scroggo wrote: > hasPosition()* Done. https://codereview.chromium.org/1645963002/diff/80001/tests/CodexTest.cpp#new... tests/CodexTest.cpp:922: return false; On 2016/02/04 15:46:24, scroggo wrote: > Should this return fStream.isAtEnd()? (I'm guessing that it does not matter, but > it seems odd.) Done.
lgtm
The CQ bit was checked by yujieqin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com Link to the patchset: https://codereview.chromium.org/1645963002/#ps140001 (title: "Add missing override")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1645963002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1645963002/140001
Description was changed from ========== Optimize the SkRawStream when the input is SkMemoryStream BUG=b/26841494 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Optimize the SkRawStream when the input is an asset stream BUG=b/26841494 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Message was sent while issue was closed.
Description was changed from ========== Optimize the SkRawStream when the input is an asset stream BUG=b/26841494 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Optimize the SkRawStream when the input is an asset stream BUG=b/26841494 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/9c7a8a464894436fc71a15b5419e818905226cdf ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://skia.googlesource.com/skia/+/9c7a8a464894436fc71a15b5419e818905226cdf |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
