[webkit-reviews] review requested: [Bug 111144] GIFImageReader to count frames and decode in one pass : [Attachment 192361] Patch

[bugzilla-daemon at webkit.org]

Hin-Chung Lam <hclam at google.com> has asked  for review:
Bug 111144: GIFImageReader to count frames and decode in one pass
https://bugs.webkit.org/show_bug.cgi?id=111144

Attachment 192361: Patch
https://bugs.webkit.org/attachment.cgi?id=192361&action=review

------- Additional Comments from Hin-Chung Lam <hclam at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=192361&action=review


This patch might be hard to review. I've added inline comments to the review
tool.

Done exhausting testing locally to cover a lot of corner cases, in addition to
bit-exact as before I ensure states (e.g. loop count, frame count, frame
status) remains the same.

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:-380
> -	       // m_bytesToConsume is the current component size because it
hasn't been updated.

This and line 387 don't do prepare / decode any more. Save the information for
later decoding step.

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:-543
> -	       if (m_frameContext) {

This blob of code remains the same, just always saved to a new frame context.

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:-669
> -	       if (query == GIFImageDecoder::GIFFullQuery && !m_frameContext)

This blob of code remains the same as before. New code saves frame information
regardless of query and frameContext is always valid so we can get rid of one
scope of {}.

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:-755
> -		   if (m_frameContext && !m_frameContext->rowsRemaining) {

This blob code doesn't make sense, removed. Also note that rowsRemaining is
irrelevant here, decoding state is moved to GIFLZWContext and done in a later
time,

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:-760
> -	       } else {

This blob of code is removed, notification of frameComplete() is moved to
decode() above.

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:784
> +    if (m_frames.isEmpty() || m_frames.last()->isComplete())

This new method is to ensure we don't add a new frame if the current frame is
not complete yet.

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:87
> +    GIFLZWContext(WebCore::GIFImageDecoder* client, GIFFrameContext*
frameContext)

Reshuffling of some members and moved GIFLZWContext to the top of file. There's
basically no new members defined in GIFLZWContext.

The ownership is now GIFImageReader owns GIFFrameContext owns GIFLZWContext.
Each GIFFrameContext can be independently decoded.

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:137
> +struct GIFLZWBlock {

New data structure for each individual LZW block.

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:164
> +    int datasize;

This readonly state is moved from GIFLZWContext to here. The concept is that
GIFLZWContext is a state machine, while GIFFrameContext is mostly readonly.

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:211
> +    size_t m_currentLzwBlock;

These are new member for keeping tracking of decoding states.

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:279
> +    bool isFirstFrame() const

Helper method to detect if the current frame is the first frame.