Bug 219387 – GPU Process: Invalid static_cast from ConcreteImageBuffer to RemoteImageBufferProxy

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Tim Horton]

As the subject says. This is split out of https://bugs.webkit.org/show_bug.cgi?id=219368

[Tim Horton]

Created attachment 415102 [details]
Patch

[EWS]

Committed r270286: <https://trac.webkit.org/changeset/270286>

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

[Radar WebKit Bug Importer]

<rdar://problem/71840749>

[Said Abou-Hallawa]

Comment on attachment 415102 [details]
Patch

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

> Source/WebKit/Shared/ConcreteShareableImageBuffer.h:34
> +class ConcreteShareableImageBuffer : public WebCore::ConcreteImageBuffer<BackendType> {

If you expose the backend from ImageBuffer (see below), I think you will not need this class.

> Source/WebKit/Shared/ConcreteShareableImageBuffer.h:39
> +    static auto create(const WebCore::FloatSize& size, WebCore::RenderingMode renderingMode, float resolutionScale, WebCore::ColorSpace colorSpace, WebCore::PixelFormat pixelFormat)

Unused argument renderingMode.

> Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:119
> +        if (WebProcess::singleton().shouldUseRemoteRenderingFor(WebCore::RenderingPurpose::DOM)) {
> +            if (m_acceleratesDrawing)
> +                handle = static_cast<AcceleratedRemoteImageBufferProxy *>(m_frontBuffer.imageBuffer.get())->createImageBufferBackendHandle();
> +            else
> +                handle = static_cast<UnacceleratedRemoteImageBufferProxy *>(m_frontBuffer.imageBuffer.get())->createImageBufferBackendHandle();
> +        } else {
> +            if (m_acceleratesDrawing)
> +                handle = static_cast<ConcreteShareableImageBuffer<AcceleratedImageBufferShareableBackend> *>(m_frontBuffer.imageBuffer.get())->createImageBufferBackendHandle();
> +            else
> +                handle = static_cast<ConcreteShareableImageBuffer<UnacceleratedImageBufferShareableBackend> *>(m_frontBuffer.imageBuffer.get())->createImageBufferBackendHandle();
> +        }

Can't this casting be cleaned by exposing the ImageBufferBackend from ImageBuffer? This code can be simplified like this:

if (m_acceleratesDrawing)
    handle = static_cast<ImageBufferShareableIOSurfaceBackend&>(*m_frontBuffer.imageBuffer->backend()).createImageBufferBackendHandle();
else
    handle = static_cast<ImageBufferShareableBitmapBackend&>(*m_frontBuffer.imageBuffer->backend()).createImageBufferBackendHandle();

> Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:196
> +        m_frontBuffer.imageBuffer = ConcreteShareableImageBuffer<AcceleratedImageBufferShareableBackend>::create(backingStoreSize(), WebCore::RenderingMode::Accelerated, 1, WebCore::ColorSpace::SRGB, pixelFormat());

No need to pass the rendering mode to a concrete ImageBuffer since the backend type already reveals the type of the backend.

[Said Abou-Hallawa]

Comment on attachment 415102 [details]
Patch

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

>> Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:196
>> +        m_frontBuffer.imageBuffer = ConcreteShareableImageBuffer<AcceleratedImageBufferShareableBackend>::create(backingStoreSize(), WebCore::RenderingMode::Accelerated, 1, WebCore::ColorSpace::SRGB, pixelFormat());
> 
> No need to pass the rendering mode to a concrete ImageBuffer since the backend type already reveals the type of the backend.

I meant to say "... the backend type reveals the renderingMode".

[Tim Horton]

(In reply to Said Abou-Hallawa from comment #4)
> Comment on attachment 415102 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415102&action=review
> 
> > Source/WebKit/Shared/ConcreteShareableImageBuffer.h:34
> > +class ConcreteShareableImageBuffer : public WebCore::ConcreteImageBuffer<BackendType> {
> 
> If you expose the backend from ImageBuffer (see below), I think you will not
> need this class.
> 
> > Source/WebKit/Shared/ConcreteShareableImageBuffer.h:39
> > +    static auto create(const WebCore::FloatSize& size, WebCore::RenderingMode renderingMode, float resolutionScale, WebCore::ColorSpace colorSpace, WebCore::PixelFormat pixelFormat)
> 
> Unused argument renderingMode.
> 
> > Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:119
> > +        if (WebProcess::singleton().shouldUseRemoteRenderingFor(WebCore::RenderingPurpose::DOM)) {
> > +            if (m_acceleratesDrawing)
> > +                handle = static_cast<AcceleratedRemoteImageBufferProxy *>(m_frontBuffer.imageBuffer.get())->createImageBufferBackendHandle();
> > +            else
> > +                handle = static_cast<UnacceleratedRemoteImageBufferProxy *>(m_frontBuffer.imageBuffer.get())->createImageBufferBackendHandle();
> > +        } else {
> > +            if (m_acceleratesDrawing)
> > +                handle = static_cast<ConcreteShareableImageBuffer<AcceleratedImageBufferShareableBackend> *>(m_frontBuffer.imageBuffer.get())->createImageBufferBackendHandle();
> > +            else
> > +                handle = static_cast<ConcreteShareableImageBuffer<UnacceleratedImageBufferShareableBackend> *>(m_frontBuffer.imageBuffer.get())->createImageBufferBackendHandle();
> > +        }
> 
> Can't this casting be cleaned by exposing the ImageBufferBackend from
> ImageBuffer? This code can be simplified like this:
> 
> if (m_acceleratesDrawing)
>     handle =
> static_cast<ImageBufferShareableIOSurfaceBackend&>(*m_frontBuffer.
> imageBuffer->backend()).createImageBufferBackendHandle();
> else
>     handle =
> static_cast<ImageBufferShareableBitmapBackend&>(*m_frontBuffer.imageBuffer-
> >backend()).createImageBufferBackendHandle();

I am hoping to figure out how to finagle createImageBufferBackendHandle onto ImageBuffer, so we don't have /any/ bifurcation or static casts. You're right that it will obsolete most of this patch, but it's trickier, and the invalid cast seemed ... very bad.

> > Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:196
> > +        m_frontBuffer.imageBuffer = ConcreteShareableImageBuffer<AcceleratedImageBufferShareableBackend>::create(backingStoreSize(), WebCore::RenderingMode::Accelerated, 1, WebCore::ColorSpace::SRGB, pixelFormat());
> 
> No need to pass the rendering mode to a concrete ImageBuffer since the
> backend type already reveals the type of the backend.

Good point.