GPU Process: IOSurfaces should not be mapped into the Web Content Process
Created attachment 415054 [details] Patch
Comment on attachment 415054 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415054&action=review > Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:110 > + if (WebProcess::singleton().shouldUseRemoteRenderingFor(WebCore::RenderingPurpose::DOM)) { > + if (m_acceleratesDrawing) Can we move this into a helper factory function instead of putting it inside encode function? > Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableUnmappedIOSurfaceBackend.cpp:43 > + ASSERT_NOT_REACHED(); Can we release assert this? > Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableUnmappedIOSurfaceBackend.cpp:63 > + ASSERT_NOT_REACHED(); Can we release assert this? And ditto for the rest.
(In reply to Ryosuke Niwa from comment #2) > Comment on attachment 415054 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=415054&action=review > > > Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:110 > > + if (WebProcess::singleton().shouldUseRemoteRenderingFor(WebCore::RenderingPurpose::DOM)) { > > + if (m_acceleratesDrawing) > > Can we move this into a helper factory function instead of putting it inside > encode function? I'm going to try to move createImageBufferBackendHandle up in the hierarchy so that this just dissolves. > > Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableUnmappedIOSurfaceBackend.cpp:43 > > + ASSERT_NOT_REACHED(); > > Can we release assert this? > > > Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableUnmappedIOSurfaceBackend.cpp:63 > > + ASSERT_NOT_REACHED(); > > Can we release assert this? And ditto for the rest. Sure, why not!
Created attachment 415063 [details] Patch
Created attachment 415064 [details] Patch
Committed r270275: <https://trac.webkit.org/changeset/270275> All reviewed patches have been landed. Closing bug and clearing flags on attachment 415064 [details].
<rdar://problem/71828486>
Looking into the API test failures. Silly me, thinking that the only GPUP bots were post-landing bots. Forgot about the API tests.
Reverted r270275 for reason: broke canvas painting Committed r270280: <https://trac.webkit.org/changeset/270280>
Whoops: 3 0x4f97b8a6f WebKit::ImageBufferShareableUnmappedIOSurfaceBackend::draw(WebCore::GraphicsContext&, WebCore::FloatRect const&, WebCore::FloatRect const&, WebCore::ImagePaintingOptions const&) 4 0x4f97d0e49 WebCore::ConcreteImageBuffer<WebKit::ImageBufferShareableUnmappedIOSurfaceBackend>::draw(WebCore::GraphicsContext&, WebCore::FloatRect const&, WebCore::FloatRect const&, WebCore::ImagePaintingOptions const&) 5 0x50a8af6d5 WebCore::GraphicsContext::drawImageBuffer(WebCore::ImageBuffer&, WebCore::FloatRect const&, WebCore::FloatRect const&, WebCore::ImagePaintingOptions const&) 6 0x50a8af774 WebCore::GraphicsContext::drawImageBuffer(WebCore::ImageBuffer&, WebCore::FloatRect const&, WebCore::ImagePaintingOptions const&) 7 0x509bd2ddb WebCore::HTMLCanvasElement::paint(WebCore::GraphicsContext&, WebCore::LayoutRect const&) 8 0x50ae37f34 WebCore::RenderHTMLCanvas::paintReplaced(WebCore::PaintInfo&, WebCore::LayoutPoint const&)
Created attachment 415182 [details] Patch
Comment on attachment 415182 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415182&action=review > Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.h:73 > + static constexpr bool canMapBackingStore = true; I think you can remove this. ImageBufferIOSurfaceBackend has canMapBackingStore = true from ImageBufferBackend. > Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:40 > +std::unique_ptr<ImageBufferShareableIOSurfaceBackend> ImageBufferShareableIOSurfaceBackend::create(const FloatSize& logicalSize, const IntSize& internalSize, float resolutionScale, ColorSpace colorSpace, PixelFormat pixelFormat, ImageBufferBackendHandle handle) Please replace internalSize with backendSize. > Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:58 > + RELEASE_ASSERT_NOT_REACHED(); > + return *(GraphicsContext*)nullptr; Should all the release_assert implementations be moved to ImageBufferBackend pure virtual methods instead? I think the release_assert implementations in ImageBufferBackend will serve the two purposes well: 'Don't create a backend unless you implement these functions "or" never call any of them'. > Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.h:45 > + ImageBufferShareableIOSurfaceBackend(const WebCore::FloatSize& logicalSize, const WebCore::IntSize& physicalSize, float resolutionScale, WebCore::ColorSpace colorSpace, WebCore::PixelFormat pixelFormat, ImageBufferBackendHandle&& handle) Please replace physicalSize with backendSize.
Created attachment 415188 [details] Patch
Committed r270342: <https://trac.webkit.org/changeset/270342> All reviewed patches have been landed. Closing bug and clearing flags on attachment 415188 [details].