rhi: Do not rely on unspecified relation between readbacks and FramesInFlight

Add a new queriable resource limit value MaxAsyncReadbackFrames. Change
the autotest to rely on this instead of relying on the unspecified,
works-by-accident relation between readbacks and FramesInFlight. This
way even if the behavior diverges in some backend in the future, clients
(well written ones, that is), will continue to function correctly.

Also clarify the docs for FramesInFlight, and change d3d and gl to return
the correct value (which is 1 from QRhi perspective; the expanded docs now
explain a bit what this really means and what it does not).

Change-Id: I0486715570a9e6fc5d3dc431694d1712875dfe01
Reviewed-by: Andy Nichols <andy.nichols@qt.io>

src/gui/rhi/qrhi.cpp

@@ -627,18 +627,27 @@ Q_LOGGING_CATEGORY(QRHI_LOG_INFO, "qt.rhi.general")
     is what some OpenGL ES implementations provide.
 
     \value FramesInFlight The number of frames the backend may keep "in
-    flight". The value has no relevance, and is unspecified, with backends like
-    OpenGL and Direct3D 11. With backends like Vulkan or Metal, it is the
-    responsibility of QRhi to block whenever starting a new frame and finding
-    the CPU is already \c{N - 1} frames ahead of the GPU (because the command
-    buffer submitted in frame no. \c{current} - \c{N} has not yet completed).
-    The value N is what is returned from here, and is typically 2. This can be
-    relevant to applications that integrate rendering done directly with the
-    graphics API, as such rendering code may want to perform double (if the
-    value is 2) buffering for resources, such as, buffers, similarly to the
-    QRhi backends themselves. The current frame slot index (a value running 0,
-    1, .., N-1, then wrapping around) is retrievable from
-    QRhi::currentFrameSlot().
+    flight": with backends like Vulkan or Metal, it is the responsibility of
+    QRhi to block whenever starting a new frame and finding the CPU is already
+    \c{N - 1} frames ahead of the GPU (because the command buffer submitted in
+    frame no. \c{current} - \c{N} has not yet completed). The value N is what
+    is returned from here, and is typically 2. This can be relevant to
+    applications that integrate rendering done directly with the graphics API,
+    as such rendering code may want to perform double (if the value is 2)
+    buffering for resources, such as, buffers, similarly to the QRhi backends
+    themselves. The current frame slot index (a value running 0, 1, .., N-1,
+    then wrapping around) is retrievable from QRhi::currentFrameSlot(). The
+    value is 1 for backends where the graphics API offers no such low level
+    control over the command submission process. Note that pipelining may still
+    happen even when this value is 1 (some backends, such as D3D11, are
+    designed to attempt to enable this, for instance, by using an update
+    strategy for uniform buffers that does not stall the pipeline), but that is
+    then not controlled by QRhi and so not reflected here in the API.
+
+    \value MaxAsyncReadbackFrames The number of \l{QRhi::endFrame()}{submitted}
+    frames (including the one that contains the readback) after which an
+    asynchronous texture or buffer readback is guaranteed to complete upon
+    \l{QRhi::beginFrame()}{starting a new frame}.
  */
 
 /*!
@@ -4339,7 +4348,15 @@ void QRhiResourceUpdateBatch::uploadStaticBuffer(QRhiBuffer *buf, const void *da
     is supported only when the QRhi::ReadBackNonUniformBuffer feature is
     reported as supported.
 
-    \a readBackTexture(), QRhi::isFeatureSupported()
+   \note The asynchronous readback is guaranteed to have completed when one of
+   the following conditions is met: \l{QRhi::finish()}{finish()} has been
+   called; or, at least \c N frames have been \l{QRhi::endFrame()}{submitted},
+   including the frame that issued the readback operation, and the
+   \l{QRhi::beginFrame()}{recording of a new frame} has been started, where \c
+   N is the \l{QRhi::resourceLimit()}{resource limit value} returned for
+   QRhi::MaxAsyncReadbackFrames.
+
+   \sa readBackTexture(), QRhi::isFeatureSupported(), QRhi::resourceLimit()
  */
 void QRhiResourceUpdateBatch::readBackBuffer(QRhiBuffer *buf, int offset, int size, QRhiBufferReadbackResult *result)
 {
@@ -4430,6 +4447,16 @@ void QRhiResourceUpdateBatch::copyTexture(QRhiTexture *dst, QRhiTexture *src, co
    happens with a byte ordered format. A \l{QRhiTexture::RGBA8}{RGBA8} texture
    maps therefore to byte ordered QImage formats, such as,
    QImage::Format_RGBA8888.
+
+   \note The asynchronous readback is guaranteed to have completed when one of
+   the following conditions is met: \l{QRhi::finish()}{finish()} has been
+   called; or, at least \c N frames have been \l{QRhi::endFrame()}{submitted},
+   including the frame that issued the readback operation, and the
+   \l{QRhi::beginFrame()}{recording of a new frame} has been started, where \c
+   N is the \l{QRhi::resourceLimit()}{resource limit value} returned for
+   QRhi::MaxAsyncReadbackFrames.
+
+   \sa readBackBuffer(), QRhi::resourceLimit()
  */
 void QRhiResourceUpdateBatch::readBackTexture(const QRhiReadbackDescription &rb, QRhiReadbackResult *result)
 {

src/gui/rhi/qrhi_p.h

@@ -1448,7 +1448,8 @@ public:
         TextureSizeMin = 1,
         TextureSizeMax,
         MaxColorAttachments,
-        FramesInFlight
+        FramesInFlight,
+        MaxAsyncReadbackFrames
     };
 
     ~QRhi();

src/gui/rhi/qrhid3d11.cpp

@@ -482,7 +482,13 @@ int QRhiD3D11::resourceLimit(QRhi::ResourceLimit limit) const
     case QRhi::MaxColorAttachments:
         return 8;
     case QRhi::FramesInFlight:
-        return 2; // dummy
+        // From our perspective. What D3D does internally is another question
+        // (there could be pipelining, helped f.ex. by our MAP_DISCARD based
+        // uniform buffer update strategy), but that's out of our hands and
+        // does not concern us here.
+        return 1;
+    case QRhi::MaxAsyncReadbackFrames:
+        return 1;
     default:
         Q_UNREACHABLE();
         return 0;

src/gui/rhi/qrhigles2.cpp

@@ -787,7 +787,11 @@ int QRhiGles2::resourceLimit(QRhi::ResourceLimit limit) const
     case QRhi::MaxColorAttachments:
         return caps.maxDrawBuffers;
     case QRhi::FramesInFlight:
-        return 2; // dummy
+        // From our perspective. What the GL impl does internally is another
+        // question, but that's out of our hands and does not concern us here.
+        return 1;
+    case QRhi::MaxAsyncReadbackFrames:
+        return 1;
     default:
         Q_UNREACHABLE();
         return 0;

src/gui/rhi/qrhimetal.mm

@@ -581,6 +581,8 @@ int QRhiMetal::resourceLimit(QRhi::ResourceLimit limit) const
         return 8;
     case QRhi::FramesInFlight:
         return QMTL_FRAMES_IN_FLIGHT;
+    case QRhi::MaxAsyncReadbackFrames:
+        return QMTL_FRAMES_IN_FLIGHT;
     default:
         Q_UNREACHABLE();
         return 0;

src/gui/rhi/qrhinull.cpp

@@ -146,7 +146,9 @@ int QRhiNull::resourceLimit(QRhi::ResourceLimit limit) const
     case QRhi::MaxColorAttachments:
         return 8;
     case QRhi::FramesInFlight:
-        return 2; // dummy
+        return 1;
+    case QRhi::MaxAsyncReadbackFrames:
+        return 1;
     default:
         Q_UNREACHABLE();
         return 0;

src/gui/rhi/qrhivulkan.cpp

@@ -4009,6 +4009,8 @@ int QRhiVulkan::resourceLimit(QRhi::ResourceLimit limit) const
         return int(physDevProperties.limits.maxColorAttachments);
     case QRhi::FramesInFlight:
         return QVK_FRAMES_IN_FLIGHT;
+    case QRhi::MaxAsyncReadbackFrames:
+        return QVK_FRAMES_IN_FLIGHT;
     default:
         Q_UNREACHABLE();
         return 0;

tests/auto/gui/rhi/qrhi/tst_qrhi.cpp

@@ -1672,9 +1672,9 @@ void tst_QRhi::renderToWindowSimple()
 
     QVERIFY(pipeline->build());
 
-    const int framesInFlight = rhi->resourceLimit(QRhi::FramesInFlight);
-    QVERIFY(framesInFlight >= 1);
-    const int FRAME_COUNT = framesInFlight + 1;
+    const int asyncReadbackFrames = rhi->resourceLimit(QRhi::MaxAsyncReadbackFrames);
+    // one frame issues the readback, then we do MaxAsyncReadbackFrames more to ensure the readback completes
+    const int FRAME_COUNT = asyncReadbackFrames + 1;
     bool readCompleted = false;
     QRhiReadbackResult readResult;
     QImage result;
@@ -1721,8 +1721,8 @@ void tst_QRhi::renderToWindowSimple()
     }
 
     // The readback is asynchronous here. However it is guaranteed that it
-    // finished at latest after rendering QRhi::FramesInFlight frames after the
-    // one that enqueues the readback.
+    // finished at latest after rendering QRhi::MaxAsyncReadbackFrames frames
+    // after the one that enqueues the readback.
     QVERIFY(readCompleted);
     QVERIFY(readbackWidth > 0);