Bug 240802 - REGRESSION(r289580): Canvas: putImageData sometimes draws nothing
Summary: REGRESSION(r289580): Canvas: putImageData sometimes draws nothing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: Safari Technology Preview
Hardware: Mac (Apple Silicon) macOS 12
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
: 240735 (view as bug list)
Depends on:
Blocks: 236305
  Show dependency treegraph
 
Reported: 2022-05-23 08:32 PDT by Bart Corremans
Modified: 2022-06-09 00:46 PDT (History)
4 users (show)

See Also:


Attachments
Render loop demonstrating the issue (33.28 KB, application/zip)
2022-05-23 08:32 PDT, Bart Corremans
no flags Details
simpler test case (1.46 KB, text/html)
2022-05-24 23:47 PDT, Said Abou-Hallawa
no flags Details
Patch (15.95 KB, patch)
2022-05-27 16:14 PDT, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bart Corremans 2022-05-23 08:32:50 PDT
Created attachment 459678 [details]
Render loop demonstrating the issue

putImageData will occasionally fail to draw anything.

I first noticed this in Technology Preview in an RDP client implementation, where we use putImageData to update the parts of the screen that have been changed. Scrolling, moving and resizing windows would result in distorted rendering.

Calling getImageData after a putImageData call will circumvent the issue.

I have created what I hope is a decent reproduction of the same issue, as the exact scenario is hard to replicate.

In this simple reproduction, a render loop copies a source canvas 20x20 pixels at a time using requestAnimationFrame.

Allow the loop to run and notice how sometimes, empty squares are left in the target canvas. This occurs less frequently when the browser tab is idle and focused, and more frequently if it is in the background, or if the user interacts with the page (e.g. by selecting text).

You can use the "Enable fix" checkbox to toggle the getImageData workaround.

This does not occur on the released 15.5 version.

I am aware of https://bugs.webkit.org/show_bug.cgi?id=229986, but I cannot reproduce it with the included workaround, so I am unsure if that issue has already been fixed.
Comment 1 Bart Corremans 2022-05-23 08:34:29 PDT
Note that if opening the index.html file directly, you will need to enable Develop -> Disable Local File Restrictions.

If you are serving using a web server, this is not needed.
Comment 2 Radar WebKit Bug Importer 2022-05-23 19:48:20 PDT
<rdar://problem/93801722>
Comment 3 Said Abou-Hallawa 2022-05-24 20:12:30 PDT
Thanks for reporting this bug.

In the attached test case, putImageData() is the only drawing operation which is applied to the target context. r289580 made PutPixelBuffer an ImageBuffer message not a display list item. So the flag RemoteDisplayListRecorderProxy::m_needsFlush is never set to true because no StreamConnection message is sent from the target canvas. So RemoteImageBufferProxy::flushDrawingContextAsync() never returns true because m_remoteDisplayList::needsFlush() always returns false. This makes flushDrawingContext() return always without waiting. When drawing the target canvas to the page we copy its backend to a NativeImage in WebProcess. This happens while the last PutPixelBuffer message is being processed by GPUProcess.

Calling getImageData() as a workaround forces GPUProcess to flush all the drawing items before returning the image data. After receiving the data from GPUProcess, if we copy the backend to NativeImage in WebProcess, we will be getting valid pixels.

Another workaround is to draw dummy anything to the target canvas. This will fix the drawing issue.

Instead of using:

    target.getImageData(0, 0, canvasWidth, canvasHeight);

We can use something like this:

    target.fillRect(0, 0, 0, 0);

This will set RemoteDisplayListRecorderProxy::m_needsFlush() to true. So when copyNativeImage() is called, flushDrawingContext() will wait until a DidFlush message is received.
Comment 4 Said Abou-Hallawa 2022-05-24 21:24:40 PDT
Pull request: https://github.com/WebKit/WebKit/pull/1003
Comment 5 Said Abou-Hallawa 2022-05-24 23:47:00 PDT
Created attachment 459745 [details]
simpler test case
Comment 6 Said Abou-Hallawa 2022-05-25 16:20:31 PDT
*** Bug 240735 has been marked as a duplicate of this bug. ***
Comment 7 EWS 2022-05-26 20:56:05 PDT
Committed r294928 (251039@main): <https://commits.webkit.org/251039@main>

Reviewed commits have been landed. Closing PR #1003 and removing active labels.
Comment 8 Bart Corremans 2022-05-27 02:16:23 PDT
Thank you for the explanation and quick resolution! I can confirm my original issue is also fixed using the latest WebKit build.
Comment 9 Said Abou-Hallawa 2022-05-27 16:14:55 PDT
Reopening to attach new patch.
Comment 10 Said Abou-Hallawa 2022-05-27 16:14:57 PDT
Created attachment 459823 [details]
Patch
Comment 11 Said Abou-Hallawa 2022-06-01 03:03:50 PDT
Pull request: https://github.com/WebKit/WebKit/pull/1206
Comment 12 EWS 2022-06-02 11:14:34 PDT
Committed r295132 (251223@main): <https://commits.webkit.org/251223@main>

Reviewed commits have been landed. Closing PR #1206 and removing active labels.