Bug 240802

Summary: REGRESSION(r289580): Canvas: putImageData sometimes draws nothing
Product: WebKit Reporter: Bart Corremans <bart.corremans>
Component: CanvasAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, rackler, sabouhallawa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Mac (Apple Silicon)   
OS: macOS 12   
See Also: https://bugs.webkit.org/show_bug.cgi?id=236305
Bug Depends on:    
Bug Blocks: 236305    
Attachments:
Description Flags
Render loop demonstrating the issue
none
simpler test case
none
Patch ews-feeder: commit-queue-

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.