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.
Note: the issue is not reproducible with png or jpg. Only happens with gif (both animated or static)
(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.
Created attachment 309704 [details] Patch
Really awesome! But do you know why is this not a problem for the CG decoder?
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.
(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.
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.
(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!
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?
(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.
Created attachment 309879 [details] Patch for landing It was easy to reproduce the issue with load-and-stall.php :-)
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));
(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.
Committed r216758: <http://trac.webkit.org/changeset/216758>