Bug 218934

Summary: [GPU Process] Share the NativeImage with GPU Process through a ShareableBitmap
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: CanvasAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, dino, ews-watchlist, gyuyoung.kim, ryuan.choi, sergio, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 217342    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
thorton: review+
Patch none

Description Said Abou-Hallawa 2020-11-13 20:33:49 PST
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.
Comment 1 Said Abou-Hallawa 2020-11-13 20:36:10 PST
Created attachment 414121 [details]
Patch
Comment 2 Said Abou-Hallawa 2020-11-13 22:57:32 PST
Created attachment 414125 [details]
Patch
Comment 3 Simon Fraser (smfr) 2020-11-18 13:17:31 PST
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()
Comment 4 Said Abou-Hallawa 2020-11-20 14:33:41 PST
Created attachment 414728 [details]
Patch
Comment 5 Said Abou-Hallawa 2020-11-20 14:54:31 PST
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.
Comment 6 Radar WebKit Bug Importer 2020-11-20 20:34:13 PST
<rdar://problem/71650376>
Comment 7 Said Abou-Hallawa 2020-12-03 21:10:44 PST
Created attachment 415383 [details]
Patch
Comment 8 Said Abou-Hallawa 2020-12-03 21:14:13 PST
Created attachment 415384 [details]
Patch
Comment 9 Tim Horton 2020-12-03 21:56:49 PST
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.
Comment 10 Said Abou-Hallawa 2020-12-04 10:54:56 PST
Created attachment 415431 [details]
Patch
Comment 11 EWS 2020-12-04 11:48:22 PST
Committed r270444: <https://trac.webkit.org/changeset/270444>

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