Fixing glClear tests.

This CL makes a few changes to fix up the basic glClear test
RGBA8Framebuffer, as well as adding a test that uploads a
texture before clearing a RGBA8 framebuffer.

It moves the wgpu::RenderPassDescriptor(RPD), as well as helper
methods for comparing RPDs from the context to the framebuffer.
The color attachments that are created in when FramebufferWgpu::clear
is called are also stored in the framebuffer now as well.

This CL also changes texture creation to use the RGBA8Unorm format
instead of RGBA8Uint format. It also adds the wgpu viewFormats
parameter to an ImageHelper. Future formats support is still TBD.

Bug: angleproject:8582
Change-Id: Idfc4182eb4d6de8a771f2f91d337564ee71df010
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/5503549
Reviewed-by: Shahbaz Youssefi <syoussefi@chromium.org>
Reviewed-by: Matthew Denton <mpdenton@chromium.org>
Commit-Queue: Liza Burakova <liza@chromium.org>

8 files changed, 127 insertions, 95 deletions

diff --git a/src/libANGLE/renderer/wgpu/ContextWgpu.cpp b/src/libANGLE/renderer/wgpu/ContextWgpu.cpp
index 4987c8e885..0e5ca07298 100644
--- a/src/libANGLE/renderer/wgpu/ContextWgpu.cpp
+++ b/src/libANGLE/renderer/wgpu/ContextWgpu.cpp
@@ -44,66 +44,6 @@ constexpr angle::PackedEnumMap<webgpu::RenderPassClosureReason, const char *>
          "Render pass closed due to starting a new render pass"},
     }};
 
-bool RenderPassColorAttachmentEqual(const wgpu::RenderPassColorAttachment &attachment1,
-                                    const wgpu::RenderPassColorAttachment &attachment2)
-{
-
-    if (attachment1.nextInChain != nullptr || attachment2.nextInChain != nullptr)
-    {
-        return false;
-    }
-
-    return attachment1.view.Get() == attachment2.view.Get() &&
-           attachment1.depthSlice == attachment2.depthSlice &&
-           attachment1.resolveTarget.Get() == attachment2.resolveTarget.Get() &&
-           attachment1.loadOp == attachment2.loadOp && attachment1.storeOp == attachment2.storeOp &&
-           attachment1.clearValue.r == attachment2.clearValue.r &&
-           attachment1.clearValue.g == attachment2.clearValue.g &&
-           attachment1.clearValue.b == attachment2.clearValue.b &&
-           attachment1.clearValue.a == attachment2.clearValue.a;
-}
-
-bool RenderPassDepthStencilAttachmentEqual(
-    const wgpu::RenderPassDepthStencilAttachment &attachment1,
-    const wgpu::RenderPassDepthStencilAttachment &attachment2)
-{
-    return attachment1.view.Get() == attachment2.view.Get() &&
-           attachment1.depthLoadOp == attachment2.depthLoadOp &&
-           attachment1.depthStoreOp == attachment2.depthStoreOp &&
-           attachment1.depthClearValue == attachment2.depthClearValue &&
-           attachment1.stencilLoadOp == attachment2.stencilLoadOp &&
-           attachment1.stencilStoreOp == attachment2.stencilStoreOp &&
-           attachment1.stencilClearValue == attachment2.stencilClearValue &&
-           attachment1.stencilReadOnly == attachment2.stencilReadOnly;
-}
-
-bool RenderPassDescEqual(const wgpu::RenderPassDescriptor &desc1,
-                         const wgpu::RenderPassDescriptor &desc2)
-{
-
-    if (desc1.nextInChain != nullptr || desc2.nextInChain != nullptr)
-    {
-        return false;
-    }
-
-    if (desc1.colorAttachmentCount != desc2.colorAttachmentCount)
-    {
-        return false;
-    }
-
-    for (uint32_t i = 0; i < desc1.colorAttachmentCount; ++i)
-    {
-        if (!RenderPassColorAttachmentEqual(desc1.colorAttachments[i], desc2.colorAttachments[i]))
-        {
-            return false;
-        }
-    }
-
-    // TODO(anglebug.com/8582): for now ignore `occlusionQuerySet` and `timestampWrites`.
-
-    return RenderPassDepthStencilAttachmentEqual(*desc1.depthStencilAttachment,
-                                                 *desc2.depthStencilAttachment);
-}
 }  // namespace
 
 ContextWgpu::ContextWgpu(const gl::State &state, gl::ErrorSet *errorSet, DisplayWgpu *display)
@@ -600,31 +540,19 @@ void ContextWgpu::handleError(GLenum errorCode,
     mErrors->handleError(errorCode, errorStream.str().c_str(), file, function, line);
 }
 
-angle::Result ContextWgpu::ensureRenderPassStarted(const wgpu::RenderPassDescriptor &desc)
+angle::Result ContextWgpu::startRenderPass(const wgpu::RenderPassDescriptor &desc)
 {
-    if (!mCurrentCommandEncoder)
-    {
-        mCurrentCommandEncoder = getDevice().CreateCommandEncoder(nullptr);
-    }
-    if (mCurrentRenderPass)
-    {
-        // TODO(anglebug.com/8582): this should eventually ignore load and store operations so we
-        // can avoid starting a new render pass in more situations.
-        if (RenderPassDescEqual(mCurrentRenderPassDesc, desc))
-        {
-            return angle::Result::Continue;
-        }
-        ANGLE_TRY(endRenderPass(webgpu::RenderPassClosureReason::NewRenderPass));
-    }
+    mCurrentCommandEncoder = getDevice().CreateCommandEncoder(nullptr);
     mCurrentRenderPass     = mCurrentCommandEncoder.BeginRenderPass(&desc);
-    mCurrentRenderPassDesc = desc;
-
     return angle::Result::Continue;
 }
 
 angle::Result ContextWgpu::endRenderPass(webgpu::RenderPassClosureReason closure_reason)
 {
-
+    if (!mCurrentRenderPass)
+    {
+        return angle::Result::Continue;
+    }
     const char *reasonText = kRenderPassClosureReason[closure_reason];
     INFO() << reasonText;
     mCurrentRenderPass.End();
diff --git a/src/libANGLE/renderer/wgpu/ContextWgpu.h b/src/libANGLE/renderer/wgpu/ContextWgpu.h
index 7fb82a6d99..6470abe804 100644
--- a/src/libANGLE/renderer/wgpu/ContextWgpu.h
+++ b/src/libANGLE/renderer/wgpu/ContextWgpu.h
@@ -262,7 +262,7 @@ class ContextWgpu : public ContextImpl
     wgpu::Device &getDevice() { return mDisplay->getDevice(); }
     wgpu::Queue &getQueue() { return mDisplay->getQueue(); }
     angle::ImageLoadContext &getImageLoadContext() { return mImageLoadContext; }
-    angle::Result ensureRenderPassStarted(const wgpu::RenderPassDescriptor &desc);
+    angle::Result startRenderPass(const wgpu::RenderPassDescriptor &desc);
     angle::Result endRenderPass(webgpu::RenderPassClosureReason closure_reason);
 
     angle::Result flush();
@@ -280,7 +280,6 @@ class ContextWgpu : public ContextImpl
 
     wgpu::CommandEncoder mCurrentCommandEncoder;
     wgpu::RenderPassEncoder mCurrentRenderPass;
-    wgpu::RenderPassDescriptor mCurrentRenderPassDesc;
 };
 
 }  // namespace rx
diff --git a/src/libANGLE/renderer/wgpu/FramebufferWgpu.cpp b/src/libANGLE/renderer/wgpu/FramebufferWgpu.cpp
index 24709ebfd3..1a3887dae2 100644
--- a/src/libANGLE/renderer/wgpu/FramebufferWgpu.cpp
+++ b/src/libANGLE/renderer/wgpu/FramebufferWgpu.cpp
@@ -16,6 +16,71 @@
 #include "libANGLE/renderer/wgpu/ContextWgpu.h"
 #include "libANGLE/renderer/wgpu/wgpu_utils.h"
 
+namespace
+{
+bool RenderPassColorAttachmentEqual(const wgpu::RenderPassColorAttachment &attachment1,
+                                    const wgpu::RenderPassColorAttachment &attachment2)
+{
+
+    if (attachment1.nextInChain != nullptr || attachment2.nextInChain != nullptr)
+    {
+        return false;
+    }
+
+    return attachment1.view.Get() == attachment2.view.Get() &&
+           attachment1.depthSlice == attachment2.depthSlice &&
+           attachment1.resolveTarget.Get() == attachment2.resolveTarget.Get() &&
+           attachment1.loadOp == attachment2.loadOp && attachment1.storeOp == attachment2.storeOp &&
+           attachment1.clearValue.r == attachment2.clearValue.r &&
+           attachment1.clearValue.g == attachment2.clearValue.g &&
+           attachment1.clearValue.b == attachment2.clearValue.b &&
+           attachment1.clearValue.a == attachment2.clearValue.a;
+}
+
+bool RenderPassDepthStencilAttachmentEqual(
+    const wgpu::RenderPassDepthStencilAttachment &attachment1,
+    const wgpu::RenderPassDepthStencilAttachment &attachment2)
+{
+    return attachment1.view.Get() == attachment2.view.Get() &&
+           attachment1.depthLoadOp == attachment2.depthLoadOp &&
+           attachment1.depthStoreOp == attachment2.depthStoreOp &&
+           attachment1.depthClearValue == attachment2.depthClearValue &&
+           attachment1.stencilLoadOp == attachment2.stencilLoadOp &&
+           attachment1.stencilStoreOp == attachment2.stencilStoreOp &&
+           attachment1.stencilClearValue == attachment2.stencilClearValue &&
+           attachment1.stencilReadOnly == attachment2.stencilReadOnly;
+}
+// TODO(anglebug.com/8582): this should eventually ignore load and store operations so we
+// can avoid starting a new render pass in more situations.
+bool RenderPassDescEqual(const wgpu::RenderPassDescriptor &desc1,
+                         const wgpu::RenderPassDescriptor &desc2)
+{
+
+    if (desc1.nextInChain != nullptr || desc2.nextInChain != nullptr)
+    {
+        return false;
+    }
+
+    if (desc1.colorAttachmentCount != desc2.colorAttachmentCount)
+    {
+        return false;
+    }
+
+    for (uint32_t i = 0; i < desc1.colorAttachmentCount; ++i)
+    {
+        if (!RenderPassColorAttachmentEqual(desc1.colorAttachments[i], desc2.colorAttachments[i]))
+        {
+            return false;
+        }
+    }
+
+    // TODO(anglebug.com/8582): for now ignore `occlusionQuerySet` and `timestampWrites`.
+
+    return RenderPassDepthStencilAttachmentEqual(*desc1.depthStencilAttachment,
+                                                 *desc2.depthStencilAttachment);
+}
+}  // namespace
+
 namespace rx
 {
 
@@ -56,8 +121,6 @@ angle::Result FramebufferWgpu::clear(const gl::Context *context, GLbitfield mask
     ContextWgpu *contextWgpu   = GetImplAs<ContextWgpu>(context);
     gl::ColorF colorClearValue = context->getState().getColorClearValue();
 
-    std::vector<wgpu::RenderPassColorAttachment> colorAttachments(
-        mState.getEnabledDrawBuffers().count());
     for (size_t enabledDrawBuffer : mState.getEnabledDrawBuffers())
     {
         wgpu::RenderPassColorAttachment colorAttachment;
@@ -70,15 +133,22 @@ angle::Result FramebufferWgpu::clear(const gl::Context *context, GLbitfield mask
         colorAttachment.clearValue.g = colorClearValue.green;
         colorAttachment.clearValue.b = colorClearValue.blue;
         colorAttachment.clearValue.a = colorClearValue.alpha;
-        colorAttachments.push_back(colorAttachment);
+        // TODO(liza): reset mCurrentColorAttachments in syncState.
+        mCurrentColorAttachments.push_back(colorAttachment);
     }
 
-    wgpu::RenderPassDescriptor renderPassDesc;
-    renderPassDesc.colorAttachmentCount = colorAttachments.size();
-    renderPassDesc.colorAttachments     = colorAttachments.data();
+    wgpu::RenderPassDescriptor newRenderPassDesc;
+    newRenderPassDesc.colorAttachmentCount = mCurrentColorAttachments.size();
+    newRenderPassDesc.colorAttachments     = mCurrentColorAttachments.data();
 
+    // Attempt to end a render pass if one has already been started.
+    if (!RenderPassDescEqual(mCurrentRenderPassDesc, newRenderPassDesc))
+    {
+        ANGLE_TRY(contextWgpu->endRenderPass(webgpu::RenderPassClosureReason::NewRenderPass));
+        mCurrentRenderPassDesc = newRenderPassDesc;
+    }
     // TODO(anglebug.com/8582): optimize this implementation.
-    ANGLE_TRY(contextWgpu->ensureRenderPassStarted(renderPassDesc));
+    ANGLE_TRY(contextWgpu->startRenderPass(mCurrentRenderPassDesc));
     ANGLE_TRY(contextWgpu->endRenderPass(webgpu::RenderPassClosureReason::NewRenderPass));
     ANGLE_TRY(contextWgpu->flush());
     return angle::Result::Continue;
diff --git a/src/libANGLE/renderer/wgpu/FramebufferWgpu.h b/src/libANGLE/renderer/wgpu/FramebufferWgpu.h
index d4262f85eb..81efa0b80c 100644
--- a/src/libANGLE/renderer/wgpu/FramebufferWgpu.h
+++ b/src/libANGLE/renderer/wgpu/FramebufferWgpu.h
@@ -78,8 +78,12 @@ class FramebufferWgpu : public FramebufferImpl
                                     size_t index,
                                     GLfloat *xy) const override;
 
+    RenderTargetWgpu *getReadPixelsRenderTarget(GLenum format) const;
+
   private:
     RenderTargetCache<RenderTargetWgpu> mRenderTargetCache;
+    wgpu::RenderPassDescriptor mCurrentRenderPassDesc;
+    std::vector<wgpu::RenderPassColorAttachment> mCurrentColorAttachments;
 };
 
 }  // namespace rx
diff --git a/src/libANGLE/renderer/wgpu/TextureWgpu.cpp b/src/libANGLE/renderer/wgpu/TextureWgpu.cpp
index 98a524fbb8..027c3a8053 100644
--- a/src/libANGLE/renderer/wgpu/TextureWgpu.cpp
+++ b/src/libANGLE/renderer/wgpu/TextureWgpu.cpp
@@ -416,7 +416,7 @@ angle::Result TextureWgpu::initializeImage(ContextWgpu *contextWgpu, ImageMipLev
         displayWgpu->getDevice(), firstLevel,
         mImage->createTextureDescriptor(textureUsage, textureDimension,
                                         gl_wgpu::getExtent3D(firstLevelExtents),
-                                        wgpu::TextureFormat::RGBA8Uint, levelCount, 1, 0));
+                                        wgpu::TextureFormat::RGBA8Unorm, levelCount, 1));
 }
 
 angle::Result TextureWgpu::redefineLevel(const gl::Context *context,
diff --git a/src/libANGLE/renderer/wgpu/wgpu_helpers.cpp b/src/libANGLE/renderer/wgpu/wgpu_helpers.cpp
index 364c7d46e9..ebe8bbfba1 100644
--- a/src/libANGLE/renderer/wgpu/wgpu_helpers.cpp
+++ b/src/libANGLE/renderer/wgpu/wgpu_helpers.cpp
@@ -15,7 +15,11 @@ namespace rx
 {
 namespace webgpu
 {
-ImageHelper::ImageHelper() {}
+ImageHelper::ImageHelper()
+{
+    // TODO: support more TextureFormats.
+    mViewFormats.push_back(wgpu::TextureFormat::RGBA8Unorm);
+}
 
 ImageHelper::~ImageHelper() {}
 
@@ -33,6 +37,10 @@ angle::Result ImageHelper::initImage(wgpu::Device &device,
 
 void ImageHelper::flushStagedUpdates(ContextWgpu *contextWgpu)
 {
+    if (mBufferQueue.empty())
+    {
+        return;
+    }
     wgpu::Device device          = contextWgpu->getDevice();
     wgpu::Queue queue            = contextWgpu->getQueue();
     wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
@@ -51,6 +59,7 @@ void ImageHelper::flushStagedUpdates(ContextWgpu *contextWgpu)
     }
     wgpu::CommandBuffer commandBuffer = encoder.Finish();
     queue.Submit(1, &commandBuffer);
+    encoder = nullptr;
     mBufferQueue.clear();
 }
 
@@ -59,8 +68,7 @@ wgpu::TextureDescriptor ImageHelper::createTextureDescriptor(wgpu::TextureUsage
                                                              wgpu::Extent3D size,
                                                              wgpu::TextureFormat format,
                                                              std::uint32_t mipLevelCount,
-                                                             std::uint32_t sampleCount,
-                                                             std::size_t viewFormatCount)
+                                                             std::uint32_t sampleCount)
 {
     wgpu::TextureDescriptor textureDescriptor = {};
     textureDescriptor.usage                   = usage;
@@ -69,7 +77,8 @@ wgpu::TextureDescriptor ImageHelper::createTextureDescriptor(wgpu::TextureUsage
     textureDescriptor.format                  = format;
     textureDescriptor.mipLevelCount           = mipLevelCount;
     textureDescriptor.sampleCount             = sampleCount;
-    textureDescriptor.viewFormatCount         = viewFormatCount;
+    textureDescriptor.viewFormatCount         = mViewFormats.size();
+    textureDescriptor.viewFormats = reinterpret_cast<wgpu::TextureFormat *>(mViewFormats.data());
     return textureDescriptor;
 }
 
diff --git a/src/libANGLE/renderer/wgpu/wgpu_helpers.h b/src/libANGLE/renderer/wgpu/wgpu_helpers.h
index 6b042c9962..ce0e470189 100644
--- a/src/libANGLE/renderer/wgpu/wgpu_helpers.h
+++ b/src/libANGLE/renderer/wgpu/wgpu_helpers.h
@@ -49,8 +49,7 @@ class ImageHelper
                                                     wgpu::Extent3D size,
                                                     wgpu::TextureFormat format,
                                                     std::uint32_t mipLevelCount,
-                                                    std::uint32_t sampleCount,
-                                                    std::size_t viewFormatCount);
+                                                    std::uint32_t sampleCount);
 
     angle::Result stageTextureUpload(ContextWgpu *contextWgpu,
                                      const gl::Extents &glExtents,
@@ -81,7 +80,8 @@ class ImageHelper
   private:
     wgpu::Texture mTexture;
     wgpu::TextureDescriptor mTextureDescriptor = {};
-    bool mInitialized                          = false;
+    std::vector<wgpu::TextureFormat> mViewFormats;
+    bool mInitialized = false;
 
     gl::LevelIndex mFirstAllocatedLevel = gl::LevelIndex(0);
 
diff --git a/src/tests/gl_tests/ClearTest.cpp b/src/tests/gl_tests/ClearTest.cpp
index dbcb2057c3..9c3dad154c 100644
--- a/src/tests/gl_tests/ClearTest.cpp
+++ b/src/tests/gl_tests/ClearTest.cpp
@@ -473,6 +473,28 @@ TEST_P(ClearTest, RGBA8Framebuffer)
     EXPECT_PIXEL_NEAR(0, 0, 128, 128, 128, 128, 1.0);
 }
 
+// Test uploading a texture and then clearing a RGBA8 Framebuffer
+TEST_P(ClearTest, TextureUploadAndRGBA8Framebuffer)
+{
+    glBindFramebuffer(GL_FRAMEBUFFER, mFBOs[0]);
+
+    GLTexture texture;
+
+    constexpr uint32_t kSize = 16;
+    std::vector<GLColor> pixelData(kSize * kSize, GLColor::blue);
+    glBindTexture(GL_TEXTURE_2D, texture);
+    glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, kSize, kSize, 0, GL_RGBA, GL_UNSIGNED_BYTE,
+                 pixelData.data());
+    glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, texture, 0);
+
+    EXPECT_PIXEL_NEAR(0, 0, 0, 0, 255, 255, 1.0);
+
+    glClearColor(0.5f, 0.5f, 0.5f, 0.5f);
+    glClear(GL_COLOR_BUFFER_BIT);
+
+    EXPECT_PIXEL_NEAR(0, 0, 128, 128, 128, 128, 1.0);
+}
+
 // Test to validate that we can go from an RGBA framebuffer attachment, to an RGB one and still
 // have a correct behavior after.
 TEST_P(ClearTest, ChangeFramebufferAttachmentFromRGBAtoRGB)