Bug 239272 – The first frame of an animation GIF is sometimes skipped on GTKWebKit and WinCairo

• Component: Images
• Status: NEW

[Yuki Sekiguchi]

Created attachment 457503 [details]
reproduced content

How to reproduce:
1. Load the attached test.gif
2. See the first frame isn't 3 white boxes

Description:
The first frame of test.gif is 3 white boxes. This should be shown. However, GTKWebKit and WinCairo sometimes show the 2nd frame, which has a black block at the right, or the 3rd frame, which has a black block at the center.

Chrome for Linux and Firefox for Linux shows the 1st frame.
Safari and WebKit nightly shows the 1st frame.

I tested r292705.

[Yuki Sekiguchi]

The following patch fixed this bug on GTKWebKit.
```
diff --git a/Source/WebCore/platform/graphics/BitmapImage.cpp b/Source/WebCore/platform/graphics/BitmapImage.cpp
index 1a0ba366f423..253f4b787ba6 100644
--- a/Source/WebCore/platform/graphics/BitmapImage.cpp
+++ b/Source/WebCore/platform/graphics/BitmapImage.cpp
@@ -480,8 +480,11 @@ BitmapImage::StartAnimationStatus BitmapImage::internalStartAnimation()
     MonotonicTime time = MonotonicTime::now();

     // Handle initial state.
-    if (!m_desiredFrameStartTime)
+    if (!m_desiredFrameStartTime) {
+        if (!frameIsCompleteAtIndex(m_currentFrame))
+            return StartAnimationStatus::IncompleteData;
         m_desiredFrameStartTime = time;
+    }

     // Setting 'm_desiredFrameStartTime' to 'time' means we are late; otherwise we are early.
     m_desiredFrameStartTime = std::max(time, m_desiredFrameStartTime + Seconds { frameDurationAtIndex(m_currentFrame) });
```

I have no idea how to test this using LayoutTest, so I don't create a patch now. Any advice is very welcome!

Description:
BitmapImage::internalStartAnimation() starts the animation timer if the current frame is *decoding* and all data is received or next frame is decoded. Therefore, even if the current frame isn't decoded, the animation timer starts. If the current frame isn't decoded, BitmapImage::draw() skips to draw the current frame.  Then, if the first frame in test.gif isn't decoded during its frame duration, the first frame isn't shown.

This patch waits for the first frame if animation haven't started.

Safari doesn't start the animation timer because the following condition is true:
```
    // Don't advance the animation to an incomplete frame.
    if (!m_source->isAllDataReceived() && !frameIsCompleteAtIndex(nextFrame)) {
        printf("BitmapImage::internalStartAnimation() return IncompleteData nextFrame\n");
        return StartAnimationStatus::IncompleteData;
    }
```

I guess that since Safari and GTKWebKit uses different decoder, only Safari becomes true the above condition.

[Yuki Sekiguchi]

(In reply to Yuki Sekiguchi from comment #1)
> The following patch fixed this bug on GTKWebKit.

Sorry. I found that this patch stops animation on some contents.
Please ignore that.

[Fujii Hironori]

Created attachment 457679 [details]
logging patch

Good catch. Thank you for the bug report.

frameDurationAtIndex returns 0 seconds at the first time.
This seems GIFImageDecoder's problem.

frameDurationAtIndex(0) = 0.000000
frameDurationAtIndex(1) = 0.000000
frameDurationAtIndex(2) = 1.000000
frameDurationAtIndex(3) = 1.000000
frameDurationAtIndex(4) = 1.000000
frameDurationAtIndex(5) = 1.000000
frameDurationAtIndex(6) = 1.000000
frameDurationAtIndex(7) = 1.000000
frameDurationAtIndex(0) = 1.000000
frameDurationAtIndex(1) = 1.000000
frameDurationAtIndex(2) = 1.000000
frameDurationAtIndex(3) = 1.000000
frameDurationAtIndex(4) = 1.000000
frameDurationAtIndex(5) = 1.000000
frameDurationAtIndex(6) = 1.000000
frameDurationAtIndex(7) = 1.000000

[Fujii Hironori]

I think there are two problems

1. If ImageDecoder::isAllDataReceived (EncodedDataStatus is Complete),
ImageDecoder::frameDurationAtIndex has to return the correct information.

2. If EncodedDataStatus is SizeAvailable, ImageDecoder::frameDurationAtIndex has to return the correct information for 0th frame,
because ImageSource::cacheMetadataAtIndex is called in the following callstack.

	WebKit2.dll!WebCore::ScalableImageDecoder::frameDurationAtIndex(unsigned __int64 index) Line 227	C++
 	WebKit2.dll!WebCore::ImageSource::cacheMetadataAtIndex(unsigned __int64 index, WebCore::SubsamplingLevel subsamplingLevel, WebCore::DecodingStatus decodingStatus) Line 277	C++
 	WebKit2.dll!WebCore::ImageSource::frameAtIndexCacheIfNeeded(unsigned __int64 index, WebCore::ImageFrame::Caching caching, const std::optional<enum WebCore::SubsamplingLevel> & subsamplingLevel) Line 450	C++
 	WebKit2.dll!WebCore::ImageSource::firstFrameMetadataCacheIfNeeded<std::optional<WebCore::IntSize>>(std::optional<WebCore::IntSize> & cachedValue, WebCore::ImageSource::MetadataType metadataType, std::optional<WebCore::IntSize>(const WebCore::ImageFrame::*)() functor, WebCore::ImageFrame::Caching caching, const std::optional<enum WebCore::SubsamplingLevel> & subsamplingLevel) Line 513	C++
 	WebKit2.dll!WebCore::ImageSource::densityCorrectedSize(WebCore::ImageOrientation orientation) Line 571	C++
 	WebKit2.dll!WebCore::ImageSource::size(WebCore::ImageOrientation orientation) Line 583	C++
 	WebKit2.dll!WebCore::BitmapImage::size(WebCore::ImageOrientation orientation) Line 89	C++
 	WebKit2.dll!WebCore::Image::isNull() Line 110	C++
 	WebKit2.dll!WebCore::CachedImage::updateBufferInternal(const WebCore::SharedBuffer & data) Line 506	C++
 	WebKit2.dll!WebCore::CachedImage::updateBuffer(const WebCore::FragmentedSharedBuffer & buffer) Line 556	C++
 	WebKit2.dll!WebCore::ImageDocument::updateDuringParsing() Line 147	C++
 	WebKit2.dll!WebCore::ImageDocumentParser::appendBytes(WebCore::DocumentWriter & __formal, const unsigned char * __formal, unsigned __int64 __formal) Line 195	C++
 	WebKit2.dll!WebCore::DocumentWriter::addData(const WebCore::SharedBuffer & data) Line 277	C++
 	WebKit2.dll!WebCore::DocumentLoader::commitData(const WebCore::SharedBuffer & data) Line 1324	C++
 	WebKit2.dll!WebKit::WebFrameLoaderClient::committedLoad(WebCore::DocumentLoader * loader, const WebCore::SharedBuffer & data) Line 1168	C++
 	WebKit2.dll!WebCore::DocumentLoader::commitLoad(const WebCore::SharedBuffer & data) Line 1189	C++
 	WebKit2.dll!WebCore::DocumentLoader::dataReceived(const WebCore::SharedBuffer & buffer) Line 1356	C++
 	WebKit2.dll!WebCore::DocumentLoader::dataReceived(WebCore::CachedResource & resource, const WebCore::SharedBuffer & buffer) Line 1330	C++
 	WebKit2.dll!WebCore::CachedRawResource::notifyClientsDataWasReceived(const WebCore::SharedBuffer & buffer) Line 145	C++
 	WebKit2.dll!WebCore::CachedRawResource::updateBuffer(const WebCore::FragmentedSharedBuffer & data) Line 81	C++
 	WebKit2.dll!WebCore::SubresourceLoader::didReceiveBuffer(const WebCore::FragmentedSharedBuffer & buffer, __int64 encodedDataLength, WebCore::DataPayloadType dataPayloadType) Line 545	C++
 	WebKit2.dll!WebCore::ResourceLoader::didReceiveData(const WebCore::SharedBuffer & buffer, __int64 encodedDataLength, WebCore::DataPayloadType dataPayloadType) Line 560	C++
 	WebKit2.dll!WebKit::WebResourceLoader::didReceiveData(const IPC::SharedBufferCopy & data, __int64 encodedDataLength) Line 241	C++
 	WebKit2.dll!IPC::callMemberFunctionImpl<WebKit::WebResourceLoader,void (__cdecl WebKit::WebResourceLoader::*)(IPC::SharedBufferCopy const &,__int64),std::tuple<IPC::SharedBufferCopy,__int64>,0,1>(WebKit::WebResourceLoader * object, void(WebKit::WebResourceLoader::*)(const IPC::SharedBufferCopy &, __int64) function, std::tuple<IPC::SharedBufferCopy,__int64> && args, std::integer_sequence<unsigned __int64,0,1> __formal) Line 126	C++
 	WebKit2.dll!IPC::callMemberFunction<WebKit::WebResourceLoader,void (__cdecl WebKit::WebResourceLoader::*)(IPC::SharedBufferCopy const &,__int64),std::tuple<IPC::SharedBufferCopy,__int64>,std::integer_sequence<unsigned __int64,0,1>>(std::tuple<IPC::SharedBufferCopy,__int64> && args, WebKit::WebResourceLoader * object, void(WebKit::WebResourceLoader::*)(const IPC::SharedBufferCopy &, __int64) function) Line 132	C++
 	WebKit2.dll!IPC::handleMessage<Messages::WebResourceLoader::DidReceiveData,WebKit::WebResourceLoader,void (__cdecl WebKit::WebResourceLoader::*)(IPC::SharedBufferCopy const &,__int64)>(IPC::Connection & connection, IPC::Decoder & decoder, WebKit::WebResourceLoader * object, void(WebKit::WebResourceLoader::*)(const IPC::SharedBufferCopy &, __int64) function) Line 197	C++
 	WebKit2.dll!WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection & connection, IPC::Decoder & decoder) Line 73	C++
 	WebKit2.dll!WebKit::NetworkProcessConnection::didReceiveMessage(IPC::Connection & connection, IPC::Decoder & decoder) Line 103	C++
 	WebKit2.dll!IPC::Connection::dispatchMessage(IPC::Decoder & decoder) Line 1109	C++
 	WebKit2.dll!IPC::Connection::dispatchMessage(std::unique_ptr<IPC::Decoder,std::default_delete<IPC::Decoder>> message) Line 1155	C++
 	WebKit2.dll!IPC::Connection::dispatchOneIncomingMessage() Line 1223	C++
 	WebKit2.dll!`IPC::Connection::enqueueIncomingMessage'::`2'::<lambda_1>::operator()() Line 1073	C++
 	WebKit2.dll!WTF::Detail::CallableWrapper<`IPC::Connection::enqueueIncomingMessage'::`2'::<lambda_1>,void>::call() Line 53	C++
 	WTF.dll!WTF::Function<void __cdecl(void)>::operator()() Line 83	C++
 	WTF.dll!WTF::RunLoop::performWork() Line 134	C++
 	WTF.dll!WTF::RunLoop::wndProc(HWND__ * hWnd, unsigned int message, unsigned __int64 wParam, __int64 lParam) Line 57	C++
 	WTF.dll!WTF::RunLoop::RunLoopWndProc(HWND__ * hWnd, unsigned int message, unsigned __int64 wParam, __int64 lParam) Line 39	C++
 	[External Code]	
 	WTF.dll!WTF::RunLoop::run() Line 74	C++
 	WebKit2.dll!WebKit::AuxiliaryProcessMainBase<WebKit::WebProcess,1>::run(int argc, char * * argv) Line 71	C++
 	WebKit2.dll!WebKit::AuxiliaryProcessMain<WebKit::WebProcessMainWin>(int argc, char * * argv) Line 97	C++
 	WebKit2.dll!WebKit::WebProcessMain(int argc, char * * argv) Line 58	C++
 	WebKitWebProcess.exe!main(int argc, char * * argv) Line 35	C++
 	[External Code]

[Fujii Hironori]

Created attachment 457792 [details]
WIP patch

I created a quick patch and tested with the comment#0 test case.
I learned this bug is not easy to fix.

[Fujii Hironori]

(In reply to Fujii Hironori from comment #4)
> 2. If EncodedDataStatus is SizeAvailable, ImageDecoder::frameDurationAtIndex
> has to return the correct information for 0th frame,
> because ImageSource::cacheMetadataAtIndex is called in the following
> callstack.

I think there are two points of view for this.

1. This is a bug that GIFImageDecoder doesn't make the first frame meta data available when the image size is available.
2. This is a bug of ImageSource that it assumes the first frame meta data is available when the image size is available.