Fix OpenGL over-eager lazy zero initialization for textures

Bug: dawn:145, dawn:348
Change-Id: I3b6e7b148a171bdcb50bcb825858cbf967c64c87
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/16983
Commit-Queue: Austin Eng <enga@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>

src/dawn_native/opengl/TextureGL.cpp

@@ -184,7 +184,7 @@ namespace dawn_native { namespace opengl {
     MaybeError Texture::ClearTexture(GLint baseMipLevel,
                                      GLint levelCount,
                                      GLint baseArrayLayer,
-                                     uint32_t layerCount,
+                                     GLint layerCount,
                                      TextureBase::ClearValue clearValue) {
         // TODO(jiawei.shao@intel.com): initialize the textures with compressed formats.
         if (GetFormat().isCompressed) {
@@ -210,19 +210,55 @@ namespace dawn_native { namespace opengl {
                     gl.StencilMask(GetStencilMaskFromStencilFormat(GetFormat().format));
                 }
 
+                auto DoClear = [&]() {
+                    if (doDepthClear && doStencilClear) {
+                        gl.ClearBufferfi(GL_DEPTH_STENCIL, 0, depth, stencil);
+                    } else if (doDepthClear) {
+                        gl.ClearBufferfv(GL_DEPTH, 0, &depth);
+                    } else if (doStencilClear) {
+                        gl.ClearBufferiv(GL_STENCIL, 0, &stencil);
+                    }
+                };
+
                 GLuint framebuffer = 0;
                 gl.GenFramebuffers(1, &framebuffer);
                 gl.BindFramebuffer(GL_DRAW_FRAMEBUFFER, framebuffer);
-                // TODO(natlee@microsoft.com): clear all mip levels and array layers.
+
-                gl.FramebufferTexture2D(GL_DRAW_FRAMEBUFFER, GL_DEPTH_STENCIL_ATTACHMENT,
+                for (GLint level = baseMipLevel; level < baseMipLevel + levelCount; ++level) {
-                                        GetGLTarget(), GetHandle(), 0);
+                    switch (GetDimension()) {
-                if (doDepthClear && doStencilClear) {
+                        case wgpu::TextureDimension::e2D:
-                    gl.ClearBufferfi(GL_DEPTH_STENCIL, 0, depth, stencil);
+                            if (GetArrayLayers() == 1) {
-                } else if (doDepthClear) {
+                                if (clearValue == TextureBase::ClearValue::Zero &&
-                    gl.ClearBufferfv(GL_DEPTH, 0, &depth);
+                                    IsSubresourceContentInitialized(level, 1, 0, 1)) {
-                } else if (doStencilClear) {
+                                    // Skip lazy clears if already initialized.
-                    gl.ClearBufferiv(GL_STENCIL, 0, &stencil);
+                                    continue;
+                                }
+                                gl.FramebufferTexture2D(GL_DRAW_FRAMEBUFFER,
+                                                        GL_DEPTH_STENCIL_ATTACHMENT, GetGLTarget(),
+                                                        GetHandle(), level);
+                                DoClear();
+                            } else {
+                                for (GLint layer = baseArrayLayer;
+                                     layer < baseArrayLayer + layerCount; ++layer) {
+                                    if (clearValue == TextureBase::ClearValue::Zero &&
+                                        IsSubresourceContentInitialized(level, 1, layer, 1)) {
+                                        // Skip lazy clears if already initialized.
+                                        continue;
+                                    }
+
+                                    gl.FramebufferTextureLayer(GL_DRAW_FRAMEBUFFER,
+                                                               GL_DEPTH_STENCIL_ATTACHMENT,
+                                                               GetHandle(), level, layer);
+                                    DoClear();
+                                }
+                            }
+                            break;
+
+                        default:
+                            UNREACHABLE();
+                    }
                 }
+
                 gl.DeleteFramebuffers(1, &framebuffer);
             } else {
                 static constexpr uint32_t MAX_TEXEL_SIZE = 16;
@@ -234,9 +270,17 @@ namespace dawn_native { namespace opengl {
                 const GLFormat& glFormat = GetGLFormat();
                 for (GLint level = baseMipLevel; level < baseMipLevel + levelCount; ++level) {
                     Extent3D mipSize = GetMipLevelPhysicalSize(level);
-                    gl.ClearTexSubImage(mHandle, level, 0, 0, baseArrayLayer, mipSize.width,
+                    for (GLint layer = baseArrayLayer; layer < baseArrayLayer + layerCount;
-                                        mipSize.height, layerCount, glFormat.format, glFormat.type,
+                         ++layer) {
-                                        clearColorData.data());
+                        if (clearValue == TextureBase::ClearValue::Zero &&
+                            IsSubresourceContentInitialized(level, 1, layer, 1)) {
+                            // Skip lazy clears if already initialized.
+                            continue;
+                        }
+                        gl.ClearTexSubImage(mHandle, level, 0, 0, layer, mipSize.width,
+                                            mipSize.height, 1, glFormat.format, glFormat.type,
+                                            clearColorData.data());
+                    }
                 }
             }
         } else {
@@ -283,21 +327,37 @@ namespace dawn_native { namespace opengl {
                 Extent3D size = GetMipLevelPhysicalSize(level);
                 switch (GetDimension()) {
                     case wgpu::TextureDimension::e2D:
-                        // TODO(natlee@microsoft.com): This will break when layerCount is greater
+                        if (GetArrayLayers() == 1) {
-                        // than 1, because the buffer is only sized for one layer.
+                            if (clearValue == TextureBase::ClearValue::Zero &&
-                        ASSERT(layerCount == 1);
+                                IsSubresourceContentInitialized(level, 1, 0, 1)) {
-                        gl.TexSubImage2D(GetGLTarget(), level, 0, 0, size.width, size.height,
+                                // Skip lazy clears if already initialized.
-                                         GetGLFormat().format, GetGLFormat().type, 0);
+                                continue;
+                            }
+                            gl.TexSubImage2D(GetGLTarget(), level, 0, 0, size.width, size.height,
+                                             GetGLFormat().format, GetGLFormat().type, 0);
+                        } else {
+                            for (GLint layer = baseArrayLayer; layer < baseArrayLayer + layerCount;
+                                 ++layer) {
+                                if (clearValue == TextureBase::ClearValue::Zero &&
+                                    IsSubresourceContentInitialized(level, 1, layer, 1)) {
+                                    // Skip lazy clears if already initialized.
+                                    continue;
+                                }
+                                gl.TexSubImage3D(GetGLTarget(), level, 0, 0, layer, size.width,
+                                                 size.height, 1, GetGLFormat().format,
+                                                 GetGLFormat().type, 0);
+                            }
+                        }
                         break;
 
                     default:
                         UNREACHABLE();
                 }
-                gl.PixelStorei(GL_UNPACK_ROW_LENGTH, 0);
-                gl.PixelStorei(GL_UNPACK_IMAGE_HEIGHT, 0);
-
-                gl.BindBuffer(GL_PIXEL_UNPACK_BUFFER, 0);
             }
+            gl.PixelStorei(GL_UNPACK_ROW_LENGTH, 0);
+            gl.PixelStorei(GL_UNPACK_IMAGE_HEIGHT, 0);
+
+            gl.BindBuffer(GL_PIXEL_UNPACK_BUFFER, 0);
         }
         if (clearValue == TextureBase::ClearValue::Zero) {
             SetIsSubresourceContentInitialized(true, baseMipLevel, levelCount, baseArrayLayer,

src/dawn_native/opengl/TextureGL.h

@@ -47,7 +47,7 @@ namespace dawn_native { namespace opengl {
         MaybeError ClearTexture(GLint baseMipLevel,
                                 GLint levelCount,
                                 GLint baseArrayLayer,
-                                uint32_t layerCount,
+                                GLint layerCount,
                                 TextureBase::ClearValue clearValue);
 
         GLuint mHandle;

src/tests/end2end/NonzeroTextureCreationTests.cpp

@@ -157,10 +157,6 @@ TEST_P(NonzeroTextureCreationTests, NonrenderableTextureFormat) {
 // Test that textures with more than 1 array layers and nonrenderable texture formats clear to 0x01
 // because toggle is enabled
 TEST_P(NonzeroTextureCreationTests, NonRenderableTextureClearWithMultiArrayLayers) {
-    // TODO(natlee@microsoft.com): skip for now on opengl because TextureClear nonrenderable
-    // textures does not create large enough buffers for array layers greater than 1.
-    DAWN_SKIP_TEST_IF(IsOpenGL());
-
     wgpu::TextureDescriptor descriptor;
     descriptor.dimension = wgpu::TextureDimension::e2D;
     descriptor.size.width = kSize;
@@ -194,9 +190,6 @@ TEST_P(NonzeroTextureCreationTests, NonRenderableTextureClearWithMultiArrayLayer
 
 // Test that all subresources of a renderable texture are filled because the toggle is enabled.
 TEST_P(NonzeroTextureCreationTests, AllSubresourcesFilled) {
-    // TODO(crbug.com/dawn/145): Implement on other platforms.
-    DAWN_SKIP_TEST_IF(!IsMetal() && !IsD3D12() && !IsVulkan());
-
     wgpu::TextureDescriptor baseDescriptor;
     baseDescriptor.dimension = wgpu::TextureDimension::e2D;
     baseDescriptor.size.width = kSize;
@@ -250,9 +243,6 @@ TEST_P(NonzeroTextureCreationTests, AllSubresourcesFilled) {
 
 // Test that all subresources of a nonrenderable texture are filled because the toggle is enabled.
 TEST_P(NonzeroTextureCreationTests, NonRenderableAllSubresourcesFilled) {
-    // TODO(crbug.com/dawn/145): Implement on other platforms.
-    DAWN_SKIP_TEST_IF(!IsMetal() && !IsD3D12() && !IsVulkan());
-
     wgpu::TextureDescriptor baseDescriptor;
     baseDescriptor.dimension = wgpu::TextureDimension::e2D;
     baseDescriptor.size.width = kSize;

src/tests/end2end/TextureZeroInitTests.cpp

@@ -691,10 +691,6 @@ TEST_P(TextureZeroInitTest, NonRenderableTextureClearUnalignedSize) {
 // This tests that the code path of CopyTextureToBuffer clears correctly for non-renderable textures
 // with more than 1 array layers
 TEST_P(TextureZeroInitTest, NonRenderableTextureClearWithMultiArrayLayers) {
-    // TODO(natlee@microsoft.com): skip for now on opengl because TextureClear nonrenderable
-    // textures does not create large enough buffers for array layers greater than 1.
-    DAWN_SKIP_TEST_IF(IsOpenGL());
-
     wgpu::TextureDescriptor descriptor =
         CreateTextureDescriptor(1, 2, wgpu::TextureUsage::CopySrc, kNonrenderableColorFormat);
     wgpu::Texture texture = device.CreateTexture(&descriptor);
@@ -871,9 +867,6 @@ TEST_P(TextureZeroInitTest, RenderingLoadingDepthStencilStoreOpClear) {
 // Test that if one mip of a texture is initialized and another is uninitialized, lazy clearing the
 // uninitialized mip does not clear the initialized mip.
 TEST_P(TextureZeroInitTest, PreservesInitializedMip) {
-    // TODO(crbug.com/dawn/145): Fix this on other backends
-    DAWN_SKIP_TEST_IF(!IsMetal() && !IsD3D12() && !IsVulkan());
-
     wgpu::TextureDescriptor sampleTextureDescriptor = CreateTextureDescriptor(
         2, 1,
         wgpu::TextureUsage::CopySrc | wgpu::TextureUsage::CopyDst | wgpu::TextureUsage::Sampled,
@@ -952,9 +945,6 @@ TEST_P(TextureZeroInitTest, PreservesInitializedMip) {
 // Test that if one layer of a texture is initialized and another is uninitialized, lazy clearing
 // the uninitialized layer does not clear the initialized layer.
 TEST_P(TextureZeroInitTest, PreservesInitializedArrayLayer) {
-    // TODO(crbug.com/dawn/145): Fix this on other backends
-    DAWN_SKIP_TEST_IF(!IsMetal() && !IsD3D12() && !IsVulkan());
-
     wgpu::TextureDescriptor sampleTextureDescriptor = CreateTextureDescriptor(
         1, 2,
         wgpu::TextureUsage::CopySrc | wgpu::TextureUsage::CopyDst | wgpu::TextureUsage::Sampled,