Bug 111137 – More cleanup in GIFImageReader

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Hin-Chung Lam]

This is another round of cleanup before incremental parsing:

* Move GIFLZWContext out of GIFFrameContext
* Use Vector<> to contain rowbuf

[Hin-Chung Lam]

Created attachment 190887 [details]
Patch

[Hin-Chung Lam]

Comment on attachment 190887 [details]
Patch

Phew this is the last cleanup. Next patch will be the implementation for incremental parsing.

[Hin-Chung Lam]

Peter: please review this patch.
Stephen: please review.

[Peter Kasting]

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.

[Hin-Chung Lam]

Created attachment 191070 [details]
Patch

[Hin-Chung Lam]

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.

[Peter Kasting]

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.

[Hin-Chung Lam]

Created attachment 191313 [details]
Patch

[Hin-Chung Lam]

Fixed all comments. I also moved rowBuffer and rowPosition into GIFLZWContext.

[Hin-Chung Lam]

Created attachment 191384 [details]
Patch

[Stephen White]

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.

[Hin-Chung Lam]

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.

[Hin-Chung Lam]

Created attachment 191798 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 191798 [details]
Patch for landing

Clearing flags on attachment: 191798

Committed r144961: <http://trac.webkit.org/changeset/144961>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.