[dawn][opengl] Support combined samplers for binding_array The OpenGL backend needs to transform all of the uses of textures into combined samplers and textures. binding_array needs to be supported there by using multiple texture units per combined sampler if needed. Changes are: - In PipelineGL loop over the binding_array if needed when assigning texture units. - SizedBindingArrayTests are enabled on OpenGL. - Creation of the placeholder sampler in PipelineGL is changed to use the reentrant Device::CreateSampler which has a known default descriptor. - One of the binding array tests for oversized array in the BGL had the WGSL binding_array with the same size as the BGL which was not what was intended to be tested. - Adds a suppression for a test that's not passing on GL yet. - Adds a targeted test for the logic added in PipelineGL. Bug: 411573957 Change-Id: I8144c68466e8073144db9b222df2a9845b1d4f51 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/252834 Reviewed-by: Geoff Lang <geofflang@chromium.org> Commit-Queue: Corentin Wallez <cwallez@chromium.org> Reviewed-by: Stephen White <senorblanco@chromium.org>
diff --git a/src/dawn/native/opengl/PipelineGL.cpp b/src/dawn/native/opengl/PipelineGL.cpp index c3eaa3f..98977db 100644 --- a/src/dawn/native/opengl/PipelineGL.cpp +++ b/src/dawn/native/opengl/PipelineGL.cpp
@@ -32,6 +32,7 @@ #include <sstream> #include <string> +#include "dawn/common/Range.h" #include "dawn/native/BindGroupLayoutInternal.h" #include "dawn/native/Device.h" #include "dawn/native/Pipeline.h" @@ -113,47 +114,56 @@ mUnitsForSamplers.resize(layout->GetNumSamplers()); mUnitsForTextures.resize(layout->GetNumSampledTextures()); + // Assign combined texture/samplers to GL texture units. GLuint textureUnit = 0; for (const auto& combined : combinedSamplers) { - std::string name = combined.GetName(); - GLint location = DAWN_GL_TRY(gl, GetUniformLocation(mProgram, name.c_str())); - - if (location == -1) { - continue; - } - - DAWN_GL_TRY(gl, Uniform1i(location, textureUnit)); + // All the texture/samplers of a binding_array are set in a single glUniform1iv, gather them + // all in this vector. + absl::InlinedVector<GLint, 1> uniformsToSet; const BindGroupLayoutInternalBase* textureBgl = layout->GetBindGroupLayout(combined.textureLocation.group); - BindingIndex textureBindingIndex = + BindingIndex textureArrayStart = textureBgl->GetBindingIndex(combined.textureLocation.binding); - GLuint textureGLIndex = indices[combined.textureLocation.group][textureBindingIndex]; - mUnitsForTextures[textureGLIndex].push_back(textureUnit); + for (auto textureArrayElement : Range(combined.textureLocation.arraySize)) { + GLuint textureGLIndex = + indices[combined.textureLocation.group][textureArrayStart + textureArrayElement]; + mUnitsForTextures[textureGLIndex].push_back(textureUnit); - if (!combined.samplerLocation) { - mPlaceholderSamplerUnits.push_back(textureUnit); - } else { - const BindGroupLayoutInternalBase* samplerBgl = - layout->GetBindGroupLayout(combined.samplerLocation->group); - BindingIndex samplerBindingIndex = - samplerBgl->GetBindingIndex(combined.samplerLocation->binding); + // Record that the placeholder sampler must be set for this texture unit if no sampler + // is used in the shader. + if (!combined.samplerLocation) { + mPlaceholderSamplerUnits.push_back(textureUnit); + } else { + // Record that the sampler used in the shader must be set for this texture unit. + const BindGroupLayoutInternalBase* samplerBgl = + layout->GetBindGroupLayout(combined.samplerLocation->group); + BindingIndex samplerBindingIndex = + samplerBgl->GetBindingIndex(combined.samplerLocation->binding); - GLuint samplerGLIndex = indices[combined.samplerLocation->group][samplerBindingIndex]; - mUnitsForSamplers[samplerGLIndex].push_back(textureUnit); + GLuint samplerGLIndex = + indices[combined.samplerLocation->group][samplerBindingIndex]; + mUnitsForSamplers[samplerGLIndex].push_back(textureUnit); + } + + uniformsToSet.push_back(textureUnit); + textureUnit++; } - textureUnit++; + std::string name = combined.GetName(); + GLint location = DAWN_GL_TRY(gl, GetUniformLocation(mProgram, name.c_str())); + // Non-arrayed GLSL variables cannot be set with glUniform1iv + if (uniformsToSet.size() == 1) { + DAWN_GL_TRY(gl, Uniform1i(location, uniformsToSet[0])); + } else { + DAWN_GL_TRY(gl, Uniform1iv(location, uniformsToSet.size(), uniformsToSet.data())); + } } if (!mPlaceholderSamplerUnits.empty()) { - SamplerDescriptor desc = {}; - DAWN_ASSERT(desc.minFilter == wgpu::FilterMode::Nearest); - DAWN_ASSERT(desc.magFilter == wgpu::FilterMode::Nearest); - DAWN_ASSERT(desc.mipmapFilter == wgpu::MipmapFilterMode::Nearest); Ref<SamplerBase> sampler; - DAWN_TRY_ASSIGN(sampler, layout->GetDevice()->GetOrCreateSampler(&desc)); + DAWN_TRY_ASSIGN(sampler, layout->GetDevice()->CreateSampler()); mPlaceholderSampler = ToBackend(std::move(sampler)); }
diff --git a/src/dawn/native/opengl/ShaderModuleGL.cpp b/src/dawn/native/opengl/ShaderModuleGL.cpp index 39a7c30..ccf8363 100644 --- a/src/dawn/native/opengl/ShaderModuleGL.cpp +++ b/src/dawn/native/opengl/ShaderModuleGL.cpp
@@ -124,13 +124,15 @@ // Helper to avoid duplicated logic for when a CombinedSampler is determined. auto AddCombinedSampler = [&](tint::BindingPoint textureWGSL, tint::BindingPoint textureRemapped, - std::optional<tint::BindingPoint> samplerWGSL, bool isPlane1 = false) { + std::optional<tint::BindingPoint> samplerWGSL, BindingIndex textureArraySize, + bool isPlane1 = false) { // Dawn needs pre-remapping WGSL bind points. CombinedSampler combinedSampler = {{ .samplerLocation = std::nullopt, .textureLocation = {{ .group = BindGroupIndex(textureWGSL.group), .binding = BindingNumber(textureWGSL.binding), + .arraySize = textureArraySize, }}, }}; if (samplerWGSL.has_value()) { @@ -168,7 +170,10 @@ if (!externalTextureExpansionMap.contains(ToTint(use.texture))) { tint::BindingPoint textureWGSL = ToTint(use.texture); tint::BindingPoint textureRemapped = {0, bindings.texture.at(textureWGSL).binding}; - AddCombinedSampler(textureWGSL, textureRemapped, sampler); + BindingIndex arraySizeInShader = metadata.bindings.at(BindGroupIndex(textureWGSL.group)) + .at(BindingNumber(textureWGSL.binding)) + .arraySize; + AddCombinedSampler(textureWGSL, textureRemapped, sampler, arraySizeInShader); continue; } @@ -177,13 +182,13 @@ tint::BindingPoint plane0WGSL = ToTint(use.texture); tint::BindingPoint plane0Remapped = { 0, bindings.external_texture.at(plane0WGSL).plane0.binding}; - AddCombinedSampler(plane0WGSL, plane0Remapped, sampler); + AddCombinedSampler(plane0WGSL, plane0Remapped, sampler, BindingIndex(1)); // Plane 1 needs its pre-remapping bind point queried from the expansion map. tint::BindingPoint plane1WGSL = externalTextureExpansionMap.at(plane0WGSL); tint::BindingPoint plane1Remapped = { 0, bindings.external_texture.at(plane0WGSL).plane1.binding}; - AddCombinedSampler(plane1WGSL, plane1Remapped, sampler, true); + AddCombinedSampler(plane1WGSL, plane1Remapped, sampler, BindingIndex(1), true); } } @@ -297,8 +302,8 @@ return o.str(); } -bool operator<(const BindingLocation& a, const BindingLocation& b) { - return std::tie(a.group, a.binding) < std::tie(b.group, b.binding); +bool operator<(const CombinedSamplerElement& a, const CombinedSamplerElement& b) { + return std::tie(a.group, a.binding, a.arraySize) < std::tie(b.group, b.binding, b.arraySize); } bool operator<(const CombinedSampler& a, const CombinedSampler& b) {
diff --git a/src/dawn/native/opengl/ShaderModuleGL.h b/src/dawn/native/opengl/ShaderModuleGL.h index bf6a502..54ae831 100644 --- a/src/dawn/native/opengl/ShaderModuleGL.h +++ b/src/dawn/native/opengl/ShaderModuleGL.h
@@ -55,19 +55,22 @@ std::string GetBindingName(BindGroupIndex group, BindingNumber bindingNumber); -#define BINDING_LOCATION_MEMBERS(X) \ - X(BindGroupIndex, group) \ - X(BindingNumber, binding) -DAWN_SERIALIZABLE(struct, BindingLocation, BINDING_LOCATION_MEMBERS){}; -#undef BINDING_LOCATION_MEMBERS +#define COMBINED_SAMPLER_ELEMENT_MEMBERS(X) \ + X(BindGroupIndex, group) \ + X(BindingNumber, binding) \ + /* Return the array size of the element in the WGSL / GLSL as OpenGL requires that a */ \ + /* non-arrayed (arraySize = 1) binding uses glUniform1i and not glUniform1iv. */ \ + X(BindingIndex, arraySize, 1) +DAWN_SERIALIZABLE(struct, CombinedSamplerElement, COMBINED_SAMPLER_ELEMENT_MEMBERS){}; +#undef COMBINED_SAMPLER_ELEMENT_MEMBERS -bool operator<(const BindingLocation& a, const BindingLocation& b); +bool operator<(const CombinedSamplerElement& a, const CombinedSamplerElement& b); #define COMBINED_SAMPLER_MEMBERS(X) \ /* OpenGL requires a sampler with texelFetch. If this is nullopt, the developer did not */ \ /* provide one and Dawn should bind a placeholder non-filtering sampler. */ \ - X(std::optional<BindingLocation>, samplerLocation) \ - X(BindingLocation, textureLocation) + X(std::optional<CombinedSamplerElement>, samplerLocation) \ + X(CombinedSamplerElement, textureLocation) DAWN_SERIALIZABLE(struct, CombinedSampler, COMBINED_SAMPLER_MEMBERS) { std::string GetName() const;
diff --git a/src/dawn/tests/end2end/BindingArrayTests.cpp b/src/dawn/tests/end2end/BindingArrayTests.cpp index 1fcce1a..8abf84ed 100644 --- a/src/dawn/tests/end2end/BindingArrayTests.cpp +++ b/src/dawn/tests/end2end/BindingArrayTests.cpp
@@ -43,6 +43,7 @@ DAWN_SUPPRESS_TEST_IF(IsD3D12() && IsWARP()); } + // A 1x1 texture with a single value to check the correct binding is used. wgpu::Texture MakeTestR8Texture(uint8_t value) { wgpu::TextureDescriptor desc; desc.size = {1, 1}; @@ -57,6 +58,23 @@ return texture; } + + // Test textures that can be used that samplers correctly clamp or wrap. + wgpu::Texture MakeTest2x1R8Texture(uint8_t left, uint8_t right) { + wgpu::TextureDescriptor desc; + desc.size = {2, 1}; + desc.usage = wgpu::TextureUsage::CopyDst | wgpu::TextureUsage::TextureBinding; + desc.format = wgpu::TextureFormat::R8Unorm; + wgpu::Texture texture = device.CreateTexture(&desc); + + wgpu::TexelCopyTextureInfo srcInfo = utils::CreateTexelCopyTextureInfo(texture); + wgpu::TexelCopyBufferLayout dstInfo = {}; + wgpu::Extent3D copySize = {2, 1, 1}; + std::array<uint8_t, 2> data = {left, right}; + queue.WriteTexture(&srcInfo, &data, sizeof(data), &dstInfo, ©Size); + + return texture; + } }; // Test accessing a binding_array with constant values. @@ -209,7 +227,7 @@ @group(0) @binding(0) var t0 : texture_2d<f32>; // Note that the layout will contain one extra entry using @binding(3) - @group(0) @binding(1) var ts : binding_array<texture_2d<f32>, 3>; + @group(0) @binding(1) var ts : binding_array<texture_2d<f32>, 2>; @group(0) @binding(4) var t3 : texture_2d<f32>; @fragment fn fs() -> @location(0) vec4f { let r = textureLoad(t0, vec2(0, 0), 0)[0]; @@ -365,6 +383,11 @@ // Test passing sampled textures of binding_array as function arguments. TEST_P(SizedBindingArrayTests, BindingArraySampledTextureAsFunctionArgument) { + // TODO(https://issues.chromium.org/411573957) OgenGL requires that indices in arrays of sampler + // be constants but even after the DirectVariableAccess transform, the index is a function + // parameter (that's constant at the callsite) and not a constant. + DAWN_SUPPRESS_TEST_IF(IsOpenGL() || IsOpenGLES()); + // Make the test pipeline wgpu::ShaderModule module = utils::CreateShaderModule(device, R"( @vertex fn vs() -> @builtin(position) vec4f { @@ -476,10 +499,72 @@ EXPECT_PIXEL_RGBA8_EQ(utils::RGBA8(3, 2, 1, 0), rp.color, 0, 0); } +// Test that multiple texture and sampler combinations are handled correctly (in particular on GL). +TEST_P(SizedBindingArrayTests, TextureAndSamplerCombination) { + // Make the test pipeline + wgpu::ShaderModule module = utils::CreateShaderModule(device, R"( + @vertex fn vs() -> @builtin(position) vec4f { + return vec4f(0, 0, 0.5, 0.5); + } + + @group(0) @binding(0) var ts : binding_array<texture_2d<f32>, 2>; + @group(0) @binding(2) var samplerClamping : sampler; + @group(0) @binding(3) var samplerWrapping : sampler; + @fragment fn fs() -> @location(0) vec4f { + let r = textureSample(ts[0], samplerWrapping, vec2(1.25, 0.5))[0]; + let g = textureSample(ts[0], samplerClamping, vec2(1.25, 0.5))[0]; + let b = textureSample(ts[1], samplerWrapping, vec2(1.25, 0.5))[0]; + let a = textureSample(ts[1], samplerClamping, vec2(1.25, 0.5))[0]; + return vec4(r, g, b, a); + } + )"); + + utils::ComboRenderPipelineDescriptor pDesc; + pDesc.vertex.module = module; + pDesc.cFragment.module = module; + pDesc.cFragment.targetCount = 1; + pDesc.cTargets[0].format = wgpu::TextureFormat::RGBA8Unorm; + pDesc.primitive.topology = wgpu::PrimitiveTopology::PointList; + wgpu::RenderPipeline testPipeline = device.CreateRenderPipeline(&pDesc); + + // Create a bind group with textures of decreasing values + wgpu::SamplerDescriptor sDesc = {}; + sDesc.addressModeU = wgpu::AddressMode::ClampToEdge; + wgpu::Sampler samplerClamping = device.CreateSampler(&sDesc); + sDesc.addressModeU = wgpu::AddressMode::Repeat; + wgpu::Sampler samplerWrapping = device.CreateSampler(&sDesc); + + wgpu::BindGroup arrayGroup = + utils::MakeBindGroup(device, testPipeline.GetBindGroupLayout(0), + { + {0, MakeTest2x1R8Texture(3, 2).CreateView()}, + {1, MakeTest2x1R8Texture(1, 0).CreateView()}, + {2, samplerClamping}, + {3, samplerWrapping}, + }); + + // Run the test + auto rp = utils::CreateBasicRenderPass(device, 1, 1); + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&rp.renderPassInfo); + + pass.SetPipeline(testPipeline); + pass.SetBindGroup(0, arrayGroup); + pass.Draw(1); + pass.End(); + + wgpu::CommandBuffer commands = encoder.Finish(); + queue.Submit(1, &commands); + + EXPECT_PIXEL_RGBA8_EQ(utils::RGBA8(3, 2, 1, 0), rp.color, 0, 0); +} + DAWN_INSTANTIATE_TEST(SizedBindingArrayTests, D3D11Backend(), D3D12Backend(), MetalBackend(), + OpenGLBackend(), + OpenGLESBackend(), VulkanBackend()); } // anonymous namespace