Bug 240100

Summary: [GPU Process] [iOS] REGRESSION(r293570): Snapshot rendering is not scaled with the device scale factor
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: Layout and RenderingAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=239527
https://bugs.webkit.org/show_bug.cgi?id=225081
Attachments:
Description Flags
Patch none

Description Said Abou-Hallawa 2022-05-04 17:24:57 PDT 
In r293570, the GPU Process was enabled for snapshot images. Before r293570, a local ImageBuffer was created in snapshotFrameRectWithClip(). Then in WebFrame::createSelectionSnapshot(), the pixels of the ImageBuffer were copied to a WebImage, which was backed by a ShareableBitmap.

r293570 made snapshotFrameRectWithClip() create a remote ImageBuffer which is moved to a WebImage in WebFrame::createSelectionSnapshot(). This remote ImageBuffer has to have ImageBufferShareableBitmapBackend so it can provide ShareableBitmap without allocating any extra memory.

The problem is in initializing the GraphicsContext of ImageBufferShareableBitmapBackend. The scaling factor is not set to the CTM of the ImageBufferShareableBitmapBackend context. See the comment in ImageBufferShareableBitmapBackend constructor and notice that this function does not call applyBaseTransformToContext().

A simple approach to fix this bug is to make snapshotFrameRectWithClip() handle the scaling outside the ImageBuffer creation. This is similar to what we do in GraphicsContext::createAlignedImageBuffer() where we scale the size and create the ImageBuffer with scaleFactor = 1.
Comment 1 Said Abou-Hallawa 2022-05-04 17:25:24 PDT 
rdar://92635752
Comment 2 Said Abou-Hallawa 2022-05-04 17:30:05 PDT 
Created attachment 458839 [details]
Patch
Comment 3 Simon Fraser (smfr) 2022-05-04 19:14:59 PDT 
Comment on attachment 458839 [details]
Patch

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

> Source/WebCore/page/FrameSnapshotting.cpp:124
> -    auto buffer = ImageBuffer::create(imageRect.size(), purpose, scaleFactor, options.colorSpace, options.pixelFormat, { }, { hostWindow });
> +    auto buffer = ImageBuffer::create(scaledImageRect.size(), purpose, 1, options.colorSpace, options.pixelFormat, { }, { hostWindow });

It's odd that ImageBuffer::create() takes a scale factor argument, but apparently that doesn't behave the way we want?
Comment 4 Said Abou-Hallawa 2022-05-04 21:53:46 PDT 
Comment on attachment 458839 [details]
Patch

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

>> Source/WebCore/page/FrameSnapshotting.cpp:124
>> +    auto buffer = ImageBuffer::create(scaledImageRect.size(), purpose, 1, options.colorSpace, options.pixelFormat, { }, { hostWindow });
> 
> It's odd that ImageBuffer::create() takes a scale factor argument, but apparently that doesn't behave the way we want?

I think it is okay that we want the scaling of the ImageBuffer to happen at the creation time. And it is working for all the other backends. But there is a problem with ImageBufferShareableBitmapBackend which I can't figure it out so far. 

This patch is correct and it fixes the snapshot bug. But we do have a bug with creating scaled unaccelerated remote ImageBuffer which we never needed before. All the canvas ImageBuffers are not scaled and all the layer ImageBuffers are accelerated. So I will file another bug for this issue which can be fixed later.
Comment 5 EWS 2022-05-05 01:20:26 PDT 
Committed r293825 (250298@main): <https://commits.webkit.org/250298@main>

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