The remote ImageBuffer proxy will create a shareable backend in the GPUProcess. Then it will give access to the backend to the remote ImageBuffer in the WebProcess by sending an ImageBufferBackendHandle. Only one shareable accelerated backend is implemented so for which is the IOSurface shareable backend. Other ports will have to implement their own shareable accelerated backends.
Created attachment 389723 [details] Patch
Created attachment 389737 [details] Patch for review
Comment on attachment 389737 [details] Patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=389737&action=review > Source/WebKit/WebProcess/GPU/RenderingBackend/ImageBufferShareableBitmapBackend.cpp:67 > + if (WTF::holds_alternative<ShareableBitmap::Handle>(handle)) { This should be if (!WTF::holds_alternative<ShareableBitmap::Handle>(handle)) {
Created attachment 391841 [details] Patch
Created attachment 391863 [details] Patch
The GKT port fails while compiling WebKit/WebProcess/WebPage/WebFrame.cpp because the following definition Display* m_display { nullptr }; in the header file WebKit/WebProcess/WebPage/gtk/AcceleratedSurfaceX11.h is ambiguous. The candidates are the global definition (not inside any namespace) typedef struct _XDisplay Display; which is defined in WebKit/WebProcess/WebPage/gtk/AcceleratedSurfaceX11.h and the WebCore namespace namespace WebCore::Display { } which is defined in WebCore/layout/displaytree/DisplayLineBox.h It looks like there is a global "using namespace WebCore" (not inside any namespace) in one of the WebKit sources or the headers which compile with WebKit/WebProcess/WebPage/WebFrame.cpp in a unified source.
Created attachment 391880 [details] Patch
Created attachment 391882 [details] Patch
Created attachment 391883 [details] Patch
Comment on attachment 391883 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391883&action=review > Source/WebKit/WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.cpp:62 > + return std::unique_ptr<ImageBufferShareableBitmapBackend>(new ImageBufferShareableBitmapBackend(size, backendSize, resolutionScale, colorSpace, WTFMove(bitmap), WTFMove(context))); make_unique<> Do you need to pass both bitmap and context in? The context can be created by the bitmap. > Source/WebKit/WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.cpp:80 > + return std::unique_ptr<ImageBufferShareableBitmapBackend>(new ImageBufferShareableBitmapBackend(logicalSize, backendSize, resolutionScale, colorSpace, WTFMove(bitmap), WTFMove(context))); make_unique<> > Source/WebKit/WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.h:64 > + RefPtr<ShareableBitmap> m_bitmap; Can this be a Ref<>... > Source/WebKit/WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.h:65 > + std::unique_ptr<WebCore::GraphicsContext> m_context; And this a GraphicsContext&? > Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:49 > + RetainPtr<CGContextRef> cgContext = surface->ensurePlatformContext(nullptr); ensurePlatformContext has a nullptr default argument. > Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:53 > + CGContextClearRect(cgContext.get(), FloatRect(FloatPoint::zero(), backendSize)); Shame that we have eagerly clear here. > Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:55 > + return std::unique_ptr<ImageBufferShareableIOSurfaceBackend>(new ImageBufferShareableIOSurfaceBackend(size, backendSize, resolutionScale, colorSpace, WTFMove(surface))); make_unique<> > Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:69 > + return std::unique_ptr<ImageBufferShareableIOSurfaceBackend>(new ImageBufferShareableIOSurfaceBackend(logicalSize, internalSize, resolutionScale, colorSpace, WTFMove(surface))); make_unique<>
Comment on attachment 391883 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391883&action=review >> Source/WebKit/WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.cpp:62 >> + return std::unique_ptr<ImageBufferShareableBitmapBackend>(new ImageBufferShareableBitmapBackend(size, backendSize, resolutionScale, colorSpace, WTFMove(bitmap), WTFMove(context))); > > make_unique<> > > Do you need to pass both bitmap and context in? The context can be created by the bitmap. If createGraphicsContext() returns nullptr for any reason, the ImageBuffer will be in a weird state. It is allocated but it can't function probably. I wanted to have two states for the returned value: 1. nullptr: Something went wrong when allocating the data members needed for the ImageBuffer. 2. valid pointer: Nothing went wrong when allocating all the data member needed for the ImageBuffer. So the ImageBuffer can function properly. >> Source/WebKit/WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.cpp:80 >> + return std::unique_ptr<ImageBufferShareableBitmapBackend>(new ImageBufferShareableBitmapBackend(logicalSize, backendSize, resolutionScale, colorSpace, WTFMove(bitmap), WTFMove(context))); > > make_unique<> Fixed. But I have to make the constructor public. >> Source/WebKit/WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.h:65 >> + std::unique_ptr<WebCore::GraphicsContext> m_context; > > And this a GraphicsContext&? No this can't be because it is created by the create() method by calling ShareableBitmap::createGraphicsContext(). The ownership of this pointer is transferred to this class in the constructor by WTFMove(context). The destructor of this class is responsible for releasing the std::unique_ptr. >> Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:49 >> + RetainPtr<CGContextRef> cgContext = surface->ensurePlatformContext(nullptr); > > ensurePlatformContext has a nullptr default argument. default argument was removed. >> Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:53 >> + CGContextClearRect(cgContext.get(), FloatRect(FloatPoint::zero(), backendSize)); > > Shame that we have eagerly clear here. This what we used to do with IOSurface ImageBuffer before the refactoring. >> Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:55 >> + return std::unique_ptr<ImageBufferShareableIOSurfaceBackend>(new ImageBufferShareableIOSurfaceBackend(size, backendSize, resolutionScale, colorSpace, WTFMove(surface))); > > make_unique<> Fixed. >> Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:69 >> + return std::unique_ptr<ImageBufferShareableIOSurfaceBackend>(new ImageBufferShareableIOSurfaceBackend(logicalSize, internalSize, resolutionScale, colorSpace, WTFMove(surface))); > > make_unique<> Fixed.
Created attachment 391932 [details] Patch
Comment on attachment 391883 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391883&action=review >>> Source/WebKit/WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.cpp:80 >>> + return std::unique_ptr<ImageBufferShareableBitmapBackend>(new ImageBufferShareableBitmapBackend(logicalSize, backendSize, resolutionScale, colorSpace, WTFMove(bitmap), WTFMove(context))); >> >> make_unique<> > > Fixed. But I have to make the constructor public. You could keep it private and use a friend declaration. See ContentFilter.h for an example.
Comment on attachment 391932 [details] Patch Clearing flags on attachment: 391932 Committed r257606: <https://trac.webkit.org/changeset/257606>
All reviewed patches have been landed. Closing bug.
<rdar://problem/59871107>