Bug 227748 – [GPU Process] Enable drawing the SVGImage in the GPU Process

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Myles C. Maxfield]

Reverts https://bugs.webkit.org/show_bug.cgi?id=227519

[Radar WebKit Bug Importer]

<rdar://problem/80582699>

[Said Abou-Hallawa]

We need to revert all these changes:

r279722
r284740
r283531

[Said Abou-Hallawa]

The intermediate ImageBuffers of the SVGImage need to be "really" compatible with the underlying ImageBuffer of the GraphicsContext. This is tracked by bug 235758.

[Said Abou-Hallawa]

Created attachment 451181 [details]
Patch

[Said Abou-Hallawa]

Created attachment 451502 [details]
Patch

[Said Abou-Hallawa]

Created attachment 451869 [details]
Patch

[Sam Weinig]

Comment on attachment 451869 [details]
Patch

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

> Source/WebCore/svg/graphics/SVGImage.cpp:206
> +    auto imageBuffer = ImageBuffer::create(size(), RenderingMode::Unaccelerated, 1, DestinationColorSpace::SRGB(), PixelFormat::BGRA8);

Why is this being hard coded to SRGB?

[Said Abou-Hallawa]

Created attachment 452221 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 452221 [details]
Patch

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

> Source/WebCore/svg/graphics/SVGImage.cpp:213
> +    auto imageBuffer = ImageBuffer::create(size(), renderingMode, ShouldUseDisplayList::No, RenderingPurpose::DOM, 1, colorSpace, PixelFormat::BGRA8, hostWindow);

Why isn't this createCompatibleBuffer()?

[Said Abou-Hallawa]

Comment on attachment 452221 [details]
Patch

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

>> Source/WebCore/svg/graphics/SVGImage.cpp:213
>> +    auto imageBuffer = ImageBuffer::create(size(), renderingMode, ShouldUseDisplayList::No, RenderingPurpose::DOM, 1, colorSpace, PixelFormat::BGRA8, hostWindow);
> 
> Why isn't this createCompatibleBuffer()?

Because it is hard to a GraphicsContext for all the callers. Here are a few examples:

- (void)getPreviewSnapshotImage:(CGImageRef*)cgImage andRects:(NSArray **)rects
+ (CGImageRef)imageForURL:(NSURL *)url
- (WebUITextIndicatorData *)initWithImage:(CGImageRef)image textIndicatorData:(const WebCore::TextIndicatorData&)indicatorData scale:(CGFloat)scale
static RetainPtr<UIImage> uiImageForImage(Image* image)

[Said Abou-Hallawa]

Comment on attachment 452221 [details]
Patch

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

>>> Source/WebCore/svg/graphics/SVGImage.cpp:213
>>> +    auto imageBuffer = ImageBuffer::create(size(), renderingMode, ShouldUseDisplayList::No, RenderingPurpose::DOM, 1, colorSpace, PixelFormat::BGRA8, hostWindow);
>> 
>> Why isn't this createCompatibleBuffer()?
> 
> Because it is hard to a GraphicsContext for all the callers. Here are a few examples:
> 
> - (void)getPreviewSnapshotImage:(CGImageRef*)cgImage andRects:(NSArray **)rects
> + (CGImageRef)imageForURL:(NSURL *)url
> - (WebUITextIndicatorData *)initWithImage:(CGImageRef)image textIndicatorData:(const WebCore::TextIndicatorData&)indicatorData scale:(CGFloat)scale
> static RetainPtr<UIImage> uiImageForImage(Image* image)

Because it is hard to *get* a GraphicsContext ...

[Simon Fraser (smfr)]

Comment on attachment 452221 [details]
Patch

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

>>>> Source/WebCore/svg/graphics/SVGImage.cpp:213
>>>> +    auto imageBuffer = ImageBuffer::create(size(), renderingMode, ShouldUseDisplayList::No, RenderingPurpose::DOM, 1, colorSpace, PixelFormat::BGRA8, hostWindow);
>>> 
>>> Why isn't this createCompatibleBuffer()?
>> 
>> Because it is hard to a GraphicsContext for all the callers. Here are a few examples:
>> 
>> - (void)getPreviewSnapshotImage:(CGImageRef*)cgImage andRects:(NSArray **)rects
>> + (CGImageRef)imageForURL:(NSURL *)url
>> - (WebUITextIndicatorData *)initWithImage:(CGImageRef)image textIndicatorData:(const WebCore::TextIndicatorData&)indicatorData scale:(CGFloat)scale
>> static RetainPtr<UIImage> uiImageForImage(Image* image)
> 
> Because it is hard to *get* a GraphicsContext ...

I see. This is just getting a NativeImage from an SVGImage without any notion of what the caller will do with it. In that case it might be risky to sometimes return an Accelerated image.

For the code paths involving the GPUP, it might make more sense to create a version of this that takes the destination context.

[Said Abou-Hallawa]

Comment on attachment 452221 [details]
Patch

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

>>>>> Source/WebCore/svg/graphics/SVGImage.cpp:213
>>>>> +    auto imageBuffer = ImageBuffer::create(size(), renderingMode, ShouldUseDisplayList::No, RenderingPurpose::DOM, 1, colorSpace, PixelFormat::BGRA8, hostWindow);
>>>> 
>>>> Why isn't this createCompatibleBuffer()?
>>> 
>>> Because it is hard to a GraphicsContext for all the callers. Here are a few examples:
>>> 
>>> - (void)getPreviewSnapshotImage:(CGImageRef*)cgImage andRects:(NSArray **)rects
>>> + (CGImageRef)imageForURL:(NSURL *)url
>>> - (WebUITextIndicatorData *)initWithImage:(CGImageRef)image textIndicatorData:(const WebCore::TextIndicatorData&)indicatorData scale:(CGFloat)scale
>>> static RetainPtr<UIImage> uiImageForImage(Image* image)
>> 
>> Because it is hard to *get* a GraphicsContext ...
> 
> I see. This is just getting a NativeImage from an SVGImage without any notion of what the caller will do with it. In that case it might be risky to sometimes return an Accelerated image.
> 
> For the code paths involving the GPUP, it might make more sense to create a version of this that takes the destination context.

I can add the new version for nativeImage in a separate patch. Or maybe I can pass an optional pointer to GraphicsContext to the existing one.

[EWS]

Committed r289981 (247367@main): <https://commits.webkit.org/247367@main>

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