Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1305123002:
Scanline decoding for gifs (Closed)
DescriptionScanline decoding for gifs
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/e9c10b9121887e8c300bd41357461418e061984d
Committed: https://skia.googlesource.com/skia/+/10522ff4cfa3cba45881354768f6185fc1109119
Patch Set 1 : #
Total comments: 41
Patch Set 2 : First round of fixes #
Total comments: 1
Patch Set 3 : Rebase #Patch Set 4 : For review #Patch Set 5 : Rebase - moved get_scaled_dimension from SkScaledCodec.cpp to SkCodecPriv.h #Patch Set 6 : Response to comments #Patch Set 7 : Move copy_color_table into initializeColorTable #Patch Set 8 : Add comments and TODOs in order to punt the reallyHasAlpha issue to sometime later #Patch Set 9 : Rebase on dm changes #Patch Set 10 : Make kScanline_Mode test kOutOfOrder correctly in dm #
Total comments: 29
Patch Set 11 : Rebase #Patch Set 12 : Read up to the first image in ReadHeader() #
Total comments: 4
Patch Set 13 : Add better comments for fTransIndex #Patch Set 14 : Compile fixes #Patch Set 15 : Compile fix #
Depends on Patchset: Messages
Total messages: 35 (15 generated)
Patchset #1 (id:1) has been deleted
https://codereview.chromium.org/1305123002/diff/20001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1305123002/diff/20001/dm/DMSrcSink.cpp#newcode93 dm/DMSrcSink.cpp:93: if (SkScanlineDecoder::kTopDown_SkScanlineOrder != scanlineDecoder->getScanlineOrder()) { In gif we don't know the scanline order until we call onStart(). This is at least worth documenting, but maybe is bad enough to consider moving more of this gif logic into ReadHeader? If we move readUpToFirstImage() and setFrameDims() to ReadHeader(), that will solve this problem. https://codereview.chromium.org/1305123002/diff/20001/include/codec/SkScanlin... File include/codec/SkScanlineDecoder.h (right): https://codereview.chromium.org/1305123002/diff/20001/include/codec/SkScanlin... include/codec/SkScanlineDecoder.h:234: virtual int onGetY() { return fCurrScanline; } The current implementation of SkGifScanlineDecoder calls the gif iterator, so this cannot be const. A possible alternate solution would be to, instead of using an iterator, create a function that takes a fCurrScanline and fHeight, and returns Y. I'm not sure of the difficulty/performance impact of this vs using the iterator. https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodecPriv.h File src/codec/SkCodecPriv.h (right): https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodecPriv.h... src/codec/SkCodecPriv.h:187: #ifndef SK_PRINT_CODEC_MESSAGES For debugging, will fix. https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:101: /* These have simply been rearranged. https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:201: , fSrcBuffer(SkNEW_ARRAY(uint8_t, this->getInfo().width())) This is a behavior change. We will allocate extra memory when the image frame width is less than the image header width. This is rare. https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:220: SkCodec::Result SkGifCodec::readUpToFirstImage(uint32_t* transIndex) { Code moved from original onGetPixels(). We now call find_trans_index() sooner, and this is safe. https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:312: bool SkGifCodec::setFrameDimensions(const GifImageDesc& desc) { Code moved from the original onGetPixels(). fFrameDims and fFrameIsSubset are new. https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:358: void SkGifCodec::initializeColorTable(int* inputColorCount, uint32_t transIndex) { Code moved from the original onGetPixels(). https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:380: SkASSERT(colorCount <= 256); Not sure if we should add a Release check here. giflib is guaranteed to give us a number <= 256 (I've looked at their code), but maybe we don't want to depend on that? https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:474: SkASSERT(fGif->ImageCount >= 1); Looks like we also rely on giflib behaving properly here. If we need a Release check in the other case, we probably also need one here. https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:604: fIter = iter; This is unfortunate. We don't know fCodec->fFrameDims.height() in the constructor so we can't initialize fIter until we get here. And (unless my C++ knowledge is letting me down), we need a copy constructor to perform this assignment. And if we want to use the default copy constructor, SkGifInterlaceIter can't have a const field. https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgif.h File src/codec/SkCodec_libgif.h (right): https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.h:101: * bounds. Will improve this comment. https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkGifInterlac... File src/codec/SkGifInterlaceIter.h (right): https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkGifInterlac... src/codec/SkGifInterlaceIter.h:13: class SkGifInterlaceIter { I think the scanline decoder needs this to be copyable. https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkScaledCodec... File src/codec/SkScaledCodec.cpp (right): https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkScaledCodec... src/codec/SkScaledCodec.cpp:222: return fScanlineDecoder->getScanlines(dst, requestedInfo.height(), rowBytes); I need to add a switch statement here. Interlaced gifs currently decode incorrectly when scale = 1.0f because kOutOfOrder scanline decodes are meant to occur one at a time. If we were to combine SkCodec and SkScanlineDecoder, we could call onGetPixels() here.
Patchset #2 (id:40001) has been deleted
msarett@google.com changed reviewers: + djsollen@google.com, scroggo@google.com
This is ready as well. I think there are a few redesign options to consider that I've mentioned in comments. https://codereview.chromium.org/1305123002/diff/60001/src/codec/SkCodecPriv.h File src/codec/SkCodecPriv.h (right): https://codereview.chromium.org/1305123002/diff/60001/src/codec/SkCodecPriv.h... src/codec/SkCodecPriv.h:187: #ifndef SK_PRINT_CODEC_MESSAGES Will fix.
https://codereview.chromium.org/1305123002/diff/20001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1305123002/diff/20001/dm/DMSrcSink.cpp#newcode90 dm/DMSrcSink.cpp:90: if (SkCodec::kSuccess != scanlineDecoder->start(info, NULL, colorPtr, colorCountPtr)) { Why did this move? Also, it seems like the comment applies to the condition below this? https://codereview.chromium.org/1305123002/diff/20001/dm/DMSrcSink.cpp#newcode93 dm/DMSrcSink.cpp:93: if (SkScanlineDecoder::kTopDown_SkScanlineOrder != scanlineDecoder->getScanlineOrder()) { On 2015/08/24 23:20:12, msarett wrote: > In gif we don't know the scanline order until we call onStart(). This is at > least worth documenting, but maybe is bad enough to consider moving more of this > gif logic into ReadHeader? > > If we move readUpToFirstImage() and setFrameDims() to ReadHeader(), that will > solve this problem. We spoke about this a little in person, but I'll add my thoughts here for posterity. The downside to moving more information into ReadHeader is that it will take longer to find out the size of the image. For a lazy client that wants the size for layout, they'll have to wait longer (not sure how much longer, which should perhaps also be considered). This makes me wonder whether it's appropriate that we specify any alpha type after only reading the header. Or perhaps we should specify kUnknown_SkAlphaType? There are a couple of good reasons to specify alpha type: - it allows the client to know whether they can use 565 - it allows the client to ask for the cheaper premul option (i.e. if the client does not care whether the colors are premultiplied, they should prefer doing less work) Okay, I've convinced myself that if we do not move the color table decode into ReadHeader, we should still report kUnpremul. What we probably really want is be able to ask the decoder afterwords (or in the call to getPixels?) what the final alpha type was. We already have that in reallyHasAlpha, although we've been discussing whether it's necessary (skbug.com/3582), and maybe there is a cleaner way to do reallyHasAlpha. https://codereview.chromium.org/1305123002/diff/20001/include/codec/SkScanlin... File include/codec/SkScanlineDecoder.h (right): https://codereview.chromium.org/1305123002/diff/20001/include/codec/SkScanlin... include/codec/SkScanlineDecoder.h:234: virtual int onGetY() { return fCurrScanline; } On 2015/08/24 23:20:12, msarett wrote: > The current implementation of SkGifScanlineDecoder calls the gif iterator, so > this cannot be const. FWIW, it does seem a little odd that it is not const, but getScanlines is not const, so the typical user will not have a const SkScanlineDecoder anyway. > > A possible alternate solution would be to, instead of using an iterator, create > a function that takes a fCurrScanline and fHeight, and returns Y. I'm not sure > of the difficulty/performance impact of this vs using the iterator. https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:201: , fSrcBuffer(SkNEW_ARRAY(uint8_t, this->getInfo().width())) On 2015/08/24 23:20:12, msarett wrote: > This is a behavior change. We will allocate extra memory when the image frame > width is less than the image header width. This is rare. Is this used in onGetPixels, or just in scanline decoding mode? https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:298: // This should not be reached. So DGifGetRecordType would have returned GIF_ERROR before we reach this switch statement? https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:380: SkASSERT(colorCount <= 256); On 2015/08/24 23:20:13, msarett wrote: > Not sure if we should add a Release check here. giflib is guaranteed to give us > a number <= 256 (I've looked at their code), but maybe we don't want to depend > on that? I think we should trust giflib in this case. I cannot imagine why that would change. I think we have a Release assert, if you'd like to add that: SK_ALWAYSBREAK (it makes you hit a breakpoint in debug mode). https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:604: fIter = iter; On 2015/08/24 23:20:13, msarett wrote: > This is unfortunate. We don't know fCodec->fFrameDims.height() in the > constructor so we can't initialize fIter until we get here. What is the problem? It seems like the constructors are fairly cheap (a few assignments). > > And (unless my C++ knowledge is letting me down), we need a copy constructor to > perform this assignment. We actually need an operator= function. > And if we want to use the default copy constructor, > SkGifInterlaceIter can't have a const field. Ah, so here's the problem - you had to make its fields no longer const and make it no longer noncopyable. I think that's fine. Alternatively, you could introduce a new method: reset(int height) You could even make it so the constructor does nothing, and reset() needs to be called before you use it (we have other classes/structs that behave this way). Then you would skip the step of setting the variables with a -1 height. https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:617: if (SkCodec::kSuccess != fCodec->initializeSwizzler(subsetDstInfo, opts.fZeroInitialized)) { nit: line too long https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:633: SkSwizzler::Fill(dst, this->dstInfo(), rowBytes, count, fCodec->fFillIndex, colorPtr, this->options().fZeroInitialized); nit: line too long https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:649: dst = SkTAddOffset<void>(dst, SkColorTypeBytesPerPixel(this->dstInfo().colorType()) * fCodec->fFrameDims.left()); nit line too long https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:676: return fIter.nextY(); This seems a little weird - it seems like we're depending on the client to call getY() in order for fIter to advance? Maybe that's a reasonable requirement. If we want getY() to remain const, we could split up SkGifIterator::nextY() into two functions: one that returns the next, and another that advances it. (It is already partially split - we can make prepareY() public, and change nextY() to no longer call it. (Maybe we'll want different names for them.) https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgif.h File src/codec/SkCodec_libgif.h (right): https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.h:99: * Sets inputColorCount to 256. Since gifs always contain 8-bit indices, It seems weird that we always set this parameter to 256. Do we need that to be in this function? I guess all places where we need to return this to the caller share this function? In that case, maybe we should go all the way, and have this function also call copy_color_table? https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.h:117: * @return kSuccess if the read is successful and kIncompleteInput is the if* https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkGifInterlac... File src/codec/SkGifInterlaceIter.h (right): https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkGifInterlac... src/codec/SkGifInterlaceIter.h:13: class SkGifInterlaceIter { On 2015/08/24 23:20:13, msarett wrote: > I think the scanline decoder needs this to be copyable. That is correct.
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:140001) has been deleted
Patchset #6 (id:180001) has been deleted
There's not a lot new going on here - some of the fixes were made preemptively in patch set 4. I think the major decision is whether to add more complexity to ReadHeader(). https://codereview.chromium.org/1305123002/diff/20001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1305123002/diff/20001/dm/DMSrcSink.cpp#newcode90 dm/DMSrcSink.cpp:90: if (SkCodec::kSuccess != scanlineDecoder->start(info, NULL, colorPtr, colorCountPtr)) { On 2015/08/27 16:35:49, scroggo wrote: > Why did this move? Also, it seems like the comment applies to the condition > below this? This moved because gifs don't know their SkScanlineOrder until after calling start(). I moved the comment back into the proper position in the rebase. https://codereview.chromium.org/1305123002/diff/20001/dm/DMSrcSink.cpp#newcode93 dm/DMSrcSink.cpp:93: if (SkScanlineDecoder::kTopDown_SkScanlineOrder != scanlineDecoder->getScanlineOrder()) { On 2015/08/27 16:35:49, scroggo wrote: > On 2015/08/24 23:20:12, msarett wrote: > > In gif we don't know the scanline order until we call onStart(). This is at > > least worth documenting, but maybe is bad enough to consider moving more of > this > > gif logic into ReadHeader? > > > > If we move readUpToFirstImage() and setFrameDims() to ReadHeader(), that will > > solve this problem. > > We spoke about this a little in person, but I'll add my thoughts here for > posterity. > > The downside to moving more information into ReadHeader is that it will take > longer to find out the size of the image. For a lazy client that wants the size > for layout, they'll have to wait longer (not sure how much longer, which should > perhaps also be considered). > > This makes me wonder whether it's appropriate that we specify any alpha type > after only reading the header. Or perhaps we should specify > kUnknown_SkAlphaType? There are a couple of good reasons to specify alpha type: > - it allows the client to know whether they can use 565 > - it allows the client to ask for the cheaper premul option (i.e. if the client > does not care whether the colors are premultiplied, they should prefer doing > less work) > > Okay, I've convinced myself that if we do not move the color table decode into > ReadHeader, we should still report kUnpremul. What we probably really want is be > able to ask the decoder afterwords (or in the call to getPixels?) what the final > alpha type was. We already have that in reallyHasAlpha, although we've been > discussing whether it's necessary (skbug.com/3582), and maybe there is a cleaner > way to do reallyHasAlpha. Just want to clarify that there are two different issues here: (1) We don't know if the image has alpha after reading the header. (2) We don't know SkScanlineOrder after reading the header. Also, my discussion of what we need to move into ReadHeader() from above is wrong. Currently, ReadHeader() calls a giflib function that does 3 things. (a) Reads signature. (b) Reads header info. (c) Reads global color table if it exists. For (1), we can find out if there is a transparent index by moving readUpToFirstImage() into ReadHeader(). readUpToFirstImage() should be lightweight, it only reads extension blocks. There can 0 to infinity extension blocks, but normally there ought to be 0 or 1. We know that the image is definitely opaque if there is no transparent index. However, you could make the argument that even if there is a transparent index, we should still use reallyHasAlpha to make sure that the transparent index is actually used. For (2), we need to move readUpToFirstImage() and DGifGetImageDesc() to ReadHeader(). DGifGetImageDesc() is lightweight - it reads ten bytes. I think it's best to decide this in person, let's discuss. https://codereview.chromium.org/1305123002/diff/20001/include/codec/SkScanlin... File include/codec/SkScanlineDecoder.h (right): https://codereview.chromium.org/1305123002/diff/20001/include/codec/SkScanlin... include/codec/SkScanlineDecoder.h:234: virtual int onGetY() { return fCurrScanline; } On 2015/08/27 16:35:49, scroggo wrote: > On 2015/08/24 23:20:12, msarett wrote: > > The current implementation of SkGifScanlineDecoder calls the gif iterator, so > > this cannot be const. > > FWIW, it does seem a little odd that it is not const, but getScanlines is not > const, so the typical user will not have a const SkScanlineDecoder anyway. > > > > > A possible alternate solution would be to, instead of using an iterator, > create > > a function that takes a fCurrScanline and fHeight, and returns Y. I'm not > sure > > of the difficulty/performance impact of this vs using the iterator. > Acknowledged. https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:201: , fSrcBuffer(SkNEW_ARRAY(uint8_t, this->getInfo().width())) On 2015/08/27 16:35:49, scroggo wrote: > On 2015/08/24 23:20:12, msarett wrote: > > This is a behavior change. We will allocate extra memory when the image frame > > width is less than the image header width. This is rare. > > Is this used in onGetPixels, or just in scanline decoding mode? This is shared by both. https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:298: // This should not be reached. On 2015/08/27 16:35:49, scroggo wrote: > So DGifGetRecordType would have returned GIF_ERROR before we reach this switch > statement? Yes I'll improve the comment. https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:604: fIter = iter; On 2015/08/27 16:35:49, scroggo wrote: > On 2015/08/24 23:20:13, msarett wrote: > > This is unfortunate. We don't know fCodec->fFrameDims.height() in the > > constructor so we can't initialize fIter until we get here. > > What is the problem? It seems like the constructors are fairly cheap (a few > assignments). > > > > And (unless my C++ knowledge is letting me down), we need a copy constructor > to > > perform this assignment. > > We actually need an operator= function. > > > And if we want to use the default copy constructor, > > SkGifInterlaceIter can't have a const field. > > Ah, so here's the problem - you had to make its fields no longer const and make > it no longer noncopyable. I think that's fine. > > Alternatively, you could introduce a new method: > > reset(int height) > > You could even make it so the constructor does nothing, and reset() needs to be > called before you use it (we have other classes/structs that behave this way). > Then you would skip the step of setting the variables with a -1 height. This was fixed in the rebase - we now deal with interlaced gifs without an iterator. https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:617: if (SkCodec::kSuccess != fCodec->initializeSwizzler(subsetDstInfo, opts.fZeroInitialized)) { On 2015/08/27 16:35:49, scroggo wrote: > nit: line too long Done. https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:633: SkSwizzler::Fill(dst, this->dstInfo(), rowBytes, count, fCodec->fFillIndex, colorPtr, this->options().fZeroInitialized); On 2015/08/27 16:35:49, scroggo wrote: > nit: line too long Done. https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:649: dst = SkTAddOffset<void>(dst, SkColorTypeBytesPerPixel(this->dstInfo().colorType()) * fCodec->fFrameDims.left()); On 2015/08/27 16:35:49, scroggo wrote: > nit line too long Done. https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.cpp:676: return fIter.nextY(); On 2015/08/27 16:35:49, scroggo wrote: > This seems a little weird - it seems like we're depending on the client to call > getY() in order for fIter to advance? Maybe that's a reasonable requirement. > > If we want getY() to remain const, we could split up SkGifIterator::nextY() into > two functions: one that returns the next, and another that advances it. (It is > already partially split - we can make prepareY() public, and change nextY() to > no longer call it. (Maybe we'll want different names for them.) Acknowledged. We may end up using this approach for performance reasons. For now, we use an alternative to the iterator. https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgif.h File src/codec/SkCodec_libgif.h (right): https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.h:117: * @return kSuccess if the read is successful and kIncompleteInput is the On 2015/08/27 16:35:50, scroggo wrote: > if* Done.
https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgif.h File src/codec/SkCodec_libgif.h (right): https://codereview.chromium.org/1305123002/diff/20001/src/codec/SkCodec_libgi... src/codec/SkCodec_libgif.h:99: * Sets inputColorCount to 256. Since gifs always contain 8-bit indices, On 2015/08/27 16:35:50, scroggo wrote: > It seems weird that we always set this parameter to 256. Do we need that to be > in this function? I guess all places where we need to return this to the caller > share this function? In that case, maybe we should go all the way, and have this > function also call copy_color_table? Sorry forgot this comment. Moving copy_color_table into initializeColorTable, which I think makes sense.
https://codereview.chromium.org/1305123002/diff/20001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1305123002/diff/20001/dm/DMSrcSink.cpp#newcode93 dm/DMSrcSink.cpp:93: if (SkScanlineDecoder::kTopDown_SkScanlineOrder != scanlineDecoder->getScanlineOrder()) { On 2015/09/01 17:50:15, msarett wrote: > On 2015/08/27 16:35:49, scroggo wrote: > > On 2015/08/24 23:20:12, msarett wrote: > > > In gif we don't know the scanline order until we call onStart(). This is at > > > least worth documenting, but maybe is bad enough to consider moving more of > > this > > > gif logic into ReadHeader? > > > > > > If we move readUpToFirstImage() and setFrameDims() to ReadHeader(), that > will > > > solve this problem. > > > > We spoke about this a little in person, but I'll add my thoughts here for > > posterity. > > > > The downside to moving more information into ReadHeader is that it will take > > longer to find out the size of the image. For a lazy client that wants the > size > > for layout, they'll have to wait longer (not sure how much longer, which > should > > perhaps also be considered). > > > > This makes me wonder whether it's appropriate that we specify any alpha type > > after only reading the header. Or perhaps we should specify > > kUnknown_SkAlphaType? There are a couple of good reasons to specify alpha > type: > > - it allows the client to know whether they can use 565 > > - it allows the client to ask for the cheaper premul option (i.e. if the > client > > does not care whether the colors are premultiplied, they should prefer doing > > less work) > > > > Okay, I've convinced myself that if we do not move the color table decode into > > ReadHeader, we should still report kUnpremul. What we probably really want is > be > > able to ask the decoder afterwords (or in the call to getPixels?) what the > final > > alpha type was. We already have that in reallyHasAlpha, although we've been > > discussing whether it's necessary (skbug.com/3582), and maybe there is a > cleaner > > way to do reallyHasAlpha. > > Just want to clarify that there are two different issues here: > (1) We don't know if the image has alpha after reading the header. > (2) We don't know SkScanlineOrder after reading the header. > > Also, my discussion of what we need to move into ReadHeader() from above is > wrong. Currently, ReadHeader() calls a giflib function that does 3 things. > (a) Reads signature. > (b) Reads header info. > (c) Reads global color table if it exists. > > For (1), we can find out if there is a transparent index by moving > readUpToFirstImage() into ReadHeader(). readUpToFirstImage() should be > lightweight, it only reads extension blocks. There can 0 to infinity extension > blocks, but normally there ought to be 0 or 1. > > We know that the image is definitely opaque if there is no transparent index. > However, you could make the argument that even if there is a transparent index, > we should still use reallyHasAlpha to make sure that the transparent index is > actually used. > > For (2), we need to move readUpToFirstImage() and DGifGetImageDesc() to > ReadHeader(). DGifGetImageDesc() is lightweight - it reads ten bytes. > > I think it's best to decide this in person, let's discuss. This issue isn't critical right now. We'll keep things as is and revisit further down the road.
https://codereview.chromium.org/1305123002/diff/280001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1305123002/diff/280001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:211: if (SkCodec::kSuccess != result && SkCodec::kIncompleteInput != result) { The weird thing about allowing incomplete here is that we will have decoded some scanlines, but left some uninitialized. https://codereview.chromium.org/1305123002/diff/280001/include/codec/SkScanli... File include/codec/SkScanlineDecoder.h (right): https://codereview.chromium.org/1305123002/diff/280001/include/codec/SkScanli... include/codec/SkScanlineDecoder.h:219: * called start(). Specifically, we do not know if gifs are interlaced From your comments elsewhere, it sounded like it should be fine to move that check into ReadHeader? I don't like that this sometimes gives the wrong order. Alternatively, calling this function could read enough of the encoded image to determine the order. https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkCodecPriv.h File src/codec/SkCodecPriv.h (right): https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkCodecPriv.... src/codec/SkCodecPriv.h:37: static int get_scaled_dimension(int srcDimension, int sampleSize) { It seems weird to me that we need this for gif, but did not need it for others. Any idea why? https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkCodec_libg... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:219: , fSrcBuffer(SkNEW_ARRAY(uint8_t, this->getInfo().width())) nit: new https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:220: , fFillIndex(SK_MaxU32) Is this a sentinel value? Maybe it should be labelled for what it means? https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:366: if (this->getInfo().width() != fFrameDims.width() || I think you can simplify this to if (fFrameDims.size() != this->getInfo().dimensions()) { https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:429: fColorTable.reset(SkNEW_ARGS(SkColorTable, (colorPtr, maxColors))); nit: new https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:439: if (NULL != fSwizzler.get()) { nit: nullptr https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:532: // the output image. We use an iterator to take care of no longer use an iterator https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:604: // Read through gif extensions to get to the image data A lot of this code looks similar to code in onGetPixels. I wonder if we can share more code? https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:699: return get_output_row_interlaced(INHERITED::onGetY(), this->dstInfo().height()); nit: extra space here https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkScaledCode... File src/codec/SkScaledCodec.cpp (right): https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkScaledCode... src/codec/SkScaledCodec.cpp:201: if (kSuccess != result && kIncompleteInput != result) { Again, I am concerned about accepting kIncomplete here. (And then returning kSuccess!)
https://codereview.chromium.org/1305123002/diff/280001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1305123002/diff/280001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:211: if (SkCodec::kSuccess != result && SkCodec::kIncompleteInput != result) { On 2015/09/02 22:45:47, scroggo wrote: > The weird thing about allowing incomplete here is that we will have decoded some > scanlines, but left some uninitialized. Actually the reason that we allow kIncomplete here is to avoid having uninitialized scanlines. We will keep going even if we have reached the end of the image stream - that way all of the remaining scanlines will be filled by the scanline decoder individually. I don't think my refactor of the Fill() in a WIP CL will give us a better solution than this. But maybe it's not so bad to have this code in the dm test? https://codereview.chromium.org/1305123002/diff/280001/include/codec/SkScanli... File include/codec/SkScanlineDecoder.h (right): https://codereview.chromium.org/1305123002/diff/280001/include/codec/SkScanli... include/codec/SkScanlineDecoder.h:219: * called start(). Specifically, we do not know if gifs are interlaced On 2015/09/02 22:45:48, scroggo wrote: > From your comments elsewhere, it sounded like it should be fine to move that > check into ReadHeader? I don't like that this sometimes gives the wrong order. > > Alternatively, calling this function could read enough of the encoded image to > determine the order. I'm happy add more to ReadHeader(). I agree that this is weird. I had thought we had decided in person to punt this issue down the road, but I'll make the fix. FYI, this fix will also help with guessing the alpha type. https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkCodecPriv.h File src/codec/SkCodecPriv.h (right): https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkCodecPriv.... src/codec/SkCodecPriv.h:37: static int get_scaled_dimension(int srcDimension, int sampleSize) { On 2015/09/02 22:45:48, scroggo wrote: > It seems weird to me that we need this for gif, but did not need it for others. > Any idea why? We need it for gif because each image frame in a gif may have different dimensions than the image size specified in the header. And we need to know the scaled size of the "image frame". We actually know the frameDims in ReadHeader (now that I have moved DGifGetImageDesc() to the header), but I don't think it makes sense to make it visible outside of SkGifCodec. https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkCodec_libg... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:219: , fSrcBuffer(SkNEW_ARRAY(uint8_t, this->getInfo().width())) On 2015/09/02 22:45:48, scroggo wrote: > nit: new Done. https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:220: , fFillIndex(SK_MaxU32) On 2015/09/02 22:45:48, scroggo wrote: > Is this a sentinel value? Maybe it should be labelled for what it means? This has changed a bit. Will add comments here. https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:366: if (this->getInfo().width() != fFrameDims.width() || On 2015/09/02 22:45:48, scroggo wrote: > I think you can simplify this to > > if (fFrameDims.size() != this->getInfo().dimensions()) { +1 https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:429: fColorTable.reset(SkNEW_ARGS(SkColorTable, (colorPtr, maxColors))); On 2015/09/02 22:45:48, scroggo wrote: > nit: new Done. https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:439: if (NULL != fSwizzler.get()) { On 2015/09/02 22:45:48, scroggo wrote: > nit: nullptr Done. https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:532: // the output image. We use an iterator to take care of On 2015/09/02 22:45:48, scroggo wrote: > no longer use an iterator Thanks, will change the comment. https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:604: // Read through gif extensions to get to the image data On 2015/09/02 22:45:48, scroggo wrote: > A lot of this code looks similar to code in onGetPixels. I wonder if we can > share more code? This has been moved to ReadHeader(), so now it's shared! There is more that we can share as well, doing this too. https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:699: return get_output_row_interlaced(INHERITED::onGetY(), this->dstInfo().height()); On 2015/09/02 22:45:48, scroggo wrote: > nit: extra space here Done. https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkScaledCode... File src/codec/SkScaledCodec.cpp (right): https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkScaledCode... src/codec/SkScaledCodec.cpp:201: if (kSuccess != result && kIncompleteInput != result) { On 2015/09/02 22:45:48, scroggo wrote: > Again, I am concerned about accepting kIncomplete here. (And then returning > kSuccess!) I would agree that we should return result at the bottom instead of kSuccess. This has been fixed. I also agree that continuing to try to getScanlines() after kIncomplete has already been returned is weird. Right now, we do this to avoid leaving uninitialized memory in the dst. My goal is to deal with this by filling uninitialized memory in the SkCodec base class implementation of onGetPixels() when kIncompleteInput is returned by a subclass. I'm adding a FIXME, but I think it's best to keep this for another CL. Let me know if you think this is ok as a temporary solution? It's worth noting that we are already using this temporary solution further down in this function - Doesn't make it any better, but it's something that already needs to be fixed. Side Note: Looks like it might be useful/necessary for the SkCodec base class to know the SkScanlineOrder in order to Fill(). Another argument for combining the two classes.
lgtm https://codereview.chromium.org/1305123002/diff/280001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1305123002/diff/280001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:211: if (SkCodec::kSuccess != result && SkCodec::kIncompleteInput != result) { On 2015/09/03 17:13:30, msarett wrote: > On 2015/09/02 22:45:47, scroggo wrote: > > The weird thing about allowing incomplete here is that we will have decoded > some > > scanlines, but left some uninitialized. > > Actually the reason that we allow kIncomplete here is to avoid having > uninitialized scanlines. We will keep going even if we have reached the end of > the image stream - that way all of the remaining scanlines will be filled by the > scanline decoder individually. Wait, so you're saying that the scanline decoder will return kIncomplete, but then the client will continue to call getScanlines? And those calls will fill, but also return kIncomplete, so the client can continue to fill the rest? That's interesting. Maybe it's the right thing to do, but it's not what I expected. > > I don't think my refactor of the Fill() in a WIP CL will give us a better > solution than this. But maybe it's not so bad to have this code in the dm test? When you say "this code", what do you mean? The code that's already here? https://codereview.chromium.org/1305123002/diff/280001/include/codec/SkScanli... File include/codec/SkScanlineDecoder.h (right): https://codereview.chromium.org/1305123002/diff/280001/include/codec/SkScanli... include/codec/SkScanlineDecoder.h:219: * called start(). Specifically, we do not know if gifs are interlaced On 2015/09/03 17:13:30, msarett wrote: > On 2015/09/02 22:45:48, scroggo wrote: > > From your comments elsewhere, it sounded like it should be fine to move that > > check into ReadHeader? I don't like that this sometimes gives the wrong order. > > > > Alternatively, calling this function could read enough of the encoded image to > > determine the order. > > I'm happy add more to ReadHeader(). I agree that this is weird. I had thought > we had decided in person to punt this issue down the road, but I'll make the > fix. We might have decided on that, but I think I did not fully understand. > > FYI, this fix will also help with guessing the alpha type. Great! https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkScaledCode... File src/codec/SkScaledCodec.cpp (right): https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkScaledCode... src/codec/SkScaledCodec.cpp:201: if (kSuccess != result && kIncompleteInput != result) { On 2015/09/03 17:13:31, msarett wrote: > On 2015/09/02 22:45:48, scroggo wrote: > > Again, I am concerned about accepting kIncomplete here. (And then returning > > kSuccess!) > > I would agree that we should return result at the bottom instead of kSuccess. > This has been fixed. > > I also agree that continuing to try to getScanlines() after kIncomplete has > already been returned is weird. Right now, we do this to avoid leaving > uninitialized memory in the dst. My goal is to deal with this by filling > uninitialized memory in the SkCodec base class implementation of onGetPixels() > when kIncompleteInput is returned by a subclass. > > I'm adding a FIXME, but I think it's best to keep this for another CL. Let me > know if you think this is ok as a temporary solution? It's worth noting that we > are already using this temporary solution further down in this function - > Doesn't make it any better, but it's something that already needs to be fixed. I'm happy with a FIXME. I think I understand what's going on here based on your comments there. As I said there, I think this is an interesting but strange behavior. Maybe it's right though. > > Side Note: > Looks like it might be useful/necessary for the SkCodec base class to know the > SkScanlineOrder in order to Fill(). Another argument for combining the two > classes. https://codereview.chromium.org/1305123002/diff/320001/src/codec/SkCodec_libg... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1305123002/diff/320001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:199: // there might be a valid transparent index, we must indicate that the image has What do you mean there "might be a valid transparent index". It seems like it is either < 256 (valid, even if it's ultimately unused) or >= 256 (invalid), and we already know that at this point. Is there something I'm missing? Ok, I figured this out - a transIndex of < 256 might still be > actual number of real colors in the color table. A comment explaining that would be nice. https://codereview.chromium.org/1305123002/diff/320001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:241: // since we don't know if fTransIndex is valid until we process the color table. ... because fTransIndex may be > the size of the color table.
https://codereview.chromium.org/1305123002/diff/280001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/1305123002/diff/280001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:211: if (SkCodec::kSuccess != result && SkCodec::kIncompleteInput != result) { On 2015/09/04 18:42:09, scroggo wrote: > On 2015/09/03 17:13:30, msarett wrote: > > On 2015/09/02 22:45:47, scroggo wrote: > > > The weird thing about allowing incomplete here is that we will have decoded > > some > > > scanlines, but left some uninitialized. > > > > Actually the reason that we allow kIncomplete here is to avoid having > > uninitialized scanlines. We will keep going even if we have reached the end > of > > the image stream - that way all of the remaining scanlines will be filled by > the > > scanline decoder individually. > > Wait, so you're saying that the scanline decoder will return kIncomplete, but > then the client will continue to call getScanlines? And those calls will fill, > but also return kIncomplete, so the client can continue to fill the rest? That's > interesting. Maybe it's the right thing to do, but it's not what I expected. > Yes that's what it does. > > > > I don't think my refactor of the Fill() in a WIP CL will give us a better > > solution than this. But maybe it's not so bad to have this code in the dm > test? > > When you say "this code", what do you mean? The code that's already here? > Yes that's what I mean. In SkScaledCodec, we use the scanline decoder in a call to onGetPixels(). Ideally, maybe the base class version of getPixels() would handle filling if onGetPixels() returns kIncomplete. Here, I'm not sure of a better strategy than keeping the code as is. https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkScaledCode... File src/codec/SkScaledCodec.cpp (right): https://codereview.chromium.org/1305123002/diff/280001/src/codec/SkScaledCode... src/codec/SkScaledCodec.cpp:201: if (kSuccess != result && kIncompleteInput != result) { On 2015/09/04 18:42:09, scroggo wrote: > On 2015/09/03 17:13:31, msarett wrote: > > On 2015/09/02 22:45:48, scroggo wrote: > > > Again, I am concerned about accepting kIncomplete here. (And then returning > > > kSuccess!) > > > > I would agree that we should return result at the bottom instead of kSuccess. > > This has been fixed. > > > > I also agree that continuing to try to getScanlines() after kIncomplete has > > already been returned is weird. Right now, we do this to avoid leaving > > uninitialized memory in the dst. My goal is to deal with this by filling > > uninitialized memory in the SkCodec base class implementation of onGetPixels() > > when kIncompleteInput is returned by a subclass. > > > > I'm adding a FIXME, but I think it's best to keep this for another CL. Let me > > know if you think this is ok as a temporary solution? It's worth noting that > we > > are already using this temporary solution further down in this function - > > Doesn't make it any better, but it's something that already needs to be fixed. > > I'm happy with a FIXME. I think I understand what's going on here based on your > comments there. As I said there, I think this is an interesting but strange > behavior. Maybe it's right though. > > > > > Side Note: > > Looks like it might be useful/necessary for the SkCodec base class to know the > > SkScanlineOrder in order to Fill(). Another argument for combining the two > > classes. > I think we can improve on this... FIXME sounds good for now. https://codereview.chromium.org/1305123002/diff/320001/src/codec/SkCodec_libg... File src/codec/SkCodec_libgif.cpp (right): https://codereview.chromium.org/1305123002/diff/320001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:199: // there might be a valid transparent index, we must indicate that the image has On 2015/09/04 18:42:09, scroggo wrote: > What do you mean there "might be a valid transparent index". It seems like it is > either < 256 (valid, even if it's ultimately unused) or >= 256 (invalid), and we > already know that at this point. Is there something I'm missing? > > Ok, I figured this out - a transIndex of < 256 might still be > actual number of > real colors in the color table. A comment explaining that would be nice. You're right this is confusing. Adding a comment. https://codereview.chromium.org/1305123002/diff/320001/src/codec/SkCodec_libg... src/codec/SkCodec_libgif.cpp:241: // since we don't know if fTransIndex is valid until we process the color table. On 2015/09/04 18:42:09, scroggo wrote: > ... because fTransIndex may be > the size of the color table. Done.
The CQ bit was checked by msarett@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/1305123002/#ps340001 (title: "Add better comments for fTransIndex")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1305123002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1305123002/340001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
The CQ bit was checked by msarett@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/1305123002/#ps360001 (title: "Compile fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1305123002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1305123002/360001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
The CQ bit was checked by msarett@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/1305123002/#ps380001 (title: "Compile fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1305123002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1305123002/380001
Message was sent while issue was closed.
Committed patchset #15 (id:380001) as https://skia.googlesource.com/skia/+/e9c10b9121887e8c300bd41357461418e061984d
Message was sent while issue was closed.
A revert of this CL (patchset #15 id:380001) has been created in https://codereview.chromium.org/1320273004/ by jcgregorio@google.com. The reason for reverting is: Breaks the build with SkScanlineDecoder.h not found: http://build.chromium.org/p/client.skia/builders/Linux%20Builder/builds/3722/... ../../third_party/skia/src/utils/SkBitmapRegionCanvas.h:10:10: fatal error: 'SkScanlineDecoder.h' file not found #include "SkScanlineDecoder.h".
The error that you referenced was caused by a different CL: https://codereview.chromium.org/1288963002/ It was then fixed by doing a manual roll: https://codereview.chromium.org/1304063013/ This change is fine, I think.
The CQ bit was checked by msarett@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1305123002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1305123002/380001
Message was sent while issue was closed.
Committed patchset #15 (id:380001) as https://skia.googlesource.com/skia/+/10522ff4cfa3cba45881354768f6185fc1109119 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
