Ensure drawable size atomicity within a frame

Revert surfacePixelSize() to be a getter only. With Metal this will
mean returning the "live" layer size (and so not the
layer.drawableSize), which is in line with what we expect with other
backends.

Instead, we leave it to the swapchain's buildOrResize() to "commit"
the size by setting drawableSize on the layer. With typical
application or Qt Quick logic this ensures that layer.drawableSize is
set once and stays static until we get to process the next resize - on
the rendering thread.

This of course would still mean that there was a race when a client
queries surfacePixelSize() to set the depth-stencil buffer size that
is associated with a swapchain. (because that must happen before
calling buildOrResize() according to the current semantics)

That can however be solved in a quite elegant way, it turns out,
because we already have a flag that indicates if a QRhiRenderBuffer is
used in combination with (and only in combination with) a
swapchain. If we simply say that setting the UsedWithSwapChainOnly
flag provides automatic sizing as well (so no setPixelSize() call is
needed), clients can simply get rid of the problematic
surfacePixelSize() query and everything works.

Task-number: QTBUG-78641
Change-Id: Ib1bfc9ef8531bcce033d1f1e5d4d5b4984d6d69f
Reviewed-by: Tor Arne Vestbø <tor.arne.vestbo@qt.io>

9 files changed, 100 insertions, 100 deletions

diff --git a/src/gui/rhi/qrhi.cpp b/src/gui/rhi/qrhi.cpp
index ee4caa47a0..cd813d8665 100644
--- a/src/gui/rhi/qrhi.cpp
+++ b/src/gui/rhi/qrhi.cpp
@@ -1981,6 +1981,14 @@ QRhiResource::Type QRhiBuffer::resourceType() const
     How the renderbuffer is implemented by a backend is not exposed to the
     applications. In some cases it may be backed by ordinary textures, while in
     others there may be a different kind of native resource used.
+
+    Renderbuffers that are used as (and are only used as) depth-stencil buffers
+    in combination with a QRhiSwapChain's color buffers should have the
+    UsedWithSwapChainOnly flag set. This serves a double purpose: such buffers,
+    depending on the backend and the underlying APIs, be more efficient, and
+    QRhi provides automatic sizing behavior to match the color buffers, which
+    means calling setPixelSize() and build() are not necessary for such
+    renderbuffers.
  */
 
 /*!
@@ -1996,12 +2004,15 @@ QRhiResource::Type QRhiBuffer::resourceType() const
     Flag values for flags() and setFlags()
 
     \value UsedWithSwapChainOnly For DepthStencil renderbuffers this indicates
-    that the renderbuffer is only used in combination with a QRhiSwapChain and
-    never in other ways. Relevant with some backends, while others ignore it.
-    With OpenGL where a separate windowing system interface API is in use (EGL,
-    GLX, etc.), the flag is important since it avoids creating any actual
-    resource as there is already a windowing system provided depth/stencil
-    buffer as requested by QSurfaceFormat.
+    that the renderbuffer is only used in combination with a QRhiSwapChain, and
+    never in any other way. This provides automatic sizing and resource
+    rebuilding, so calling setPixelSize() or build() is not needed whenever
+    this flag is set. This flag value may also trigger backend-specific
+    behavior, for example with OpenGL, where a separate windowing system
+    interface API is in use (EGL, GLX, etc.), the flag is especially important
+    as it avoids creating any actual renderbuffer resource as there is already
+    a windowing system provided depth/stencil buffer as requested by
+    QSurfaceFormat.
  */
 
 /*!
@@ -3351,7 +3362,7 @@ QRhiResource::Type QRhiGraphicsPipeline::resourceType() const
       {
           sc = rhi->newSwapChain();
           ds = rhi->newRenderBuffer(QRhiRenderBuffer::DepthStencil,
-                                    QSize(), // no need to set the size yet
+                                    QSize(), // no need to set the size here due to UsedWithSwapChainOnly
                                     1,
                                     QRhiRenderBuffer::UsedWithSwapChainOnly);
           sc->setWindow(window);
@@ -3363,9 +3374,6 @@ QRhiResource::Type QRhiGraphicsPipeline::resourceType() const
 
       void resizeSwapChain()
       {
-          const QSize outputSize = sc->surfacePixelSize();
-          ds->setPixelSize(outputSize);
-          ds->build();
           hasSwapChain = sc->buildOrResize();
       }
 
@@ -3559,10 +3567,24 @@ QRhiResource::Type QRhiSwapChain::resourceType() const
     \return The size of the window's associated surface or layer. Do not assume
     this is the same as QWindow::size() * QWindow::devicePixelRatio().
 
-    Can be called before buildOrResize() (but with window() already set), which
-    allows setting the correct size for the depth-stencil buffer that is then
-    used together with the swapchain's color buffers. Also used in combination
-    with currentPixelSize() to detect size changes.
+    \note Can also be called before buildOrResize(), if at least window() is
+    already set) This in combination with currentPixelSize() allows to detect
+    when a swapchain needs to be resized. However, watch out for the fact that
+    the size of the underlying native object (surface, layer, or similar) is
+    "live", so whenever this function is called, it returns the latest value
+    reported by the underlying implementation, without any atomicity guarantee.
+    Therefore, using this function to determine pixel sizes for graphics
+    resources that are used in a frame is strongly discouraged. Rely on
+    currentPixelSize() instead which returns a size that is atomic and will not
+    change between buildOrResize() invocations.
+
+    \note For depth-stencil buffers used in combination with the swapchain's
+    color buffers, it is strongly recommended to rely on the automatic sizing
+    and rebuilding behavior provided by the
+    QRhiRenderBuffer:UsedWithSwapChainOnly flag. Avoid querying the surface
+    size via this function just to get a size that can be passed to
+    QRhiRenderBuffer::setPixelSize() as that would suffer from the lack of
+    atomicity as described above.
 
     \sa currentPixelSize()
   */
diff --git a/src/gui/rhi/qrhid3d11.cpp b/src/gui/rhi/qrhid3d11.cpp
index 0b1822c0f5..e076bc7def 100644
--- a/src/gui/rhi/qrhid3d11.cpp
+++ b/src/gui/rhi/qrhid3d11.cpp
@@ -3939,9 +3939,16 @@ bool QD3D11SwapChain::buildOrResize()
                  m_depthStencil->sampleCount(), m_sampleCount);
     }
     if (m_depthStencil && m_depthStencil->pixelSize() != pixelSize) {
-        qWarning("Depth-stencil buffer's size (%dx%d) does not match the surface size (%dx%d). Expect problems.",
-                 m_depthStencil->pixelSize().width(), m_depthStencil->pixelSize().height(),
-                 pixelSize.width(), pixelSize.height());
+        if (m_depthStencil->flags().testFlag(QRhiRenderBuffer::UsedWithSwapChainOnly)) {
+            m_depthStencil->setPixelSize(pixelSize);
+            if (!m_depthStencil->build())
+                qWarning("Failed to rebuild swapchain's associated depth-stencil buffer for size %dx%d",
+                         pixelSize.width(), pixelSize.height());
+        } else {
+            qWarning("Depth-stencil buffer's size (%dx%d) does not match the surface size (%dx%d). Expect problems.",
+                     m_depthStencil->pixelSize().width(), m_depthStencil->pixelSize().height(),
+                     pixelSize.width(), pixelSize.height());
+        }
     }
 
     currentFrameSlot = 0;
diff --git a/src/gui/rhi/qrhigles2.cpp b/src/gui/rhi/qrhigles2.cpp
index de892c4430..dfa0351a8d 100644
--- a/src/gui/rhi/qrhigles2.cpp
+++ b/src/gui/rhi/qrhigles2.cpp
@@ -3707,6 +3707,13 @@ bool QGles2SwapChain::buildOrResize()
     m_currentPixelSize = surfacePixelSize();
     pixelSize = m_currentPixelSize;
 
+    if (m_depthStencil && m_depthStencil->flags().testFlag(QRhiRenderBuffer::UsedWithSwapChainOnly)
+            && m_depthStencil->pixelSize() != pixelSize)
+    {
+        m_depthStencil->setPixelSize(pixelSize);
+        m_depthStencil->build();
+    }
+
     rt.d.rp = QRHI_RES(QGles2RenderPassDescriptor, m_renderPassDesc);
     rt.d.pixelSize = pixelSize;
     rt.d.dpr = float(m_window->devicePixelRatio());
diff --git a/src/gui/rhi/qrhimetal.mm b/src/gui/rhi/qrhimetal.mm
index d4043f00a5..68886a6edf 100644
--- a/src/gui/rhi/qrhimetal.mm
+++ b/src/gui/rhi/qrhimetal.mm
@@ -1036,44 +1036,11 @@ void QRhiMetal::setVertexInput(QRhiCommandBuffer *cb,
     }
 }
 
-QSize safeOutputSize(QRhiMetal *rhiD, QMetalCommandBuffer *cbD)
-{
-    QSize size = cbD->currentTarget->pixelSize();
-
-    // So now we have the issue that the texture (drawable) size may have
-    // changed again since swapchain buildOrResize() was called. This can
-    // happen for example when interactively resizing the window a lot in one
-    // go, and command buffer building happens on a dedicated thread (f.ex.
-    // using the threaded render loop of Qt Quick).
-    //
-    // This is only an issue when running in debug mode with XCode because Metal
-    // validation will fail when setting viewport or scissor with the real size
-    // being smaller than what we think it is. So query the drawable size right
-    // here, in debug mode at least.
-    //
-    // In addition, we have to take the smaller of the two widths and heights
-    // to be safe, apparently. In some cases validation seems to think that the
-    // "render pass width" (or height) is the old(?) value.
-
-#ifdef QT_DEBUG
-    if (cbD->currentTarget->resourceType() == QRhiResource::RenderTarget) {
-        Q_ASSERT(rhiD->currentSwapChain);
-        const QSize otherSize = rhiD->currentSwapChain->surfacePixelSize();
-        size.setWidth(qMin(size.width(), otherSize.width()));
-        size.setHeight(qMin(size.height(), otherSize.height()));
-    }
-#else
-    Q_UNUSED(rhiD);
-#endif
-
-    return size;
-}
-
 void QRhiMetal::setViewport(QRhiCommandBuffer *cb, const QRhiViewport &viewport)
 {
     QMetalCommandBuffer *cbD = QRHI_RES(QMetalCommandBuffer, cb);
     Q_ASSERT(cbD->recordingPass == QMetalCommandBuffer::RenderPass);
-    const QSize outputSize = safeOutputSize(this, cbD);
+    const QSize outputSize = cbD->currentTarget->pixelSize();
 
     // x,y is top-left in MTLViewportRect but bottom-left in QRhiViewport
     float x, y, w, h;
@@ -1105,7 +1072,7 @@ void QRhiMetal::setScissor(QRhiCommandBuffer *cb, const QRhiScissor &scissor)
     QMetalCommandBuffer *cbD = QRHI_RES(QMetalCommandBuffer, cb);
     Q_ASSERT(cbD->recordingPass == QMetalCommandBuffer::RenderPass);
     Q_ASSERT(QRHI_RES(QMetalGraphicsPipeline, cbD->currentGraphicsPipeline)->m_flags.testFlag(QRhiGraphicsPipeline::UsesScissor));
-    const QSize outputSize = safeOutputSize(this, cbD);
+    const QSize outputSize = cbD->currentTarget->pixelSize();
 
     // x,y is top-left in MTLScissorRect but bottom-left in QRhiScissor
     int x, y, w, h;
@@ -3529,20 +3496,8 @@ QRhiRenderTarget *QMetalSwapChain::currentFrameRenderTarget()
 
 QSize QMetalSwapChain::surfacePixelSize()
 {
-    // may be called before build, must not access other than m_*
-
-    NSView *v = (NSView *) m_window->winId();
-    if (v) {
-        CAMetalLayer *layer = (CAMetalLayer *) [v layer];
-        if (layer) {
-            CGSize size = layer.bounds.size;
-            size.width *= layer.contentsScale;
-            size.height *= layer.contentsScale;
-            layer.drawableSize = size;
-            return QSize(int(size.width), int(size.height));
-        }
-    }
-    return QSize();
+    Q_ASSERT(m_window);
+    return m_window->size() * m_window->devicePixelRatio();
 }
 
 QRhiRenderPassDescriptor *QMetalSwapChain::newCompatibleRenderPassDescriptor()
@@ -3593,8 +3548,9 @@ bool QMetalSwapChain::buildOrResize()
         return false;
     }
 
-    NSView *v = (NSView *) window->winId();
-    d->layer = (CAMetalLayer *) [v layer];
+    NSView *view = reinterpret_cast<NSView *>(window->winId());
+    Q_ASSERT(view);
+    d->layer = static_cast<CAMetalLayer *>(view.layer);
     Q_ASSERT(d->layer);
 
     chooseFormats();
@@ -3623,7 +3579,15 @@ bool QMetalSwapChain::buildOrResize()
         d->layer.opaque = YES;
     }
 
-    m_currentPixelSize = surfacePixelSize();
+    // Now set the layer's drawableSize which will stay set to the same value
+    // until the next buildOrResize(), thus ensuring atomicity with regards to
+    // the drawable size in frames.
+    CGSize layerSize = d->layer.bounds.size;
+    layerSize.width *= d->layer.contentsScale;
+    layerSize.height *= d->layer.contentsScale;
+    d->layer.drawableSize = layerSize;
+
+    m_currentPixelSize = QSizeF::fromCGSize(layerSize).toSize();
     pixelSize = m_currentPixelSize;
 
     [d->layer setDevice: rhiD->d->dev];
@@ -3644,9 +3608,16 @@ bool QMetalSwapChain::buildOrResize()
                  m_depthStencil->sampleCount(), m_sampleCount);
     }
     if (m_depthStencil && m_depthStencil->pixelSize() != pixelSize) {
-        qWarning("Depth-stencil buffer's size (%dx%d) does not match the layer size (%dx%d). Expect problems.",
-                 m_depthStencil->pixelSize().width(), m_depthStencil->pixelSize().height(),
-                 pixelSize.width(), pixelSize.height());
+        if (m_depthStencil->flags().testFlag(QRhiRenderBuffer::UsedWithSwapChainOnly)) {
+            m_depthStencil->setPixelSize(pixelSize);
+            if (!m_depthStencil->build())
+                qWarning("Failed to rebuild swapchain's associated depth-stencil buffer for size %dx%d",
+                         pixelSize.width(), pixelSize.height());
+        } else {
+            qWarning("Depth-stencil buffer's size (%dx%d) does not match the layer size (%dx%d). Expect problems.",
+                     m_depthStencil->pixelSize().width(), m_depthStencil->pixelSize().height(),
+                     pixelSize.width(), pixelSize.height());
+        }
     }
 
     rtWrapper.d->pixelSize = pixelSize;
diff --git a/src/gui/rhi/qrhivulkan.cpp b/src/gui/rhi/qrhivulkan.cpp
index 444c91dd75..3668aaa47b 100644
--- a/src/gui/rhi/qrhivulkan.cpp
+++ b/src/gui/rhi/qrhivulkan.cpp
@@ -6315,9 +6315,16 @@ bool QVkSwapChain::buildOrResize()
                  m_depthStencil->sampleCount(), m_sampleCount);
     }
     if (m_depthStencil && m_depthStencil->pixelSize() != pixelSize) {
-        qWarning("Depth-stencil buffer's size (%dx%d) does not match the surface size (%dx%d). Expect problems.",
-                 m_depthStencil->pixelSize().width(), m_depthStencil->pixelSize().height(),
-                 pixelSize.width(), pixelSize.height());
+        if (m_depthStencil->flags().testFlag(QRhiRenderBuffer::UsedWithSwapChainOnly)) {
+            m_depthStencil->setPixelSize(pixelSize);
+            if (!m_depthStencil->build())
+                qWarning("Failed to rebuild swapchain's associated depth-stencil buffer for size %dx%d",
+                         pixelSize.width(), pixelSize.height());
+        } else {
+            qWarning("Depth-stencil buffer's size (%dx%d) does not match the surface size (%dx%d). Expect problems.",
+                     m_depthStencil->pixelSize().width(), m_depthStencil->pixelSize().height(),
+                     pixelSize.width(), pixelSize.height());
+        }
     }
 
     if (!m_renderPassDesc)
diff --git a/tests/manual/rhi/hellominimalcrossgfxtriangle/hellominimalcrossgfxtriangle.cpp b/tests/manual/rhi/hellominimalcrossgfxtriangle/hellominimalcrossgfxtriangle.cpp
index 3c39ff1719..ea1fefc308 100644
--- a/tests/manual/rhi/hellominimalcrossgfxtriangle/hellominimalcrossgfxtriangle.cpp
+++ b/tests/manual/rhi/hellominimalcrossgfxtriangle/hellominimalcrossgfxtriangle.cpp
@@ -298,7 +298,7 @@ void Window::init()
     m_sc = m_r->newSwapChain();
     // allow depth-stencil, although we do not actually enable depth test/write for the triangle
     m_ds = m_r->newRenderBuffer(QRhiRenderBuffer::DepthStencil,
-                                QSize(), // no need to set the size yet
+                                QSize(), // no need to set the size here, due to UsedWithSwapChainOnly
                                 1,
                                 QRhiRenderBuffer::UsedWithSwapChainOnly);
     releasePool << m_ds;
@@ -376,16 +376,12 @@ void Window::releaseResources()
 
 void Window::resizeSwapChain()
 {
-    const QSize outputSize = m_sc->surfacePixelSize();
-
-    m_ds->setPixelSize(outputSize);
-    m_ds->build(); // == m_ds->release(); m_ds->build();
-
-    m_hasSwapChain = m_sc->buildOrResize();
+    m_hasSwapChain = m_sc->buildOrResize(); // also handles m_ds
 
     m_elapsedMs = 0;
     m_elapsedCount = 0;
 
+    const QSize outputSize = m_sc->currentPixelSize();
     m_proj = m_r->clipSpaceCorrMatrix();
     m_proj.perspective(45.0f, outputSize.width() / (float) outputSize.height(), 0.01f, 100.0f);
     m_proj.translate(0, 0, -4);
diff --git a/tests/manual/rhi/multiwindow/multiwindow.cpp b/tests/manual/rhi/multiwindow/multiwindow.cpp
index 4c5d5c345a..4d5de16a58 100644
--- a/tests/manual/rhi/multiwindow/multiwindow.cpp
+++ b/tests/manual/rhi/multiwindow/multiwindow.cpp
@@ -400,7 +400,7 @@ void Window::init()
 {
     m_sc = r.r->newSwapChain();
     m_ds = r.r->newRenderBuffer(QRhiRenderBuffer::DepthStencil,
-                                QSize(), // no need to set the size yet
+                                QSize(),
                                 1,
                                 QRhiRenderBuffer::UsedWithSwapChainOnly);
     m_releasePool << m_ds;
@@ -427,13 +427,9 @@ void Window::releaseResources()
 
 void Window::resizeSwapChain()
 {
-    const QSize outputSize = m_sc->surfacePixelSize();
-
-    m_ds->setPixelSize(outputSize);
-    m_ds->build();
-
     m_hasSwapChain = m_sc->buildOrResize();
 
+    const QSize outputSize = m_sc->currentPixelSize();
     m_proj = r.r->clipSpaceCorrMatrix();
     m_proj.perspective(45.0f, outputSize.width() / (float) outputSize.height(), 0.01f, 1000.0f);
     m_proj.translate(0, 0, -4);
diff --git a/tests/manual/rhi/multiwindow_threaded/multiwindow_threaded.cpp b/tests/manual/rhi/multiwindow_threaded/multiwindow_threaded.cpp
index 53185bddb2..37c6cd04c3 100644
--- a/tests/manual/rhi/multiwindow_threaded/multiwindow_threaded.cpp
+++ b/tests/manual/rhi/multiwindow_threaded/multiwindow_threaded.cpp
@@ -441,7 +441,7 @@ void Renderer::init()
 {
     m_sc = r->newSwapChain();
     m_ds = r->newRenderBuffer(QRhiRenderBuffer::DepthStencil,
-                              QSize(), // no need to set the size yet
+                              QSize(),
                               1,
                               QRhiRenderBuffer::UsedWithSwapChainOnly);
     m_releasePool << m_ds;
@@ -543,11 +543,9 @@ void Renderer::render(bool newlyExposed, bool wakeBeforePresent)
 
     auto buildOrResizeSwapChain = [this] {
         qDebug() << "renderer" << this << "build or resize swapchain for window" << window;
-        const QSize outputSize = m_sc->surfacePixelSize();
-        qDebug() << "  size is" << outputSize;
-        m_ds->setPixelSize(outputSize);
-        m_ds->build();
         m_hasSwapChain = m_sc->buildOrResize();
+        const QSize outputSize = m_sc->currentPixelSize();
+        qDebug() << "  size is" << outputSize;
         m_proj = r->clipSpaceCorrMatrix();
         m_proj.perspective(45.0f, outputSize.width() / (float) outputSize.height(), 0.01f, 100.0f);
         m_proj.translate(0, 0, -4);
diff --git a/tests/manual/rhi/shared/examplefw.h b/tests/manual/rhi/shared/examplefw.h
index cd62530c82..d28bbea0a8 100644
--- a/tests/manual/rhi/shared/examplefw.h
+++ b/tests/manual/rhi/shared/examplefw.h
@@ -307,7 +307,7 @@ void Window::init()
     m_sc = m_r->newSwapChain();
     // allow depth-stencil, although we do not actually enable depth test/write for the triangle
     m_ds = m_r->newRenderBuffer(QRhiRenderBuffer::DepthStencil,
-                                QSize(), // no need to set the size yet
+                                QSize(), // no need to set the size here, due to UsedWithSwapChainOnly
                                 sampleCount,
                                 QRhiRenderBuffer::UsedWithSwapChainOnly);
     m_sc->setWindow(this);
@@ -344,16 +344,12 @@ void Window::releaseResources()
 
 void Window::resizeSwapChain()
 {
-    const QSize outputSize = m_sc->surfacePixelSize();
-
-    m_ds->setPixelSize(outputSize);
-    m_ds->build(); // == m_ds->release(); m_ds->build();
-
-    m_hasSwapChain = m_sc->buildOrResize();
+    m_hasSwapChain = m_sc->buildOrResize(); // also handles m_ds
 
     m_frameCount = 0;
     m_timer.restart();
 
+    const QSize outputSize = m_sc->currentPixelSize();
     m_proj = m_r->clipSpaceCorrMatrix();
     m_proj.perspective(45.0f, outputSize.width() / (float) outputSize.height(), 0.01f, 1000.0f);
     m_proj.translate(0, 0, -4);