Skip to content
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

Closed
kvark opened this issue Jun 8, 2021 · 14 comments
Closed

Return a texture instead of a view by the swapchain #89

kvark opened this issue Jun 8, 2021 · 14 comments
Labels
has resolution Issue is resolved, just needs to be done presentation Presenting images to surfaces like windows and canvases

Comments

@kvark
Copy link
Collaborator

kvark commented Jun 8, 2021

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.

typedef WGPUTextureView (*WGPUProcSwapChainGetCurrentTextureView)(WGPUSwapChain swapChain);

Should be:

typedef WGPUTexture (*WGPUProcSwapChainGetCurrentTexture)(WGPUSwapChain swapChain);
@Kangz
Copy link
Collaborator

Kangz commented Jun 8, 2021

Sounds good, we should also match the new merged GPUCanvasContent and GPUSwapChain by changing from creating a new swapchain on each configure to instead reconfiguring the same swapchain.

And also decide how to give signals about the swapchain being suboptimal / outdated.

@kainino0x
Copy link
Collaborator

kainino0x commented Jun 8, 2021

I don't remember the exact rationale for returning texture views, but that's no longer a concern?
https://github.com/webgpu-native/webgpu-headers/pull/13/files#r336157359

@kvark
Copy link
Collaborator Author

kvark commented Jun 9, 2021

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.

bors bot added a commit to gfx-rs/wgpu that referenced this issue Jul 2, 2021
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>
@lisanlow
Copy link

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?
With reference to the discussion in this issue, is there any change in general direction to keep native API to be render-only?

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.
Existing wgpuCommandEncoderCopyTextureToTexture API supports only copying from texture to texture but not texture view.

@kainino0x
Copy link
Collaborator

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.

@litherum
Copy link

Would be nice to have a resolution for this. We're running into it.

@Kangz
Copy link
Collaborator

Kangz commented Jun 26, 2022

I'm surprised you are running into it while implementing WebKit. Browsers need a lot more special logic to implement the GPUCanvasContext than what any reasonable WGPUSwapChain would give. For example on macOS my understanding is that the canvas textures are internally IOSurfaces that have been wrapped in an MTLTexture themselves wrapped in a GPUTexture. The browser compositor is in control of these IOSurfaces so they can't come from a WGPUSwapChain. Am I missing something? Happy to talk about it offline too if you'd like.

@tuankiet65
Copy link

Hi, I'm spearheading the effort to get GPUCanvasContext working in WebKit as part of my internship. You aren't missing anything - I'm following how WebGL is implemented in WebKit, which is "draw into an IOSurface then use that as the canvas content". This requires access to the IOSurfaces in the swap chain. At first, I tried to have to have the swapchain entirely within WGPUSwapChain and have private API to wrangle the IOSurface out of it. It worked but was super clunky, so right now I just moved the swap chain into WebKit itself.

One issue going down this path is that we need to extend the WebGPU texture creation API to optionally accept an IOSurface so we can create a texture out of it. If somebody else were to replace our WebGPU implementation with Dawn or wgpu, then it wouldn't work because Dawn/wgpu doesn't support that exact private API. I'm curious on how Chrome does it - does Chrome uses Dawn APIs that are not exposed through this header or not? (quick glance through the code says yes but I'm unsure)

@Kangz
Copy link
Collaborator

Kangz commented Jul 26, 2022

In Dawn / Chromium there are a number of private APIs that are used to create WGPUTextures from platform objects. For Metal/IOSurfaces, it would be WrapIOSurface. Note that you'll also need an equivalent of the function below it for proper ordering of Metal commands between parts of WebKit. The private APIs for other backends are in the same folder if you're interested.

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.

@tuankiet65
Copy link

Note that you'll also need an equivalent of the function below it for proper ordering of Metal commands between parts of WebKit

Can you expand more on this?

@Kangz
Copy link
Collaborator

Kangz commented Jul 27, 2022

See the comment above WaitForCommandsToBeScheduled

@litherum
Copy link

Would be nice to have a resolution for this. We're running into it now, again.

litherum added a commit to litherum/WebKit that referenced this issue Jan 23, 2023
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:
webkit-early-warning-system pushed a commit to litherum/WebKit that referenced this issue Jan 30, 2023
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
@austinEng
Copy link
Collaborator

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.

@kainino0x kainino0x added the has resolution Issue is resolved, just needs to be done label May 24, 2023
@kainino0x kainino0x added the presentation Presenting images to surfaces like windows and canvases label Jun 29, 2023
@rajveermalviya
Copy link
Collaborator

This is done, it can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has resolution Issue is resolved, just needs to be done presentation Presenting images to surfaces like windows and canvases
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants