Bug 240368 – [GPU Process] Drawing the PDF snapshot should take place in WebProcess

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Said Abou-Hallawa]

Generating a PDFDocument for a web page should take place in the WebProcess.

To do that, the ImageBufferBackend should never draw itself to the destination GraphicsContext. Instead we need to get a NativeImage from the backend and draw it in the destination GraphicsContext. The virtual method copyNativeImage() knows to access the backend and how to get the pixels out of it in the WebProcess.

[Said Abou-Hallawa]

rdar://91660159

[Said Abou-Hallawa]

Created attachment 459276 [details]
Patch

[Said Abou-Hallawa]

Created attachment 459347 [details]
Patch

[Said Abou-Hallawa]

Created attachment 459387 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Said Abou-Hallawa]

We decided to keep the canvas 2D behavior on macOS as it is. If we have access to the ImageBuffer backend, we should copy the NativeImage from it directly instead of going through the IPC. This will make us avoid a regression on the MotionMark/Images sub-test.

[Said Abou-Hallawa]

Created attachment 459465 [details]
Patch

[Kimmo Kinnunen]

Comment on attachment 459347 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=459347&action=review

Great direction. I hope we do the same with ImageBuffer at some point, e.g. make GraphicsContextXX::drawImageBuffer use ImageBuffeR::copyNativeImage().

> Source/WebCore/platform/graphics/ConcreteImageBuffer.h:195
> +                image->drawPattern(destContext, destRect, adjustedSrcRect, patternTransform, phase, spacing, options);

In a later patch, we should add backend->finalizeDrawIntoContext() here.

> Source/WebCore/platform/graphics/ConcreteImageBuffer.h:228
> +                destContext.drawNativeImage(*image, backendSize, destRect, adjustedSrcRect, options);

In a later patch, we should add backend->finalizeDrawIntoContext() here.

> Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.cpp:172
> +}

There's two cases why invalidateCachedNativeImage() needs to be done:
1) Memory optimization, so that a particular ImageBuffer which holds an IOSurface would not  ALSO hold an CGImage that would hold a malloc buffer
2) Correctness aspect, where the underlying IOSurface has been modified but the CGImage taken out of the IOSurface through a particular CGIOSurfaceContext does not know about that modification.

Here in this spot, finalizeDrawIntoContext, we take care of the memory aspect.

The correctness aspect is taken care of:
i)  invalidating the cache after putimagedata (For any IOSurface operation)
ii) invalidating the cache in WP after GPUP has modified the surface

So you need to still take care of ii)
Suggestion how to do it:

"flush" equals "I know I have modified the the image buffer and will read the results out-of-band".
 
So add invalidateCachedNativeImage() in all WP-side remote backends.

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:183
> +        // Copy the image from the backend if it is accessible.

The code says literally the same thing as the comment. Maybe remove the comment.

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:187
>          const_cast<RemoteImageBufferProxy*>(this)->flushDrawingContext();

Waiting for the flush should happen before copying the image.

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:199
> +        // Copy the image from the backend if it is accessible.

The code says literally the same thing as the comment. Maybe remove the comment.

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:203
>          const_cast<RemoteImageBufferProxy*>(this)->flushDrawingContext();

Waiting for the flush should happen before copying the image.

[Kimmo Kinnunen]

Comment on attachment 459465 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=459465&action=review

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:184
> +        if (canMapBackingStore())

It would be better to leave these perf optimization hunks to a separate patch?

[Kimmo Kinnunen]

Thinking about it:

The correctness aspect is taken care of:
i)  Invalidating the CGImage cache entry in the CGIOSurfaceContext after putimagedata (For any IOSurface operation), before next copy image
ii) Invalidating the CGImage cache entry in the CGIOSurfaceContext in WP after GPUP has modified the surface, before next copy image
iii) Forcing read back of IOSurface contents to memory in already copied CGImage *before* GPU has modified the surface

[Simon Fraser (smfr)]

Comment on attachment 459465 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=459465&action=review

> Source/WebCore/platform/graphics/ConcreteImageBuffer.h:196
>          }

Why is there no finalizeDrawIntoContext() call here? Why aren't prepareToDrawIntoContext/finalizeDrawIntoContext always paired?

> Source/WebCore/platform/graphics/ConcreteImageBuffer.h:229
>          }

Why is there no finalizeDrawIntoContext() call here? Why aren't prepareToDrawIntoContext/finalizeDrawIntoContext always paired?

[Said Abou-Hallawa]

Created attachment 459527 [details]
Patch

[Said Abou-Hallawa]

Created attachment 459529 [details]
Patch

[Kimmo Kinnunen]

Comment on attachment 459529 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=459529&action=review

> Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.cpp:-185
> -    // Accelerated to/from unaccelerated image buffers need complex caching. We trust that

Here you are losing the intended memory optimization of clearing the cache after a draw

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:182
> +

I still think this could be a separate change, as I don't think it's entirely correct.

[Said Abou-Hallawa]

Comment on attachment 459529 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=459529&action=review

>> Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.cpp:-185
>> -    // Accelerated to/from unaccelerated image buffers need complex caching. We trust that
> 
> Here you are losing the intended memory optimization of clearing the cache after a draw

I am not sure whether CG clears the cache immediately after we draw the empty rect or it just sets a dirty flag but let's assume it does clears the cache. This code runs only when we draw the image of the IOSurface to a un-accelerated GraphicsContext. This is the case of applying filter, mask or clip-path. Do you think we need to optimize the memory in these cases only?

>> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:182
>> +
> 
> I still think this could be a separate change, as I don't think it's entirely correct.

One layout test failed without this change on https://ews-build.s3-us-west-2.amazonaws.com/macOS-AppleSilicon-Big-Sur-Debug-WK2-Tests-EWS/459276-30746/results.html. It looks like the CGImage interpolation is different on AS. So to fix this bug I need to get the CGImage form the backend in WebProcess. Also I did not want to cause any temporary regression.

But why do think this is not correct?

[Said Abou-Hallawa]

Comment on attachment 459465 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=459465&action=review

>> Source/WebCore/platform/graphics/ConcreteImageBuffer.h:196
>>          }
> 
> Why is there no finalizeDrawIntoContext() call here? Why aren't prepareToDrawIntoContext/finalizeDrawIntoContext always paired?

I think we do not need prepareToDrawIntoContext() or finalizeDrawIntoContext() here. I made ImageBufferShareableMappedIOSurfaceBackend::copyNativeImage() invalidate the platform image cache when the IOSurface changes.

>> Source/WebCore/platform/graphics/ConcreteImageBuffer.h:229
>>          }
> 
> Why is there no finalizeDrawIntoContext() call here? Why aren't prepareToDrawIntoContext/finalizeDrawIntoContext always paired?

Ditto.

[Said Abou-Hallawa]

Comment on attachment 459347 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=459347&action=review

>> Source/WebCore/platform/graphics/ConcreteImageBuffer.h:195
>> +                image->drawPattern(destContext, destRect, adjustedSrcRect, patternTransform, phase, spacing, options);
> 
> In a later patch, we should add backend->finalizeDrawIntoContext() here.

I think we do not need prepareToDrawIntoContext() or finalizeDrawIntoContext() here. I made ImageBufferShareableMappedIOSurfaceBackend::copyNativeImage() invalidate the platform image cache when the IOSurface changes.

>> Source/WebCore/platform/graphics/ConcreteImageBuffer.h:228
>> +                destContext.drawNativeImage(*image, backendSize, destRect, adjustedSrcRect, options);
> 
> In a later patch, we should add backend->finalizeDrawIntoContext() here.

Ditto.

>> Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.cpp:172
>> +}
> 
> There's two cases why invalidateCachedNativeImage() needs to be done:
> 1) Memory optimization, so that a particular ImageBuffer which holds an IOSurface would not  ALSO hold an CGImage that would hold a malloc buffer
> 2) Correctness aspect, where the underlying IOSurface has been modified but the CGImage taken out of the IOSurface through a particular CGIOSurfaceContext does not know about that modification.
> 
> Here in this spot, finalizeDrawIntoContext, we take care of the memory aspect.
> 
> The correctness aspect is taken care of:
> i)  invalidating the cache after putimagedata (For any IOSurface operation)
> ii) invalidating the cache in WP after GPUP has modified the surface
> 
> So you need to still take care of ii)
> Suggestion how to do it:
> 
> "flush" equals "I know I have modified the the image buffer and will read the results out-of-band".
>  
> So add invalidateCachedNativeImage() in all WP-side remote backends.

There are three cases for the ImageBuffer backed by an IOSurface:

1. ImageBufferIOSurfaceBackend: in this case the IOSurface is local. All the drawing happens in the same process and IOSurface is not shared.. The only thing we need to worry about is invalidating the image cache after calling putImageData(). We do not need to invalidate the image cache before or after getting the platform image from IOSurface since CG does the invalidation itself once we draw anything to it.
2. ImageBufferRemoteIOSurfaceBackend: in this case, the IOSurface is remote but it is not shared. All the drawing happens in GPUProcess and WebProcess does not have any access to IOSurface. Currently this can happen only on iOS when GPUProcess for DOM rendering is enabled. If the WebProcess wants an NativeImage from this IOSurface, this has to happen through an IPC message from RemoteImageBufferProxy::copyNativeImage().
3. ImageBufferShareableMappedIOSurfaceBackend: in this case, the IOSurface is remote and it is shared. All the drawing happens in GPUProcess but WebProcess can have any access to IOSurface. Currently this can happen only on macOS when GPUProcess for Canvas is enabled. If the WebProcess wants an NativeImage from this IOSurface, it can get it directly from the IOSurface but it has to ensure the cached is not stale. And we can do this by invalidating the image cache if the IOSurface seed was incremented since last time an image was copied from it. So we need to implement ImageBufferShareableMappedIOSurfaceBackend::copyNativeImage().

>> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:183
>> +        // Copy the image from the backend if it is accessible.
> 
> The code says literally the same thing as the comment. Maybe remove the comment.

removed.

>> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:187
>>          const_cast<RemoteImageBufferProxy*>(this)->flushDrawingContext();
> 
> Waiting for the flush should happen before copying the image.

ConcreteImageBuffer::copyNativeImage() has it own flushDrawingContext().

>> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:199
>> +        // Copy the image from the backend if it is accessible.
> 
> The code says literally the same thing as the comment. Maybe remove the comment.

ditto.

>> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:203
>>          const_cast<RemoteImageBufferProxy*>(this)->flushDrawingContext();
> 
> Waiting for the flush should happen before copying the image.

ditto.

[Kimmo Kinnunen]

> If the WebProcess wants an NativeImage from this IOSurface, it can get it directly from the IOSurface but it has to ensure the cached is not stale.

That ensuring is not enough.
It has to ensure that the existing CGImage instances pointing to that IOSurface are invalidated *before* the GPUP modifies that IOSurface.
Otherwise when the existing CGImage instances are used, they show the new content, and not the content at the snapshot time.

[Kimmo Kinnunen]

(In reply to Said Abou-Hallawa from comment #15)
> Comment on attachment 459529 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=459529&action=review
> 
> >> Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.cpp:-185
> >> -    // Accelerated to/from unaccelerated image buffers need complex caching. We trust that
> > 
> > Here you are losing the intended memory optimization of clearing the cache after a draw
> 
> I am not sure whether CG clears the cache immediately after we draw the
> empty rect or it just sets a dirty flag but let's assume it does clears the
> cache. This code runs only when we draw the image of the IOSurface to a
> un-accelerated GraphicsContext. This is the case of applying filter, mask or
> clip-path. Do you think we need to optimize the memory in these cases only?


No, I don't think that's the case necessarily. At least at the time I worked on it on bug 231471.
Draw to snapshots, toDataURL, ImageBitmap, it's not clear if these don't have bitmap as destination.

> >> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:182
> >> +
> > 
> > I still think this could be a separate change, as I don't think it's entirely correct.
> 
> One layout test failed without this change on
> https://ews-build.s3-us-west-2.amazonaws.com/macOS-AppleSilicon-Big-Sur-
> Debug-WK2-Tests-EWS/459276-30746/results.html. It looks like the CGImage
> interpolation is different on AS. So to fix this bug I need to get the
> CGImage form the backend in WebProcess. Also I did not want to cause any
> temporary regression.
> 
> But why do think this is not correct?

So are you saying you get different results from 
a) WP asks GPUP to read the surface
b) WP reads the surface


I think you yourself are saying one or another is not correct?

[Said Abou-Hallawa]

Created attachment 459604 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 459529 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=459529&action=review

>>>> Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.cpp:-185
>>>> -    // Accelerated to/from unaccelerated image buffers need complex caching. We trust that
>>> 
>>> Here you are losing the intended memory optimization of clearing the cache after a draw
>> 
>> I am not sure whether CG clears the cache immediately after we draw the empty rect or it just sets a dirty flag but let's assume it does clears the cache. This code runs only when we draw the image of the IOSurface to a un-accelerated GraphicsContext. This is the case of applying filter, mask or clip-path. Do you think we need to optimize the memory in these cases only?
> 
> No, I don't think that's the case necessarily. At least at the time I worked on it on bug 231471.
> Draw to snapshots, toDataURL, ImageBitmap, it's not clear if these don't have bitmap as destination.

I put this code back in a new function named ImageBufferIOSurfaceBackend::finalizeDrawIntoContext(). This function will be called from ConcreteImageBuffer::draw().

[EWS]

Committed r294536 (250791@main): <https://commits.webkit.org/250791@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 459604 [details].