Bug 217596 – [GPU Process][Resource caching 6/7]: Cache the NativeImage in GPU Process and allow referencing it with its RemoteResourceIdentifier

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Said Abou-Hallawa]

Currently we draw the NativeImage into a ShareableBitmap and send it to the GPU Process. The GPU Process receives the ShareableBitmap and convert it to a NativeImage. These steps happen every time the same NativeImage is drawn. This would require memory, CPU cycles and IPC communications. We need to cache the BitmapImage frame once and reference it with its RemoteResourceIdentifier.

The life cycle of the cached NativeImages will be determined by the Web Process. If the BitmapImage decides to destroy its decoded data, the GPU Process will delete the same NativeImages which correspond to the same deleted decoded frames.

[Said Abou-Hallawa]

Created attachment 411086 [details]
Patch

[Said Abou-Hallawa]

Created attachment 411096 [details]
Patch

[Said Abou-Hallawa]

Created attachment 411098 [details]
Patch

[Said Abou-Hallawa]

Created attachment 411125 [details]
Patch for review

[Radar WebKit Bug Importer]

<rdar://problem/70424954>

[Said Abou-Hallawa]

Created attachment 412064 [details]
Patch

[Said Abou-Hallawa]

Created attachment 412070 [details]
Patch for review

[Said Abou-Hallawa]

Created attachment 412586 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 412586 [details]
Patch

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

> Source/WebCore/platform/graphics/BitmapImage.cpp:214
> +static inline void drawNativeImage(BitmapImage& image, GraphicsContext& context, const NativeImagePtr& nativeImage, const FloatRect& destRect, const FloatRect& srcRect, const IntSize& srcSize, const ImagePaintingOptions& options)

It's really confusing for this to take both a BitmapImage and a NativeImage. Can the latter be derived from the former?

> Source/WebCore/platform/graphics/GraphicsContext.cpp:734
> +void GraphicsContext::drawNativeImage(const NativeImagePtr& image, const FloatSize& imageSize, const FloatRect& destRect, const FloatRect& srcRect, const ImagePaintingOptions& options, CachedImage* cachedImage)

This is a layering violation. You can't use CachedImage in platform code.

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:1736
> +    WEBCORE_EXPORT DrawNativeImage(const NativeImageType&, const FloatSize& imageSize, const FloatRect& destRect, const FloatRect& srcRect, const ImagePaintingOptions&);

I don't think it makes sense to encode draw native image. If we're drawing an image with GPUP, can't it always be represented as an ImageBuffer?

> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:190
> +void Recorder::drawNativeImage(const NativeImagePtr& image, const FloatSize& imageSize, const FloatRect& destRect, const FloatRect& srcRect, const ImagePaintingOptions& options, CachedImage* cachedImage)

layering violation.

[Said Abou-Hallawa]

The life cycle of the PlatformImage will be controlled by a new called called NativeImage, see bug 218427.

[Said Abou-Hallawa]

Created attachment 412945 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Said Abou-Hallawa]

Created attachment 412947 [details]
Patch for review

[Myles C. Maxfield]

Comment on attachment 412947 [details]
Patch for review

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

> Source/WebCore/platform/graphics/GraphicsContext.cpp:761
> +    if (m_impl && image.isPDFDocumentImage())

Why should we need to check PDF document images here? This seems to break encapsulation.

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:179
> +void RemoteRenderingBackend::cachePlatformImage(const PlatformImagePtr& image, RenderingResourceIdentifier renderingResourceIdentifier)

I don't see the type PlatformImagePtr defined anywhere.

> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:84
> +bool RemoteResourceCacheProxy::lockRemoteNativeImageForRemoteClient(NativeImage& image, RenderingResourceIdentifier remoteClientIdentifier)

I don't see the NativeImage type defined anywhere.

[Myles C. Maxfield]

Comment on attachment 412947 [details]
Patch for review

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

> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.cpp:51
> +PlatformImagePtr RemoteResourceCache::cachedPlatformImage(RenderingResourceIdentifier renderingResourceIdentifier)

Nit: It's not really a "cache" if there is no automatic eviction. This seems more like a "repository" or something like that.

[Myles C. Maxfield]

Comment on attachment 412947 [details]
Patch for review

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

This looks like a good direction.

> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.h:59
> +    void nativeImageWillBeDeleted(WebCore::RenderingResourceIdentifier) override;

final?

[Myles C. Maxfield]

Comment on attachment 412947 [details]
Patch for review

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

>> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:84
>> +bool RemoteResourceCacheProxy::lockRemoteNativeImageForRemoteClient(NativeImage& image, RenderingResourceIdentifier remoteClientIdentifier)
> 
> I don't see the NativeImage type defined anywhere.

Oh, it's in https://bugs.webkit.org/show_bug.cgi?id=218427

[Said Abou-Hallawa]

Created attachment 413740 [details]
Patch

[Said Abou-Hallawa]

(In reply to Said Abou-Hallawa from comment #18)
> Created attachment 413740 [details]
> Patch

Three notes about this patch:

1. PDFDocumentImage will be broken but the rest of images should be fine with this patch. Once it is landed I am going to file a bug for PDFDocumentImage.

2. The following DisplayList items should be removed in a follow-up patch since they are used with this patch:

    DrawImage,
    DrawTiledImage,
    DrawTiledScaledImage,

3. Drawing a canvas into another canvas is blocked by bug 218773.

[Said Abou-Hallawa]

Created attachment 413742 [details]
Patch

[Said Abou-Hallawa]

(In reply to Said Abou-Hallawa from comment #19)
> (In reply to Said Abou-Hallawa from comment #18)
> > Created attachment 413740 [details]
> > Patch
> 
> Three notes about this patch:
> 
> 1. PDFDocumentImage will be broken but the rest of images should be fine
> with this patch. Once it is landed I am going to file a bug for
> PDFDocumentImage.
> 
> 2. The following DisplayList items should be removed in a follow-up patch
> since they are used with this patch:
> 
>     DrawImage,
>     DrawTiledImage,
>     DrawTiledScaledImage,
> 
> 3. Drawing a canvas into another canvas is blocked by bug 218773.

A fourth comment

4. I think the class ImageDataReference can be replaced by just

using ImageDataReference = RefPtr<ImageData>;

[Wenson Hsieh]

Comment on attachment 413742 [details]
Patch

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

I think this patch looks pretty good! A couple of minor comments:

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:1280
> +    DrawNativeImage(RenderingResourceIdentifier renderingResourceIdentifier, const FloatSize& imageSize, const FloatRect& destRect, const FloatRect& srcRect, const ImagePaintingOptions& options)

Nit - I think we might actually be able to remove this and just use the default constructor instead?

> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:52
> +bool RemoteResourceCacheProxy::cacheNativeImage(NativeImage& image)

Nit - it wasn't obvious at first what the bool argument here represents; could we use an enum instead, like `enum class AddedNewEntry : bool { No, Yes };`?

[Said Abou-Hallawa]

Created attachment 413754 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 413742 [details]
Patch

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

>> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:1280
>> +    DrawNativeImage(RenderingResourceIdentifier renderingResourceIdentifier, const FloatSize& imageSize, const FloatRect& destRect, const FloatRect& srcRect, const ImagePaintingOptions& options)
> 
> Nit - I think we might actually be able to remove this and just use the default constructor instead?

What do you mean by the default constructor in this context? The default constructor does not expect any parameters and all the members of this class are private.

>> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:52
>> +bool RemoteResourceCacheProxy::cacheNativeImage(NativeImage& image)
> 
> Nit - it wasn't obvious at first what the bool argument here represents; could we use an enum instead, like `enum class AddedNewEntry : bool { No, Yes };`?

I changed the code such that we do not need to return a bool from this function. I made sending the CacheNativeImage message initiated by this function. This will correspond to what we do in RemoteResourceCacheProxy::nativeImageWillBeDeleted() where we initiate sending the ReleaseRemoteResource message.

[Wenson Hsieh]

Comment on attachment 413742 [details]
Patch

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

>>> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:1280
>>> +    DrawNativeImage(RenderingResourceIdentifier renderingResourceIdentifier, const FloatSize& imageSize, const FloatRect& destRect, const FloatRect& srcRect, const ImagePaintingOptions& options)
>> 
>> Nit - I think we might actually be able to remove this and just use the default constructor instead?
> 
> What do you mean by the default constructor in this context? The default constructor does not expect any parameters and all the members of this class are private.

My bad — that's correct, we need this constructor because the members are private.

>>> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:52
>>> +bool RemoteResourceCacheProxy::cacheNativeImage(NativeImage& image)
>> 
>> Nit - it wasn't obvious at first what the bool argument here represents; could we use an enum instead, like `enum class AddedNewEntry : bool { No, Yes };`?
> 
> I changed the code such that we do not need to return a bool from this function. I made sending the CacheNativeImage message initiated by this function. This will correspond to what we do in RemoteResourceCacheProxy::nativeImageWillBeDeleted() where we initiate sending the ReleaseRemoteResource message.

👍🏻

[Said Abou-Hallawa]

(In reply to Said Abou-Hallawa from comment #21)
> (In reply to Said Abou-Hallawa from comment #19)
> > (In reply to Said Abou-Hallawa from comment #18)
> > > Created attachment 413740 [details]
> > > Patch
> > 
> > Three notes about this patch:
> > 
> > 1. PDFDocumentImage will be broken but the rest of images should be fine
> > with this patch. Once it is landed I am going to file a bug for
> > PDFDocumentImage.
> > 
> > 2. The following DisplayList items should be removed in a follow-up patch
> > since they are used with this patch:
> > 
> >     DrawImage,
> >     DrawTiledImage,
> >     DrawTiledScaledImage,
> > 
> > 3. Drawing a canvas into another canvas is blocked by bug 218773.
> 
> A fourth comment
> 
> 4. I think the class ImageDataReference can be replaced by just
> 
> using ImageDataReference = RefPtr<ImageData>;

5. Maybe it is time to add something like DisplayList::ResourceCache and have both
RemoteResourceCacheProxy and RemoteResourceCache inherit from it.

namespace DisplayList {

class ResourceCache {
public:
    virtual void cacheImageBuffer(Ref<ImageBuffer>&&);
    virtual void releaseImageBuffer(WebCore::RenderingResourceIdentifier);
    ImageBuffer* cachedImageBuffer(RenderingResourceIdentifier);

    virtual void cacheNativeImage(WebCore::NativeImage&);
    virtual void releaseNativeImage(WebCore::RenderingResourceIdentifier);

    const ImageBufferHashMap& imageBuffers() const { return m_imageBuffers; }
    const NativeImageHashMap& nativeImages() const { return m_nativeImages; }

private:
    ImageBufferHashMap m_imageBuffers;
    NativeImageHashMap m_nativeImages;
};

}

And a pointer to ResourceCache can be passed to DisplayList::Replayer.

[Said Abou-Hallawa]

Created attachment 413761 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 413761 [details]
Patch

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

> Source/WebCore/platform/graphics/NativeImage.cpp:48
> +    if (m_observer)
> +        m_observer->releaseNativeImage(m_renderingResourceIdentifier);

The observer is holding a ref to the NativeImage, so this code will never get hit. I think you're leaking all the images.

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.messages.in:30
> +    CacheNativeImage(WebCore::NativeImageReference nativeImage)

I think it's deceptive to state that we can encode NativeImages like this. We can't losslessly encode PDF images, SVG Images etc. I think this should take ShareableBitmap and RenderingRsourceIdentifier arguments.

> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.h:55
>      WebCore::ImageBufferHashMap m_imageBuffers;
> +    WebCore::NativeImageHashMap m_nativeImages;

At some point m_imageBuffers will go away, and m_nativeImages will be SharableBitmaps, right?

[Said Abou-Hallawa]

Created attachment 413786 [details]
Patch

[Said Abou-Hallawa]

The failure on iOS is happening because custom painting uses DisplayList for drawing. Because I removed recording the DrawImage, calling BitmapImage() is now allowed while the GraphicsContext is backed by a DisplayList::Recorder. And because allowSubsampling is allowed on iOS only, we end up calling the function BitmapImage::subsamplingLevelForScaleFactor() which expects a GraphicsContext with a valid platformContext(). I think the fix for this function is to check context.hasPlatformContext() before using context.platformContext().

[Said Abou-Hallawa]

Created attachment 413842 [details]
Patch

[Said Abou-Hallawa]

Created attachment 413847 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 413761 [details]
Patch

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

>> Source/WebCore/platform/graphics/NativeImage.cpp:48
>> +        m_observer->releaseNativeImage(m_renderingResourceIdentifier);
> 
> The observer is holding a ref to the NativeImage, so this code will never get hit. I think you're leaking all the images.

Yes you right. I also mistakenly replaced the type of RemoteResourceCacheProxy::m_imageBuffers from 

    using RemoteImageBufferProxyHashMap = HashMap<WebCore::RenderingResourceIdentifier, WebCore::ImageBuffer*>

to NativeImageHashMap which is 

    using ImageBufferHashMap = HashMap<RenderingResourceIdentifier, Ref<ImageBuffer>>;

So the NativeImage and the ImageBuffers were leaking when GPU rendering for canvas is enabled. I made RemoteResourceCacheProxy holds raw pointer for ImageBuffer and NativeImage and I made sure the destructor of NativeImage is being called when the decoded frame is destroyed.

>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.messages.in:30
>> +    CacheNativeImage(WebCore::NativeImageReference nativeImage)
> 
> I think it's deceptive to state that we can encode NativeImages like this. We can't losslessly encode PDF images, SVG Images etc. I think this should take ShareableBitmap and RenderingRsourceIdentifier arguments.

I do not fully understand this comment. I think:

1. We need to decode the PlatformImagePtr into a SharableBitmap inside the NativeImage. So encoding/decoding the NativeImage will be done by sending a Handle from WebP to GPUP.
2. We need to have CachePDFDocumentImage([no sure what should be here but maybe a SharedBuffer])
3. For the reset of the images they are already fixed by removing the recording from the GraphicsContext functions. For example, SVGImage should work if DOM rendering is enabled.

>> Source/WebKit/GPUProcess/graphics/RemoteResourceCache.h:55
>> +    WebCore::NativeImageHashMap m_nativeImages;
> 
> At some point m_imageBuffers will go away, and m_nativeImages will be SharableBitmaps, right?

m_imageBuffers have to stay since this is where the RemoteImageBuffers in the GPU side can live. The m_nativeImages have to stay as well but the underlying data will be a SharableBitmap instead of PlatformImagePtr. These two HashMaps are used to

1. Keep the resources alive in the GPU side
2. Resolve the renderingResourceIdentifier in DrawImageBuffer and DrawNativeImage to a resource when replaying back the DisplayList.

[Simon Fraser (smfr)]

Comment on attachment 413847 [details]
Patch

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

> Source/WebCore/platform/graphics/NativeImage.h:61
> +    void setRenderingResourceIdentifier(RenderingResourceIdentifier renderingResourceIdentifier) { m_renderingResourceIdentifier = renderingResourceIdentifier; }

It's a bit weird to have m_renderingResourceIdentifier be initialized to the generated value, then set here. Should RenderingResourceIdentifier be passed into the constructor as an Optional<>?

> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:1300
> +    RenderingResourceIdentifier m_renderingResourceIdentifier;

We can all this m_imageIdentifier here

> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:201
> +    if (!m_delegate)
> +        m_displayList.cacheNativeImage(image);
> +    else
> +        m_delegate->cacheNativeImage(image);

This is a bit confusing. It assumes knowledge of delegate behavior.

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:65
>          if (!m_remoteRenderingBackendProxy)
>              return;
>          flushDrawingContext();
> +        m_remoteRenderingBackendProxy->remoteResourceCacheProxy().releaseImageBuffer(m_renderingResourceIdentifier);
>          m_remoteRenderingBackendProxy->releaseRemoteResource(m_renderingResourceIdentifier);

Please move to the .cpp file.

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:89
>      {
>          ASSERT(m_remoteRenderingBackendProxy);
> +        m_remoteRenderingBackendProxy->remoteResourceCacheProxy().cacheImageBuffer(*this);
> +
>          m_drawingContext.displayList().setItemBufferClient(this);
>          m_drawingContext.displayList().setTracksDrawingItemExtents(false);
>      }

Move to the cpp file.

> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:191
> +    void cacheNativeImage(WebCore::NativeImage& image) override
> +    {
> +        if (m_remoteRenderingBackendProxy)
> +            m_remoteRenderingBackendProxy->remoteResourceCacheProxy().cacheNativeImage(image);
> +    }

Put in the cpp file.

> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.h:54
> +    using ImageBufferHashMap = HashMap<WebCore::RenderingResourceIdentifier, WebCore::ImageBuffer*>;
> +    using NativeImageHashMap = HashMap<WebCore::RenderingResourceIdentifier, WebCore::NativeImage*>;

Can we use WeakPtr here?

[Said Abou-Hallawa]

Created attachment 413875 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 413847 [details]
Patch

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

>> Source/WebCore/platform/graphics/NativeImage.h:61
>> +    void setRenderingResourceIdentifier(RenderingResourceIdentifier renderingResourceIdentifier) { m_renderingResourceIdentifier = renderingResourceIdentifier; }
> 
> It's a bit weird to have m_renderingResourceIdentifier be initialized to the generated value, then set here. Should RenderingResourceIdentifier be passed into the constructor as an Optional<>?

This function was removed. Instead a new create and a new constructor were added to take PlatformImagePtr and RenderingResourceIdentifier.

>> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:1300
>> +    RenderingResourceIdentifier m_renderingResourceIdentifier;
> 
> We can all this m_imageIdentifier here

The name of this member was changed to m_imageIdentifier.

>> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:201
>> +        m_delegate->cacheNativeImage(image);
> 
> This is a bit confusing. It assumes knowledge of delegate behavior.

Yes this is right. I was trying to save a call to DisplayList::cacheNativeImage() but calling it unconditionally is cleaner and can make the DisplayList re-playable by itself.

>> Source/WebKit/WebProcess/GPU/graphics/RemoteImageBufferProxy.h:191
>> +    }
> 
> Put in the cpp file.

There is no cpp file for this header file.

>> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.h:54
>> +    using NativeImageHashMap = HashMap<WebCore::RenderingResourceIdentifier, WebCore::NativeImage*>;
> 
> Can we use WeakPtr here?

Yes we can. Done.

[EWS]

Committed r269708: <https://trac.webkit.org/changeset/269708>

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