Bug 174230

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: ImagesAssignee: Said Abou-Hallawa <sabouhallawa>
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   
Description Flags
Patch none

Description Said Abou-Hallawa 2017-07-06 17:26:44 PDT
After <http://trac.webkit.org/changeset/219045>, assume we have only one image in a page and this image is loaded into separate chunks. Assume the following sequence of events happens:

1. Part-1 of image is loaded.
2. Image rectangle is repainted.
3. BitmapImage::draw() is called.
4. Request-1 has been requested for Part-1 of this image.
5. RenderImage adds itself to CachedImage::m_pendingImageDrawingClients.
6. Part-2 of image is loaded.
7. Image rectangle is repainted.
8. BitmapImage::draw() is called.
9. Request-2 has been requested for Part-2 of this image. The request will happen BitmapImage::draw() allows multiple requests for the same frame if new data is added to the image source.
10. RenderImage will not add itself to CachedImage::m_pendingImageDrawingClients since it is already there.
11. BitmapImage::imageFrameAvailableAtIndex() is called for Request-1.
12. CachedImage::imageFrameAvailable() repaints the image rectangle and "clears its m_pendingImageDrawingClients".
13. BitmapImage::draw() is called.
14. Frame for Request-1 is drawn.
15. BitmapImage::imageFrameAvailableAtIndex() is called for Request-2.
16. CachedImage::imageFrameAvailable() does not repaint the image rectangle because its m_pendingImageDrawingClients is empty.

Result: we end up having incomplete frame for the image even after the image finishes loading.
Comment 1 Said Abou-Hallawa 2017-07-06 17:45:35 PDT
Created attachment 314780 [details]
Comment 2 Said Abou-Hallawa 2017-07-06 17:46:23 PDT
I am trying to write a test for this patch.
Comment 3 Said Abou-Hallawa 2017-07-06 18:03:21 PDT
Created attachment 314781 [details]
Comment 4 Said Abou-Hallawa 2017-07-10 10:05:14 PDT
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 5 Radar WebKit Bug Importer 2017-07-18 13:42:04 PDT
Comment 6 Said Abou-Hallawa 2017-07-20 14:07:38 PDT
Comment 7 Simon Fraser (smfr) 2017-07-21 13:49:38 PDT
Comment on attachment 314781 [details]

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;

Comment 8 Said Abou-Hallawa 2017-07-22 02:09:08 PDT
Created attachment 316181 [details]
Comment 9 Said Abou-Hallawa 2017-07-22 02:11:15 PDT
Created attachment 316182 [details]
Comment 10 Said Abou-Hallawa 2017-07-22 04:34:29 PDT
Created attachment 316193 [details]
Comment 11 WebKit Commit Bot 2017-07-22 05:12:37 PDT
Comment on attachment 316193 [details]

Clearing flags on attachment: 316193

Committed r219762: <http://trac.webkit.org/changeset/219762>
Comment 12 WebKit Commit Bot 2017-07-22 05:12:38 PDT
All reviewed patches have been landed.  Closing bug.