diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp index 6445fc8d..24119c9f 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp @@ -7194,6 +7194,93 @@ void main() { return true; } + // A glClear on the DEFAULT framebuffer is parked as a pending clear and folded into the next + // render pass's loadOp. With no draw in between there is no render pass, so a readback that + // followed such a clear blitted the untouched swapchain image and returned the PREVIOUS + // frame's colour - which is exactly what the whole KHR-GL40.draw_indirect.negative-* family + // sees (clear, an erroring draw that never executes, glReadPixels expecting zeroes). + // + // Materializing it means clearing the acquired swapchain image itself, which is why this + // cannot reuse MaterializePendingClearForTexture: the default FBO's colour attachment is a + // placeholder ITextureObject, and syncing it would allocate and clear an unrelated image. + Bool VulkanRenderer::MaterializePendingClearForDefaultFramebuffer(VkCommandBuffer commandBuffer, + MG_State::GLState::FramebufferObject& fbo, + FramebufferAttachmentType attachmentType) { + if (!fbo.IsDefaultFramebuffer() || attachmentType == FramebufferAttachmentType::None) { + return true; + } + const auto& attachment = fbo.GetAttachment(attachmentType); + if (!attachment.IsTexture() || attachment.IsRenderbuffer()) { + return true; + } + ClearAttachmentPayload payload{}; + if (!m_clearManager->GetPendingClear(attachment, payload)) { + return true; + } + if ((payload.mask & GL_COLOR_BUFFER_BIT) == 0) { + // Depth/stencil on the default framebuffer keeps the loadOp route; the readback + // path for it declines default framebuffers outright (ReadDepthStencilPixels). + return true; + } + MOBILEGL_ASSERT(VkRenderPassManager::GetActiveRenderPass() == nullptr || + commandBuffer != m_frameContext.GetCurrent().commandBuffer, + "MaterializePendingClearForDefaultFramebuffer requires no active render pass"); + + const VkImage swapchainImage = m_swapchainObject.GetImage(m_imageIndexAcquired); + if (swapchainImage == VK_NULL_HANDLE) { + return false; + } + VkImageLayout currentLayout = m_swapchainObject.GetImageLayout(m_imageIndexAcquired); + VkPipelineStageFlags srcStageMask = VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT; + VkAccessFlags srcAccessMask = 0; + GetImageTransitionSourceState(currentLayout, srcStageMask, srcAccessMask); + VkImageLayout clearLayout = currentLayout; + if (!VkTextureManager::TransitionImageLayout(commandBuffer, swapchainImage, clearLayout, + VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, srcStageMask, + VK_PIPELINE_STAGE_TRANSFER_BIT, srcAccessMask, + VK_ACCESS_TRANSFER_WRITE_BIT, VK_IMAGE_ASPECT_COLOR_BIT)) { + return false; + } + + // The clear colour goes in verbatim, alpha included. Forcing opaque alpha here is what + // makes a glClear(0,0,0,0) read back as (0,0,0,1) - the default framebuffer's placeholder + // attachment can describe an alpha-less format while the swapchain image it stands for + // has a real alpha channel. + VkClearColorValue clearColor{}; + clearColor.float32[0] = payload.color.x(); + clearColor.float32[1] = payload.color.y(); + clearColor.float32[2] = payload.color.z(); + clearColor.float32[3] = payload.color.w(); + VkImageSubresourceRange range{}; + range.aspectMask = VK_IMAGE_ASPECT_COLOR_BIT; + range.baseMipLevel = 0; + range.levelCount = 1; + range.baseArrayLayer = 0; + range.layerCount = 1; + vkCmdClearColorImage(commandBuffer, swapchainImage, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, &clearColor, 1, + &range); + + VkImageLayout settledLayout = VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL; + VkPipelineStageFlags dstStageMask = VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT; + VkAccessFlags dstAccessMask = 0; + GetImageTransitionDestinationState(VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL, dstStageMask, dstAccessMask); + if (!VkTextureManager::TransitionImageLayout(commandBuffer, swapchainImage, settledLayout, + VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL, + VK_PIPELINE_STAGE_TRANSFER_BIT, dstStageMask, + VK_ACCESS_TRANSFER_WRITE_BIT, dstAccessMask, + VK_IMAGE_ASPECT_COLOR_BIT)) { + return false; + } + m_swapchainObject.SetImageLayout(m_imageIndexAcquired, VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL); + + // Popped, not left behind: the clear has executed, so letting the next render pass load + // it again as a loadOp would erase whatever is drawn between here and there. + m_clearManager->PopPendingClear(attachment); + MGLOG_D("MaterializePendingClearForDefaultFramebuffer: swapchain image %u pending clear materialized", + m_imageIndexAcquired); + return true; + } + Bool VulkanRenderer::TryBlitToDefaultFramebufferWithShader(FrameContext::FrameData& frame, MG_State::GLState::FramebufferObject& readFbo, MG_State::GLState::FramebufferObject& drawFbo, @@ -8294,7 +8381,18 @@ void main() { // rehash on that insertion, invalidating any RenderbufferResource*/TextureResource* // obtained beforehand - so ResolveColorBlitBinding's cached `trackedLayout` pointer // must be taken AFTER this, never before it. - if (!readIsDefaultFbo) { + // + // The default framebuffer needs this just as much, and used to be excluded: its clear is + // parked the same way, and with no draw between the clear and the readback no render + // pass ever runs to fold it in, so the readback returned the previous frame's image + // (KHR-GL40.draw_indirect.negative-*). It only takes a different materializer because the + // image to clear is the acquired swapchain image, not the attachment's placeholder + // texture. + if (readIsDefaultFbo) { + const Bool clearReady = MaterializePendingClearForDefaultFramebuffer(frame.commandBuffer, *readFbo, + readFbo->GetReadBuffer()); + MOBILEGL_ASSERT(clearReady, "ReadPixels: failed to materialize the default framebuffer's pending clear"); + } else { const auto& sourceAttachment = readFbo->GetAttachment(readFbo->GetReadBuffer()); auto sourceTexture = sourceAttachment.GetTexture(); if (sourceTexture != nullptr) { diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.h index 2f4710eb..17daf23c 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.h @@ -1118,6 +1118,13 @@ namespace MobileGL::MG_Backend::DirectVulkan { Bool MaterializePendingClearForRenderbuffer( VkCommandBuffer commandBuffer, const SharedPtr& renderbuffer); + // The default framebuffer's twin of the two above. It cannot go through + // MaterializePendingClearForTexture: the default FBO's colour attachment is a + // placeholder texture object, and syncing THAT would clear a texture image nobody + // presents instead of the acquired swapchain image. + Bool MaterializePendingClearForDefaultFramebuffer(VkCommandBuffer commandBuffer, + MG_State::GLState::FramebufferObject& fbo, + FramebufferAttachmentType attachmentType); VkPipeline GetOrCreateBlitPipeline(const RenderPassEntry& renderPassEntry); Bool GenerateDepthMipmapWithShader(FrameContext::FrameData& frame, MG_State::GLState::ITextureObject& texture, diff --git a/MobileGL/MG_IntegrationTest/CMakeLists.txt b/MobileGL/MG_IntegrationTest/CMakeLists.txt index 77f82712..f621b3e0 100644 --- a/MobileGL/MG_IntegrationTest/CMakeLists.txt +++ b/MobileGL/MG_IntegrationTest/CMakeLists.txt @@ -57,6 +57,7 @@ add_executable(MobileGLIntegrationTest Scenarios/AdvertisedLimitsScenario.cpp Scenarios/PixelStoreSweepScenario.cpp Scenarios/FragCoordOriginScenario.cpp + Scenarios/ClearThenReadPixelsScenario.cpp ) target_include_directories(MobileGLIntegrationTest PRIVATE diff --git a/MobileGL/MG_IntegrationTest/Scenarios/ClearThenReadPixelsScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/ClearThenReadPixelsScenario.cpp new file mode 100644 index 00000000..230fa553 --- /dev/null +++ b/MobileGL/MG_IntegrationTest/Scenarios/ClearThenReadPixelsScenario.cpp @@ -0,0 +1,183 @@ +// MobileGL - MobileGL/MG_IntegrationTest/Scenarios/ClearThenReadPixelsScenario.cpp +// Copyright (c) 2025-2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header +// +// Scenario - A CLEAR OF THE DEFAULT FRAMEBUFFER IS VISIBLE TO glReadPixels WITH NO DRAW BETWEEN. +// +// DirectVulkan parks a glClear as a pending clear and folds it into the next render pass's +// loadOp. When nothing is drawn after the clear there is no render pass, and the readback path +// used to materialize pending clears only for USER framebuffers - so a readback right after a +// clear of the DEFAULT framebuffer blitted the untouched swapchain image and handed back the +// previous frame's colour. +// +// That is the whole of KHR-GL40.draw_indirect.negative-* (12 Magma failures): each case clears, +// issues a draw that correctly raises INVALID_OPERATION and therefore never executes, then reads +// the frame back expecting (0,0,0,0) and gets the previous case's (0.1,0.2,0.3,1). The staleness +// cannot appear in one frame, so the scenario paints a frame first and clears in the next. +// +// The alpha assertion is the second half of the same census finding: a cleared default +// framebuffer read back (0,0,0,1) where (0,0,0,0) was written, because the clear was routed +// through the default FBO's placeholder attachment, whose format can lack alpha, rather than +// through the swapchain image that actually has one. +// +// DirectGLES is the built-in control: a native GL driver has no deferred-clear model at all, so +// a failure there would mean the scenario, not the backend. + +#include +#include + +#include "../Harness/HeadlessGL.h" +#include "../Harness/ScenarioFixture.h" + +#ifdef GLAPI +#undef GLAPI +#endif +#define GL_GLEXT_PROTOTYPES +#include +#include +#undef GL_GLEXT_PROTOTYPES + +namespace MGITest { + namespace { + + constexpr const char* kVS = R"(#version 330 core +in vec2 aPos; +void main() { gl_Position = vec4(aPos, 0.0, 1.0); } +)"; + + // The colour KHR-GL40.draw_indirect's fshSimple paints, so a stale readback shows up as + // the same value the conformance log reports. + constexpr const char* kFS = R"(#version 330 core +out vec4 o_color; +void main() { o_color = vec4(0.1, 0.2, 0.3, 1.0); } +)"; + + class ClearThenReadPixelsScenario : public ScenarioTest {}; + + void DrawFullViewportQuad(unsigned int program) { + static const float kQuad[] = {-1.0f, -1.0f, 1.0f, -1.0f, -1.0f, 1.0f, 1.0f, 1.0f}; + GLuint vao = 0, vbo = 0; + glGenVertexArrays(1, &vao); + glBindVertexArray(vao); + glGenBuffers(1, &vbo); + glBindBuffer(GL_ARRAY_BUFFER, vbo); + glBufferData(GL_ARRAY_BUFFER, sizeof(kQuad), kQuad, GL_STATIC_DRAW); + glEnableVertexAttribArray(0); + glVertexAttribPointer(0, 2, GL_FLOAT, GL_FALSE, 2 * sizeof(float), nullptr); + glUseProgram(program); + glDrawArrays(GL_TRIANGLE_STRIP, 0, 4); + glBindVertexArray(0); + glDeleteBuffers(1, &vbo); + glDeleteVertexArrays(1, &vao); + } + + } // namespace + + TEST_F(ClearThenReadPixelsScenario, ClearWithNoDrawIsVisibleToDefaultFramebufferReadPixels) { + if (!Ready()) return; + HeadlessGL& gl = Gl(); + const int width = gl.Width(); + const int height = gl.Height(); + ASSERT_GE(width, 8); + ASSERT_GE(height, 8); + + std::string error; + const unsigned int program = CompileProgram(kVS, kFS, &error); + ASSERT_NE(program, 0u) << error; + + // Frame 1: paint the whole default framebuffer, so there IS something stale to return. + BindDefaultFramebuffer(); + glViewport(0, 0, width, height); + glDisable(GL_SCISSOR_TEST); + glDisable(GL_DEPTH_TEST); + ClearTo(1.0f, 1.0f, 1.0f, 1.0f); + DrawFullViewportQuad(program); + { + const Image painted = ReadPixels(width, height); + const Rgba8 centre = painted.At(width / 2, height / 2); + ASSERT_NEAR(centre.r, 26, 2) << "the setup frame did not paint; the staleness test would be vacuous"; + ASSERT_NEAR(centre.g, 51, 2); + ASSERT_NEAR(centre.b, 77, 2); + } + gl.EndFrame(); + + // Frame 2: clear to transparent black and read back with NO draw at all. + BindDefaultFramebuffer(); + glViewport(0, 0, width, height); + ClearTo(0.0f, 0.0f, 0.0f, 0.0f); + const Image cleared = ReadPixels(width, height); + EXPECT_EQ(FirstGLError(), 0u); + + int nonZero = 0; + int firstX = -1; + int firstY = -1; + Rgba8 firstOffender{}; + for (int y = 0; y < height; ++y) { + for (int x = 0; x < width; ++x) { + const Rgba8 pixel = cleared.At(x, y); + if (pixel.r == 0 && pixel.g == 0 && pixel.b == 0 && pixel.a == 0) continue; + if (nonZero == 0) { + firstX = x; + firstY = y; + firstOffender = pixel; + } + ++nonZero; + } + } + EXPECT_EQ(nonZero, 0) << "glClear(0,0,0,0) followed by glReadPixels with no draw returned " << nonZero + << " of " << (width * height) << " non-zero pixels; first at (" << firstX << ", " + << firstY << ") = (" << static_cast(firstOffender.r) << ", " + << static_cast(firstOffender.g) << ", " << static_cast(firstOffender.b) + << ", " << static_cast(firstOffender.a) << ")"; + + gl.EndFrame(); + glDeleteProgram(program); + } + + // The same claim for a sub-rect read, which is the shape the conformance suite uses most and + // the one whose orientation handling is separate (see OrientationScenario). + TEST_F(ClearThenReadPixelsScenario, ClearWithNoDrawIsVisibleToASubRectReadback) { + if (!Ready()) return; + HeadlessGL& gl = Gl(); + const int width = gl.Width(); + const int height = gl.Height(); + ASSERT_GE(width, 8); + ASSERT_GE(height, 8); + + std::string error; + const unsigned int program = CompileProgram(kVS, kFS, &error); + ASSERT_NE(program, 0u) << error; + + BindDefaultFramebuffer(); + glViewport(0, 0, width, height); + glDisable(GL_SCISSOR_TEST); + glDisable(GL_DEPTH_TEST); + DrawFullViewportQuad(program); + gl.EndFrame(); + + BindDefaultFramebuffer(); + glViewport(0, 0, width, height); + ClearTo(0.0f, 0.0f, 0.0f, 0.0f); + const int rectWidth = width / 2; + const int rectHeight = height / 2; + const Image cleared = ReadPixelsRect(width / 4, height / 4, rectWidth, rectHeight); + EXPECT_EQ(FirstGLError(), 0u); + + int nonZero = 0; + for (int y = 0; y < rectHeight; ++y) { + for (int x = 0; x < rectWidth; ++x) { + const Rgba8 pixel = cleared.At(x, y); + if (pixel.r != 0 || pixel.g != 0 || pixel.b != 0 || pixel.a != 0) ++nonZero; + } + } + EXPECT_EQ(nonZero, 0) << nonZero << " of " << (rectWidth * rectHeight) + << " pixels in a sub-rect read after a draw-free clear were not zero"; + + gl.EndFrame(); + glDeleteProgram(program); + } +} // namespace MGITest diff --git a/MobileGL/MG_Test/State/NegativeApiErrorsTest.cpp b/MobileGL/MG_Test/State/NegativeApiErrorsTest.cpp index 151e61d9..267808e7 100644 --- a/MobileGL/MG_Test/State/NegativeApiErrorsTest.cpp +++ b/MobileGL/MG_Test/State/NegativeApiErrorsTest.cpp @@ -38,6 +38,7 @@ #include #include #include +#include #include using namespace MobileGL;