Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1194703002:
onGetScanlines and onSkipScanlines for interlaced pngs (Closed)
DescriptionImplemented onGetScanlines and onSkipScanlines for interlaced pngs
Modified DM tests to be faster when decoding interlaced pngs
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/0a4c3cbfd77e11901c66ac29bf1417a42b87fd31
Patch Set 1 #
Total comments: 27
Patch Set 2 : changing variable names for clarity, updating constructor #
Total comments: 2
Patch Set 3 : editing SkPngInterlacedScanlineDecoder's member variables, handling kCouldNotRewind case #
Total comments: 2
Patch Set 4 : casting int to size_t which caused warning #Patch Set 5 : fixing size_t to int conversion warning #
Messages
Total messages: 23 (7 generated)
emmaleer@google.com changed reviewers: + msarett@google.com, scroggo@google.com
https://codereview.chromium.org/1194703002/diff/1/src/codec/SkCodec_libpng.cpp File src/codec/SkCodec_libpng.cpp (right): https://codereview.chromium.org/1194703002/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:659: fCurrentRow = 0; Should these variables be defined in the cosntructor, or in onGetScanlines()? They could be defined in either place
All my comments are nits. This looks good. https://codereview.chromium.org/1194703002/diff/1/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1194703002/diff/1/dm/DMSrcSink.cpp#newcode213 dm/DMSrcSink.cpp:213: char* buffer = SkNEW_ARRAY(char, h * rowBytes); Can we allocate this buffer with some kind of maxSubsetHeight rather than the height of the full image? https://codereview.chromium.org/1194703002/diff/1/dm/DMSrcSink.cpp#newcode268 dm/DMSrcSink.cpp:268: bufferRow += rowBytes; I have a theory that it is faster to copy from a row buffer than from an image sized buffer (and it's also more memory efficient). Obviously, this is not an option got interlaced pngs and this is a correctness test, so what you have here is perfect. But this is something to keep in mind/test when we implement subset decodes in BitmapRegionDecoder. https://codereview.chromium.org/1194703002/diff/1/src/codec/SkCodec_libpng.cpp File src/codec/SkCodec_libpng.cpp (right): https://codereview.chromium.org/1194703002/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:659: fCurrentRow = 0; On 2015/06/18 21:05:41, emmaleer wrote: > Should these variables be defined in the cosntructor, or in onGetScanlines()? > They could be defined in either place I prefer them here. For interlaced png, you probably only intend for onGetScanlines() to be called once, but it looks like it can be called multiple times. If the variables are in the constructor, we are sure that the initialization only happens once. Also, I think it would be nice to use the initializer list to define them. For example, ... , fHasAlpha(false) , fCurrentRow(0) , fHeight(dstInfo.height()) ... is equivalent to what you wrote. https://codereview.chromium.org/1194703002/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:674: fStorageData.reset(count * fSrcRowBytes); Maybe this belongs on the constructor as well? Do we need both fStorageData and fBuffer to be fields? Same question for fStorageGarbage and fGarbageRow.
https://codereview.chromium.org/1194703002/diff/1/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1194703002/diff/1/dm/DMSrcSink.cpp#newcode213 dm/DMSrcSink.cpp:213: char* buffer = SkNEW_ARRAY(char, h * rowBytes); On 2015/06/19 13:35:54, msarett wrote: > Can we allocate this buffer with some kind of maxSubsetHeight rather than the > height of the full image? >Yes, I changed it to use the maxSubsetHeight https://codereview.chromium.org/1194703002/diff/1/dm/DMSrcSink.cpp#newcode268 dm/DMSrcSink.cpp:268: bufferRow += rowBytes; On 2015/06/19 13:35:54, msarett wrote: > I have a theory that it is faster to copy from a row buffer than from an image > sized buffer (and it's also more memory efficient). Obviously, this is not an > option got interlaced pngs and this is a correctness test, so what you have here > is perfect. > > But this is something to keep in mind/test when we implement subset decodes in > BitmapRegionDecoder. >Yes I think it is faster to copy from a row buffer, since that row will still be cached at the time of copying.. for interlaced pngs you can do it row by row (as implemented before) but it takes forever, since you decode the entire image height times. That's why I changed it to do all the Scanlines at once https://codereview.chromium.org/1194703002/diff/1/src/codec/SkCodec_libpng.cpp File src/codec/SkCodec_libpng.cpp (right): https://codereview.chromium.org/1194703002/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:659: fCurrentRow = 0; On 2015/06/19 13:35:54, msarett wrote: > On 2015/06/18 21:05:41, emmaleer wrote: > > Should these variables be defined in the cosntructor, or in onGetScanlines()? > > They could be defined in either place > > I prefer them here. For interlaced png, you probably only intend for > onGetScanlines() to be called once, but it looks like it can be called multiple > times. If the variables are in the constructor, we are sure that the > initialization only happens once. > > Also, I think it would be nice to use the initializer list to define them. > For example, > ... > , fHasAlpha(false) > , fCurrentRow(0) > , fHeight(dstInfo.height()) > ... > is equivalent to what you wrote. Done. https://codereview.chromium.org/1194703002/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:674: fStorageData.reset(count * fSrcRowBytes); On 2015/06/19 13:35:54, msarett wrote: > Maybe this belongs on the constructor as well? Do we need both fStorageData and > fBuffer to be fields? Same question for fStorageGarbage and fGarbageRow. >I can't put fStorageData in the constructor because we don't know what count will be until onGetScanlines is called >Yes we need both fStorageData and fBuffer because fStorageData is the allocated memory, and fBuffer is the pointer to that memory >I changed the names of them to fStorage and fStoragep so it's more clear that one is a pointer to the other
https://codereview.chromium.org/1194703002/diff/1/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1194703002/diff/1/dm/DMSrcSink.cpp#newcode167 dm/DMSrcSink.cpp:167: bitmap.getAddr(0, 0), decodeInfo.height(), decodeInfo.minRowBytes()); This is a little tricky: in this case it happens to be correct to use decodeInfo.minRowBytes(), but it's not guaranteed. As the client of getScanlines, you're providing the pixel memory, so you know what you used for the rowbytes. In this case, you're using memory allocated by SkBitmap::tryAllocPixels (line 137), which initialized the bitmap with a particular rowBytes, so you can ask the bitmap for its rowbytes: bitmap.rowBytes(). https://codereview.chromium.org/1194703002/diff/1/dm/DMSrcSink.cpp#newcode254 dm/DMSrcSink.cpp:254: subsetScanlineDecoder->getScanlines(buffer, currentSubsetHeight, rowBytes); nit: I believe this exceeds 100 characters. We try to keep each line under 100. https://codereview.chromium.org/1194703002/diff/1/dm/DMSrcSink.cpp#newcode261 dm/DMSrcSink.cpp:261: "message %d", fPath.c_str(), y-1, (int) subsetResult); It probably is not useful to print the y value, since we do all the requested rows at once. https://codereview.chromium.org/1194703002/diff/1/dm/DMSrcSink.cpp#newcode267 dm/DMSrcSink.cpp:267: memcpy(subsetBm.getAddr(0, subsetY), bufferRow + x*bpp, currentSubsetWidth*bpp); nit: over 100 characters. https://codereview.chromium.org/1194703002/diff/1/dm/DMSrcSink.cpp#newcode268 dm/DMSrcSink.cpp:268: bufferRow += rowBytes; On 2015/06/19 13:35:54, msarett wrote: > I have a theory that it is faster to copy from a row buffer than from an image > sized buffer (and it's also more memory efficient). Obviously, this is not an > option got interlaced pngs and this is a correctness test, so what you have here > is perfect. > > But this is something to keep in mind/test when we implement subset decodes in > BitmapRegionDecoder. Agreed. We should confirm that theory in order to decide the best way to do it. If it in fact is faster for non-interlaced (it's definitely slower for interlaced), we'll need to have an API that tells you whether the image is interlaced. (Maybe it should not specify interlacing per se, but give some kind of hint about the best way to use it.) Could you add a comment explaining why we're doing all the lines at once, and acknowledging that it may not always be the fastest way to do it (but this is a correctness test so that's okay)? https://codereview.chromium.org/1194703002/diff/1/src/codec/SkCodec_libpng.cpp File src/codec/SkCodec_libpng.cpp (right): https://codereview.chromium.org/1194703002/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:659: fCurrentRow = 0; On 2015/06/19 13:35:54, msarett wrote: > On 2015/06/18 21:05:41, emmaleer wrote: > > Should these variables be defined in the cosntructor, or in onGetScanlines()? > > They could be defined in either place > > I prefer them here. For interlaced png, you probably only intend for > onGetScanlines() to be called once, but it looks like it can be called multiple > times. If the variables are in the constructor, we are sure that the > initialization only happens once. > > Also, I think it would be nice to use the initializer list to define them. > For example, > ... > , fHasAlpha(false) > , fCurrentRow(0) > , fHeight(dstInfo.height()) > ... > is equivalent to what you wrote. Taking that a step further, I believe you can even put fStorageGarbage in the initializer list, since it has a constructor that does the same thing as reset(). It can be a little tricky though, since you want to initialize it with the value of another member variable, and member variables will get initialized in the order they are declared, rather than their ordering in the initializer list. I think we have the warning turned on (as an error) that fires if the two orderings do not match (although I don't think you'd get a warning if the orderings did match, but you initialized them in the wrong order). All that said, for some member variables (i.e. those with dependencies on each other), it might be preferable to initialize in the body of the constructor. https://codereview.chromium.org/1194703002/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:668: fCodec->handleRewind(); If handleRewind returns false (which it can for a stream that cannot be rewound, although we will typically use it with a stream that can be), this should return kCouldNotRewind. https://codereview.chromium.org/1194703002/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:674: fStorageData.reset(count * fSrcRowBytes); On 2015/06/19 14:28:08, emmaleer wrote: > On 2015/06/19 13:35:54, msarett wrote: > > Maybe this belongs on the constructor as well? Do we need both fStorageData > and > > fBuffer to be fields? Same question for fStorageGarbage and fGarbageRow. > > >I can't put fStorageData in the constructor because we don't know what count > will be until onGetScanlines is called > >Yes we need both fStorageData and fBuffer because fStorageData is the allocated > memory, and fBuffer is the pointer to that memory > >I changed the names of them to fStorage and fStoragep so it's more clear that > one is a pointer to the other (FYI: If you put ">"s at the beginning of each line in your response, it looks like it's quoting text, making it hard to distinguish between what you wrote and what you're replying to.) I believe what Matt is saying is that instead of making fStorageData and fBuffer "fields" (another word for member variables), you could just put them on the stack, since they're only used inside this function. fStorageGarbage should arguably remain a field, though, since you'll use the same one for the next call to onGetScanlines (if there is another). https://codereview.chromium.org/1194703002/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:707: fCurrentRow += count; I think if we've already read rows, you'll want to do a rewind here as well. (If we've only skipped rows previously, you do not need to rewind, but given that that is presumably an uncommon use case, you may not need to take it into consideration - but maybe add a comment that it wouldn't be necessary.) The tricky thing is that this is a little bit of a mismatch between my previous thoughts on rewinding and this particular situation: A) (old thinking) The base class SkCodec has a function called rewindIfNeeded. It does a couple of things: - It attempts to rewind if the stream has already been read (past initial setup) - It makes a note that the stream has already been read B) (this situation) You want to rewind the stream if it has already been read, but you do *not* want to mark it as read (since you don't want the next call to getScanlines to require a rewind - it would still work, but it would be less efficient). (Come to think of it, solving this problem will solve the earlier problem I mentioned, where you don't want to rewind if all you've done so far is skip.) I think the simplest solution will be to use a boolean in this class that keeps track of whether you've read any data yet. Maybe fCurrentRow is enough? It doesn't solve the consecutive calls to skipScanlines, but as I mentioned earlier, I think that's an okay case to handle poorly. This method might give us the wrong behavior if someone tried to do the following: SkCodec* codec = SkCodec::NewFromStream(); SkScanlineDecoder* sd = codec->getScanlineDecoder(); codec->getPixels(); // After this call we need to rewind sd->getScanlines(); // If we only check our local info, rather than rewindIfNeeded // sd won't know to rewind. Maybe that's a dumb use case, although I think it is currently supported. The other way would be to plumb an option to rewindIfNeeded to only check if a rewind is necessary, but not tell it the next call will require a rewind. This would support the dumb use case (and might be more straightforward). That was kind of a brain dump of my thoughts on this, so I may not have explained it well... Let me know if you want to discuss in person. https://codereview.chromium.org/1194703002/diff/20001/src/codec/SkCodec_libpn... File src/codec/SkCodec_libpng.cpp (right): https://codereview.chromium.org/1194703002/diff/20001/src/codec/SkCodec_libpn... src/codec/SkCodec_libpng.cpp:724: uint8_t* fStoragep; nit: In Skia, I think we would typically name this fStoragePtr (which of course, differs from other conventions...)
https://codereview.chromium.org/1194703002/diff/1/src/codec/SkCodec_libpng.cpp File src/codec/SkCodec_libpng.cpp (right): https://codereview.chromium.org/1194703002/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:707: fCurrentRow += count; On 2015/06/19 15:45:53, scroggo wrote: > I think if we've already read rows, you'll want to do a rewind here as well. (If > we've only skipped rows previously, you do not need to rewind, but given that > that is presumably an uncommon use case, you may not need to take it into > consideration - but maybe add a comment that it wouldn't be necessary.) > > The tricky thing is that this is a little bit of a mismatch between my previous > thoughts on rewinding and this particular situation: > > A) (old thinking) The base class SkCodec has a function called rewindIfNeeded. > It does a couple of things: > - It attempts to rewind if the stream has already been read (past initial > setup) > - It makes a note that the stream has already been read > > B) (this situation) You want to rewind the stream if it has already been read, > but you do *not* want to mark it as read (since you don't want the next call to > getScanlines to require a rewind - it would still work, but it would be less > efficient). (Come to think of it, solving this problem will solve the earlier > problem I mentioned, where you don't want to rewind if all you've done so far is > skip.) > > I think the simplest solution will be to use a boolean in this class that keeps > track of whether you've read any data yet. Maybe fCurrentRow is enough? It > doesn't solve the consecutive calls to skipScanlines, but as I mentioned > earlier, I think that's an okay case to handle poorly. > > This method might give us the wrong behavior if someone tried to do the > following: > > SkCodec* codec = SkCodec::NewFromStream(); > SkScanlineDecoder* sd = codec->getScanlineDecoder(); > codec->getPixels(); // After this call we need to rewind > sd->getScanlines(); // If we only check our local info, rather than > rewindIfNeeded > // sd won't know to rewind. > > Maybe that's a dumb use case, although I think it is currently supported. > > The other way would be to plumb an option to rewindIfNeeded to only check if a > rewind is necessary, but not tell it the next call will require a rewind. This > would support the dumb use case (and might be more straightforward). > > That was kind of a brain dump of my thoughts on this, so I may not have > explained it well... Let me know if you want to discuss in person. And, I was confused... The way the other scanline decoders are written, you do *not* need to call handleRewind inside onGetScanlines - that is called inside getScanlineDecoder - the expectation is that you will call it again if you want to start from the beginning. But you will need to do a rewind if you get some scanlines, and then attempt to get more (even if they are not the following ones).
https://codereview.chromium.org/1194703002/diff/1/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1194703002/diff/1/dm/DMSrcSink.cpp#newcode167 dm/DMSrcSink.cpp:167: bitmap.getAddr(0, 0), decodeInfo.height(), decodeInfo.minRowBytes()); On 2015/06/19 15:45:52, scroggo wrote: > This is a little tricky: in this case it happens to be correct to use > decodeInfo.minRowBytes(), but it's not guaranteed. > > As the client of getScanlines, you're providing the pixel memory, so you know > what you used for the rowbytes. In this case, you're using memory allocated by > SkBitmap::tryAllocPixels (line 137), which initialized the bitmap with a > particular rowBytes, so you can ask the bitmap for its rowbytes: > bitmap.rowBytes(). Done. https://codereview.chromium.org/1194703002/diff/1/dm/DMSrcSink.cpp#newcode254 dm/DMSrcSink.cpp:254: subsetScanlineDecoder->getScanlines(buffer, currentSubsetHeight, rowBytes); On 2015/06/19 15:45:52, scroggo wrote: > nit: I believe this exceeds 100 characters. We try to keep each line under 100. Acknowledged. https://codereview.chromium.org/1194703002/diff/1/dm/DMSrcSink.cpp#newcode261 dm/DMSrcSink.cpp:261: "message %d", fPath.c_str(), y-1, (int) subsetResult); On 2015/06/19 15:45:52, scroggo wrote: > It probably is not useful to print the y value, since we do all the requested > rows at once. Acknowledged. https://codereview.chromium.org/1194703002/diff/1/dm/DMSrcSink.cpp#newcode268 dm/DMSrcSink.cpp:268: bufferRow += rowBytes; On 2015/06/19 15:45:52, scroggo wrote: > On 2015/06/19 13:35:54, msarett wrote: > > I have a theory that it is faster to copy from a row buffer than from an image > > sized buffer (and it's also more memory efficient). Obviously, this is not an > > option got interlaced pngs and this is a correctness test, so what you have > here > > is perfect. > > > > But this is something to keep in mind/test when we implement subset decodes in > > BitmapRegionDecoder. > > Agreed. We should confirm that theory in order to decide the best way to do it. > If it in fact is faster for non-interlaced (it's definitely slower for > interlaced), we'll need to have an API that tells you whether the image is > interlaced. (Maybe it should not specify interlacing per se, but give some kind > of hint about the best way to use it.) > > Could you add a comment explaining why we're doing all the lines at once, and > acknowledging that it may not always be the fastest way to do it (but this is a > correctness test so that's okay)? I added a comment explaining why we decode this way. I agree that if it's faster to decode non-interlaced line by line, then we will need some way to determine which method to use, based on if the image is interlaced or not. Should we add a flag which specifies the best decoding strategy, either line-by-line, or all lines at once? Even if non-interlaced images aren't faster line by line, it might be good to specify that interlaced images are much faster when decoding all lines at once. https://codereview.chromium.org/1194703002/diff/1/src/codec/SkCodec_libpng.cpp File src/codec/SkCodec_libpng.cpp (right): https://codereview.chromium.org/1194703002/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:659: fCurrentRow = 0; On 2015/06/19 15:45:53, scroggo wrote: > On 2015/06/19 13:35:54, msarett wrote: > > On 2015/06/18 21:05:41, emmaleer wrote: > > > Should these variables be defined in the cosntructor, or in > onGetScanlines()? > > > They could be defined in either place > > > > I prefer them here. For interlaced png, you probably only intend for > > onGetScanlines() to be called once, but it looks like it can be called > multiple > > times. If the variables are in the constructor, we are sure that the > > initialization only happens once. > > > > Also, I think it would be nice to use the initializer list to define them. > > For example, > > ... > > , fHasAlpha(false) > > , fCurrentRow(0) > > , fHeight(dstInfo.height()) > > ... > > is equivalent to what you wrote. > > Taking that a step further, I believe you can even put fStorageGarbage in the > initializer list, since it has a constructor that does the same thing as > reset(). It can be a little tricky though, since you want to initialize it with > the value of another member variable, and member variables will get initialized > in the order they are declared, rather than their ordering in the initializer > list. I think we have the warning turned on (as an error) that fires if the two > orderings do not match (although I don't think you'd get a warning if the > orderings did match, but you initialized them in the wrong order). > > All that said, for some member variables (i.e. those with dependencies on each > other), it might be preferable to initialize in the body of the constructor. I left fStorageGarbage in the body of the constructor, since all the other scanlineDecoder classes do this, and I wanted to keep them consistent for readability. https://codereview.chromium.org/1194703002/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:668: fCodec->handleRewind(); On 2015/06/19 15:45:53, scroggo wrote: > If handleRewind returns false (which it can for a stream that cannot be rewound, > although we will typically use it with a stream that can be), this should return > kCouldNotRewind. Done. https://codereview.chromium.org/1194703002/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:674: fStorageData.reset(count * fSrcRowBytes); On 2015/06/19 15:45:53, scroggo wrote: > On 2015/06/19 14:28:08, emmaleer wrote: > > On 2015/06/19 13:35:54, msarett wrote: > > > Maybe this belongs on the constructor as well? Do we need both fStorageData > > and > > > fBuffer to be fields? Same question for fStorageGarbage and fGarbageRow. > > > > >I can't put fStorageData in the constructor because we don't know what count > > will be until onGetScanlines is called > > >Yes we need both fStorageData and fBuffer because fStorageData is the > allocated > > memory, and fBuffer is the pointer to that memory > > >I changed the names of them to fStorage and fStoragep so it's more clear that > > one is a pointer to the other > > (FYI: If you put ">"s at the beginning of each line in your response, it looks > like it's quoting text, making it hard to distinguish between what you wrote and > what you're replying to.) > > I believe what Matt is saying is that instead of making fStorageData and fBuffer > "fields" (another word for member variables), you could just put them on the > stack, since they're only used inside this function. > > fStorageGarbage should arguably remain a field, though, since you'll use the > same one for the next call to onGetScanlines (if there is another). Good to know, I thought ">" was used for a new line. fStorage and fStoragePtr are no longer fields https://codereview.chromium.org/1194703002/diff/1/src/codec/SkCodec_libpng.cp... src/codec/SkCodec_libpng.cpp:707: fCurrentRow += count; On 2015/06/19 16:00:40, scroggo wrote: > On 2015/06/19 15:45:53, scroggo wrote: > > I think if we've already read rows, you'll want to do a rewind here as well. > (If > > we've only skipped rows previously, you do not need to rewind, but given that > > that is presumably an uncommon use case, you may not need to take it into > > consideration - but maybe add a comment that it wouldn't be necessary.) > > > > The tricky thing is that this is a little bit of a mismatch between my > previous > > thoughts on rewinding and this particular situation: > > > > A) (old thinking) The base class SkCodec has a function called rewindIfNeeded. > > It does a couple of things: > > - It attempts to rewind if the stream has already been read (past initial > > setup) > > - It makes a note that the stream has already been read > > > > B) (this situation) You want to rewind the stream if it has already been read, > > but you do *not* want to mark it as read (since you don't want the next call > to > > getScanlines to require a rewind - it would still work, but it would be less > > efficient). (Come to think of it, solving this problem will solve the earlier > > problem I mentioned, where you don't want to rewind if all you've done so far > is > > skip.) > > > > I think the simplest solution will be to use a boolean in this class that > keeps > > track of whether you've read any data yet. Maybe fCurrentRow is enough? It > > doesn't solve the consecutive calls to skipScanlines, but as I mentioned > > earlier, I think that's an okay case to handle poorly. > > > > This method might give us the wrong behavior if someone tried to do the > > following: > > > > SkCodec* codec = SkCodec::NewFromStream(); > > SkScanlineDecoder* sd = codec->getScanlineDecoder(); > > codec->getPixels(); // After this call we need to rewind > > sd->getScanlines(); // If we only check our local info, rather than > > rewindIfNeeded > > // sd won't know to rewind. > > > > Maybe that's a dumb use case, although I think it is currently supported. > > > > The other way would be to plumb an option to rewindIfNeeded to only check if a > > rewind is necessary, but not tell it the next call will require a rewind. This > > would support the dumb use case (and might be more straightforward). > > > > That was kind of a brain dump of my thoughts on this, so I may not have > > explained it well... Let me know if you want to discuss in person. > > And, I was confused... > > The way the other scanline decoders are written, you do *not* need to call > handleRewind inside onGetScanlines - that is called inside getScanlineDecoder - > the expectation is that you will call it again if you want to start from the > beginning. > > But you will need to do a rewind if you get some scanlines, and then attempt to > get more (even if they are not the following ones). I added the boolean fRewindNeeded to keep track if we've called onGetScanlines before and need to rewind https://codereview.chromium.org/1194703002/diff/20001/src/codec/SkCodec_libpn... File src/codec/SkCodec_libpng.cpp (right): https://codereview.chromium.org/1194703002/diff/20001/src/codec/SkCodec_libpn... src/codec/SkCodec_libpng.cpp:724: uint8_t* fStoragep; On 2015/06/19 15:45:53, scroggo wrote: > nit: In Skia, I think we would typically name this fStoragePtr (which of course, > differs from other conventions...) Acknowledged.
https://codereview.chromium.org/1194703002/diff/40001/src/codec/SkCodec_libpn... File src/codec/SkCodec_libpng.cpp (right): https://codereview.chromium.org/1194703002/diff/40001/src/codec/SkCodec_libpn... src/codec/SkCodec_libpng.cpp:675: fRewindNeeded = true; You'll still need to set this to rewind next time if we did not need to rewind this time, right? We should probably make sure we test that rewinding works for an interlaced png. Do we have such an image checked in to resources? tests/CodexTest.cpp already tests the scanline decoder after a rewind - although it doesn't test the case we're concerned about here: - call getScanlines(n) - call getScanlines again (It sort of does, in the extra slow way - it does one scanline at a time....) Come to think of it, this should be covered by the striping test that Matt added to DM, so long as we have an interlaced PNG in the Google Storage bucket, which stores images we test (I don't think we've told you about that, so I'll dig up again the info about it...)
lgtm https://codereview.chromium.org/1194703002/diff/40001/src/codec/SkCodec_libpn... File src/codec/SkCodec_libpng.cpp (right): https://codereview.chromium.org/1194703002/diff/40001/src/codec/SkCodec_libpn... src/codec/SkCodec_libpng.cpp:675: fRewindNeeded = true; On 2015/06/22 14:46:25, scroggo wrote: > You'll still need to set this to rewind next time if we did not need to rewind > this time, right? Nvm. As discussed in person, this should work as is.
The CQ bit was checked by emmaleer@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1194703002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
casting int to size_t in onGetScanlines, as it caused a warning when I tried to commit
On 2015/06/22 16:57:21, emmaleer wrote: > casting int to size_t in onGetScanlines, as it caused a warning when I tried to > commit still lgtm. Feel free to commit
The CQ bit was checked by emmaleer@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1194703002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by emmaleer@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/1194703002/#ps80001 (title: "fixing size_t to int conversion warning")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1194703002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/0a4c3cbfd77e11901c66ac29bf1417a42b87fd31 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
