Summary: | REGRESSION(r219045): A partially loaded image may not be repainted when its complete frame finishes decoding | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Said Abou-Hallawa <sabouhallawa> | ||||||||||||
Component: | Images | Assignee: | Said Abou-Hallawa <sabouhallawa> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | buildbot, cdumez, commit-queue, dbates, japhet, simon.fraser, thorton, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Said Abou-Hallawa
2017-07-06 17:26:44 PDT
Created attachment 314780 [details]
Patch
I am trying to write a test for this patch. Created attachment 314781 [details]
Patch
I tried to create a layout test for this patch. It took me a long time to do so. But at the end I was not able to find a test that fails without the patch and screed with it. The reason is there are three things that I need to control and overlap their timing: 1) Image loading 2) Image decoding 3) Page rendering I could have controlled the timing of the image loading by changing http/tests/resources/load-and-stall.php and made it stall for specific period of time after sending a specific chunk of data. I could also have controlled the timing of the image decoding be changing the decoding thread in ImageFrameCache::startAsyncDecodingQueue() to sleep for a specific period of time before sending the decoded frame to the main thread. But I could not control the timing of the page rendering. The page repaint and rendering is efficient such that it coalesces multiple repaints in one rendering. Also the network layer may decide to delay sending small chunks of data to the Web process till it receives more data. But more importantly I could not overlap the timing between these three entities in the same order I explained in the sequence above. I saw this bug with a test page with many large images: 50 images and on average each image is 3MB. Comment on attachment 314781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314781&action=review > Source/WebCore/ChangeLog:23 > + decoding, CachedImage will keep its HashSet of pending drawing clients as > + is if the image frame is partially loaded frame. "as is if"? > Source/WebCore/platform/graphics/BitmapImage.cpp:454 > + DecodingStatus decodingStatus = frameDecodingStatusAtIndex(m_currentFrame); > + if (m_currentFrameDecodingStatus == DecodingStatus::Decoding) > + m_currentFrameDecodingStatus = decodingStatus; It's very confusing that you can't just assign frameDecodingStatusAtIndex(m_currentFrame) to m_currentFrameDecodingStatus. Why are these things not the same? > Source/WebCore/platform/graphics/BitmapImage.cpp:515 > + if (m_currentFrameDecodingStatus == DecodingStatus::Decoding) > + m_currentFrameDecodingStatus = decodingStatus; Ditto. Created attachment 316181 [details]
Patch
Created attachment 316182 [details]
Patch
Created attachment 316193 [details]
Patch
Comment on attachment 316193 [details] Patch Clearing flags on attachment: 316193 Committed r219762: <http://trac.webkit.org/changeset/219762> All reviewed patches have been landed. Closing bug. |