[CoordinatedGraphics] Use a swapchain of three buffers for WebGL - #75637
carlosgcampos wants to merge 1 commit into
Conversation
|
EWS run on previous version of this PR (hash 480b7d1) Details |
| if (!m_textureWrapper) | ||
| return false; | ||
|
|
||
| return !m_textureWrapper->hasOneRef(); |
There was a problem hiding this comment.
So the isInUse state is purely checked via the fact if a Ref() is hold.. ok, but that will break TextureMapper, no? There you only pass a raw ID to the compositor, not a Ref?
There was a problem hiding this comment.
if we answer this isInUse incorrectly, we could end up reusing the buffer too early, no? Are we sure that dropping the last ref (CPU-wise) means that the GPU is really finished with that buffer? I cannot see any guarantee for that...
There was a problem hiding this comment.
It doesn't break texture mapper, it keeps it in its current state, no change in behavior.
There was a problem hiding this comment.
if we answer this isInUse incorrectly, we could end up reusing the buffer too early, no? Are we sure that dropping the last ref (CPU-wise) means that the GPU is really finished with that buffer? I cannot see any guarantee for that...
Exactly the situation we currently have without this patch.
There was a problem hiding this comment.
Regarding the last reference. It means skia has notified that the resource can be re-used/deleted. For GBM that's enough, for the non-GBM case we could use a release fence, but I'm not sure it's worth for a fallback code that it's only used for testing
| m_layerContentsDisplayDelegate = GraphicsLayerContentsDisplayDelegateCoordinated::create(); | ||
| } | ||
|
|
||
| GraphicsContextGLCoordinated::~GraphicsContextGLCoordinated() |
There was a problem hiding this comment.
Here are no more isInUse() checks -- can we have the case where we still have pending/committed promise images in use by the compositor? that would then reference stale textures, that you just destroyed? Not sure about the dstruction order here.
There was a problem hiding this comment.
When the context is destroyed we need to release the textures no matter what. So, yes, the compositor might still have a stale texture. There's no change in behavior here either.
| @@ -77,7 +77,13 @@ GraphicsContextGLGBM::GraphicsContextGLGBM(GraphicsContextGLAttributes&& attribu | |||
|
|
|||
| GraphicsContextGLGBM::~GraphicsContextGLGBM() | |||
There was a problem hiding this comment.
SAme question as in the GraphicsContextCoordinatedGL destructor: in GPUP mode, don't we need some kind of check if the remote side still uses the buffers?
There was a problem hiding this comment.
The GBM case is different, this is not a problem because CoordinatedPlatformLayerBufferDMABuf keeps a reference of the DMABufBuffer, GPU process imports the buffer, so it will be alive as long as it's references, like the DMABuf buffers we share with the UI process.
https://bugs.webkit.org/show_bug.cgi?id=325950 Reviewed by NOBODY (OOPS!). We currently use two fixed buffers, but the compositor hasn't released the previous one when we do the swap, so a third one is needed. Use a swapchain based on a fixed size array of 3 buffers similar to the one used by Cocoa implementation. If we need to bind a new buffer and next one is still in used by the compositor, it's moved to an vector to be released later when no longer in use and a new one is allocated. The same happens when we release current buffers due to a reshape, buffers still used by the compositor are moved to the obsolete buffers vector. In the GBM implementation the drawing buffer keeps a reference of the DMABufBuffer so that we know it's no longer in use by the compositor when it only has one reference. For non-GBM implementation a thread safe ref counted class is used to wrap the inner texture ID that is passed to the compositor to keep a reference while the texture is in use. This patch also moves the coordinated graphics implementation of GraphicsContextGL to its own file and renames the API test. Test: Tools/TestWebKitAPI/Tests/WebCore/glib/GraphicsContextGLCoordinated.cpp * Source/WebCore/platform/CoordinatedGraphics.cmake: * Source/WebCore/platform/graphics/coordinated/CoordinatedPlatformLayerBufferSkiaImage.cpp: (WebCore::PromiseWebGLImageContext::PromiseWebGLImageContext): (WebCore::PromiseWebGLImageContext::promiseImageTexture): (WebCore::CoordinatedPlatformLayerBufferSkiaImage::create): (): Deleted. * Source/WebCore/platform/graphics/coordinated/CoordinatedPlatformLayerBufferSkiaImage.h: * Source/WebCore/platform/graphics/coordinated/GraphicsContextGLCoordinated.cpp: Added. (WebCore::GraphicsContextGLCoordinated::create): (WebCore::GraphicsContextGLCoordinated::GraphicsContextGLCoordinated): (WebCore::GraphicsContextGLCoordinated::~GraphicsContextGLCoordinated): (WebCore::GraphicsContextGLCoordinated::platformInitialize): (WebCore::GraphicsContextGLCoordinated::DrawingBuffer::DrawingBuffer): (WebCore::GraphicsContextGLCoordinated::DrawingBuffer::operator=): (WebCore::GraphicsContextGLCoordinated::DrawingBuffer::isInUse const): (WebCore::GraphicsContextGLCoordinated::DrawingBuffer::release): (WebCore::GraphicsContextGLCoordinated::createDrawingBuffer const): (WebCore::GraphicsContextGLCoordinated::destroyDrawingBuffer const): (WebCore::GraphicsContextGLCoordinated::freeDrawingBuffers): (WebCore::GraphicsContextGLCoordinated::freeObsoleteDrawingBuffers): (WebCore::GraphicsContextGLCoordinated::bindNextDrawingBuffer): (WebCore::GraphicsContextGLCoordinated::reshapeDrawingBuffer): (WebCore::GraphicsContextGLCoordinated::prepareForDisplay): (WebCore::GraphicsContextGLCoordinated::readCompositedResults): * Source/WebCore/platform/graphics/coordinated/GraphicsContextGLCoordinated.h: Added. (WebCore::GraphicsContextGLCoordinated::DrawingBuffer::operator bool const): (WebCore::GraphicsContextGLCoordinated::DrawingBuffer::texture const): (WebCore::GraphicsContextGLCoordinated::drawingBuffer): (WebCore::GraphicsContextGLCoordinated::displayBuffer): * Source/WebCore/platform/graphics/coordinated/GraphicsContextGLCoordinatedEpoxy.cpp: Renamed from Source/WebCore/platform/graphics/coordinated/GraphicsContextGLEGLCoordinated.cpp. (WebCore::GraphicsContextGLCoordinated::setupCurrentTexture const): * Source/WebCore/platform/graphics/egl/GraphicsContextGLEGL.cpp: (WebCore::createWebProcessGraphicsContextGL): (WebCore::GraphicsContextGLEGL::~GraphicsContextGLEGL): (WebCore::GraphicsContextGLEGL::platformInitialize): (WebCore::GraphicsContextGLEGL::attachDrawingBufferTexture): (WebCore::GraphicsContextGLEGL::reshapeDrawingBuffer): (WebCore::GraphicsContextGLEGL::prepareForDisplay): (WebCore::GraphicsContextGLEGL::swapCompositorTexture): Deleted. * Source/WebCore/platform/graphics/egl/GraphicsContextGLEGL.h: * Source/WebCore/platform/graphics/gbm/GraphicsContextGLGBM.cpp: (WebCore::GraphicsContextGLGBM::~GraphicsContextGLGBM): (WebCore::GraphicsContextGLGBM::DrawingBuffer::DrawingBuffer): (WebCore::GraphicsContextGLGBM::DrawingBuffer::operator=): (WebCore::GraphicsContextGLGBM::DrawingBuffer::isInUse const): (WebCore::GraphicsContextGLGBM::DrawingBuffer::release): (WebCore::GraphicsContextGLGBM::destroyDrawingBuffer const): (WebCore::GraphicsContextGLGBM::freeDrawingBuffers): (WebCore::GraphicsContextGLGBM::freeObsoleteDrawingBuffers): (WebCore::GraphicsContextGLGBM::bindNextDrawingBuffer): (WebCore::GraphicsContextGLGBM::prepareForDisplay): (WebCore::GraphicsContextGLGBM::prepareForDisplayWithFinishedSignal): (WebCore::GraphicsContextGLGBM::readCompositedResults): * Source/WebCore/platform/graphics/gbm/GraphicsContextGLGBM.h: * Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGLGBM.cpp: (WebKit::RemoteGraphicsContextGLGBM::prepareForDisplay): * Tools/TestWebKitAPI/PlatformGTK.cmake: * Tools/TestWebKitAPI/PlatformWPE.cmake: * Tools/TestWebKitAPI/Tests/WebCore/glib/GraphicsContextGLCoordinated.cpp: Renamed from Tools/TestWebKitAPI/Tests/WebCore/glib/GraphicsContextGLEGL.cpp. (TestWebKitAPI::WebCore::createTestedGraphicsContextGL): (TestWebKitAPI::WebCore::AnyContextAttributeTest::antialias const): (TestWebKitAPI::WebCore::AnyContextAttributeTest::preserveDrawingBuffer const): (TestWebKitAPI::WebCore::AnyContextAttributeTest::isWebGL2 const): (TestWebKitAPI::WebCore::AnyContextAttributeTest::attributes): (TestWebKitAPI::WebCore::AnyContextAttributeTest::createTestContext): (TestWebKitAPI::checkReadPixel): (TestWebKitAPI::TEST_F(GraphicsContextGLCoordinatedTest, ClearBufferIncorrectSizes)): (TestWebKitAPI::TEST_F(GraphicsContextGLCoordinatedTest, DestroyWithoutMakingCurrent)): (TestWebKitAPI::TEST_F(GraphicsContextGLCoordinatedTest, TwoLinks)): (TestWebKitAPI::TEST_F(GraphicsContextGLCoordinatedTest, CopyNativeImageNoDrawingBufferReturnsNullptr)): (TestWebKitAPI::TEST_F(GraphicsContextGLCoordinatedTest, CopyNativeImageIsNotYFlipped)): (TestWebKitAPI::TEST_F(GraphicsContextGLCoordinatedTest, CopyImageAndMutateDrawingBuffer)): (TestWebKitAPI::TEST_P): (TestWebKitAPI::TEST_F(GraphicsContextGLCoordinatedReadPixelsTest, readPixelsSuccess)): (TestWebKitAPI::TEST_F(GraphicsContextGLCoordinatedReadPixelsTest, readPixelsTooLargeRect)): (TestWebKitAPI::TEST_F(GraphicsContextGLCoordinatedReadPixelsTest, readPixelsWithStatusSuccess)): (TestWebKitAPI::TEST_F(GraphicsContextGLCoordinatedReadPixelsTest, readPixelsWithStatusTooLargeRect)): (TestWebKitAPI::TEST_F(GraphicsContextGLCoordinatedReshapeTest, reshapeSuccess)): (TestWebKitAPI::TEST_F(GraphicsContextGLCoordinatedReshapeTest, reshapeWidthTooLarge)): (TestWebKitAPI::TEST_F(GraphicsContextGLCoordinatedReshapeTest, reshapeHeightTooLarge)): * Source/WebCore/platform/graphics/android/GraphicsContextGLAndroid.h:
480b7d1 to
e2129ef
Compare
|
EWS run on current version of this PR (hash e2129ef) Details |
🧪 mac-wk23 participants
e2129ef
e2129ef