From bb1d9bab36d779e595a924e3218d4066d84fca38 Mon Sep 17 00:00:00 2001 From: Laszlo Agocs Date: Wed, 15 Nov 2023 15:21:20 +0100 Subject: [PATCH] rhi: Make sample count selection logic be closer to Qt 5 Pick-to: 6.6 6.5 Fixes: QTBUG-119148 Change-Id: Ia119ab3ced9da08853c608aa256bde08a6fd8d4e Reviewed-by: Andy Nichols Reviewed-by: Qt CI Bot --- src/gui/rhi/qrhi.cpp | 35 +++++++++++++++ src/gui/rhi/qrhi_p.h | 2 + src/gui/rhi/qrhid3d11.cpp | 18 +++----- src/gui/rhi/qrhid3d11_p.h | 2 +- src/gui/rhi/qrhid3d12.cpp | 24 ++++------- src/gui/rhi/qrhid3d12_p.h | 2 +- src/gui/rhi/qrhigles2.cpp | 11 ----- src/gui/rhi/qrhigles2_p.h | 1 - src/gui/rhi/qrhimetal.mm | 11 ----- src/gui/rhi/qrhimetal_p.h | 1 - src/gui/rhi/qrhivulkan.cpp | 20 +++------ src/gui/rhi/qrhivulkan_p.h | 2 +- tests/auto/gui/rhi/qrhi/tst_qrhi.cpp | 64 +++++++++++++++++++++++++++- 13 files changed, 124 insertions(+), 69 deletions(-) diff --git a/src/gui/rhi/qrhi.cpp b/src/gui/rhi/qrhi.cpp index 67b8d2d134..0be717500d 100644 --- a/src/gui/rhi/qrhi.cpp +++ b/src/gui/rhi/qrhi.cpp @@ -8161,6 +8161,41 @@ bool QRhiImplementation::sanityCheckShaderResourceBindings(QRhiShaderResourceBin return true; } +int QRhiImplementation::effectiveSampleCount(int sampleCount) const +{ + // Stay compatible with QSurfaceFormat and friends where samples == 0 means the same as 1. + const int s = qBound(1, sampleCount, 64); + const QList supported = supportedSampleCounts(); + int result = 1; + + // Stay compatible with Qt 5 in that requesting an unsupported sample count + // is not an error (although we still do a categorized debug print about + // this), and rather a supported value, preferably a close one, not just 1, + // is used instead. This is actually deviating from Qt 5 as that performs a + // clamping only and does not handle cases such as when sample count 2 is + // not supported but 4 is. (OpenGL handles things like that gracefully, + // other APIs may not, so improve this by picking the next largest, or in + // absence of that, the largest value; this with the goal to not reduce + // quality by rather picking a larger-than-requested value than a smaller one) + + for (int i = 0, ie = supported.count(); i != ie; ++i) { + // assumes the 'supported' list is sorted + if (supported[i] >= s) { + result = supported[i]; + break; + } + } + + if (result != s) { + if (result == 1 && !supported.isEmpty()) + result = supported.last(); + qCDebug(QRHI_LOG_INFO, "Attempted to set unsupported sample count %d, using %d instead", + sampleCount, result); + } + + return result; +} + /*! \internal */ diff --git a/src/gui/rhi/qrhi_p.h b/src/gui/rhi/qrhi_p.h index 926247d386..b5429372a8 100644 --- a/src/gui/rhi/qrhi_p.h +++ b/src/gui/rhi/qrhi_p.h @@ -232,6 +232,8 @@ public: return a.d.binding < b.d.binding; } + int effectiveSampleCount(int sampleCount) const; + QRhi *q; static const int MAX_SHADER_CACHE_ENTRIES = 128; diff --git a/src/gui/rhi/qrhid3d11.cpp b/src/gui/rhi/qrhid3d11.cpp index d357a6b2db..54e94bf2f3 100644 --- a/src/gui/rhi/qrhid3d11.cpp +++ b/src/gui/rhi/qrhid3d11.cpp @@ -441,19 +441,13 @@ QList QRhiD3D11::supportedSampleCounts() const return { 1, 2, 4, 8 }; } -DXGI_SAMPLE_DESC QRhiD3D11::effectiveSampleCount(int sampleCount) const +DXGI_SAMPLE_DESC QRhiD3D11::effectiveSampleDesc(int sampleCount) const { DXGI_SAMPLE_DESC desc; desc.Count = 1; desc.Quality = 0; - // Stay compatible with QSurfaceFormat and friends where samples == 0 means the same as 1. - int s = qBound(1, sampleCount, 64); - - if (!supportedSampleCounts().contains(s)) { - qWarning("Attempted to set unsupported sample count %d", sampleCount); - return desc; - } + const int s = effectiveSampleCount(sampleCount); desc.Count = UINT(s); if (s > 1) @@ -3100,7 +3094,7 @@ bool QD3D11RenderBuffer::create() return false; QRHI_RES_RHI(QRhiD3D11); - sampleDesc = rhiD->effectiveSampleCount(m_sampleCount); + sampleDesc = rhiD->effectiveSampleDesc(m_sampleCount); D3D11_TEXTURE2D_DESC desc = {}; desc.Width = UINT(m_pixelSize.width()); @@ -3271,7 +3265,7 @@ bool QD3D11Texture::prepareCreate(QSize *adjustedSize) QRHI_RES_RHI(QRhiD3D11); dxgiFormat = toD3DTextureFormat(m_format, m_flags); mipLevelCount = uint(hasMipMaps ? rhiD->q->mipLevelsForSize(size) : 1); - sampleDesc = rhiD->effectiveSampleCount(m_sampleCount); + sampleDesc = rhiD->effectiveSampleDesc(m_sampleCount); if (sampleDesc.Count > 1) { if (isCube) { qWarning("Cubemap texture cannot be multisample"); @@ -4447,7 +4441,7 @@ bool QD3D11GraphicsPipeline::create() rastDesc.SlopeScaledDepthBias = m_slopeScaledDepthBias; rastDesc.DepthClipEnable = true; rastDesc.ScissorEnable = m_flags.testFlag(UsesScissor); - rastDesc.MultisampleEnable = rhiD->effectiveSampleCount(m_sampleCount).Count > 1; + rastDesc.MultisampleEnable = rhiD->effectiveSampleDesc(m_sampleCount).Count > 1; HRESULT hr = rhiD->dev->CreateRasterizerState(&rastDesc, &rastState); if (FAILED(hr)) { qWarning("Failed to create rasterizer state: %s", @@ -5088,7 +5082,7 @@ bool QD3D11SwapChain::createOrResize() swapChainFlags |= DXGI_SWAP_CHAIN_FLAG_ALLOW_TEARING; if (!swapChain) { - sampleDesc = rhiD->effectiveSampleCount(m_sampleCount); + sampleDesc = rhiD->effectiveSampleDesc(m_sampleCount); colorFormat = DEFAULT_FORMAT; srgbAdjustedColorFormat = m_flags.testFlag(sRGB) ? DEFAULT_SRGB_FORMAT : DEFAULT_FORMAT; diff --git a/src/gui/rhi/qrhid3d11_p.h b/src/gui/rhi/qrhid3d11_p.h index 9652458de8..7644748407 100644 --- a/src/gui/rhi/qrhid3d11_p.h +++ b/src/gui/rhi/qrhid3d11_p.h @@ -750,7 +750,7 @@ public: bool offsetOnlyChange); void resetShaderResources(); void executeCommandBuffer(QD3D11CommandBuffer *cbD); - DXGI_SAMPLE_DESC effectiveSampleCount(int sampleCount) const; + DXGI_SAMPLE_DESC effectiveSampleDesc(int sampleCount) const; void finishActiveReadbacks(); void reportLiveObjects(ID3D11Device *device); void clearShaderCache(); diff --git a/src/gui/rhi/qrhid3d12.cpp b/src/gui/rhi/qrhid3d12.cpp index a508a3c1fa..db4a87e9bd 100644 --- a/src/gui/rhi/qrhid3d12.cpp +++ b/src/gui/rhi/qrhid3d12.cpp @@ -3058,24 +3058,18 @@ void QRhiD3D12::waitGpu() } } -DXGI_SAMPLE_DESC QRhiD3D12::effectiveSampleCount(int sampleCount, DXGI_FORMAT format) const +DXGI_SAMPLE_DESC QRhiD3D12::effectiveSampleDesc(int sampleCount, DXGI_FORMAT format) const { DXGI_SAMPLE_DESC desc; desc.Count = 1; desc.Quality = 0; - // Stay compatible with QSurfaceFormat and friends where samples == 0 means the same as 1. - int s = qBound(1, sampleCount, 64); - - if (!supportedSampleCounts().contains(s)) { - qWarning("Attempted to set unsupported sample count %d", sampleCount); - return desc; - } + const int s = effectiveSampleCount(sampleCount); if (s > 1) { D3D12_FEATURE_DATA_MULTISAMPLE_QUALITY_LEVELS msaaInfo = {}; msaaInfo.Format = format; - msaaInfo.SampleCount = s; + msaaInfo.SampleCount = UINT(s); if (SUCCEEDED(dev->CheckFeatureSupport(D3D12_FEATURE_MULTISAMPLE_QUALITY_LEVELS, &msaaInfo, sizeof(msaaInfo)))) { if (msaaInfo.NumQualityLevels > 0) { desc.Count = UINT(s); @@ -3946,7 +3940,7 @@ bool QD3D12RenderBuffer::create() case QRhiRenderBuffer::Color: { dxgiFormat = toD3DTextureFormat(backingFormat(), {}); - sampleDesc = rhiD->effectiveSampleCount(m_sampleCount, dxgiFormat); + sampleDesc = rhiD->effectiveSampleDesc(m_sampleCount, dxgiFormat); D3D12_RESOURCE_DESC resourceDesc = {}; resourceDesc.Dimension = D3D12_RESOURCE_DIMENSION_TEXTURE2D; resourceDesc.Width = UINT64(m_pixelSize.width()); @@ -3987,7 +3981,7 @@ bool QD3D12RenderBuffer::create() case QRhiRenderBuffer::DepthStencil: { dxgiFormat = DS_FORMAT; - sampleDesc = rhiD->effectiveSampleCount(m_sampleCount, dxgiFormat); + sampleDesc = rhiD->effectiveSampleDesc(m_sampleCount, dxgiFormat); D3D12_RESOURCE_DESC resourceDesc = {}; resourceDesc.Dimension = D3D12_RESOURCE_DIMENSION_TEXTURE2D; resourceDesc.Width = UINT64(m_pixelSize.width()); @@ -4143,7 +4137,7 @@ bool QD3D12Texture::prepareCreate(QSize *adjustedSize) QRHI_RES_RHI(QRhiD3D12); dxgiFormat = toD3DTextureFormat(m_format, m_flags); mipLevelCount = uint(hasMipMaps ? rhiD->q->mipLevelsForSize(size) : 1); - sampleDesc = rhiD->effectiveSampleCount(m_sampleCount, dxgiFormat); + sampleDesc = rhiD->effectiveSampleDesc(m_sampleCount, dxgiFormat); if (sampleDesc.Count > 1) { if (isCube) { qWarning("Cubemap texture cannot be multisample"); @@ -4297,7 +4291,7 @@ bool QD3D12Texture::create() bool needsOptimizedClearValueSpecified = false; UINT resourceFlags = 0; - if (m_flags.testFlag(RenderTarget)) { + if (m_flags.testFlag(RenderTarget) || sampleDesc.Count > 1) { if (isDepth) resourceFlags |= D3D12_RESOURCE_FLAG_ALLOW_DEPTH_STENCIL; else @@ -5575,7 +5569,7 @@ bool QD3D12GraphicsPipeline::create() } QD3D12RenderPassDescriptor *rpD = QRHI_RES(QD3D12RenderPassDescriptor, m_renderPassDesc); - const DXGI_SAMPLE_DESC sampleDesc = rhiD->effectiveSampleCount(m_sampleCount, DXGI_FORMAT(rpD->colorFormat[0])); + const DXGI_SAMPLE_DESC sampleDesc = rhiD->effectiveSampleDesc(m_sampleCount, DXGI_FORMAT(rpD->colorFormat[0])); struct { QD3D12PipelineStateSubObject rootSig; @@ -6204,7 +6198,7 @@ void QD3D12SwapChain::chooseFormats() "(or Use HDR is Off in the Display Settings), ignoring HDR format request"); } } - sampleDesc = rhiD->effectiveSampleCount(m_sampleCount, colorFormat); + sampleDesc = rhiD->effectiveSampleDesc(m_sampleCount, colorFormat); } bool QD3D12SwapChain::createOrResize() diff --git a/src/gui/rhi/qrhid3d12_p.h b/src/gui/rhi/qrhid3d12_p.h index 15567de542..c6d4123c09 100644 --- a/src/gui/rhi/qrhid3d12_p.h +++ b/src/gui/rhi/qrhid3d12_p.h @@ -1179,7 +1179,7 @@ public: void setPipelineCacheData(const QByteArray &data) override; void waitGpu(); - DXGI_SAMPLE_DESC effectiveSampleCount(int sampleCount, DXGI_FORMAT format) const; + DXGI_SAMPLE_DESC effectiveSampleDesc(int sampleCount, DXGI_FORMAT format) const; bool ensureDirectCompositionDevice(); bool startCommandListForCurrentFrameSlot(ID3D12GraphicsCommandList1 **cmdList); void enqueueResourceUpdates(QD3D12CommandBuffer *cbD, QRhiResourceUpdateBatch *resourceUpdates); diff --git a/src/gui/rhi/qrhigles2.cpp b/src/gui/rhi/qrhigles2.cpp index fc57cbd072..ec1a17459e 100644 --- a/src/gui/rhi/qrhigles2.cpp +++ b/src/gui/rhi/qrhigles2.cpp @@ -1126,17 +1126,6 @@ QList QRhiGles2::supportedSampleCounts() const return supportedSampleCountList; } -int QRhiGles2::effectiveSampleCount(int sampleCount) const -{ - // Stay compatible with QSurfaceFormat and friends where samples == 0 means the same as 1. - const int s = qBound(1, sampleCount, 64); - if (!supportedSampleCounts().contains(s)) { - qWarning("Attempted to set unsupported sample count %d", sampleCount); - return 1; - } - return s; -} - QRhiSwapChain *QRhiGles2::createSwapChain() { return new QGles2SwapChain(this); diff --git a/src/gui/rhi/qrhigles2_p.h b/src/gui/rhi/qrhigles2_p.h index 7cbad733a0..bd33a4fffb 100644 --- a/src/gui/rhi/qrhigles2_p.h +++ b/src/gui/rhi/qrhigles2_p.h @@ -890,7 +890,6 @@ public: QGles2RenderTargetData *enqueueBindFramebuffer(QRhiRenderTarget *rt, QGles2CommandBuffer *cbD, bool *wantsColorClear = nullptr, bool *wantsDsClear = nullptr); void enqueueBarriersForPass(QGles2CommandBuffer *cbD); - int effectiveSampleCount(int sampleCount) const; QByteArray shaderSource(const QRhiShaderStage &shaderStage, QShaderVersion *shaderVersion); bool compileShader(GLuint program, const QRhiShaderStage &shaderStage, QShaderVersion *shaderVersion); bool linkProgram(GLuint program); diff --git a/src/gui/rhi/qrhimetal.mm b/src/gui/rhi/qrhimetal.mm index 48690df5d9..657fc801a0 100644 --- a/src/gui/rhi/qrhimetal.mm +++ b/src/gui/rhi/qrhimetal.mm @@ -683,17 +683,6 @@ QVector QRhiMetal::supportedSampleCounts() const return caps.supportedSampleCounts; } -int QRhiMetal::effectiveSampleCount(int sampleCount) const -{ - // Stay compatible with QSurfaceFormat and friends where samples == 0 means the same as 1. - const int s = qBound(1, sampleCount, 64); - if (!supportedSampleCounts().contains(s)) { - qWarning("Attempted to set unsupported sample count %d", sampleCount); - return 1; - } - return s; -} - QRhiSwapChain *QRhiMetal::createSwapChain() { return new QMetalSwapChain(this); diff --git a/src/gui/rhi/qrhimetal_p.h b/src/gui/rhi/qrhimetal_p.h index 15f869c3d8..f539148b2c 100644 --- a/src/gui/rhi/qrhimetal_p.h +++ b/src/gui/rhi/qrhimetal_p.h @@ -454,7 +454,6 @@ public: const QRhiCommandBuffer::DynamicOffset *dynamicOffsets, bool offsetOnlyChange, const QShader::NativeResourceBindingMap *nativeResourceBindingMaps[SUPPORTED_STAGES]); - int effectiveSampleCount(int sampleCount) const; struct TessDrawArgs { QMetalCommandBuffer *cbD; enum { diff --git a/src/gui/rhi/qrhivulkan.cpp b/src/gui/rhi/qrhivulkan.cpp index f01d2de395..242263a080 100644 --- a/src/gui/rhi/qrhivulkan.cpp +++ b/src/gui/rhi/qrhivulkan.cpp @@ -3989,18 +3989,12 @@ QList QRhiVulkan::supportedSampleCounts() const return result; } -VkSampleCountFlagBits QRhiVulkan::effectiveSampleCount(int sampleCount) +VkSampleCountFlagBits QRhiVulkan::effectiveSampleCountBits(int sampleCount) { - // Stay compatible with QSurfaceFormat and friends where samples == 0 means the same as 1. - sampleCount = qBound(1, sampleCount, 64); - - if (!supportedSampleCounts().contains(sampleCount)) { - qWarning("Attempted to set unsupported sample count %d", sampleCount); - return VK_SAMPLE_COUNT_1_BIT; - } + const int s = effectiveSampleCount(sampleCount); for (const auto &qvk_sampleCount : qvk_sampleCounts) { - if (qvk_sampleCount.count == sampleCount) + if (qvk_sampleCount.count == s) return qvk_sampleCount.mask; } @@ -6051,7 +6045,7 @@ bool QVkRenderBuffer::create() return false; QRHI_RES_RHI(QRhiVulkan); - samples = rhiD->effectiveSampleCount(m_sampleCount); + samples = rhiD->effectiveSampleCountBits(m_sampleCount); switch (m_type) { case QRhiRenderBuffer::Color: @@ -6192,7 +6186,7 @@ bool QVkTexture::prepareCreate(QSize *adjustedSize) qWarning("Too many mip levels (%d, max is %d), truncating mip chain", mipLevelCount, maxLevels); mipLevelCount = maxLevels; } - samples = rhiD->effectiveSampleCount(m_sampleCount); + samples = rhiD->effectiveSampleCountBits(m_sampleCount); if (samples > VK_SAMPLE_COUNT_1_BIT) { if (isCube) { qWarning("Cubemap texture cannot be multisample"); @@ -7307,7 +7301,7 @@ bool QVkGraphicsPipeline::create() VkPipelineMultisampleStateCreateInfo msInfo = {}; msInfo.sType = VK_STRUCTURE_TYPE_PIPELINE_MULTISAMPLE_STATE_CREATE_INFO; - msInfo.rasterizationSamples = rhiD->effectiveSampleCount(m_sampleCount); + msInfo.rasterizationSamples = rhiD->effectiveSampleCountBits(m_sampleCount); pipelineInfo.pMultisampleState = &msInfo; VkPipelineDepthStencilStateCreateInfo dsInfo = {}; @@ -7704,7 +7698,7 @@ bool QVkSwapChain::ensureSurface() } } - samples = rhiD->effectiveSampleCount(m_sampleCount); + samples = rhiD->effectiveSampleCountBits(m_sampleCount); quint32 presModeCount = 0; rhiD->vkGetPhysicalDeviceSurfacePresentModesKHR(rhiD->physDev, surface, &presModeCount, nullptr); diff --git a/src/gui/rhi/qrhivulkan_p.h b/src/gui/rhi/qrhivulkan_p.h index 7531c9e567..66e7882ad8 100644 --- a/src/gui/rhi/qrhivulkan_p.h +++ b/src/gui/rhi/qrhivulkan_p.h @@ -760,7 +760,7 @@ public: void releaseSwapChainResources(QRhiSwapChain *swapChain); VkFormat optimalDepthStencilFormat(); - VkSampleCountFlagBits effectiveSampleCount(int sampleCount); + VkSampleCountFlagBits effectiveSampleCountBits(int sampleCount); bool createDefaultRenderPass(QVkRenderPassDescriptor *rpD, bool hasDepthStencil, VkSampleCountFlagBits samples, diff --git a/tests/auto/gui/rhi/qrhi/tst_qrhi.cpp b/tests/auto/gui/rhi/qrhi/tst_qrhi.cpp index e98ed9e06e..1c3da1ea42 100644 --- a/tests/auto/gui/rhi/qrhi/tst_qrhi.cpp +++ b/tests/auto/gui/rhi/qrhi/tst_qrhi.cpp @@ -84,6 +84,8 @@ private slots: void renderPassDescriptorCompatibility(); void renderPassDescriptorClone_data(); void renderPassDescriptorClone(); + void textureWithSampleCount_data(); + void textureWithSampleCount(); void renderToTextureSimple_data(); void renderToTextureSimple(); @@ -315,8 +317,13 @@ void tst_QRhi::create() QVERIFY(resUpd); resUpd->release(); - QVERIFY(!rhi->supportedSampleCounts().isEmpty()); - QVERIFY(rhi->supportedSampleCounts().contains(1)); + const QVector supportedSampleCounts = rhi->supportedSampleCounts(); + QVERIFY(!supportedSampleCounts.isEmpty()); + QVERIFY(supportedSampleCounts.contains(1)); + for (int i = 1; i < supportedSampleCounts.count(); ++i) { + // Verify the list is sorted. Internally the backends rely on this. + QVERIFY(supportedSampleCounts[i] > supportedSampleCounts[i - 1]); + } QVERIFY(rhi->ubufAlignment() > 0); QCOMPARE(rhi->ubufAligned(123), aligned(123, rhi->ubufAlignment())); @@ -4753,6 +4760,59 @@ void tst_QRhi::pipelineCache() } } +void tst_QRhi::textureWithSampleCount_data() +{ + rhiTestData(); +} + +void tst_QRhi::textureWithSampleCount() +{ + QFETCH(QRhi::Implementation, impl); + QFETCH(QRhiInitParams *, initParams); + + QScopedPointer rhi(QRhi::create(impl, initParams, QRhi::Flags(), nullptr)); + if (!rhi) + QSKIP("QRhi could not be created, skipping testing renderpass descriptors"); + + if (!rhi->isFeatureSupported(QRhi::MultisampleTexture)) + QSKIP("No multisample texture support with this backend, skipping"); + + { + QScopedPointer tex(rhi->newTexture(QRhiTexture::RGBA8, QSize(512, 512), 1)); + QVERIFY(tex->create()); + } + + // Ensure 0 is accepted the same way as 1. + { + QScopedPointer tex(rhi->newTexture(QRhiTexture::RGBA8, QSize(512, 512), 0)); + QVERIFY(tex->create()); + } + + // Note that we intentionally do not pass in RenderTarget in flags. Where + // matters for create(), the backend is expected to act as if it was + // specified whenever samples > 1. (in practice it does not make sense to not + // have the flag for an msaa texture, but we only care about create() here) + + // Pick the commonly supported sample count of 4. + { + QScopedPointer tex(rhi->newTexture(QRhiTexture::RGBA8, QSize(512, 512), 4)); + QVERIFY(tex->create()); + } + + // Now a bogus value that is typically in-between the supported values. + { + QScopedPointer tex(rhi->newTexture(QRhiTexture::RGBA8, QSize(512, 512), 3)); + QVERIFY(tex->create()); + } + + // Now a bogus value that is out of range. + { + QScopedPointer tex(rhi->newTexture(QRhiTexture::RGBA8, QSize(512, 512), 123)); + QVERIFY(tex->create()); + } +} + + void tst_QRhi::textureImportOpenGL() { #ifdef TST_GL