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

Improve PresentationMode Api #2711

Closed
cwfitzgerald opened this issue Jun 2, 2022 · 9 comments · Fixed by #2803
Closed

Improve PresentationMode Api #2711

cwfitzgerald opened this issue Jun 2, 2022 · 9 comments · Fixed by #2803
Labels
area: wsi Issues with swapchain management or windowing type: enhancement New feature or request
Milestone

Comments

@cwfitzgerald
Copy link
Member

cwfitzgerald commented Jun 2, 2022

This comment #2711 (comment) has a much cleaner and easier to use api that I think we should use after a discussion.

Is your feature request related to a problem? Please describe.

The set of Immediate, Fifo, and Mailbox isn't really representative of all the options that our backends provide us for swapchain management. It also forces us to explicitly fallback from Mailbox to FIFO when it isn't available. I've gotten requests for fallback to Immediate because what they wanted was "run at fast as possible" with "no tearing" as a benefit.

Thinking about a design, let's look at the options that are available to us.

Vulkan:

  • Immediate: No Wait, Tearing,
  • Fifo: Waiting, Tearing.
  • Fifo-Relaxed: Waiting if fast, tearing if slow.
  • Mailbox: No wait, no tearing

Metal:

  • Sync On: Waiting, No Tearing.
  • Sync Off: No Waiting, Tearing.

DXGI:

  • Sync Interval 0: No Waiting, No Tearing
  • Sync Interval 1: Waiting, No Tearing
  • Sync Interval 0 Allow Tearing: No Waiting, No Tearing (this has something to do with VFR, figuring this out now)

GL:

  • Swap Interval 0: No Waiting, Tearing,
  • Swap Interval 1: Waiting, No Tearing

Describe the solution you'd like

I want an api which allows the user to express their preferences for waiting and tearing.

enum PresentModeTearing {
    AllowTearing,
    NoTearing,
}

enum PresentModeWaiting {
    WaitForSync,
    AllowNoWait,
}

struct PresentMode {
    tearing: PresentModeTearing,
    sync: PresentModeWaiting,
}

Which will result in the following:

Tearing, Sync Vulkan DXGI GL Metal
NoTearing, AllowNoWait [Mailbox, FIFO] Sync 0 Swap 1 Sync On
NoTearing, WaitForSync [FIFO] Sync 1 Swap 1 Sync On
AllowTearing, AllowNoWait [Mailbox, Immediate, FIFO, FIFO_Relaxed] Sync 0 Swap 0 Sync Off
AllowTearing, WaitForSync [FIFO_RELAXED, FIFO] Sync 1 Swap 0 Sync Off

Describe alternatives you've considered

It isn't very representative to allow the user to directly be able to choice modes when there are so many different ways to represent the combinations and the possible fallbacks. These two enums communicate intent so we can make the right decision.

@cwfitzgerald cwfitzgerald added type: enhancement New feature or request area: api Issues related to API surface labels Jun 2, 2022
@cwfitzgerald cwfitzgerald added this to the Release 0.13 milestone Jun 2, 2022
@kpreid
Copy link
Contributor

kpreid commented Jun 2, 2022

…express their preferences…

enum PresentModeWaiting {
    WaitForSync,
    AllowNoWait,
}

What about “prefers no wait”? That also seems necessary to be able to pick out Immediate or Mailbox from the Vulkan modes?

(I'm not especially familiar with the topic; just reading what's written here.)

@jimblandy
Copy link
Member

Assuming that @kpreid's correct that users may want a "don't wait" option (I also am not too familiar with this topic):

It seems like the two axes can be in conflict on Vulkan: there's no option that never tears, but never waits. Do we need to let people prioritize?

struct SurfaceConfiguration {
    present_mode_by_preference: &[PresentMode],
}

Obviously it's preferable to keep it simple.

@cwfitzgerald
Copy link
Member Author

cwfitzgerald commented Jun 2, 2022

After some introspection I'm not really a fan of this api and after discussion on the matrix we came up with a better api.

fn Surface::get_supported_present_modes(&self, adapter: &Adapter) -> PresentModes;

With PresentModes being a bitflags of available modes.

Then we will change the PresentMode enum to include automatic modes.

enum PresentMode {
    // Mailbox -> Immediate (Fifo on web) 
    AutoNoVsync, 
    // FifoRelaxed -> Fifo
    AutoVsync, 
    #[default]
    Fifo,
    FifoRelaxed,
    Mailbox,
    Immediate
}

This gives the users full power to do what they need, but also provide simple, well defined automatic settings.

As for enumerating modes:

VK would passthrough their modes.

DX12 would be Mailbox or FIFO (with optional Immediate if tearing is allowed).

Metal and OpenGL would be FIFO or Immediate.

@Kangz
Copy link

Kangz commented Jun 2, 2022

Since you asked my opinion, while I agree that we should expose the capabilities of the implementation to the application, I find it a bit unfortunate that applications will have to adapt to an arbitrary list of present modes because they risk breaking on systems they didn't test against. But maybe for swpchain that's ok, esp with Auto[No]Vsync.

The other thing is that translation from bitfield to enum is a bit awkward to do. What about

fn Surface::supports_present_mode(&self, mode: PresentMode) -> bool

That's guaranteed to return true for Auto[No]Vsync?

Also isn't the possible modes a property of a device + surface combination and not just a surface (the surface is independent of even a backend, so the capability will depend on GL vs. Vk).

@cwfitzgerald
Copy link
Member Author

cwfitzgerald commented Jun 2, 2022

Also isn't the possible modes a property of a device + surface combination and not just a surface (the surface is independent of even a backend, so the capability will depend on GL vs. Vk).

It is, which is why there's an adapter argument, so we can know the presentation mode for that adapter on that surface.

The other thing is that translation from bitfield to enum is a bit awkward to do. What about

I'm not a big fan as that would mean calling into the api 4 times to enumerate all the values when we already have all of the formats at hand internally.

I would prefer returning a vector of modes over this. The main advantage of bitflags is that you can let the user do simpler for residency if modes.contains(PresentMode::Mailbox) { .. } (or modes & PRESENT_MODE_MAILBOX != 0 in c) vs needing to do linear scans.

@scoopr
Copy link
Contributor

scoopr commented Jun 2, 2022

I'll posit one extra use-case, which to me could warrant an extra enum variant, but I'll let you decide.
That would be perhaps called COMPOSITOR_SYNC, which is different from vsync, in that it would wait for next compositor present. This is already exposed in wgpu-hal for metal, you can set present_with_transaction.

I believe at least on mac this is required for flashless-resizes, so that when you render in response you resize event, the chrome is rendered in sync. Also if you are trying to sync up native UI elements to gpu rendered elements. I haven't looked at other platforms too closely.

This would be useful for more traditional gui-type apps, enabling a hopefully more polished look.
Annoyingly this is one of those features that might need more careful coordination with the windowing side (winit) and the gpu presentation (wgpu) which is always not easy.

How much of these other presentation modes have orthogonal properties? I kind of see (vsync-to-monitor vs vsync-to-compositor vs novsync) + (2frames vs 3frames) + (allow skipping if newer frame vs no skipping), but unsure how well that maps to reality. Also which of those can be dynamically controlled without full swapchain rebuild? presentWithTransaction looks like it could be enabled dynamically, so that could be enabled only when something is moving/resizing, but not sure if anything does that.

@Kangz
Copy link

Kangz commented Jun 3, 2022

I'm not a big fan as that would mean calling into the api 4 times to enumerate all the values when we already have all of the formats at hand internally.

The set you're returning as a bitfields is the exact same thing. People will look if specific bits are set so conceptually it's the same as calling the supports_mode API 4 times. However the bitfield requires the enum<->bitfield conversion for users, and also introduces some ordering of the modes which doesn't have a semantic value.

However the two Auto modes + the rest seem good.

@cwfitzgerald
Copy link
Member Author

@scoopr from what I can see, I think that would be a part of an optional argument to present as opposed to a setting on the swapchain. It's a very good think to keep in mind and likely deserves its own issue, but I don't think needs to block this.

However the two Auto modes + the rest seem good.

@Kangz 👍🏻

The set you're returning as a bitfields is the exact same thing. People will look if specific bits are set so conceptually it's the same as calling the supports_mode API 4 times. However the bitfield requires the enum<->bitfield conversion for users, and also introduces some ordering of the modes which doesn't have a semantic value.

I guess my main concern is that either we'd need to store the supported modes internally or we'd need to re-query all the time, both seem a bit silly. That being said, I don't think it's that big of a deal and if you think it would be a better api, especially over the C border, I would be totally okay with it. @kvark thoughts?

@kvark
Copy link
Member

kvark commented Jun 5, 2022

Expressed on the matrix. I don't think supports_present_mode is the best way to proceed for us. Instead, we have the SurfaceCapabilities struct, and it can contain a list of supported modes. Also, it's not necessary to include the Auto modes there, but having them in the enum is good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: wsi Issues with swapchain management or windowing type: enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants