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

Tue Mar 12 01:15:55 PDT 2013

Hin-Chung Lam <hclam at google.com> has canceled Hin-Chung Lam
<hclam at google.com>'s request 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

>> Source/WebCore/ChangeLog:15
>> +	    This change fixed the performance problem of decoding GIF files
when
> 
> Nit: fixed -> fixes.	You might want to refer here to the existing
description of the particular performance problem that you're removing from
GIFImageDecoder::frameCount().

Done.

>> Source/WebCore/ChangeLog:24
>> +	    implementaiton. However this algorithm does not perform any
decoding
> 
> Nit: implementaiton -> implementation

Done.

>> Source/WebCore/ChangeLog:29
>> +	    implementation look through all frame information saved and decode
> 
> Nit: look -> looks, decode -> decodes

Done.

>> Source/WebCore/ChangeLog:32
>> +	    Because of the separate of parse and decode, each frame can be
decoded
> 
> Nit: separate of parse and decode -> separation of parsing and decoding

Done.

>> Source/WebCore/ChangeLog:33
>> +	    separately. This paves the way to support seeking in GIF file.
> 
> Nit: file -> files

Done.

>> Source/WebCore/ChangeLog:38
>> +	    Exhaustive testing is done locally with a corpus of 60k images.
> 
> Nit: is -> was

Done.

>> Source/WebCore/ChangeLog:43
>> +	    other image viewers, e.g. Preview on Mac.
> 
> I'd be interested in knowing what kind of bustage, specifically, is decoded
differently.

The difference here is because of this: New code adds a new GIFFrameContext for
GIFControlExtension and GIFImageHeader and those two files stop right between
these two states, result in an empty frame. Old code doesn't add a new frame
for GIFControlExtension but only at GIFImageHeader.

To solve this difference I can check if image header is defined before counting
as a new frame. However no matter I add a new frame for GIFControlExtension or
GIFImageHeader, both will result in an empty frame if the file truncates before
a LZW block appear. See the fix in setHeaderDefined() and imagesCount().

>> Source/WebCore/ChangeLog:45
>> +	    These were also no crashing when decoding the entire corpus.
> 
> Nit: These were -> There was

Done.

>> Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:111
>> +	else if (m_reader && (!m_reader->imagesCount() ||
m_reader->parseFailed()))
> 
> Nit: I'd just combine this with the previous conditional with ||

Done.

>> Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:327
>> +	const int oldSize = m_frameBufferCache.size();
> 
> Nit: size_t

Done.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:246
>> +		    return false;
> 
> Does returning false here always cause us to set the decode failed bit?  It's
not instantly obvious, especially the way decoding responsibility now seems to
use more layers than before.

Yes, return false here always results in m_client->setFailed() being called.
I'll comments to clarify.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:325
>> +	// Check for no remaining rows can handle bad GIF data with extra
blocks.
> 
> I can't parse this.  Does it mean "Some bad GIFs have extra blocks beyond the
last row, which we don't want to decode."?

Right. Updated comments.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:336
>> +	if (m_currentLzwBlock == m_lzwBlocks.size() ||
!m_lzwContext->hasRemainingRows()) {
> 
> Doesn't this have to be true in order for control flow to reach here?

You're right. This is actually a bug. I've added a unit test for this. The bug
was that it reset the LZW context when progressively decoding the file, as the
decoder processes more blocks. The unit test uncovers another problem that LZW
decoding is preformed before |datasize| and other fields properly defined. They
are fixed in the latest patch.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:347
>> +	// Try to be tolerant and don't report parsing errors but we will not
parse again.
> 
> Why is important to be tolerant here?  Why not just fail entirely, which is
what I think the old code did?	Wouldn't this remove the "parse failed"
variable and method?  Is this the source of the behavior change?

Added comments for this. Previous implementation uses a separate GIFImageReader
for counting frames, which means counting can fail but decoding cannot. The
code here is to match previous implementation.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:354
>> +	while (m_currentDecodingFrame < m_frames.size() &&
m_currentDecodingFrame < haltAtFrame) {
> 
> Nit: Simpler:
> 
> while (m_currentDecodingFrame < std::min(m_frames.size(), haltAtFrame)) {
> 
> This also suggests that haltAtFrame (and other frame count numbers anywhere
else) should perhaps be a size_t?

Added a FIXME for this. Try not to change this because this involves changing
GIFImageDecoder as well.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:359
>> +	    if (!success)
> 
> Just call decode() directly in this statement and eliminate the temp.

Done.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:371
>> +	// All frames decoded.
> 
> Nit: Comment adds nothing

Removed.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:395
>> +	    const size_t currentComponentSize = m_bytesToConsume;
> 
> This second temp doesn't seem necessary; can't we just call addLzwBlock()
with m_bytesToConsume, as the old doLZW() call used to do?

Done.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:567
>>		}
> 
> Nit: Simpler, and more accurate comment.  You may need a "!!" or
"static_cast<bool>()" if the first line produces a compiler warning.  Even with
that, the code is not only shorter, but it reads more clearly to me, because
the conditional is testing a named variable whose meaning is obvious.
> 
> currentFrame->isTransparent = *currentComponent & 0x1;
> if (currentFrame->isTransparent)
>     currentFrame->tpixel = currentComponent[3];
> 
> // We ignore the "user input" bit.

Done.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:571
>> +		currentFrame->disposalMethod =
(WebCore::ImageFrame::FrameDisposalMethod)disposalMethod;
> 
> Nit: Let's switch this to a C++-style cast

Done.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:574
>> +		// Let's choose 3 (the more popular)
> 
> Better comment:
> 
> // Some specs say that disposal method 3 is "overwrite previous", others that
setting the third bit of the field (i.e. method 4) is.	We map both to the same
value.

Done.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:697
>> +		    currentFrame->interlaced = false;
> 
> Nit: Simpler.  Same comment about compiler warning applies.
> 
> currentFrame->interlaced = currentComponent[8] & 0x40;

Done.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:708
>> +		    // images progressively
> 
> Nit: If you hoist this comment to live above the conditional, the code
becomes:
> 
> currentFrame->progressiveDisplay = isFirstFrame();
> 
> I wonder if the comment implies that we could expand this to cover
non-transparent frames too, or something.

Done. Added a FIXME for the case you mentioned.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:716
>>		    int numColors = 2 << (currentComponent[8] & 0x7);
> 
> Nit: How about this.	(Compiler warning note, again)
> 
> currentFrame->isLocalColormapDefined = currentComponent[8] & 0x80;
> if (currentFrame->isLocalColormapDefined) {
>     // The three low-order bits of currentComponent[8] specify the bits per
pixel.
>     int numColors = 2 << (currentComponent[8] & 0x7);
> ...

currentFrame->isLocalColormapDefined actually has an additional meaning that
the entire local colormap is received so we can't set it early (See line 742).
Changing this to accomodate comments.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:768
>>		break;
> 
> Nit: I'd make this return false

Done.

>>> 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.
> 
> OK... I'm a little fuzzy on this, the two callsites look kind of like they
unconditionally added a frame, but I'm not really sure.

There are two control blocks that can adda a new frame, which is not desirable
and avoided here. For the case if frame list empty we should always add a new
frame.

The second condition is tricky: in the stream, either GIFControlExtension or
GIFImageHeader can come first, but we don't want to add a new frame twice. We
know that there will only be one "data complete" signal for each frame, and we
only want to start a new frame if the previous frame is data complete.

>>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:279
>>> +	 bool isFirstFrame() const
>> 
>> Helper method to detect if the current frame is the first frame.
> 
> Maybe it should be named "decodingFirstFrame()"? 
"currentFrameIsFirstFrame()"?

Done.

>> Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp:178
>> +	// There are two frames scanned because we have not encountered  a
decoding
> 
> Nit: Extra space

Done. There's no need to update these two tests.