rhi: metal: Switch back to presentDrawable

This convenience should be, according to the Apple docs, equivalent to
calling present from a scheduled handler. (which on its own makes it
unclear why we switched in the first place)

In practice it seems the two approaches are not identical. It looks
like that once a frame is submitted earlier than the next display link
callback, the throttling behavior we implement in beginFrame()
(waiting on the semaphore for the completion of the appropriate
command list etc.) starts exhibiting unexpected behavior, not
correctly throttling the thread to the refresh rate. Changing back to
presentDrawable does not exhibit this at all.

The suspicion is that presentDrawable is probably doing more than what
the docs suggest, and so is not fully equivalent to calling present
manually from a scheduled handler.

Therefore, switch to presentDrawable now, which restores the expected
cross-platform behavior, but make a note of the oddity, and also
prepare the hellominimalcrossgfxtriangle manual test to provide an
easy, self-contained application to allow experimenting in the future,
if needed.

This allows Qt Quick render thread animations to advance at the
expected speed (because the render thread is correctly throttled to
the refresh rate), even if the render thread decides to generate a new
frame right away, without waiting for the next display link update.

Without this patch, attempting to get updates not via requestUpdate(),
but by other means (timer etc.) leads to incorrect throttling, and so
the triangle in the test app is rotating faster than expected - but
only with Metal. Running with OpenGL on macOS or with any API on any
other platform the behavior will be correct. Even if scheduling
updates without display link is not efficient, and should be
discouraged, not doing so cannot break the core contract of vsync
throttling, i.e. the thread cannot run faster just because it renders
a frame not in response to an UpdateRequest.

Amends 98b60450f7ce6b16464392747ab8721f30add15e (effectively reverts
but keeps the code and the notes because we might want to clear this
up some day)

Fixes: QTBUG-103415
Change-Id: Id3bd43e94785384142337564ce4b2644bf257100
Reviewed-by: Tor Arne Vestbø <tor.arne.vestbo@qt.io>
(cherry picked from commit f814cc6a7911f6cf14ce443f41c2336bc1d213c6)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

2 files changed, 33 insertions, 4 deletions

diff --git a/src/gui/rhi/qrhimetal.mm b/src/gui/rhi/qrhimetal.mm
index c4317e5f0f..6e469f5b6d 100644
--- a/src/gui/rhi/qrhimetal.mm
+++ b/src/gui/rhi/qrhimetal.mm
@@ -1494,10 +1494,25 @@ QRhi::FrameOpResult QRhiMetal::endFrame(QRhiSwapChain *swapChain, QRhi::EndFrame
 
     const bool needsPresent = !flags.testFlag(QRhi::SkipPresent);
     if (needsPresent) {
-        auto drawable = swapChainD->d->curDrawable;
-        [swapChainD->cbWrapper.d->cb addScheduledHandler:^(id<MTLCommandBuffer>) {
-            [drawable present];
-        }];
+        // beginFrame-endFrame without a render pass inbetween means there is no
+        // drawable, handle this gracefully because presentDrawable does not like
+        // null arguments.
+        if (id<CAMetalDrawable> drawable = swapChainD->d->curDrawable) {
+            // QTBUG-103415: while the docs suggest the following two approaches are
+            // equivalent, there is a difference in case a frame is recorded earlier than
+            // (i.e. not in response to) the next CVDisplayLink callback. Therefore, stick
+            // with presentDrawable, which gives results identical to OpenGL, and all other
+            // platforms, i.e. throttles to vsync as expected, meaning constant 15-17 ms with
+            // a 60 Hz screen, no jumps with smaller intervals, regardless of when the frame
+            // is submitted by the app)
+#if 1
+            [swapChainD->cbWrapper.d->cb presentDrawable: drawable];
+#else
+            [swapChainD->cbWrapper.d->cb addScheduledHandler:^(id<MTLCommandBuffer>) {
+                [drawable present];
+            }];
+#endif
+        }
     }
 
     // Must not hold on to the drawable, regardless of needsPresent
diff --git a/tests/manual/rhi/hellominimalcrossgfxtriangle/window.cpp b/tests/manual/rhi/hellominimalcrossgfxtriangle/window.cpp
index 153bd2c37f..2a3475538d 100644
--- a/tests/manual/rhi/hellominimalcrossgfxtriangle/window.cpp
+++ b/tests/manual/rhi/hellominimalcrossgfxtriangle/window.cpp
@@ -3,6 +3,7 @@
 
 #include "window.h"
 #include <QPlatformSurfaceEvent>
+#include <QTimer>
 
 Window::Window(QRhi::Implementation graphicsApi)
     : m_graphicsApi(graphicsApi)
@@ -196,7 +197,20 @@ void Window::render()
 
     m_rhi->endFrame(m_sc.get());
 
+    // Always request the next frame via requestUpdate(). On some platforms this is backed
+    // by a platform-specific solution, e.g. CVDisplayLink on macOS, which is potentially
+    // more efficient than a timer, queued metacalls, etc.
+    //
+    // However, the rendering behavior is identical no matter how the next round of
+    // rendering is triggered: the rendering thread is throttled to the presentation rate
+    // (either in beginFrame() or endFrame()) so the triangle should rotate at the exact
+    // same speed no matter which approach is taken here.
+
+#if 1
     requestUpdate();
+#else
+    QTimer::singleShot(0, this, [this] { render(); });
+#endif
 }
 
 void Window::customInit()