Bug 239527 – [GPU Process] Make WebImage be backed by ImageBuffer

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

[Said Abou-Hallawa]

WebImage is used to take page snapshots. Currently a snapshot can be drawn in WebProcess and used by UIProcess. With enabling GPUProcess for rendering, the snapshot will be recorded in WebProcess, drawn in GPUProcess and used in UIProcess. This will require replacing the ShareableBitmap in WebImage by ImageBuffer.

[Said Abou-Hallawa]

rdar://91113628

[Said Abou-Hallawa]

Created attachment 457949 [details]
Patch

[Said Abou-Hallawa]

Created attachment 457963 [details]
Patch

[Said Abou-Hallawa]

Created attachment 458006 [details]
Patch

[Said Abou-Hallawa]

Created attachment 458014 [details]
Patch

[Said Abou-Hallawa]

Created attachment 458114 [details]
Patch

[Said Abou-Hallawa]

Created attachment 458240 [details]
Patch

[Said Abou-Hallawa]

Created attachment 458243 [details]
Patch

[Said Abou-Hallawa]

Created attachment 458244 [details]
Patch

[Said Abou-Hallawa]

Created attachment 458331 [details]
Patch

[Said Abou-Hallawa]

Created attachment 458333 [details]
Patch

[Said Abou-Hallawa]

Created attachment 458338 [details]
Patch

[Said Abou-Hallawa]

Created attachment 458477 [details]
Patch

[Said Abou-Hallawa]

(In reply to Said Abou-Hallawa from comment #13)
> Created attachment 458477 [details]
> Patch

This patch fixes the failures in the api-mac tests. The change in WebChromeClient::createImageBuffer() fixes the timeout in two api tests.

[Tim Horton]

Comment on attachment 458477 [details]
Patch

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

> Source/WebCore/page/FrameSnapshotting.cpp:84
> +    auto& document = *frame.document();

Should this be a strong reference?

> Source/WebCore/platform/graphics/ConcreteImageBuffer.h:296
> +        ref(); // Balanced by deref below.

Could this be a RefPtr that gets captured by the block, instead of this raw ref/deref?

> Source/WebKit/Shared/UserData.cpp:402
> +        result = WebImage::create(*paramters, WTFMove(handle));

Spelling: "paramters"

> Source/WebKit/Shared/WebImage.cpp:38
> +    // Create shareable RemoteImageBuffer if GPU Process is enabled.

I kind of see the need for these comments, but they feel indicative of how weird some of this code has gotten :)

I don't think they're quite worded correctly, though, because e.g. this code has NO IDEA why ChromeClient might or might not give you a imagebuffer (doesn't know that it's about GPUP), and would get stale if that reason changed. So maybe we leave them out? Or word them differently ("allow the chrome client to override the image if it wants to" etc. etc. but then it's fairly obvious from reading the code).

> Source/WebKit/Shared/WebImage.h:48
> +    static RefPtr<WebImage> create(const WebCore::IntSize&, ImageOptions, const WebCore::DestinationColorSpace& = WebCore::DestinationColorSpace::SRGB(), WebCore::ChromeClient* = nullptr);

Wasn't Sam pushing us in the direction of "never default to SRGB, always make clients say it, so you can fix them one-by-one"?

[Said Abou-Hallawa]

Created attachment 458484 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 458477 [details]
Patch

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

>> Source/WebCore/page/FrameSnapshotting.cpp:84
>> +    auto& document = *frame.document();
> 
> Should this be a strong reference?

Fixed. This was changed to
    Ref document = *frame.document();

>> Source/WebCore/platform/graphics/ConcreteImageBuffer.h:296
>> +        ref(); // Balanced by deref below.
> 
> Could this be a RefPtr that gets captured by the block, instead of this raw ref/deref?

Unfortunately we can't do that. A lambda can be used as a pointer to function as long as it doesn't capture variables.

>> Source/WebKit/Shared/UserData.cpp:402
>> +        result = WebImage::create(*paramters, WTFMove(handle));
> 
> Spelling: "paramters"

Fixed.

>> Source/WebKit/Shared/WebImage.cpp:38
>> +    // Create shareable RemoteImageBuffer if GPU Process is enabled.
> 
> I kind of see the need for these comments, but they feel indicative of how weird some of this code has gotten :)
> 
> I don't think they're quite worded correctly, though, because e.g. this code has NO IDEA why ChromeClient might or might not give you a imagebuffer (doesn't know that it's about GPUP), and would get stale if that reason changed. So maybe we leave them out? Or word them differently ("allow the chrome client to override the image if it wants to" etc. etc. but then it's fairly obvious from reading the code).

I agree. Comments were removed.

>> Source/WebKit/Shared/WebImage.h:48
>> +    static RefPtr<WebImage> create(const WebCore::IntSize&, ImageOptions, const WebCore::DestinationColorSpace& = WebCore::DestinationColorSpace::SRGB(), WebCore::ChromeClient* = nullptr);
> 
> Wasn't Sam pushing us in the direction of "never default to SRGB, always make clients say it, so you can fix them one-by-one"?

Fixed. Default color space was removed.

[EWS]

Committed r293570 (250084@main): <https://commits.webkit.org/250084@main>

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