Bug 174230 – REGRESSION(r219045): A partially loaded image may not be repainted when its complete frame finishes decoding

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

Created attachment 314780 [details]
Patch

[Said Abou-Hallawa]

I am trying to write a test for this patch.

[Said Abou-Hallawa]

Created attachment 314781 [details]
Patch

[Said Abou-Hallawa]

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.

[Radar WebKit Bug Importer]

<rdar://problem/33384272>

[Said Abou-Hallawa]

<rdar://problem/33436337>

[Simon Fraser (smfr)]

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.

[Said Abou-Hallawa]

Created attachment 316181 [details]
Patch

[Said Abou-Hallawa]

Created attachment 316182 [details]
Patch

[Said Abou-Hallawa]

Created attachment 316193 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 316193 [details]
Patch

Clearing flags on attachment: 316193

Committed r219762: <http://trac.webkit.org/changeset/219762>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.