Bug 170432 – [GTK] GIF images are not properly loaded the first time

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Alberto Lopez Perez]

On WebKitGTK+ from trunk r214853 I see that GIF Images are not properly loaded the first time:

Running the minibrowser like this, always produces a small square image indicating fail to load rather than showing the cats:

HOME=$(mktemp -d) Tools/Scripts/run-minibrowser --gtk https://trac.webkit.org/export/214855/webkit/trunk/LayoutTests/svg/hixie/perf/resources/smallcats.gif


Notice the HOME=$(mktemp -d) which sees a temporal home before executing the browser (this is required to reproduce the bug)

Hitting reload doesn't fix the issue.

However, if I opening a new tab on the minibrowser and load the URL on the new tab, then the image is displayed as expected-

So it looks like a cache issue somewhere, like if it failed to pass the image data the first time the image is loaded.

[Carlos Alberto Lopez Perez]

Note: the issue is not reproducible with png or jpg. Only happens with gif (both animated or static)

[Michael Catanzaro]

(In reply to Carlos Alberto Lopez Perez from comment #0)
> Notice the HOME=$(mktemp -d) which sees a temporal home before executing the
> browser (this is required to reproduce the bug)

I think it's required to reproduce if you've loaded the image before, because it works once it's cached. The problem is if it's not cached yet. Just loading that image in Epiphany failed for me the first time I clicked the link, but it works fine now.

[Carlos Garcia Campos]

Created attachment 309704 [details]
Patch

[Michael Catanzaro]

Really awesome!

But do you know why is this not a problem for the CG decoder?

[Carlos Alberto Lopez Perez]

Comment on attachment 309704 [details]
Patch

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

Amazing!

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:482
> +#if !USE(CG)
> +    if (m_frames.isEmpty() && isDecoderAvailable())
> +        return m_decoder->size();
> +#endif

I think we should add a comment here briefly mentioning the tricky issue explained on the changelog for the next developer that touches this code.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #4)
> Really awesome!
> 
> But do you know why is this not a problem for the CG decoder?

I guess CG decoder doesn't claim to know the size until it has (at least partially) decoded the first frame, but I have no idea.

[Michael Catanzaro]

OK... let's wait a day before committing to see if Said has any comments. And I agree with Carlos Lopez, this is really tricky and it's important to add a comment to explain what's going on here.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #7)
> OK... let's wait a day before committing to see if Said has any comments.
> And I agree with Carlos Lopez, this is really tricky and it's important to
> add a comment to explain what's going on here.

Sure!

[Michael Catanzaro]

Wait, we surely need a test for this, right? Did this really not break any existing tests? If it needs a network situation then we could try to reproduce it using an HTTP-based test?

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #9)
> Wait, we surely need a test for this, right? Did this really not break any
> existing tests? If it needs a network situation then we could try to
> reproduce it using an HTTP-based test?

I'm not sure if we need some real network latency to reproduce this, I'll try to make a test.

[Carlos Garcia Campos]

Created attachment 309879 [details]
Patch for landing

It was easy to reproduce the issue with load-and-stall.php :-)

[Fujii Hironori]

WinCairo port can't show all GIF from http: due to this bug but from file:.
I think it's because curl port uses small buffer size.

BTW, I have a question. Looking at the source code comment
in Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:

>        case GIFGlobalHeader: {
>            // This is the height and width of the "screen" or frame into which images are rendered. The
>            // individual images can be smaller than the screen size and located with an origin anywhere
>            // within the screen.
>            m_screenWidth = GETINT16(currentComponent);
>            m_screenHeight = GETINT16(currentComponent + 2);
>
>            // CALLBACK: Inform the decoderplugin of our size.
>            // Note: A subsequent frame might have dimensions larger than the "screen" dimensions.
>            if (m_client && !m_client->setSize(WebCore::IntSize(m_screenWidth, m_screenHeight)))
>                return false;

It seems that decoder's size is the screen size.

Real individual image size seems the size specified in GIFImageDecoder::initFrameBuffer.

>    buffer->backingStore()->setFrameRect(IntRect(left, top, right - left, bottom - top));

[Carlos Garcia Campos]

(In reply to Fujii Hironori from comment #12)
> WinCairo port can't show all GIF from http: due to this bug but from file:.
> I think it's because curl port uses small buffer size.
> 
> BTW, I have a question. Looking at the source code comment
> in Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:
> 
> >        case GIFGlobalHeader: {
> >            // This is the height and width of the "screen" or frame into which images are rendered. The
> >            // individual images can be smaller than the screen size and located with an origin anywhere
> >            // within the screen.
> >            m_screenWidth = GETINT16(currentComponent);
> >            m_screenHeight = GETINT16(currentComponent + 2);
> >
> >            // CALLBACK: Inform the decoderplugin of our size.
> >            // Note: A subsequent frame might have dimensions larger than the "screen" dimensions.
> >            if (m_client && !m_client->setSize(WebCore::IntSize(m_screenWidth, m_screenHeight)))
> >                return false;
> 
> It seems that decoder's size is the screen size.
> 
> Real individual image size seems the size specified in
> GIFImageDecoder::initFrameBuffer.
> 
> >    buffer->backingStore()->setFrameRect(IntRect(left, top, right - left, bottom - top));

I don't think there's any problem, because we are not caching that size, when frames are available, the actual frame sizes will be used instead.

[Carlos Garcia Campos]

Committed r216758: <http://trac.webkit.org/changeset/216758>