Presentation Surface API by kvark · Pull Request #2882 · gfx-rs/gfx

kvark · 2019-07-03T15:27:07Z

Fixes #2863
PR checklist:

make succeeds (on *nix)
make reftests succeeds
tested examples with the following backends: Metal
rustfmt run on changed code

kvark · 2019-07-03T15:31:55Z

This works to the point of rendering the modified quad example on macOS. The API is not quite good yet, we need some more iteration on it. Requesting feedback!

mtak- · 2019-07-04T00:05:21Z

src/backend/metal/src/window.rs

+        };
+        let can_set_display_sync = is_mac && caps.has_version_at_least(10, 13);
+        let drawable_size = {
+            let bounds: CGRect = msg_send![render_layer, bounds];


Can this be changed to self.inner.dimensions() to take scaling into account?

I tried that and got incorrect scaling on the quad example, didn't investigate further

This doesn't look right to me. bounds returns the drawable size in logical pixels. drawableSize ought to be used to get the drawable size in physical pixels. Even if we change this, it appears that this is trying to set the drawable size to the same value that it already is. I'm not sure what was intended here.

Shouldn't we be storing the new extents of the swapchain image in SurfaceSwapchainConfig and be setting the drawable size to that instead? Currently, there doesn't seem to be any way to specify the extents of the swapchain.

Quad is using logical pixels for everything. Which is why setting the drawable size to bounds (logical size) works for quad.

mtak- · 2019-07-04T00:42:44Z

The trickiest part of testing this out on our codebase was having to (maybe) create the framebuffers after acquire_image which is somewhat the point of this. I wound up just using a HashMap like the quad example. I don't have much feedback on the API except that it was easy to port over to it. The metal implementation does fix our resizing problems on iOS/macos - everything is butter smooth.

ghost · 2019-07-04T07:04:37Z

I'm still seeing IOSurface leaks with this implementation from nextDrawable when the window is in fullscreen mode. Are we freeing the drawables correctly?

ghost · 2019-07-04T08:45:44Z

src/backend/metal/src/window.rs

+            (drawable.to_owned(), texture.to_owned())
+        });
+
+        let id = raw.as_ptr() as usize as hal::SwapchainImageId; //HACK


Would we be able to use drawableID here?

According to the docs, this would just be monotonically increasing. Is there use for this ID then? The original idea behind the ID was that the users can keep associated framebuffers... I'd really like to get rid of this ID semantics though, it's not very sound. The main blocker is a potential use of the swapchain image in a framebuffer that has other images :/

I interpreted the documentation as the ID increasing every time a new drawable is created, but if an existing drawable is recycled, no new drawable would be created, and thus the ID would be the same. I notice that it doesn't explicitly state this, so I would have to test what it returns. Regardless, I realize now that drawableID is only available on iOS 10.3+ and tvOS 10.3+. It is not available on macOS.

It seems borderline impossible to implement unique id without running into ABA. The metal backend would still have to track MTLTextures somehow.

ghost · 2019-07-04T18:34:39Z

examples/quad/main.rs

+            viewport.rect.w = resize_dims.width as _;
+            viewport.rect.h = resize_dims.height as _;
            recreate_swapchain = false;
+            framebuffers.clear();


We ought to call destroy_framebuffer here on the framebuffers before dropping them.

kvark · 2019-07-05T20:04:47Z

Thanks @aleksijuvani and @mtak- for review and early testing of this!

The next step in the API, as discussed here and on gitter, would be to remove the IDs completely and instead ask the users to create the framebuffers every frame and delete when appropriate. Metal and DX12 backends would make sure to release the swapchain image-associated resources on present().

kvark · 2019-07-10T22:41:11Z

Here is the second iteration of the API. We aren't storing MTLTexture objects for the views of the surface swapchain any more, so I don't expect any leaks. Please take a look!

ghost · 2019-07-12T08:05:21Z

Looks good! I'm not seeing any more leaks. The swapchain image extents will still need to be addressed. The current implementation causes incorrect scaling in my application.

mtak- · 2019-07-12T18:04:12Z

Scaling is also incorrect in my application. Is creating a Framebuffer during a render OK on vulkan?

I am looking forward to this merge!

kvark · 2019-07-12T18:30:29Z

@mtak-

Is creating a Framebuffer during a render OK on vulkan?

That is still a big unknown in the air. We have somewhat compelling evidence that it's fine: neither DX11/12 or Metal have concepts for framebuffers, so these must be purely logical in the driver and lightweight.

I'll fix the scaling and try to eliminate kind() method as suggested by @omni-viral . Also will move out the things into a separate trait.

kvark · 2019-07-12T19:05:46Z

Alright, I addressed all the concerns in these comments now. Let's all have one more look (3rd iteration?) before we proceed with the other backends

mtak- · 2019-07-12T21:11:47Z

Scaling issues are fixed. Looks good to me!

ghost · 2019-07-13T13:20:19Z

Looks good to me. Scaling seems correct in my application as well. Is there any reason we're no longer allowing the user to configure the size of the swapchain images? I see that we're now always setting it to the bounds of the NSView.

kvark · 2019-07-13T14:06:34Z

@aleksijuvani this is now also addressed. Looking into Vulkan and friends...

msiglreith · 2019-07-15T10:30:24Z

src/backend/gl/src/queue.rs

+        _image: crate::SurfaceImage,
+    ) -> Result<Option<hal::window::Suboptimal>, hal::window::PresentError> {
+        #[cfg(all(feature = "glutin", not(target_arch = "wasm32")))]
+        surface.context.swap_buffers().unwrap();


I don't think this work that easily. How would the backend know when it should render into the default framebuffer?

you are right, GL backend needs more stuff...

msiglreith · 2019-07-15T10:34:35Z

src/backend/vulkan/src/window.rs

+        self.swapchain = Some(SurfaceSwapchain {
+            swapchain,
+            device: Arc::clone(&device.raw),
+            fence: device.create_fence(false).unwrap(),


need to destruct this one somewhere

ghost · 2019-07-23T09:06:34Z

src/backend/vulkan/src/window.rs

+            None => None,
+        };
+
+        let (swapchain, images) = device.create_swapchain(self, config, old)?;


There's a validation error on program exit about the swapchain not being destroyed prior to the surface being destroyed. The swapchain ought to be destroyed when the surface is destroyed.

mtak- · 2019-07-30T23:25:48Z

src/backend/vulkan/src/window.rs

    }

    #[cfg(target_os = "android")]
    pub fn create_surface_android(


The calls to create_surface_from_wayland and create_surface_android need to be updated to remove the width/height parameters.

kvark · 2019-08-01T03:31:22Z

As I was working through DX11 implementation (see last commit), I realized that DX11/12, Metal, and Vulkan all want different things:

Metal wants all associated framebuffers to be removed around present() time
DX11/12 wants all the associated framebuffers to be removed before configure_swapchain() time. This is a limitation of ResizeBuffers call that requires no references to the swapchain buffers.
Vulkan wants the associated framebuffers to live for as long as GPU uses them...

If the user is responsible for creation/deletion of these framebuffers, the Metal and DX backends would have a hard time erasing the internal mutable state of these objects while the user is still owning them. This is possible but really clunky and not pretty.

Given that and the fact that in Metal and DX framebuffers are "logical" objects that don't have a low-level counterpart (and thus easy/fast to create), it seems like the best way to proceed would be to make the Vulkan backend cache and track the swapchain image views and associated framebuffers. From the user perspective, they'd have to remove all the associated resource before the next call to either acquire() or configure_swapchain().
@omni-viral @msiglreith does that sound feasible to you?

kvark · 2019-08-12T21:39:12Z

Went through the pains of rebasing, and also implemented the core Vulkan logic of handling the API. It doesn't work yet...

ghost · 2019-08-13T16:06:18Z

src/backend/metal/src/window.rs

+            // it here. The drawable size and the layer size don't correlate
+            #[cfg(target_os = "ios")]
+            {
+                if let Some(view) = surface.inner.view {


Suggested change

if let Some(view) = surface.inner.view {

if let Some(view) = self.view {

ghost · 2019-08-13T16:41:20Z

I've tested the latest changes with my application, and both DirectX 12 and Metal seem to be working. Vulkan crashes after presenting a single frame and complains about deleting an invalid framebuffer, but I think that's to be expected for now?

kvark · 2019-08-13T17:03:59Z

Thank you for testing!
Yes, I'm still figuring out what to do with Vulkan. This PR ended up being a huge time/effort investment that I didn't anticipate.

kvark · 2019-08-13T19:14:18Z

Alright, Vulkan works now. I'm nailing down the last remaining pieces.
@msiglreith please have another look

msiglreith

lgtm, great work.
(skimmed over the metal parts)

bors r+

msiglreith · 2019-08-13T20:56:45Z

src/backend/dx12/src/window.rs

+                width: 16,
+                height: 16,
+            } ..= w::Extent2D {
+                width: 4096,


are these values arbitrary?

pretty much. Is this wrong? does this have to match the current extent only?

2882: Presentation Surface API r=msiglreith a=kvark Fixes #2863 PR checklist: - [x] `make` succeeds (on *nix) - [x] `make reftests` succeeds - [x] tested examples with the following backends: Metal - [x] `rustfmt` run on changed code Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com> Co-authored-by: Dzmitry Malyshau <dmalyshau@mozilla.com>

kvark · 2019-08-13T21:23:27Z

@grovesNL there is a bit of repetition in the GL window modules now, but I'm not capable of cleaning it up nicely, since I don't want to dive into the zoo of GL WSI semantics in this PR...

bors · 2019-08-13T21:59:57Z

Build failed

continuous-integration/travis-ci/push

kvark · 2019-08-13T22:04:49Z

Bors retry

…

On Aug 13, 2019, at 17:59, bors[bot] ***@***.***> wrote: Build failed continuous-integration/travis-ci/push — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

2882: Presentation Surface API r=msiglreith a=kvark Fixes #2863 PR checklist: - [x] `make` succeeds (on *nix) - [x] `make reftests` succeeds - [x] tested examples with the following backends: Metal - [x] `rustfmt` run on changed code Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com> Co-authored-by: Dzmitry Malyshau <dmalyshau@mozilla.com>

bors · 2019-08-13T22:27:02Z

Build failed

continuous-integration/travis-ci/push

kvark · 2019-08-14T00:42:16Z

Bors retry

…

On Aug 13, 2019, at 18:27, bors[bot] ***@***.***> wrote: Build failed continuous-integration/travis-ci/push — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

2882: Presentation Surface API r=msiglreith a=kvark Fixes #2863 PR checklist: - [x] `make` succeeds (on *nix) - [x] `make reftests` succeeds - [x] tested examples with the following backends: Metal - [x] `rustfmt` run on changed code Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com> Co-authored-by: Dzmitry Malyshau <dmalyshau@mozilla.com>

bors · 2019-08-14T01:03:36Z

Build failed

continuous-integration/travis-ci/push

kvark · 2019-08-14T03:31:20Z

bors r=msiglreith

2882: Presentation Surface API r=msiglreith a=kvark Fixes #2863 PR checklist: - [x] `make` succeeds (on *nix) - [x] `make reftests` succeeds - [x] tested examples with the following backends: Metal - [x] `rustfmt` run on changed code Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com> Co-authored-by: Dzmitry Malyshau <dmalyshau@mozilla.com>

bors · 2019-08-14T04:26:53Z

Build succeeded

continuous-integration/travis-ci/push

mtak- reviewed Jul 4, 2019

View reviewed changes

ghost reviewed Jul 4, 2019

View reviewed changes

kvark force-pushed the alt-swapchain branch from 7fd2d0d to 5a4af00 Compare July 10, 2019 22:39

kvark mentioned this pull request Jul 12, 2019

[mtl] fullscreen blocks waiting for next drawable if swapchain image count is 2 #2900

Open

kvark changed the title ~~Surface-based swapchain API~~ Presentation Surface API Jul 12, 2019

msiglreith reviewed Jul 15, 2019

View reviewed changes

kvark mentioned this pull request Jul 18, 2019

How to handle window resizing on macOS gfx-rs/wgpu#249

Closed

ghost suggested changes Jul 23, 2019

View reviewed changes

kvark mentioned this pull request Jul 29, 2019

[meta] Application testing gfx-rs/portability#125

Open

75 tasks

mtak- reviewed Jul 30, 2019

View reviewed changes

kvark force-pushed the alt-swapchain branch 2 times, most recently from 814546a to 541e696 Compare August 1, 2019 01:51

ghost mentioned this pull request Aug 1, 2019

iOS: add support for controlling the home indicator, and Exclusive video mode rust-windowing/winit#1078

Merged

6 tasks

kvark force-pushed the alt-swapchain branch from 5b845c7 to 73eb1f4 Compare August 12, 2019 21:38

kvark force-pushed the alt-swapchain branch from 73eb1f4 to 7f4c586 Compare August 12, 2019 22:15

ghost suggested changes Aug 13, 2019

View reviewed changes

kvark force-pushed the alt-swapchain branch from 4ad11af to 2270644 Compare August 13, 2019 16:07

kvark force-pushed the alt-swapchain branch 2 times, most recently from 96f8fa4 to 6dc6e91 Compare August 13, 2019 19:12

Alternative swapchain model

40a905f

kvark force-pushed the alt-swapchain branch from 6dc6e91 to 40a905f Compare August 13, 2019 19:20

kvark requested a review from msiglreith August 13, 2019 19:20

kvark marked this pull request as ready for review August 13, 2019 19:20

Cargo fmt pass

a2cf6c1

msiglreith approved these changes Aug 13, 2019

View reviewed changes

Disable X11 init in Travis

417593c

bors bot merged commit 417593c into gfx-rs:master Aug 14, 2019

kvark deleted the alt-swapchain branch August 14, 2019 14:54

kvark mentioned this pull request Aug 31, 2019

Use the new swapchain model of gfx-rs gfx-rs/wgpu#322

Closed

parasyte mentioned this pull request Sep 5, 2019

macOS: "acquire_image failed, re-creating" gfx-rs/wgpu-rs#74

Closed

	if let Some(view) = surface.inner.view {
	if let Some(view) = self.view {

Conversation

kvark commented Jul 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kvark commented Jul 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtak- commented Jul 4, 2019

Uh oh!

ghost commented Jul 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kvark commented Jul 5, 2019

Uh oh!

kvark commented Jul 10, 2019

Uh oh!

ghost commented Jul 12, 2019

Uh oh!

mtak- commented Jul 12, 2019

Uh oh!

kvark commented Jul 12, 2019

Uh oh!

kvark commented Jul 12, 2019

Uh oh!

mtak- commented Jul 12, 2019

Uh oh!

ghost commented Jul 13, 2019

Uh oh!

kvark commented Jul 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kvark commented Aug 1, 2019

Uh oh!

kvark commented Aug 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 13, 2019

Uh oh!

kvark commented Aug 13, 2019

Uh oh!

kvark commented Aug 13, 2019

Uh oh!

msiglreith left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kvark commented Aug 13, 2019

Uh oh!

bors bot commented Aug 13, 2019

Build failed

Uh oh!

kvark commented Jul 3, 2019 •

edited

Loading