RESOLVED FIXED 219387
GPU Process: Invalid static_cast from ConcreteImageBuffer to RemoteImageBufferProxy
https://bugs.webkit.org/show_bug.cgi?id=219387
Summary GPU Process: Invalid static_cast from ConcreteImageBuffer to RemoteImageBuffe...
Tim Horton
Reported 2020-11-30 23:51:09 PST
As the subject says. This is split out of https://bugs.webkit.org/show_bug.cgi?id=219368
Attachments
Patch (10.26 KB, patch)
2020-11-30 23:52 PST, Tim Horton
no flags
Tim Horton
Comment 1 2020-11-30 23:52:06 PST
EWS
Comment 2 2020-12-01 00:25:03 PST
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
Comment 3 2020-12-01 00:26:16 PST
Said Abou-Hallawa
Comment 4 2020-12-01 01:01:56 PST
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 5 2020-12-01 01:08:28 PST
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
Comment 6 2020-12-01 01:08:47 PST
(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.
Note You need to log in before you can comment on or make changes to this bug.