-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Return a texture instead of a view by the swapchain #89
Comments
Sounds good, we should also match the new merged And also decide how to give signals about the swapchain being suboptimal / outdated. |
I don't remember the exact rationale for returning texture views, but that's no longer a concern? |
It was me basically pushing for the views, that's all the reason it was. But it turns out, people do want to do more with swapchain images, even if it's slow. |
1578: Fix destruction of resources r=kvark a=kvark **Connections** Fixes #1552 Also fixes the VVL about swapchain frame destruction. **Description** Swapchain view was never registered in the device's root Hub. So the old logic of adding it to "suspected resources" didn't fire up correctly. The new logic goes straight into the submission tracker. This path will cease to exist when either Hubs are removed, or API changes of webgpu-native/webgpu-headers#89 are accepted. The other resource destruction errors happened because we gathered all the resources that were abandoned by the user, and held alive only by the submitted command buffers, and we added them to the suspected list. Then we'd scan the list in `maintain`, see that they can be removed, and try to associate their destruction with one of the submissions. But the current submission was not added yet! So the logic thinks it can just remove the resources right away in this case, assuming the submission is not found because it's long gone in past. **Testing** Tested on our examples. Co-authored-by: Dzmitry Malyshau <kvark@fastmail.com>
Hi @kvark and @kainino0x, may I check with you if there has been any decision made on providing native API to return texture (instead of view) from the swapchain? I am currently working on a project where code is structured for primitives to be rendered to a texture followed by copying the texture to the swapchain for display on browser. |
I think we already decided to make this change, just no one has implemented it. But more than changing Texture to TextureView, we also wanted to change the swapchain interface to match more closely to the upstream API. Yet it hasn't been a priority for us (Chromium) to fix this because this isn't used when implementing the JS API on top of the C API. |
Would be nice to have a resolution for this. We're running into it. |
I'm surprised you are running into it while implementing WebKit. Browsers need a lot more special logic to implement the |
Hi, I'm spearheading the effort to get One issue going down this path is that we need to extend the WebGPU texture creation API to optionally accept an |
In Dawn / Chromium there are a number of private APIs that are used to create In the future we could define webgpu.h APIs for platform texture import and export, but IMHO we should wait a little bit until all browsers have figured out how they want to do it. Otherwise we'll have extension we keep modifying as browser needs change. |
Can you expand more on this? |
See the comment above |
Would be nice to have a resolution for this. We're running into it now, again. |
https://bugs.webkit.org/show_bug.cgi?id=250990 rdar://104537242 Reviewed by NOBODY (OOPS!). According to the IDL for GPUCanvasContext and WebGPU.h, SwapChain is supposed to have 2 methods: getCurrentTexture(), and present(). This patch adds those APIs to all the various plumbing sites throught PAL, WebCore, and WebKit. present() is straightforward; it's just a void function. getCurrentTexture() is a getter instead of a creation method, so it is supposed to return objects that are owned by the swap chain itself. For this reason, the implementation of getCurrentTexture() has to internally retain the result, only actually recreating the texture when there is no existing object retained. There's one more hiccup - GPUCanvasContext says the function is supposed to be getCurrentTexture(), whereas WebGPU.h says it's supposed to be getCurrentTextureView(). webgpu-native/webgpu-headers#89 is about making these two things match, bu that issue isn't resolved yet. For now, we can just implement both until that upstream issue is resolved. * Source/WebCore/Modules/WebGPU/GPUSwapChain.cpp: (WebCore::GPUSwapChain::clearCurrentTextureAndView): (WebCore::GPUSwapChain::ensureCurrentTextureAndView): (WebCore::GPUSwapChain::getCurrentTexture): (WebCore::GPUSwapChain::getCurrentTextureView): (WebCore::GPUSwapChain::present): * Source/WebCore/Modules/WebGPU/GPUSwapChain.h: * Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUDeviceImpl.cpp: (PAL::WebGPU::DeviceImpl::createSwapChain): * Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUSwapChainImpl.cpp: (PAL::WebGPU::SwapChainImpl::SwapChainImpl): (PAL::WebGPU::SwapChainImpl::clearCurrentTextureAndView): (PAL::WebGPU::SwapChainImpl::ensureCurrentTextureAndView): (PAL::WebGPU::SwapChainImpl::getCurrentTexture): (PAL::WebGPU::SwapChainImpl::getCurrentTextureView): (PAL::WebGPU::SwapChainImpl::present): * Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUSwapChainImpl.h: * Source/WebCore/PAL/pal/graphics/WebGPU/WebGPUSwapChain.h: * Source/WebGPU/WebGPU/Instance.mm: (wgpuGetProcAddress): * Source/WebGPU/WebGPU/PresentationContext.h: * Source/WebGPU/WebGPU/PresentationContext.mm: (WebGPU::PresentationContext::getCurrentTexture): (wgpuSwapChainGetCurrentTexture): * Source/WebGPU/WebGPU/PresentationContextCoreAnimation.h: (WebGPU::PresentationContextCoreAnimation::Configuration::Configuration): * Source/WebGPU/WebGPU/PresentationContextCoreAnimation.mm: (WebGPU::PresentationContextCoreAnimation::configure): (WebGPU::PresentationContextCoreAnimation::Configuration::generateCurrentFrameState): (WebGPU::PresentationContextCoreAnimation::getCurrentTexture): * Source/WebGPU/WebGPU/PresentationContextIOSurface.h: * Source/WebGPU/WebGPU/PresentationContextIOSurface.mm: (WebGPU::PresentationContextIOSurface::getCurrentTexture): * Source/WebGPU/WebGPU/WebGPUExt.h: * Source/WebKit/GPUProcess/graphics/WebGPU/RemoteSwapChain.cpp: (WebKit::RemoteSwapChain::getCurrentTexture): (WebKit::RemoteSwapChain::getCurrentTextureView): (WebKit::RemoteSwapChain::present): * Source/WebKit/GPUProcess/graphics/WebGPU/RemoteSwapChain.h: * Source/WebKit/GPUProcess/graphics/WebGPU/RemoteSwapChain.messages.in: * Source/WebKit/WebKit.xcodeproj/xcshareddata/xcschemes/WebKit.xcscheme: * Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemoteDeviceProxy.cpp: (WebKit::WebGPU::RemoteDeviceProxy::createTexture): (WebKit::WebGPU::RemoteDeviceProxy::createSurfaceTexture): * Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemoteSwapChainProxy.cpp: (WebKit::WebGPU::RemoteSwapChainProxy::clearCurrentTextureAndView): (WebKit::WebGPU::RemoteSwapChainProxy::ensureCurrentTextureAndView): (WebKit::WebGPU::RemoteSwapChainProxy::getCurrentTexture): (WebKit::WebGPU::RemoteSwapChainProxy::getCurrentTextureView): (WebKit::WebGPU::RemoteSwapChainProxy::present): * Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemoteSwapChainProxy.h: * Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemoteTextureProxy.cpp: (WebKit::WebGPU::RemoteTextureProxy::RemoteTextureProxy): (WebKit::WebGPU::RemoteTextureProxy::createView): * Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemoteTextureProxy.h: * Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemoteTextureViewProxy.cpp: (WebKit::WebGPU::RemoteTextureViewProxy::RemoteTextureViewProxy): * Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemoteTextureViewProxy.h:
https://bugs.webkit.org/show_bug.cgi?id=250990 rdar://104537242 Reviewed by Tadeu Zagallo. This patch does 3 things: 1. Migrates getCurrentTextureView() to getCurrentTexture(). GPUCanvasContext uses getCurrentTexture(), but WebGPU.h exposes getCurrentTextureView(). GPUCanvasContext represents the source of truth; WebGPU.h is outdated, so we should use getCurrentTexture(). webgpu-native/webgpu-headers#89 is about updating WebGPU.h. 2. Implements getCurrentTexture(). These are a little tricky because they're supposed to be +0 getters. However, that's only relevant for calling directly into WebGPU.framework; everywhere else in WebKit we use owning Refs, so in WebKit, a +0 object is really just an object whose creator has a ref to it. The tricky parts about dealing with WebGPU.framework will be fixed in https://bugs.webkit.org/show_bug.cgi?id=250958; this patch hooks just those parts up intentionally incorrectly and adds a FIXME with a link to that bug. These functions aren't called yet (that's for https://bugs.webkit.org/show_bug.cgi?id=250995) so it's okay that this part doesn't actually work yet. WebGPU.framework continues to implement getCurrentTextureView() just for completeness. 3. Adds present(), and hooks it up to wgpuSwapChainPresent(). This part is super straightforward. * Source/WebCore/Modules/WebGPU/GPUPresentationContext.cpp: (WebCore::GPUPresentationContext::getCurrentTexture): (WebCore::GPUPresentationContext::present): * Source/WebCore/Modules/WebGPU/GPUPresentationContext.h: * Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUPresentationContextImpl.cpp: (PAL::WebGPU::PresentationContextImpl::configure): (PAL::WebGPU::PresentationContextImpl::getCurrentTexture): (PAL::WebGPU::PresentationContextImpl::present): * Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUPresentationContextImpl.h: * Source/WebCore/PAL/pal/graphics/WebGPU/WebGPUPresentationContext.h: * Source/WebCore/html/canvas/GPUCanvasContextCocoa.cpp: (WebCore::GPUCanvasContextCocoa::getCurrentTexture): * Source/WebGPU/WebGPU/PresentationContext.h: * Source/WebGPU/WebGPU/PresentationContext.mm: (WebGPU::PresentationContext::getCurrentTexture): (wgpuSwapChainGetCurrentTexture): * Source/WebGPU/WebGPU/PresentationContextCoreAnimation.h: (WebGPU::PresentationContextCoreAnimation::Configuration::Configuration): * Source/WebGPU/WebGPU/PresentationContextCoreAnimation.mm: (WebGPU::PresentationContextCoreAnimation::configure): (WebGPU::PresentationContextCoreAnimation::Configuration::generateCurrentFrameState): (WebGPU::PresentationContextCoreAnimation::getCurrentTexture): * Source/WebGPU/WebGPU/PresentationContextIOSurface.h: * Source/WebGPU/WebGPU/PresentationContextIOSurface.mm: (WebGPU::PresentationContextIOSurface::getCurrentTexture): * Source/WebGPU/WebGPU/WebGPUExt.h: * Source/WebKit/GPUProcess/graphics/WebGPU/RemotePresentationContext.cpp: (WebKit::RemotePresentationContext::getCurrentTexture): (WebKit::RemotePresentationContext::present): * Source/WebKit/GPUProcess/graphics/WebGPU/RemotePresentationContext.h: * Source/WebKit/GPUProcess/graphics/WebGPU/RemotePresentationContext.messages.in: * Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemoteDeviceProxy.cpp: (WebKit::WebGPU::RemoteDeviceProxy::createTexture): (WebKit::WebGPU::RemoteDeviceProxy::createSurfaceTexture): * Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemotePresentationContextProxy.cpp: (WebKit::WebGPU::RemotePresentationContextProxy::getCurrentTexture): (WebKit::WebGPU::RemotePresentationContextProxy::present): * Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemotePresentationContextProxy.h: * Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemoteTextureProxy.cpp: (WebKit::WebGPU::RemoteTextureProxy::RemoteTextureProxy): * Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemoteTextureProxy.h: Canonical link: https://commits.webkit.org/259585@main
I think the consensus here is to switch this to WGPUTexture; we just need to make the change. Reading the comments, Mozilla had initially pushed for WGPUTextureView but has since reversed that opinion since many people want WGPUTexture out of the swapchain. |
This is done, it can be closed. |
As a follow-up to #88, we should start aligning this API closer to upstream.
Switching to a texture from a view is one such step.
Should be:
The text was updated successfully, but these errors were encountered: