Summary: | More cleanup in GIFImageReader | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hin-Chung Lam <hclam> | ||||||||||||
Component: | Images | Assignee: | Hin-Chung Lam <hclam> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | pkasting, senorblanco, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 98098, 111395 | ||||||||||||||
Attachments: |
|
Description
Hin-Chung Lam
2013-02-28 21:15:59 PST
Created attachment 190887 [details]
Patch
Comment on attachment 190887 [details]
Patch
Phew this is the last cleanup. Next patch will be the implementation for incremental parsing.
Peter: please review this patch. Stephen: please review. Comment on attachment 190887 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190887&action=review I'm not sure precisely what all this buys us, but whatever. > Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:196 > +bool GIFImageDecoder::haveDecodedRow(unsigned frameIndex, const Vector<unsigned char>& rowBuffer, unsigned width, unsigned rowNumber, unsigned repeatCount, bool writeTransparentPixels) |width| here (and elsewhere) should definitely be a size_t. |rowNumber| should probably be a size_t. |repeatCount| should maybe be a size_t. > Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:682 > if (m_screenWidth < width) { Nit: How about just m_screenWidth = std::max(m_screenWidth, width); > Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:821 > + m_lzwContext.oldcode = -1; Seems like the constructor could set this to -1 and save this line, unless prepareLZWContext() needs to reset variables that might have already been modified? For similar reasons, we may be able to eliminate setting |datum|, |bits|, and |stackp|. > Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:826 > + // Initialize the tables. Do we avoid doing this in the constructor in order to save memory in case we never do a GIFFullQuery? If not, we should do this there. If so, we should probably explicitly declare these vectors with initial size/capacity of 0. > Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:835 > + m_lzwContext.suffix.fill(0); Doesn't Vector::resize() zero-fill already? > Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:48 > #define MAX_BITS 4097 /* 2^MAX_LZW_BITS+1 */ Shouldn't this be MAX_BYTES, not MAX_BITS? It seems like it is a byte count everywhere we use it. > Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:86 > + size_t rowp; // Current output pointer. This is no longer a pointer, so the comment needs updating, and the name could probably be improved, too (size xCoord?). Also, how come |rowbuf| is moving to the GIFImageReader, but this offset counter isn't? Shouldn't they stick together? > Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:158 > + Vector<unsigned short> prefix; // LZW decoding tables. None of these three comments tells me anything meaningful. Created attachment 191070 [details]
Patch
Comment on attachment 190887 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190887&action=review Thanks for review. Most of the changes in this patch is to prepare for the next round. I separated LZW decoding states into GIFLZWContext such that there can be multiple GIFFrameContext that shares the same deceoding context and output buffer. unsigned char* to Vector<> is just an aesthetic choice. >> Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:196 >> +bool GIFImageDecoder::haveDecodedRow(unsigned frameIndex, const Vector<unsigned char>& rowBuffer, unsigned width, unsigned rowNumber, unsigned repeatCount, bool writeTransparentPixels) > > |width| here (and elsewhere) should definitely be a size_t. |rowNumber| should probably be a size_t. |repeatCount| should maybe be a size_t. width and rowNumber are size_t now. Keeping repeatCount as unsigned. >> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:682 >> if (m_screenWidth < width) { > > Nit: How about just > > m_screenWidth = std::max(m_screenWidth, width); Done. >> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:821 >> + m_lzwContext.oldcode = -1; > > Seems like the constructor could set this to -1 and save this line, unless prepareLZWContext() needs to reset variables that might have already been modified? > > For similar reasons, we may be able to eliminate setting |datum|, |bits|, and |stackp|. This method is called on every frame so these values need to be reset before decoding a frame. >> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:826 >> + // Initialize the tables. > > Do we avoid doing this in the constructor in order to save memory in case we never do a GIFFullQuery? If not, we should do this there. If so, we should probably explicitly declare these vectors with initial size/capacity of 0. Yes the original code did this to save memory. >> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:835 >> + m_lzwContext.suffix.fill(0); > > Doesn't Vector::resize() zero-fill already? This method is called on every frame so this array needs to be reset. >> Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:48 >> #define MAX_BITS 4097 /* 2^MAX_LZW_BITS+1 */ > > Shouldn't this be MAX_BYTES, not MAX_BITS? It seems like it is a byte count everywhere we use it. Done. >> Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:86 >> + size_t rowp; // Current output pointer. > > This is no longer a pointer, so the comment needs updating, and the name could probably be improved, too (size xCoord?). > > Also, how come |rowbuf| is moving to the GIFImageReader, but this offset counter isn't? Shouldn't they stick together? Good observation. In the next round of change we'll have more than one GIFFrameContext. Each representing the decoding states of an individual frame. They share one row buffer because there's only one frame decoding at a time, but each keeps its own decoding states. >> Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:158 >> + Vector<unsigned short> prefix; // LZW decoding tables. > > None of these three comments tells me anything meaningful. Removing them. Comment on attachment 191070 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191070&action=review Seems generally OK to me, the splitting of |m_rowBuffer| from |rowp| is probably my main remaining concern. > Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:233 > unsigned rowsRemaining = m_frameContext->rowsRemaining; Nit: If rowNumber is a size_t elsewhere, this should probably be a size_t too. > Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:823 > + // Initialize the tables. Nit: Might want to expand this comment to note what cases lead to us not doing this in the constructor. > Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:85 > unsigned rowsRemaining; // Rows remaining to be output. > unsigned irow; // Current output row, starting at zero. Similarly, probably both of these should be size_t. > Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:86 > + size_t rowp; // Current output pointer. So, you said "good observation" on wanting this to stick together with the row buffer itself, but the two are still in the same places they were. Are you planning to make some sort of change here? > Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:222 > + bool prepareLZWContext(int datasize); Nit: I suggest a comment noting when this gets called, making it clear that we need to reset a bunch of variables each frame that will have been written in the meantime. Consider making this a member of GIFLZWContext. I think it makes more sense there, and it also moves a tiny bit closer to a world where the members of that could perhaps be private or read-only or something. Created attachment 191313 [details]
Patch
Fixed all comments. I also moved rowBuffer and rowPosition into GIFLZWContext. Created attachment 191384 [details]
Patch
Comment on attachment 191384 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191384&action=review Looks good. r=me > Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:392 > m_frameContext->datasize = datasize; > - m_frameContext->clearCode = clearCode; > - m_frameContext->avail = m_frameContext->clearCode + 2; > - m_frameContext->oldcode = -1; > - m_frameContext->codesize = m_frameContext->datasize + 1; > - m_frameContext->codemask = (1 << m_frameContext->codesize) - 1; > - m_frameContext->datum = m_frameContext->bits = 0; > - > - // Init the tables. > - if (!m_frameContext->suffix) > - m_frameContext->suffix = new unsigned char[MAX_BITS]; > - > - // Clearing the whole suffix table lets us be more tolerant of bad data. > - memset(m_frameContext->suffix, 0, MAX_BITS); > - for (int i = 0; i < m_frameContext->clearCode; i++) > - m_frameContext->suffix[i] = i; > - > - if (!m_frameContext->stack) > - m_frameContext->stack = new unsigned char[MAX_BITS]; > - m_frameContext->stackp = m_frameContext->stack; > + // Initialize LZW parser/decoder. > + if (!m_lzwContext.prepareToDecode(m_screenWidth, datasize)) > + return m_client ? m_client->setFailed() : false; Not sure if it's a problem, but in the error case, this code previously used to early-return before m_frameContext->datasize was set. Now it'll be set unconditionally. Comment on attachment 191384 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191384&action=review >> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:392 >> + return m_client ? m_client->setFailed() : false; > > Not sure if it's a problem, but in the error case, this code previously used to early-return before m_frameContext->datasize was set. Now it'll be set unconditionally. Not really a problem here. The value is set but error will happen in prepareToDecode() which is slightly later. In general the direction of this code is moving to is that it will be more tolerant to errors in the parsing stage, fatal error will be deferred when decode actually happens. Created attachment 191798 [details]
Patch for landing
Comment on attachment 191798 [details] Patch for landing Clearing flags on attachment: 191798 Committed r144961: <http://trac.webkit.org/changeset/144961> All reviewed patches have been landed. Closing bug. |