Bug 232411 – [GPU Process] Pattern should hold SourceImage which can be converted to a NativeImage only when needed

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Cameron McCormack (:heycam)]

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?

[Radar WebKit Bug Importer]

<rdar://problem/84998738>

[Said Abou-Hallawa]

Created attachment 444306 [details]
Patch

[Said Abou-Hallawa]

Created attachment 444334 [details]
Patch

[Said Abou-Hallawa]

Created attachment 444336 [details]
Patch

[Said Abou-Hallawa]

Created attachment 444339 [details]
Patch

[Said Abou-Hallawa]

Created attachment 444341 [details]
Patch

[Said Abou-Hallawa]

Created attachment 444342 [details]
Patch

[Said Abou-Hallawa]

Created attachment 444423 [details]
Patch

[Cameron McCormack (:heycam)]

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?

[Cameron McCormack (:heycam)]

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.

[Cameron McCormack (:heycam)]

Comment on attachment 444423 [details]
Patch

Clearing review awaiting replies to the comments.

[Said Abou-Hallawa]

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())
}

[Said Abou-Hallawa]

Created attachment 449781 [details]
Patch

[Darin Adler]

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.

[Said Abou-Hallawa]

Created attachment 449788 [details]
Patch

[Said Abou-Hallawa]

Created attachment 449791 [details]
Patch

[Said Abou-Hallawa]

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.

[Darin Adler]

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.

[Said Abou-Hallawa]

Created attachment 449904 [details]
Patch

[Said Abou-Hallawa]

Created attachment 449994 [details]
Patch

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

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.

[Darin Adler]

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.

[Said Abou-Hallawa]

Created attachment 450475 [details]
Patch

[EWS]

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].