Bug 218934 – [GPU Process] Share the NativeImage with GPU Process through a ShareableBitmap

• Component: Canvas
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

We need to replace the decoded CGImageRef with a ShareableBitmap which can be shared and used by Web Process and GPU Process. A CGImageRef can be created from ShareableBitmap when drawing the image is needed.

[Said Abou-Hallawa]

Created attachment 414121 [details]
Patch

[Said Abou-Hallawa]

Created attachment 414125 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 414125 [details]
Patch

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

> Source/WebCore/platform/graphics/NativeImage.cpp:67
> +    m_observer = &observer;
> +    m_platformImage = nullptr;

It's weird (and very non-obvious from the API) that seeing an Observer clears the platform image.

> Source/WebCore/platform/graphics/NativeImage.cpp:81
> +        ASSERT(m_observer);
> +        m_platformImage = m_observer->platformImageOfNativeImage(m_renderingResourceIdentifier);

But now we have both an observer AND a native image!

> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:49
> +    m_nativeImages.add(renderingResourceIdentifier, NativeImage::create(*this, renderingResourceIdentifier));

Why don't we just create the PlatformImage here and pass it into the NativeImage constructor?

> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.h:51
> +    WebCore::PlatformImagePtr platformImageOfNativeImage(WebCore::RenderingResourceIdentifier) override;

platformImageForIdentifier()

[Said Abou-Hallawa]

Created attachment 414728 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 414125 [details]
Patch

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

>> Source/WebCore/platform/graphics/NativeImage.cpp:67
>> +    m_platformImage = nullptr;
> 
> It's weird (and very non-obvious from the API) that seeing an Observer clears the platform image.

I renamed NativeImage::Observer to be NativeImage::ImageSource. So setting the ImageSource of the NativeImage should clear the platform image since it will provide it when it is needed.

>> Source/WebCore/platform/graphics/NativeImage.cpp:81
>> +        m_platformImage = m_observer->platformImageOfNativeImage(m_renderingResourceIdentifier);
> 
> But now we have both an observer AND a native image!

Yes I did not want to break the non GPUP scenario if the same NativeImage is drawn later in WebP. This can be revisited later when all the NativeImages are drawn either in GPUP or WebP but not both.

>> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:49
>> +    m_nativeImages.add(renderingResourceIdentifier, NativeImage::create(*this, renderingResourceIdentifier));
> 
> Why don't we just create the PlatformImage here and pass it into the NativeImage constructor?

Done. In fact this function RemoteResourceCache::cacheNativeImage() now takes a NativeImage. This cleans the interface and makes caching the NativeImage look very similar to caching the ImageBuffer.

>> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.h:51
>> +    WebCore::PlatformImagePtr platformImageOfNativeImage(WebCore::RenderingResourceIdentifier) override;
> 
> platformImageForIdentifier()

Name was changed.

[Radar WebKit Bug Importer]

<rdar://problem/71650376>

[Said Abou-Hallawa]

Created attachment 415383 [details]
Patch

[Said Abou-Hallawa]

Created attachment 415384 [details]
Patch

[Tim Horton]

Comment on attachment 415384 [details]
Patch

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

> Source/WebKit/Shared/ShareableBitmap.h:133
> +    WebCore::PlatformImagePtr createPlatfromImage() { return makeCGImageCopy(); }

Typo: platfrom. it's in a few places

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:-1103
> -void ArgumentCoder<Ref<NativeImage>>::encode(Encoder& encoder, const Ref<NativeImage>& image)

I'm shocked this wasn't used for anything else, I thought for sure it predated this project.

[Said Abou-Hallawa]

Created attachment 415431 [details]
Patch

[EWS]

Committed r270444: <https://trac.webkit.org/changeset/270444>

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