Bug 232411 - [GPU Process] Pattern should hold SourceImage which can be converted to a NativeImage only when needed
Summary: [GPU Process] Pattern should hold SourceImage which can be converted to a Nat...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on: 235467
Blocks: 234165 236508
  Show dependency treegraph
 
Reported: 2021-10-27 17:14 PDT by Cameron McCormack (:heycam)
Modified: 2022-02-11 13:26 PST (History)
23 users (show)

See Also:


Attachments
Patch (40.72 KB, patch)
2021-11-15 14:45 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (46.01 KB, patch)
2021-11-15 20:20 PST, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (46.06 KB, patch)
2021-11-15 20:36 PST, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (46.02 KB, patch)
2021-11-15 20:57 PST, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (46.05 KB, patch)
2021-11-15 21:08 PST, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (46.65 KB, patch)
2021-11-15 21:32 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (48.81 KB, patch)
2021-11-16 12:44 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (32.29 KB, patch)
2022-01-23 22:06 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (34.59 KB, patch)
2022-01-24 01:36 PST, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (34.69 KB, patch)
2022-01-24 01:55 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (49.39 KB, patch)
2022-01-25 01:55 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (51.21 KB, patch)
2022-01-25 20:41 PST, Said Abou-Hallawa
darin: review+
Details | Formatted Diff | Diff
Patch (50.47 KB, patch)
2022-01-31 16:48 PST, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron McCormack (:heycam) 2021-10-27 17:14:26 PDT
Currently all uses of createPattern() requires the GPU process to send a copy of the pattern source image data to the Web process, because CanvasPattern (and Pattern underneath it) works with a NativeImage.  We should be able to avoid this, and leave the pattern source data in the GPU process.  Maybe making Pattern work on ImageBuffers would help?
Comment 1 Radar WebKit Bug Importer 2021-11-03 17:15:19 PDT
<rdar://problem/84998738>
Comment 2 Said Abou-Hallawa 2021-11-15 14:45:26 PST
Created attachment 444306 [details]
Patch
Comment 3 Said Abou-Hallawa 2021-11-15 20:20:33 PST
Created attachment 444334 [details]
Patch
Comment 4 Said Abou-Hallawa 2021-11-15 20:36:16 PST
Created attachment 444336 [details]
Patch
Comment 5 Said Abou-Hallawa 2021-11-15 20:57:10 PST
Created attachment 444339 [details]
Patch
Comment 6 Said Abou-Hallawa 2021-11-15 21:08:49 PST
Created attachment 444341 [details]
Patch
Comment 7 Said Abou-Hallawa 2021-11-15 21:32:22 PST
Created attachment 444342 [details]
Patch
Comment 8 Said Abou-Hallawa 2021-11-16 12:44:44 PST
Created attachment 444423 [details]
Patch
Comment 9 Cameron McCormack (:heycam) 2021-11-16 13:43:22 PST
Comment on attachment 444423 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [GPU Process ] Pattern should hold an ImageBuffer which can be converted to a NativeImage when needed

"[GPU Process]" (without the space)

> Source/WebCore/ChangeLog:67
> +        Pass the HosWindow to SVGRenderingContext::createImageBuffer() so it can

"HostWindow"

> Source/WebCore/ChangeLog:74
> +        so  it can create a remote ImageBuffer if the GPU Process DOM rendering

"so it" (without the extra space)

> Source/WebKit/ChangeLog:3
> +        [GPU Process ] Pattern should hold an ImageBuffer which can be converted to a NativeImage when needed

"[GPU Process]" (without the space)

> Source/WebKit/ChangeLog:8
> +        Handle encoding amd decoding the SourceImage for recording and replaying

"and"

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1997
> +    return RefPtr<CanvasPattern> { CanvasPattern::create(imageBuffer.releaseNonNull(), repeatX, repeatY, originClean) };

Can drop the `<CanvasPattern>`

> Source/WebCore/platform/graphics/SourceImage.cpp:57
> +    auto nativeImage = ImageBuffer::sinkIntoNativeImage(WTFMove(imageBuffer));

sinkIntoNativeImage takes a RefPtr<ImageBuffer>, but imageBuffer here is a Ref<ImageBuffer>, so WTFMove isn't doing anything useful.

> Source/WebCore/platform/graphics/SourceImage.cpp:80
> +    auto imageBuffer = ImageBuffer::create(nativeImage->size(), RenderingMode::Unaccelerated, 1, DestinationColorSpace::SRGB(), PixelFormat::BGRA8);

We shouldn't use sRGB unconditionally here. It's really dependent on what the caller wants. So for a CanvasPattern object it would be the color space of the canvas. (I think this will cause a failure of some tests I added for wide gamut canvas and patterns, but sadly it won't show up on EWS, since wide gamut canvas is only available in Monterey+.)

> Source/WebCore/platform/graphics/SourceImage.h:43
> +    NativeImage* nativeImageIfExists() const;
> +    NativeImage* nativeImage();
> +
> +    ImageBuffer* imageBufferIfExists() const;
> +    ImageBuffer* imageBuffer();

nativeImage() and imageBuffer() will switch the current m_image value to be a NativeImage or ImageBuffer, respectively. Can we ever get into a situation where we alternate between wanting a NativeImage and an ImageBuffer? If so that would result in a bunch of churn.

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:290
> +        encoder << state.strokePattern->tileImageIdentifier();

Instead of grabbing out the tileImageIdentifier() here, should we add an encode() implementation for SourceImage and write out state.strokePattern->tileImage() here instead?

> Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:164
> -        ASSERT(std::holds_alternative<Ref<T>>(iterator->value));
> -        return std::get<Ref<T>>(iterator->value).ptr();
> +        if (auto* value = std::get_if<Ref<T>>(&iterator->value))
> +            return value->ptr();
> +        return nullptr;

I'm wondering if this is a valuable assertion to keep. Maybe it would be better to encode in the IPC arguments whether we're using a NativeImage or an ImageBuffer, rather than trying both when looking up the resource?

> Source/WebKit/WebProcess/GPU/graphics/RemoteDisplayListRecorderProxy.cpp:419
> +void RemoteDisplayListRecorderProxy::recordResourceUse(SourceImage& image)

Where do we call this from?
Comment 10 Cameron McCormack (:heycam) 2021-11-16 14:08:03 PST
Comment on attachment 444423 [details]
Patch

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

>> Source/WebCore/platform/graphics/SourceImage.cpp:57
>> +    auto nativeImage = ImageBuffer::sinkIntoNativeImage(WTFMove(imageBuffer));
> 
> sinkIntoNativeImage takes a RefPtr<ImageBuffer>, but imageBuffer here is a Ref<ImageBuffer>, so WTFMove isn't doing anything useful.

Ignore that. RefPtr(Ref<T>&&) will obviously do something useful.
Comment 11 Cameron McCormack (:heycam) 2021-12-01 23:22:56 PST
Comment on attachment 444423 [details]
Patch

Clearing review awaiting replies to the comments.
Comment 12 Said Abou-Hallawa 2022-01-23 22:06:32 PST
Comment on attachment 444423 [details]
Patch

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

>> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1997
>> +    return RefPtr<CanvasPattern> { CanvasPattern::create(imageBuffer.releaseNonNull(), repeatX, repeatY, originClean) };
> 
> Can drop the `<CanvasPattern>`

No we can't because the return type is ExceptionOr<RefPtr<CanvasPattern>>.  The compiler can't deduce the type of the RefPtr template in this case.

>> Source/WebCore/platform/graphics/SourceImage.cpp:80
>> +    auto imageBuffer = ImageBuffer::create(nativeImage->size(), RenderingMode::Unaccelerated, 1, DestinationColorSpace::SRGB(), PixelFormat::BGRA8);
> 
> We shouldn't use sRGB unconditionally here. It's really dependent on what the caller wants. So for a CanvasPattern object it would be the color space of the canvas. (I think this will cause a failure of some tests I added for wide gamut canvas and patterns, but sadly it won't show up on EWS, since wide gamut canvas is only available in Monterey+.)

This function is only called form InspectorCanvas::buildArrayForCanvasPattern() which uses sRGB. If any other call is introduced in the future we can pass DestinationColorSpace as a parameter to this function.

>> Source/WebCore/platform/graphics/SourceImage.h:43
>> +    ImageBuffer* imageBuffer();
> 
> nativeImage() and imageBuffer() will switch the current m_image value to be a NativeImage or ImageBuffer, respectively. Can we ever get into a situation where we alternate between wanting a NativeImage and an ImageBuffer? If so that would result in a bunch of churn.

This is true but most of the time we create a temporary ImageBuffer and then sink it to a NativeImage. This method will delay creating the NativeImage till we actually need it. This is exactly what we need for applying the pattern in the GPUProcess where we create the ImageBuffer in the WebProcess but we need the NativeImage in GPUProcess.

>> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:290
>> +        encoder << state.strokePattern->tileImageIdentifier();
> 
> Instead of grabbing out the tileImageIdentifier() here, should we add an encode() implementation for SourceImage and write out state.strokePattern->tileImage() here instead?

This is done in r288412.

>> Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:164
>> +        return nullptr;
> 
> I'm wondering if this is a valuable assertion to keep. Maybe it would be better to encode in the IPC arguments whether we're using a NativeImage or an ImageBuffer, rather than trying both when looking up the resource?

This assertion was removed in r287911.

>> Source/WebKit/WebProcess/GPU/graphics/RemoteDisplayListRecorderProxy.cpp:419
>> +void RemoteDisplayListRecorderProxy::recordResourceUse(SourceImage& image)
> 
> Where do we call this from?

This is called from 

Recorder::appendStateChangeItem()
{
    recordResourceUse(pattern->tileImage());
}


Recorder::drawFilteredImageBuffer()
{
    recordResourceUse(feImage.sourceImage())
}
Comment 13 Said Abou-Hallawa 2022-01-23 22:06:51 PST
Created attachment 449781 [details]
Patch
Comment 14 Darin Adler 2022-01-23 23:26:26 PST
Comment on attachment 449781 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1956
> +    return RefPtr<CanvasPattern> { CanvasPattern::create({ nativeImage.releaseNonNull() }, repeatX, repeatY, originClean) };

Since you are touching these source lines, it’s worth noting we can probably change them to RefPtr instead of RefPtr<CanvasPattern> now.

> Source/WebCore/platform/graphics/Pattern.h:68
> +    SourceImage& tileImage() const { return m_tileImage; }

Can we change this to return a const reference? As far as I can tell, this is only used for encoding, and so there's no need for this to return a non-const reference, even though this still needs to be mutable in the class of rthe benefit of the tileNativeImage() and tileImagebuffer() functions.
Comment 15 Said Abou-Hallawa 2022-01-24 01:36:50 PST
Created attachment 449788 [details]
Patch
Comment 16 Said Abou-Hallawa 2022-01-24 01:55:25 PST
Created attachment 449791 [details]
Patch
Comment 17 Said Abou-Hallawa 2022-01-24 02:34:32 PST
Comment on attachment 449781 [details]
Patch

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

>> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1956
>> +    return RefPtr<CanvasPattern> { CanvasPattern::create({ nativeImage.releaseNonNull() }, repeatX, repeatY, originClean) };
> 
> Since you are touching these source lines, it’s worth noting we can probably change them to RefPtr instead of RefPtr<CanvasPattern> now.

The type can't be dropped. Because the function returns ExceptionOr<RefPtr<CanvasPattern>>, the compiler can't to deduce the type of the returned RefPtr template and gives this error message:

No viable constructor or deduction guide for deduction of template arguments of 'RefPtr'

>> Source/WebCore/platform/graphics/Pattern.h:68
>> +    SourceImage& tileImage() const { return m_tileImage; }
> 
> Can we change this to return a const reference? As far as I can tell, this is only used for encoding, and so there's no need for this to return a non-const reference, even though this still needs to be mutable in the class of rthe benefit of the tileNativeImage() and tileImagebuffer() functions.

Yes it can. Fixed.
Comment 18 Darin Adler 2022-01-24 16:38:45 PST
Comment on attachment 449781 [details]
Patch

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

>>> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1956
>>> +    return RefPtr<CanvasPattern> { CanvasPattern::create({ nativeImage.releaseNonNull() }, repeatX, repeatY, originClean) };
>> 
>> Since you are touching these source lines, it’s worth noting we can probably change them to RefPtr instead of RefPtr<CanvasPattern> now.
> 
> The type can't be dropped. Because the function returns ExceptionOr<RefPtr<CanvasPattern>>, the compiler can't to deduce the type of the returned RefPtr template and gives this error message:
> 
> No viable constructor or deduction guide for deduction of template arguments of 'RefPtr'

I will fix that "no viable" thing. That is a bug where RefPtr can’t deduce from Ref for some reason, but should be easy to fix in RefPtr.h or Ref.h. No need to worry about it in this patch.
Comment 19 Said Abou-Hallawa 2022-01-25 01:55:24 PST
Created attachment 449904 [details]
Patch
Comment 20 Said Abou-Hallawa 2022-01-25 20:41:37 PST
Created attachment 449994 [details]
Patch
Comment 21 Said Abou-Hallawa 2022-01-25 22:20:07 PST
The crash on mac-AS-debug-wk2 due to deleting the NativeImage not on the main thread does exist without this change and it has been slowing down this bot: https://ews-build.s3-us-west-2.amazonaws.com/macOS-AppleSilicon-Big-Sur-Debug-WK2-Tests-EWS/r449984-22190/results.html.
Comment 22 Said Abou-Hallawa 2022-01-26 11:45:17 PST
Comment on attachment 449994 [details]
Patch

I fixed two layout tests failures; one of them exists before this patch. The patch has changed significantly since it was r+ed. So I am setting the r? back for the final version.
Comment 23 Darin Adler 2022-01-27 11:39:36 PST
Comment on attachment 449994 [details]
Patch

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

> Source/WebCore/platform/graphics/SourceImage.cpp:108
> +            return IntSize();

Maybe just use "{ }" instead of "IntSize()".

> Source/WebCore/platform/graphics/SourceImage.cpp:110
> +            }
> +        );

I would outdent these two lines by 4 spaces.

> Source/WebCore/platform/graphics/displaylists/DisplayListResourceHeap.h:45
> +    virtual std::optional<SourceImage> getSourceImage(RenderingResourceIdentifier) const = 0;

WebKit coding style tells us to not use get for all these function names, but it seems we have diverged from that tradition here in DisplayList.

> Source/WebCore/platform/graphics/displaylists/DisplayListResourceHeap.h:82
> +            return SourceImage(*nativeImage);

I might have written:

    return { { *nativeImage } };

Or, if one of these works and helps with reference count churn (not sure either works):

    return { { WTFMove(*nativeImage) } };
    return { { nativeImage.releaseNonNull() ) } };

> Source/WebCore/platform/graphics/displaylists/DisplayListResourceHeap.h:85
> +            return SourceImage(*imageBuffer);

Ditto.

> Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:98
> +            return WebCore::SourceImage(*nativeImage);

Ditto.

> Source/WebKit/GPUProcess/graphics/QualifiedResourceHeap.h:101
> +            return WebCore::SourceImage(*imageBuffer);

Ditto.
Comment 24 Said Abou-Hallawa 2022-01-31 16:48:18 PST
Created attachment 450475 [details]
Patch
Comment 25 EWS 2022-01-31 18:58:33 PST
Committed r288865 (246617@main): <https://commits.webkit.org/246617@main>

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