OpenGL: fix multiplanar external textures.

There are two problems with multiplanar textures in the OpenGL backend.

1) When the the multiplanar external texture transform runs, it splits
a texture_external variable into two texture_2d variables. The
Resolver then creates new texture/sampler pairs for the samplers with
which the plane1 texture is used, and new binding points for those
that bubble up to the global level. Since Dawn collects
sampler/texture pairs from the Inspector before the transforms run, it
doesn't know about these extra pairs, doesn't name them and doesn't
bind anything to them.

2) The multiplanar transform also inserts calls to
textureDimensions(), a texture-only builtin that doesn't require a
sampler. In the CombineSamplers transform, since OpenGLES doesn't have
the concept of a texture-only sampler2d, this necessitates the
creation of new binding points for the texture with a "placeholder"
sampler, which Dawn also doesn't know about, so it doesn't name them
or bind anything to them.

Fix #1 is on the Dawn side: when iterating over external texture
bindings, create a map from plane0 texture -> plane1 texture. When
iterating over the sampler/texture uses that the Tint inspector has
discovered, if the texture is an external texture, add a new pair with
the same sampler and the plane1 texture.

Fix for #2 is on the Tint side: if a function contains both a
texture-only reference and a texture/sampler reference to the same
texture, use the latter and remove the former. Texture-only builtins
won't use the sampler anyway. This is essentially an optimization, but
it's one we know will run on the functions generated by the
multiplanar transform, since it always calls textureDimensions() in a
function also containing the textureSample() call on the same texture.

Change-Id: I531ab09acdc47b435d72033aabd9eefa57f8f6e3
Bug: tint:1774
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/152660
Kokoro: Kokoro <[email protected]>
Reviewed-by: Ben Clayton <[email protected]>
Commit-Queue: Stephen White <[email protected]>

Author: SenorBlanco

src/dawn/native/opengl/ShaderModuleGL.cpp

@@ -58,12 +58,26 @@ tint::glsl::writer::Version::Standard ToTintGLStandard(opengl::OpenGLVersion::St
 
 using BindingMap = std::unordered_map<tint::BindingPoint, tint::BindingPoint>;
 
+opengl::CombinedSampler* AppendCombinedSampler(opengl::CombinedSamplerInfo* info,
+                                               tint::inspector::SamplerTexturePair pair,
+                                               tint::BindingPoint placeholderBindingPoint) {
+    info->emplace_back();
+    opengl::CombinedSampler* combinedSampler = &info->back();
+    combinedSampler->usePlaceholderSampler = pair.sampler_binding_point == placeholderBindingPoint;
+    combinedSampler->samplerLocation.group = BindGroupIndex(pair.sampler_binding_point.group);
+    combinedSampler->samplerLocation.binding = BindingNumber(pair.sampler_binding_point.binding);
+    combinedSampler->textureLocation.group = BindGroupIndex(pair.texture_binding_point.group);
+    combinedSampler->textureLocation.binding = BindingNumber(pair.texture_binding_point.binding);
+    return combinedSampler;
+}
+
 #define GLSL_COMPILATION_REQUEST_MEMBERS(X)                                                      \
     X(const tint::Program*, inputProgram)                                                        \
     X(std::string, entryPointName)                                                               \
     X(SingleShaderStage, stage)                                                                  \
     X(tint::ExternalTextureOptions, externalTextureOptions)                                      \
     X(BindingMap, glBindings)                                                                    \
+    X(BindingMap, externalTextureExpansionMap)                                                   \
     X(tint::TextureBuiltinsFromUniformOptions, textureBuiltinsFromUniform)                       \
     X(std::optional<tint::ast::transform::SubstituteOverride::Config>, substituteOverrideConfig) \
     X(LimitsForCompilationRequest, limits)                                                       \
@@ -164,16 +178,17 @@ ResultOrError<GLuint> ShaderModule::CompileShader(
     // variables to the 1D space.
     const BindingInfoArray& moduleBindingInfo =
         GetEntryPoint(programmableStage.entryPoint).bindings;
-    std::unordered_map<BindingPoint, BindingPoint> glBindings;
+    BindingMap glBindings;
+    BindingMap externalTextureExpansionMap;
     for (BindGroupIndex group : IterateBitSet(layout->GetBindGroupLayoutsMask())) {
+        uint32_t groupAsInt = static_cast<uint32_t>(group);
         const BindGroupLayoutInternalBase* bgl = layout->GetBindGroupLayout(group);
         const auto& indices = layout->GetBindingIndexInfo()[group];
         const auto& groupBindingInfo = moduleBindingInfo[group];
         for (const auto& [bindingNumber, bindingInfo] : groupBindingInfo) {
             BindingIndex bindingIndex = bgl->GetBindingIndex(bindingNumber);
             GLuint shaderIndex = indices[bindingIndex];
-            BindingPoint srcBindingPoint{static_cast<uint32_t>(group),
-                                         static_cast<uint32_t>(bindingNumber)};
+            BindingPoint srcBindingPoint{groupAsInt, static_cast<uint32_t>(bindingNumber)};
             BindingPoint dstBindingPoint{0, shaderIndex};
             if (srcBindingPoint != dstBindingPoint) {
                 glBindings.emplace(srcBindingPoint, dstBindingPoint);
@@ -183,12 +198,12 @@ ResultOrError<GLuint> ShaderModule::CompileShader(
         for (const auto& [_, expansion] : bgl->GetExternalTextureBindingExpansionMap()) {
             uint32_t plane1Slot = indices[bgl->GetBindingIndex(expansion.plane1)];
             uint32_t paramsSlot = indices[bgl->GetBindingIndex(expansion.params)];
-            glBindings.emplace(
-                BindingPoint{static_cast<uint32_t>(group), static_cast<uint32_t>(expansion.plane1)},
-                BindingPoint{0u, plane1Slot});
-            glBindings.emplace(
-                BindingPoint{static_cast<uint32_t>(group), static_cast<uint32_t>(expansion.params)},
-                BindingPoint{0u, paramsSlot});
+            BindingPoint plane0{groupAsInt, static_cast<uint32_t>(expansion.plane0)};
+            BindingPoint plane1{groupAsInt, static_cast<uint32_t>(expansion.plane1)};
+            BindingPoint params{groupAsInt, static_cast<uint32_t>(expansion.params)};
+            glBindings.emplace(plane1, BindingPoint{0u, plane1Slot});
+            glBindings.emplace(params, BindingPoint{0u, paramsSlot});
+            externalTextureExpansionMap[plane0] = plane1;
         }
     }
 
@@ -213,6 +228,7 @@ ResultOrError<GLuint> ShaderModule::CompileShader(
     req.entryPointName = programmableStage.entryPoint;
     req.externalTextureOptions = BuildExternalTextureTransformBindings(layout);
     req.glBindings = std::move(glBindings);
+    req.externalTextureExpansionMap = std::move(externalTextureExpansionMap);
     req.textureBuiltinsFromUniform = std::move(textureBuiltinsFromUniform);
     req.substituteOverrideConfig = std::move(substituteOverrideConfig);
     req.limits = LimitsForCompilationRequest::Create(limits.v1);
@@ -277,21 +293,26 @@ ResultOrError<GLuint> ShaderModule::CompileShader(
             auto uses = inspector.GetSamplerTextureUses(r.entryPointName, placeholderBindingPoint);
             CombinedSamplerInfo combinedSamplerInfo;
             for (const auto& use : uses) {
-                combinedSamplerInfo.emplace_back();
+                CombinedSampler* info =
+                    AppendCombinedSampler(&combinedSamplerInfo, use, placeholderBindingPoint);
 
-                CombinedSampler* info = &combinedSamplerInfo.back();
-                if (use.sampler_binding_point == placeholderBindingPoint) {
-                    info->usePlaceholderSampler = true;
+                if (info->usePlaceholderSampler) {
                     needsPlaceholderSampler = true;
                     tintOptions.placeholder_binding_point = placeholderBindingPoint;
-                } else {
-                    info->usePlaceholderSampler = false;
                 }
-                info->samplerLocation.group = BindGroupIndex(use.sampler_binding_point.group);
-                info->samplerLocation.binding = BindingNumber(use.sampler_binding_point.binding);
-                info->textureLocation.group = BindGroupIndex(use.texture_binding_point.group);
-                info->textureLocation.binding = BindingNumber(use.texture_binding_point.binding);
                 tintOptions.binding_map[use] = info->GetName();
+
+                // If the texture has an associated plane1 texture (ie., it's an external texture),
+                // append a new combined sampler with the same sampler and the plane1 texture.
+                BindingMap::iterator plane1Texture =
+                    r.externalTextureExpansionMap.find(use.texture_binding_point);
+                if (plane1Texture != r.externalTextureExpansionMap.end()) {
+                    tint::inspector::SamplerTexturePair plane1Use{use.sampler_binding_point,
+                                                                  plane1Texture->second};
+                    CombinedSampler* plane1Info = AppendCombinedSampler(
+                        &combinedSamplerInfo, plane1Use, placeholderBindingPoint);
+                    tintOptions.binding_map[plane1Use] = plane1Info->GetName();
+                }
             }
 
             tintOptions.binding_remapper_options.binding_points = std::move(r.glBindings);

src/dawn/tests/end2end/ExternalTextureTests.cpp

@@ -207,10 +207,6 @@ TEST_P(ExternalTextureTests, SampleExternalTexture) {
 }
 
 TEST_P(ExternalTextureTests, SampleMultiplanarExternalTexture) {
-    // TODO(crbug.com/tint/1774): Tint has an issue compiling shaders that use external textures on
-    // OpenGL/OpenGLES.
-    DAWN_SUPPRESS_TEST_IF(IsOpenGL() || IsOpenGLES());
-
     wgpu::Texture sampledTexturePlane0 =
         Create2DTexture(device, kWidth, kHeight, wgpu::TextureFormat::R8Unorm,
                         wgpu::TextureUsage::TextureBinding | wgpu::TextureUsage::RenderAttachment);
@@ -305,10 +301,6 @@ TEST_P(ExternalTextureTests, SampleMultiplanarExternalTexture) {
 TEST_P(ExternalTextureTests, SampleMultiplanarExternalTextureNorm16) {
     DAWN_TEST_UNSUPPORTED_IF(!IsNorm16TextureFormatsSupported());
 
-    // TODO(crbug.com/tint/1774): Tint has an issue compiling shaders that use external textures on
-    // OpenGL/OpenGLES.
-    DAWN_SUPPRESS_TEST_IF(IsOpenGL() || IsOpenGLES());
-
     wgpu::Texture sampledTexturePlane0 =
         Create2DTexture(device, kWidth, kHeight, wgpu::TextureFormat::R16Unorm,
                         wgpu::TextureUsage::TextureBinding | wgpu::TextureUsage::RenderAttachment);
@@ -560,10 +552,6 @@ TEST_P(ExternalTextureTests, RotateAndOrFlipSinglePlane) {
 // square in the lower left and a blue square in the lower right. The image is then sampled as an
 // external texture and rotated 0, 90, 180, and 270 degrees with and without the y-axis flipped.
 TEST_P(ExternalTextureTests, RotateAndOrFlipMultiplanar) {
-    // TODO(crbug.com/tint/1774): Tint has an issue compiling shaders that use external textures on
-    // OpenGL/OpenGLES.
-    DAWN_SUPPRESS_TEST_IF(IsOpenGL() || IsOpenGLES());
-
     const wgpu::ShaderModule sourceTexturePlane0FsModule = utils::CreateShaderModule(device, R"(
         @fragment fn main(@builtin(position) FragCoord : vec4f)
                                  -> @location(0) vec4f {
@@ -906,10 +894,6 @@ TEST_P(ExternalTextureTests, CropSinglePlane) {
 // This test draws a 2x2 multi-colored square surrounded by a 1px black border. We test the external
 // texture crop functionality by cropping to specific ranges inside the texture.
 TEST_P(ExternalTextureTests, CropMultiplanar) {
-    // TODO(crbug.com/tint/1774): Tint has an issue compiling shaders that use external textures on
-    // OpenGL/OpenGLES.
-    DAWN_SUPPRESS_TEST_IF(IsOpenGL() || IsOpenGLES());
-
     const wgpu::ShaderModule sourceTexturePlane0FsModule = utils::CreateShaderModule(device, R"(
         @fragment fn main(@builtin(position) FragCoord : vec4f)
                                  -> @location(0) vec4f {

src/tint/lang/glsl/writer/ast_raise/combine_samplers.cc

@@ -145,6 +145,51 @@ struct CombineSamplers::State {
         }
     }
 
+    /// Insert a new texture/sampler pair into the combined samplers maps (global or local, as
+    /// appropriate). If local, also add a function parameter to "params".
+    /// @param pair the texture/sampler pair to insert
+    /// @param fn the function scope in which to insert (if local)
+    /// @param params the calling function's parameter list to modify (if local)
+    void InsertPair(sem::VariablePair pair,
+                    const sem::Function* fn,
+                    tint::Vector<const ast::Parameter*, 8>* params) {
+        const sem::Variable* texture_var = pair.first;
+        const sem::Variable* sampler_var = pair.second;
+        std::string name = texture_var->Declaration()->name->symbol.Name();
+        if (sampler_var) {
+            name += "_" + sampler_var->Declaration()->name->symbol.Name();
+        }
+        if (IsGlobal(pair)) {
+            // Both texture and sampler are global; add a new global variable
+            // to represent the combined sampler (if not already created).
+            GetOrCreate(global_combined_texture_samplers_, pair,
+                        [&] { return CreateCombinedGlobal(texture_var, sampler_var, name); });
+        } else {
+            // Either texture or sampler (or both) is a function parameter;
+            // add a new function parameter to represent the combined sampler.
+            ast::Type type = CreateCombinedASTTypeFor(texture_var, sampler_var);
+            auto* var = ctx.dst->Param(ctx.dst->Symbols().New(name), type);
+            params->Push(var);
+            function_combined_texture_samplers_[fn][pair] = var;
+        }
+    }
+
+    /// For a given texture, find any texture/sampler pair with a non-null sampler in the given
+    /// function scope.
+    /// @param texture_var the texture variable of interest
+    /// @param fn the function scope in which to search
+    /// @returns the full pair, if found
+    const sem::VariablePair* FindFullTextureSamplerPair(const sem::Variable* texture_var,
+                                                        const sem::Function* fn) {
+        for (auto pairIter = fn->TextureSamplerPairs().begin();
+             pairIter != fn->TextureSamplerPairs().end(); pairIter++) {
+            if (pairIter->first == texture_var && pairIter->second) {
+                return pairIter;
+            }
+        }
+        return nullptr;
+    }
+
     /// Runs the transform
     /// @returns the new program or SkipTransform if the transform is not required
     ApplyResult Run() {
@@ -177,25 +222,30 @@ struct CombineSamplers::State {
                 }
                 Vector<const ast::Parameter*, 8> params;
                 for (auto pair : fn->TextureSamplerPairs()) {
-                    const sem::Variable* texture_var = pair.first;
-                    const sem::Variable* sampler_var = pair.second;
-                    std::string name = texture_var->Declaration()->name->symbol.Name();
-                    if (sampler_var) {
-                        name += "_" + sampler_var->Declaration()->name->symbol.Name();
+                    if (!pair.second) {
+                        continue;
                     }
-                    if (IsGlobal(pair)) {
-                        // Both texture and sampler are global; add a new global variable
-                        // to represent the combined sampler (if not already created).
-                        GetOrCreate(global_combined_texture_samplers_, pair, [&] {
-                            return CreateCombinedGlobal(texture_var, sampler_var, name);
-                        });
+                    InsertPair(pair, fn, &params);
+                }
+                for (auto pair : fn->TextureSamplerPairs()) {
+                    if (pair.second) {
+                        continue;
+                    }
+                    // Look for another pair with a non-null sampler.
+                    // NOTE: this is O(N^2) in the number of pairs, since
+                    // FindFullTextureSamplerPair() also loops over all pairs. If this proves
+                    // problematic, it could be optimized.
+                    if (const sem::VariablePair* fullPair =
+                            FindFullTextureSamplerPair(pair.first, fn)) {
+                        if (IsGlobal(pair)) {
+                            global_combined_texture_samplers_[pair] =
+                                global_combined_texture_samplers_[*fullPair];
+                        } else {
+                            auto* var = function_combined_texture_samplers_[fn][*fullPair];
+                            function_combined_texture_samplers_[fn][pair] = var;
+                        }
                     } else {
-                        // Either texture or sampler (or both) is a function parameter;
-                        // add a new function parameter to represent the combined sampler.
-                        ast::Type type = CreateCombinedASTTypeFor(texture_var, sampler_var);
-                        auto* var = ctx.dst->Param(ctx.dst->Symbols().New(name), type);
-                        params.Push(var);
-                        function_combined_texture_samplers_[fn][pair] = var;
+                        InsertPair(pair, fn, &params);
                     }
                 }
                 // Filter out separate textures and samplers from the original
@@ -285,6 +335,12 @@ struct CombineSamplers::State {
                         if (IsGlobal(pair)) {
                             continue;
                         }
+                        // Texture-only pairs do not require a function parameter if they've been
+                        // replaced by a real pair.
+                        if (!pair.second && FindFullTextureSamplerPair(pair.first, callee)) {
+                            continue;
+                        }
+
                         const sem::Variable* texture_var = pair.first;
                         const sem::Variable* sampler_var = pair.second;
                         if (auto* param = texture_var->As<sem::Parameter>()) {

src/tint/lang/glsl/writer/ast_raise/combine_samplers_test.cc

@@ -794,14 +794,12 @@ fn main() -> vec4<f32> {
 }
 )";
     auto* expect = R"(
-@group(0) @binding(0) @internal(disable_validation__binding_point_collision) var fred : texture_2d<f32>;
-
 @group(0) @binding(0) @internal(disable_validation__binding_point_collision) var barney : texture_2d<f32>;
 
 @group(0) @binding(0) @internal(disable_validation__binding_point_collision) var placeholder_sampler : sampler;
 
 fn main() -> vec4<f32> {
-  return (textureLoad(fred, vec2<i32>(), 0) + textureSample(barney, placeholder_sampler, vec2<f32>()));
+  return (textureLoad(barney, vec2<i32>(), 0) + textureSample(barney, placeholder_sampler, vec2<f32>()));
 }
 )";
 

test/tint/bug/tint/942.wgsl.expected.glsl

@@ -28,7 +28,6 @@ uint tint_div(uint lhs, uint rhs) {
   return (lhs / ((rhs == 0u) ? 1u : rhs));
 }
 
-uniform highp sampler2D inputTex_1;
 uniform highp sampler2D inputTex_samp;
 
 void tint_symbol(uvec3 WorkGroupID, uvec3 LocalInvocationID, uint local_invocation_index) {
@@ -41,7 +40,7 @@ void tint_symbol(uvec3 WorkGroupID, uvec3 LocalInvocationID, uint local_invocati
   }
   barrier();
   uint filterOffset = tint_div((params.inner.filterDim - 1u), 2u);
-  uvec2 dims = uvec2(textureSize(inputTex_1, 0));
+  uvec2 dims = uvec2(textureSize(inputTex_samp, 0));
   uvec2 baseIndex = (((WorkGroupID.xy * uvec2(params.inner.blockDim, 4u)) + (LocalInvocationID.xy * uvec2(4u, 1u))) - uvec2(filterOffset, 0u));
   {
     for(uint r = 0u; (r < 4u); r = (r + 1u)) {