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

Support VK_PRESENT_MODE_IMMEDIATE_KHR if VkPresentTimeGOOGLE::desiredPresentTime is zero. #1936

Conversation

billhollings
Copy link
Contributor

@billhollings billhollings commented Jun 6, 2023

  • [MTLDrawable presentAtTime:] syncs to display vsync. To support VK_PRESENT_MODE_IMMEDIATE_KHR while using VkPresentTimeGOOGLE::presentID, only call presentAtTime: if VkPresentTimeGOOGLE::desiredPresentTime has been explicitly set to a non-zero value.
  • Clarify initially clearing MVKImagePresentInfo to all zeros.

Fixes issue #1925.

…PresentTime is zero.

- [MTLDrawable presentAtTime:] syncs to display vsync. To support
  VK_PRESENT_MODE_IMMEDIATE_KHR while using VkPresentTimeGOOGLE::presentID,
  only call presentAtTime: if VkPresentTimeGOOGLE::desiredPresentTime has
  been explicitly set to a non-zero value.
- Clarify initially clearing MVKImagePresentInfo to all zeros.
@billhollings billhollings merged commit 4949c34 into KhronosGroup:main Jun 7, 2023
7 checks passed
@billhollings billhollings deleted the present-imm-mode-unless-present-time branch June 7, 2023 22:42
@SRSaunders
Copy link
Contributor

SRSaunders commented Jun 15, 2023

A quick follow-up to this fix. I decided to upgrade my dev system from Monterey to Ventura and noticed some interesting differences related to this issue, fortunately on the positive side:

  1. Independent of this issue, Apple seems to have made a change to the compositing/present handling in Ventura that now properly respects vsync frame rate for windowed environments with MoltenVK. Previously (Monterey and earlier), when using MoltenVK in a windowed environment with vsync on, the frame rate would appear double the monitor refresh rate. Only in fullscreen mode would the frame rate equal the monitor refresh rate. This issue now seems fixed in Ventura, with windowed environments showing correct vsync behaviour and frame rate.
  2. Specifically regarding this fix, it seems that without it and when presentInfo.hasPresentTime = true and desiredPresentTime = 0, the new Ventura compositing/present handling behaves poorly and causes severe display flickering when frame rates exceed the refresh rate, almost like it is skipping display frames or inserting blank frames. This did not happen in Monterey and earlier. I was able to observe this behaviour using the stock 1.3.250 SDK (which does not yet include your fix) and also by reverting your fix in the latest dev stream of MoltenVK standalone. So it appears that presentationTime = 0 may have meaning to presentAtTime, but as far as I can tell it is not good behaviour for this situation.

In summary, this fix was good in Monterey, but even better in Ventura.

UPDATE: I am speculating here, but I wonder if presentAtTime: presentationTime = 0 now under certain situations means drop this frame on the floor and don't present it? If so, this perhaps could give you a mechanism (coupled with vsync timing calculation logic) to support VK_PRESENT_MODE_MAILBOX_KHR. Maybe a bit of a stretch but interesting to think about.

@billhollings
Copy link
Contributor Author

Thanks for returning here with an update!

In summary, this fix was good in Monterey, but even better in Ventura.

Wonderful! Actually, I built the fix in Ventura 13.3. So, that might have been a factor in our back-and-forth discussion on the issue itself?

UPDATE: I am speculating here, but I wonder if presentAtTime: presentationTime = 0 now under certain situations means drop this frame on the floor and don't present it? If so, this perhaps could give you a mechanism (coupled with vsync timing calculation logic) to support VK_PRESENT_MODE_MAILBOX_KHR. Maybe a bit of a stretch but interesting to think about.

Ah. An interesting idea, if it works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants