Bug 111137

Summary: More cleanup in GIFImageReader
Product: WebKit Reporter: Hin-Chung Lam <hclam>
Component: ImagesAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Hin-Chung Lam 2013-02-28 21:15:59 PST
This is another round of cleanup before incremental parsing:

* Move GIFLZWContext out of GIFFrameContext
* Use Vector<> to contain rowbuf
Comment 1 Hin-Chung Lam 2013-02-28 22:45:37 PST
Created attachment 190887 [details]
Patch
Comment 2 Hin-Chung Lam 2013-02-28 22:47:31 PST
Comment on attachment 190887 [details]
Patch

Phew this is the last cleanup. Next patch will be the implementation for incremental parsing.
Comment 3 Hin-Chung Lam 2013-02-28 22:52:52 PST
Peter: please review this patch.
Stephen: please review.
Comment 4 Peter Kasting 2013-03-01 15:43:42 PST
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.
Comment 5 Hin-Chung Lam 2013-03-01 16:46:16 PST
Created attachment 191070 [details]
Patch
Comment 6 Hin-Chung Lam 2013-03-01 16:48:12 PST
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 7 Peter Kasting 2013-03-02 14:53:59 PST
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.
Comment 8 Hin-Chung Lam 2013-03-04 14:37:14 PST
Created attachment 191313 [details]
Patch
Comment 9 Hin-Chung Lam 2013-03-04 14:38:05 PST
Fixed all comments. I also moved rowBuffer and rowPosition into GIFLZWContext.
Comment 10 Hin-Chung Lam 2013-03-04 20:02:02 PST
Created attachment 191384 [details]
Patch
Comment 11 Stephen White 2013-03-06 08:28:26 PST
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 12 Hin-Chung Lam 2013-03-06 11:19:37 PST
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.
Comment 13 Hin-Chung Lam 2013-03-06 11:20:53 PST
Created attachment 191798 [details]
Patch for landing
Comment 14 WebKit Review Bot 2013-03-06 12:11:12 PST
Comment on attachment 191798 [details]
Patch for landing

Clearing flags on attachment: 191798

Committed r144961: <http://trac.webkit.org/changeset/144961>
Comment 15 WebKit Review Bot 2013-03-06 12:11:16 PST
All reviewed patches have been landed.  Closing bug.