Summary: | GPU Process: Invalid static_cast from ConcreteImageBuffer to RemoteImageBufferProxy | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tim Horton <thorton> | ||||
Component: | Layout and Rendering | Assignee: | Tim Horton <thorton> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | bfulgham, eric.carlson, ews-watchlist, glenn, jer.noble, philipj, sabouhallawa, sergio, simon.fraser, webkit-bug-importer, zalan | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | Other | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Tim Horton
2020-11-30 23:51:09 PST
Created attachment 415102 [details]
Patch
Committed r270286: <https://trac.webkit.org/changeset/270286> All reviewed patches have been landed. Closing bug and clearing flags on attachment 415102 [details]. 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. 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". (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. |