-
Notifications
You must be signed in to change notification settings - Fork 547
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
Presentation Surface API #2882
Presentation Surface API #2882
Conversation
This works to the point of rendering the modified |
src/backend/metal/src/window.rs
Outdated
}; | ||
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]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be changed to self.inner.dimensions()
to take scaling into account?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried that and got incorrect scaling on the quad example, didn't investigate further
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quad is using logical pixels for everything. Which is why setting the drawable size to bounds
(logical size) works for quad.
The trickiest part of testing this out on our codebase was having to (maybe) create the framebuffers after |
I'm still seeing |
src/backend/metal/src/window.rs
Outdated
(drawable.to_owned(), texture.to_owned()) | ||
}); | ||
|
||
let id = raw.as_ptr() as usize as hal::SwapchainImageId; //HACK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we be able to use drawableID
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems borderline impossible to implement unique id without running into ABA. The metal backend would still have to track MTLTextures somehow.
examples/quad/main.rs
Outdated
recreate_swapchain = false; | ||
framebuffers.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We ought to call destroy_framebuffer
here on the framebuffers before dropping them.
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(). |
Here is the second iteration of the API. We aren't storing |
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. |
Scaling is also incorrect in my application. Is creating a I am looking forward to this merge! |
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 |
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 |
Scaling issues are fixed. Looks good to me! |
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 |
@aleksijuvani this is now also addressed. Looking into Vulkan and friends... |
src/backend/gl/src/queue.rs
Outdated
_image: crate::SurfaceImage, | ||
) -> Result<Option<hal::window::Suboptimal>, hal::window::PresentError> { | ||
#[cfg(all(feature = "glutin", not(target_arch = "wasm32")))] | ||
surface.context.swap_buffers().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this work that easily. How would the backend know when it should render into the default framebuffer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right, GL backend needs more stuff...
self.swapchain = Some(SurfaceSwapchain { | ||
swapchain, | ||
device: Arc::clone(&device.raw), | ||
fence: device.create_fence(false).unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to destruct this one somewhere
None => None, | ||
}; | ||
|
||
let (swapchain, images) = device.create_swapchain(self, config, old)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/backend/vulkan/src/window.rs
Outdated
@@ -164,15 +165,13 @@ impl Instance { | |||
unsafe { w_loader.create_wayland_surface(&info, None) }.expect("WaylandSurface failed") | |||
}; | |||
|
|||
self.create_surface_from_vk_surface_khr(surface, width, height, 1) | |||
self.create_surface_from_vk_surface_khr(surface) | |||
} | |||
|
|||
#[cfg(target_os = "android")] | |||
pub fn create_surface_android( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The calls to create_surface_from_wayland
and create_surface_android
need to be updated to remove the width/height parameters.
814546a
to
541e696
Compare
As I was working through DX11 implementation (see last commit), I realized that DX11/12, Metal, and Vulkan all want different things:
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(). |
Went through the pains of rebasing, and also implemented the core Vulkan logic of handling the API. It doesn't work yet... |
src/backend/metal/src/window.rs
Outdated
// it here. The drawable size and the layer size don't correlate | ||
#[cfg(target_os = "ios")] | ||
{ | ||
if let Some(view) = surface.inner.view { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if let Some(view) = surface.inner.view { | |
if let Some(view) = self.view { |
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? |
Thank you for testing! |
96f8fa4
to
6dc6e91
Compare
Alright, Vulkan works now. I'm nailing down the last remaining pieces. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, great work.
(skimmed over the metal parts)
bors r+
width: 16, | ||
height: 16, | ||
} ..= w::Extent2D { | ||
width: 4096, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these values arbitrary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
@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... |
Build failed |
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>
Build failed |
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>
Build failed |
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>
Build succeeded |
Fixes #2863
PR checklist:
make
succeeds (on *nix)make reftests
succeedsrustfmt
run on changed code