Bug 109662 – GIFImageReader to read from source data directly

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

[Hin-Chung Lam]

GIFImageReader has a legacy thing  that keeps a small internal buffer to copy into. The code calls it the "hold" pattern. The bug is to remove this read pattern because WebKit holds the entire file. Change the code such that it reads from a large memory that holds the entire file.

[Hin-Chung Lam]

Created attachment 189326 [details]
Patch

[Hin-Chung Lam]

Created attachment 189328 [details]
Patch

[Hin-Chung Lam]

Created attachment 189329 [details]
Patch

[Hin-Chung Lam]

Peter: Would you please review this patch? 

Stephen: Please review after Peter.

More patches coming, thanks guys!

[Peter Kasting]

Comment on attachment 189329 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189329&action=review

> Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:54
> +    if (m_reader)
> +        m_reader->setData(data);

For consistency with other decoders, put this immediately below the superclass call.  (See BMPImageDecoder::setData().)

> Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:90
>          GIFImageReader reader(0);

Long-term, are you planning to get rid of this by making it possible to non-destructively query |m_reader| for this information?

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:93
> +        m_bytesInNextBlock = (n); \

This name seems a bit misleading, maybe m_bytesRequested?  (Since this is "how many bytes we want next", but that might not be the full length of a block, just the length of the next component our parser is looking for, right?)

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:204
> +bool GIFImageReader::doLZW(const unsigned char* block, unsigned bytesInBlock)

Nit: Seems better to use size_t for byte counts instead of unsigned. (several places)

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:375
> +    // Variables:

I'm not a fan of this big comment block.  Descriptive variable names ("q" sucks) plus comments above variables where they're declared (as needed) are sufficient.

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:391
> +    // This loop reads as many blocks from |m_data| as possible.
> +    // Beginning of each iteration dataPosition will be advanced m_bytesInNextBlock to point
> +    // to the next block, len is decremented accordingly.

Again, we don[t actually increment block-at-a-time, just individual-parsed-component at a time, right?

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:486
> +                // The entire global colormap is received so advance read position.

Nit: This comment seems unnecessary.

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:787
> +                    // Not enough bytes to read the entire colormap, read it as next block.

Tiny nit: Word this identically to how you did the global colormap comment.

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:792
> +                // There's enough bytes for the entire colormap so advance data pointer.

Nit: This comment seems unnecessary.

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:-897
> -        m_client->decodingHalted(0);

You seem to have removed all the decodingHalted() calls, shouldn't you remove that function itself?  And doesn't that mean that GIFImageDecoder::m_readOffset isn't being updated anymore?  (You're now tracking that as GIFImageReader::m_bytesRead, aren't you?)

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:201
> +        return colormap(m_globalColormapPosition, m_globalColormapSize);

It seems like for safety this should have some kind of check that we've actually read the global colormap yet, as we do for the local colormap.

If we do this, we ought to be able to remove the sanity-check in colormap(), which means that colormap() itself can be removed and replaced by a direct call to data().

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:226
> +    const unsigned char* data(unsigned dataPosition) const

It seems like we mostly need this because we're casting char* to unsigned char*.  If callers took a char*, they could just do "m_data->data() + position" directly.

This is perhaps a viral change, so it may not be suitable for doing in this CL, but are there real problems with changing various places that use unsigned char* to just char*?  This will eliminate some casting elsewhere too.

[Hin-Chung Lam]

Created attachment 189388 [details]
Patch

[Hin-Chung Lam]

Comment on attachment 189329 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189329&action=review

>> Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:54
>> +        m_reader->setData(data);
> 
> For consistency with other decoders, put this immediately below the superclass call.  (See BMPImageDecoder::setData().)

Done.

>> Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:90
>>          GIFImageReader reader(0);
> 
> Long-term, are you planning to get rid of this by making it possible to non-destructively query |m_reader| for this information?

Yes that's definitely the short term goal. You'll see this n^2 behavior removed in a few patches.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:93
>> +        m_bytesInNextBlock = (n); \
> 
> This name seems a bit misleading, maybe m_bytesRequested?  (Since this is "how many bytes we want next", but that might not be the full length of a block, just the length of the next component our parser is looking for, right?)

In that case I'll just revert it back to m_bytesToConsume. I don't see much difference between m_bytesToConsume or m_bytesRequested.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:204
>> +bool GIFImageReader::doLZW(const unsigned char* block, unsigned bytesInBlock)
> 
> Nit: Seems better to use size_t for byte counts instead of unsigned. (several places)

Done.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:375
>> +    // Variables:
> 
> I'm not a fan of this big comment block.  Descriptive variable names ("q" sucks) plus comments above variables where they're declared (as needed) are sufficient.

Okay, removing this.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:391
>> +    // to the next block, len is decremented accordingly.
> 
> Again, we don[t actually increment block-at-a-time, just individual-parsed-component at a time, right?

s/block/component/

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:486
>> +                // The entire global colormap is received so advance read position.
> 
> Nit: This comment seems unnecessary.

Done.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:787
>> +                    // Not enough bytes to read the entire colormap, read it as next block.
> 
> Tiny nit: Word this identically to how you did the global colormap comment.

Done.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:792
>> +                // There's enough bytes for the entire colormap so advance data pointer.
> 
> Nit: This comment seems unnecessary.

Done.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:-897
>> -        m_client->decodingHalted(0);
> 
> You seem to have removed all the decodingHalted() calls, shouldn't you remove that function itself?  And doesn't that mean that GIFImageDecoder::m_readOffset isn't being updated anymore?  (You're now tracking that as GIFImageReader::m_bytesRead, aren't you?)

Yeah removing decodingHalted in the next patch.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:201
>> +        return colormap(m_globalColormapPosition, m_globalColormapSize);
> 
> It seems like for safety this should have some kind of check that we've actually read the global colormap yet, as we do for the local colormap.
> 
> If we do this, we ought to be able to remove the sanity-check in colormap(), which means that colormap() itself can be removed and replaced by a direct call to data().

Done.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:226
>> +    const unsigned char* data(unsigned dataPosition) const
> 
> It seems like we mostly need this because we're casting char* to unsigned char*.  If callers took a char*, they could just do "m_data->data() + position" directly.
> 
> This is perhaps a viral change, so it may not be suitable for doing in this CL, but are there real problems with changing various places that use unsigned char* to just char*?  This will eliminate some casting elsewhere too.

This function is for casting and doing the + offset. I don't want to do this offset everywhere. Yes I think returning char* might have consequences, we should do it separately in another patch.

[Stephen White]

Comment on attachment 189388 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189388&action=review

Out of curiosity, are there tests which cover the colormap cases?

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:371
> +    // If there's not enough bytes for parsing this component then return.

Nit:  there's not -> there are not.

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:376
> +    // Beginning of each iteration dataPosition will be advanced by m_bytesToConsume to point

s/Beginning of each iteration/At the beginning of each iteration,/

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:377
> +    // to the next component, len is decremented accordingly.

That's two to's.  :)

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:229
> +    bool decodeInternal(unsigned dataPosition, unsigned len, WebCore::GIFImageDecoder::GIFQuery = WebCore::GIFImageDecoder::GIFFullQuery, unsigned haltAtFrame = -1);

The only caller I see of this function supplies all the parameters.  Are the defaults necessary?

[Peter Kasting]

Comment on attachment 189388 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189388&action=review

I can't set r+, but this seems basically OK to me.

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:358
> +    if (m_bytesRead > m_data->size())

When can this actually happen?  Doesn't the ASSERT in setRemainingBytes() prevent this?

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:364
> +bool GIFImageReader::decodeInternal(unsigned dataPosition, unsigned len, GIFImageDecoder::GIFQuery query, unsigned haltAtFrame)

I'd probably change these unsigneds to size_ts as well, along with most others in this file; but perhaps that sort of change should be done separately.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:371
>> +    // If there's not enough bytes for parsing this component then return.
> 
> Nit:  there's not -> there are not.

Or just remove the comment.  The code is pretty clear.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:377
>> +    // to the next component, len is decremented accordingly.
> 
> That's two to's.  :)

"to point to" is OK.  But ", len is decremented accordingly" might be better as ".  len will be decremented accordingly."

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:382
> +        // Eat the current component from the buffer, q keeps pointed at current component.

How about: "// Mark the current component as consumed.  Note that q will remain pointed at this component until the next loop iteration."

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:466
> +                    // Not enough bytes to read the entire global colormap this time so read it next.

How about: "// Wait until we have enough bytes to consume the entire colormap at once." (2 places)

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:107
> +    unsigned localColormapPosition; // Per-image colormap.

Similarly, this sort of thing would also ideally be a size_t.

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:172
> +        , m_bytesToConsume(6) // Number of bytes for the first read.

Perhaps the comment should say why the first read needs to be 6 bytes?

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:202
> +        if (!m_isGlobalColormapDefined)

FWIW, all these could be rewritten more succinctly as something like:

    return m_isGlobalColormapDefined ? data(m_globalColormapPosition) : 0;

Note also that you could choose to store the "colormap defined" state in the position variable by using static_cast<size_t>(-1).  Or, assuming GIFs have to start with a valid header, you could use 0 as a signal value, since we couldn't have the colormap appear before the header.

I don't feel strongly about which route gets chosen.

[Stephen White]

Comment on attachment 189388 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189388&action=review

>>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:377
>>> +    // to the next component, len is decremented accordingly.
>> 
>> That's two to's.  :)
> 
> "to point to" is OK.  But ", len is decremented accordingly" might be better as ".  len will be decremented accordingly."

Oh, sorry, I missed the "point" (bad word wrap).

[Hin-Chung Lam]

Created attachment 189557 [details]
Patch

[Hin-Chung Lam]

Comment on attachment 189388 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189388&action=review

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:358
>> +    if (m_bytesRead > m_data->size())
> 
> When can this actually happen?  Doesn't the ASSERT in setRemainingBytes() prevent this?

I'll change this to an assert.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:364
>> +bool GIFImageReader::decodeInternal(unsigned dataPosition, unsigned len, GIFImageDecoder::GIFQuery query, unsigned haltAtFrame)
> 
> I'd probably change these unsigneds to size_ts as well, along with most others in this file; but perhaps that sort of change should be done separately.

Done. I've scanned the code to use size_t as much as possible.

>>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:371
>>> +    // If there's not enough bytes for parsing this component then return.
>> 
>> Nit:  there's not -> there are not.
> 
> Or just remove the comment.  The code is pretty clear.

Removed.

>>>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:377
>>>> +    // to the next component, len is decremented accordingly.
>>> 
>>> That's two to's.  :)
>> 
>> "to point to" is OK.  But ", len is decremented accordingly" might be better as ".  len will be decremented accordingly."
> 
> Oh, sorry, I missed the "point" (bad word wrap).

Done.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:382
>> +        // Eat the current component from the buffer, q keeps pointed at current component.
> 
> How about: "// Mark the current component as consumed.  Note that q will remain pointed at this component until the next loop iteration."

Done.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:466
>> +                    // Not enough bytes to read the entire global colormap this time so read it next.
> 
> How about: "// Wait until we have enough bytes to consume the entire colormap at once." (2 places)

Done.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:107
>> +    unsigned localColormapPosition; // Per-image colormap.
> 
> Similarly, this sort of thing would also ideally be a size_t.

Done.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:172
>> +        , m_bytesToConsume(6) // Number of bytes for the first read.
> 
> Perhaps the comment should say why the first read needs to be 6 bytes?

Done.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:202
>> +        if (!m_isGlobalColormapDefined)
> 
> FWIW, all these could be rewritten more succinctly as something like:
> 
>     return m_isGlobalColormapDefined ? data(m_globalColormapPosition) : 0;
> 
> Note also that you could choose to store the "colormap defined" state in the position variable by using static_cast<size_t>(-1).  Or, assuming GIFs have to start with a valid header, you could use 0 as a signal value, since we couldn't have the colormap appear before the header.
> 
> I don't feel strongly about which route gets chosen.

I would like to stick with have a position and a boolean.

The reason is boolean value has a dual meaning of the colormap being defined and fully received. Using one variable we'll have to change the parsing logic if colormap is partially received.

>> Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:229
>> +    bool decodeInternal(unsigned dataPosition, unsigned len, WebCore::GIFImageDecoder::GIFQuery = WebCore::GIFImageDecoder::GIFFullQuery, unsigned haltAtFrame = -1);
> 
> The only caller I see of this function supplies all the parameters.  Are the defaults necessary?

Good one! I find those defaults distracting, will remove them.

[Hin-Chung Lam]

ping for review. :)

[Stephen White]

Comment on attachment 189557 [details]
Patch

r=me (with Peter's LG on the previous patch).

[WebKit Review Bot]

Comment on attachment 189557 [details]
Patch

Rejecting attachment 189557 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-03', 'validate-changelog', '--non-interactive', 189557, '--port=chromium-xvfb']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

/mnt/git/webkit-commit-queue/Source/WebKit/chromium/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/16750271

[Hin-Chung Lam]

Created attachment 190059 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 190059 [details]
Patch for landing

Clearing flags on attachment: 190059

Committed r143936: <http://trac.webkit.org/changeset/143936>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 110789

[Hin-Chung Lam]

Test failure because Android failed to open GIF test files. :(

Will submit another patch that disables test on Android.

[Hin-Chung Lam]

Created attachment 190158 [details]
Patch

[Hin-Chung Lam]

This should fix Android, data files should be loaded properly this time.

[Hin-Chung Lam]

PLease have a look. This is a reland of the same patch with fix to use Platform::current()->unitTestSupport()->webKitRootDir() to get the directory for data files. This should fix unit test failures on Android.

[Stephen White]

Comment on attachment 190158 [details]
Patch

OK.  r=me

[Hin-Chung Lam]

Committed r144100: <http://trac.webkit.org/changeset/144100>