Bug 236671 – Allow downcast<> to work with ImageBufferBackends that support sharing

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Process Model

[Simon Fraser (smfr)]

Allow downcast<> to work with ImageBufferBackends that support sharing

[Simon Fraser (smfr)]

Created attachment 452095 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 452095 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageBufferBackend.h:143
> +    virtual ImageBufferBackendSharing* toBackendSharing() { return nullptr; }

It is sad that we can't make ImageBufferBackend inherit ImageBufferBackendSharing and have to introduce such function to avoid the diamond inheritance problem.

> Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:119
> +    auto handleFromBuffer = [](WebCore::ImageBuffer& buffer) -> std::optional<ImageBufferBackendHandle> {

This is definitely cleaner and safer than the existing code. But we still have to deal with the backend and have to downcast it. Is there a way to move this call to ImageBuffer such that this code can be replaced by a single call?

> Source/WebKit/WebProcess/GPU/graphics/ImageBufferBackendHandleSharing.h:37
> +    virtual ImageBufferBackendHandle createBackendHandle() const = 0;

Can these functions in ImageBuffer/ImageBufferBackend also be moved to this class?

    virtual bool isInUse() const = 0;
    virtual void releaseGraphicsContext() = 0;
    virtual void releaseBufferToPool() = 0;

    // Returns true on success.
    virtual bool setVolatile() = 0;
    virtual VolatilityState setNonVolatile() = 0;

    virtual std::unique_ptr<ThreadSafeImageBufferFlusher> createFlusher() = 0;

I think they are specific to WebKit and they are only used by RemoteLayerBackingStore.

> Source/WebKit/WebProcess/GPU/graphics/ImageBufferBackendHandleSharing.h:45
> +SPECIALIZE_TYPE_TRAITS_IMAGE_BUFFER_BACKEND_SHARING(WebKit::ImageBufferBackendHandleSharing, isImageBufferBackendHandleSharing())

SPECIALIZE_TYPE_TRAITS_IMAGE_BUFFER_BACKEND_SHARING() is defined in ImageBufferBackend.h and it is only used here. Why do not we move the macro here also?

[Simon Fraser (smfr)]

Created attachment 452127 [details]
Patch

[Simon Fraser (smfr)]

(In reply to Said Abou-Hallawa from comment #2)
> Comment on attachment 452095 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452095&action=review
> 
> > Source/WebCore/platform/graphics/ImageBufferBackend.h:143
> > +    virtual ImageBufferBackendSharing* toBackendSharing() { return nullptr; }
> 
> It is sad that we can't make ImageBufferBackend inherit
> ImageBufferBackendSharing and have to introduce such function to avoid the
> diamond inheritance problem.

Indeed.

> > Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:119
> > +    auto handleFromBuffer = [](WebCore::ImageBuffer& buffer) -> std::optional<ImageBufferBackendHandle> {
> 
> This is definitely cleaner and safer than the existing code. But we still
> have to deal with the backend and have to downcast it. Is there a way to
> move this call to ImageBuffer such that this code can be replaced by a
> single call?

ImageBuffer (in WebCore) can't know about ImageBufferBackendHandle (in WebKit).
> 
> > Source/WebKit/WebProcess/GPU/graphics/ImageBufferBackendHandleSharing.h:37
> > +    virtual ImageBufferBackendHandle createBackendHandle() const = 0;
> 
> Can these functions in ImageBuffer/ImageBufferBackend also be moved to this
> class?
> 
>     virtual bool isInUse() const = 0;
>     virtual void releaseGraphicsContext() = 0;
>     virtual void releaseBufferToPool() = 0;
> 
>     // Returns true on success.
>     virtual bool setVolatile() = 0;
>     virtual VolatilityState setNonVolatile() = 0;
> 
>     virtual std::unique_ptr<ThreadSafeImageBufferFlusher> createFlusher() =
> 0;
> 
> I think they are specific to WebKit and they are only used by
> RemoteLayerBackingStore.

Maybe but not in this patch.

> > Source/WebKit/WebProcess/GPU/graphics/ImageBufferBackendHandleSharing.h:45
> > +SPECIALIZE_TYPE_TRAITS_IMAGE_BUFFER_BACKEND_SHARING(WebKit::ImageBufferBackendHandleSharing, isImageBufferBackendHandleSharing())
> 
> SPECIALIZE_TYPE_TRAITS_IMAGE_BUFFER_BACKEND_SHARING() is defined in
> ImageBufferBackend.h and it is only used here. Why do not we move the macro
> here also?

Generally we put the "#define SPECIALIZE_TYPE_TRAITS_IMAGE_BUFFER_BACKEND_SHARING()" macro in the base class file, but I can move it.

[Simon Fraser (smfr)]

Created attachment 452148 [details]
Patch

[EWS]

Committed r289911 (247341@main): <https://commits.webkit.org/247341@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/89034498>

[Simon Fraser (smfr)]

Reopening to attach new patch.

[Simon Fraser (smfr)]

Created attachment 452266 [details]
Patch

[Simon Fraser (smfr)]

Patch added by mistake.