Bug 240100 - [GPU Process] [iOS] REGRESSION(r293570): Snapshot rendering is not scaled with the device scale factor
Summary: [GPU Process] [iOS] REGRESSION(r293570): Snapshot rendering is not scaled wit...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-05-04 17:24 PDT by Said Abou-Hallawa
Modified: 2022-05-05 01:20 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.76 KB, patch)
2022-05-04 17:30 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].