From a6c362c6ceeaead5656da33ac84810a01b924348 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Tue, 11 Aug 2026 02:51:22 -0400 Subject: [PATCH] [Fix, Test] (MG_Backend/DirectVulkan): convert every default-framebuffer rectangle between GL and display Y origins - viewport, scissor, ReadPixels offset, rect-capable readback remap, blit source --- .../DirectVulkan/Renderer/VulkanRenderer.cpp | 216 ++++++++++++++---- .../MG_IntegrationTest/Harness/HeadlessGL.cpp | 6 +- .../MG_IntegrationTest/Harness/HeadlessGL.h | 13 +- .../Scenarios/OrientationScenario.cpp | 203 ++++++++++++++++ 4 files changed, 389 insertions(+), 49 deletions(-) diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp index 6bbbd1ed..d0c0a74e 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp @@ -220,6 +220,47 @@ namespace MobileGL::MG_Backend::DirectVulkan { return static_cast((static_cast(value) * toExtent + fromExtent / 2) / fromExtent); } + // --------------------------------------------------------------------------------------- + // Default-framebuffer rectangles. + // + // GL's window origin is the BOTTOM-left. The default framebuffer's Vulkan image is stored in + // DISPLAY (top-left) orientation, and the difference is reconciled for VERTICES by negating + // gl_Position.y - but only for default-FBO draws (GetShaderTransformFlags -> + // CompileOptionBit::PositionYFlip, applied in ProgramFactory::InsertPositionFixup). + // + // Rectangles were never converted. The viewport, the scissor and the ReadPixels copy offset + // all used the GL bottom-origin Y verbatim as a Vulkan top-origin Y, which is correct only + // when y == H - y - h (full height, or vertically centred) - and full height is the only case + // any test ever exercised. In the conformance suite the errors CANCEL in placement (the draw + // lands in Vulkan rows [y, y+h) and the readback copies the same rows back) and compose into + // an exact vertical flip: 1,759 of Magma's 1,793 non-passing cases, 861 vertical flips and + // nothing else across all of gl33. + // + // The mapping below is derived from - and at full extent exactly reproduces - the pixel + // mapping RemapDefaultFboReadbackToGLOrientation has always used: + // identity : image(x, H-1-y) -> flip Y + // 180 : image(W-1-x, y) -> mirror X (the rotation already flips the rows) + // Quarter turns swap the axes; nothing in this renderer models that (the readback declines to + // remap them and the viewport path only rescales), so they are left exactly as they were. + struct DefaultFramebufferRectMapping { + Bool flipY = false; + Bool mirrorX = false; + }; + + static DefaultFramebufferRectMapping GetDefaultFramebufferRectMapping( + VkSurfaceTransformFlagBitsKHR preTransform) { + if (preTransform == VK_SURFACE_TRANSFORM_ROTATE_180_BIT_KHR) return {false, true}; + if (IsQuarterTurnPreTransform(preTransform)) return {false, false}; + return {true, false}; + } + + // [origin, origin+size) counted from one end is [extent-origin-size, extent-origin) counted + // from the other. A full-extent rect is a fixed point, which is why this can be introduced + // without moving anything that works today. + static Int MapDefaultFramebufferRectAxis(Int origin, Int size, Int extent, Bool invert) { + return invert ? extent - origin - size : origin; + } + // Redundant dynamic-state elimination for the per-draw hot path: within one // command-buffer recording, a vkCmdSet* whose values already match what the // command buffer holds is skipped. Valid because every PipelineFactory @@ -417,6 +458,17 @@ namespace MobileGL::MG_Backend::DirectVulkan { viewportHeight = ScaleFramebufferCoordinate(viewportHeight, logicalExtent.y(), framebufferExtent.y()); } + // The GL viewport rect, expressed against the default framebuffer's stored orientation. + // A full-height viewport is unchanged by this, which is why every existing scenario keeps + // its exact behaviour. + if (isDefaultFramebuffer) { + const DefaultFramebufferRectMapping mapping = GetDefaultFramebufferRectMapping(preTransform); + viewportX = MapDefaultFramebufferRectAxis(viewportX, viewportWidth, framebufferExtent.x(), + mapping.mirrorX); + viewportY = MapDefaultFramebufferRectAxis(viewportY, viewportHeight, framebufferExtent.y(), + mapping.flipY); + } + VkViewport viewport{}; viewport.x = static_cast(viewportX); viewport.y = static_cast(viewportY); @@ -518,11 +570,25 @@ namespace MobileGL::MG_Backend::DirectVulkan { return scissor; } + // The clamped rect, re-expressed against the default framebuffer's stored orientation. Same + // conversion as the viewport - and it must be the same one, or the scissor would cut a band + // the draw never touched. + static VkRect2D MapScissorRectToDefaultFramebuffer(VkRect2D scissor, const IntVec2& framebufferExtent, + VkSurfaceTransformFlagBitsKHR preTransform) { + const DefaultFramebufferRectMapping mapping = GetDefaultFramebufferRectMapping(preTransform); + scissor.offset.x = MapDefaultFramebufferRectAxis(scissor.offset.x, static_cast(scissor.extent.width), + framebufferExtent.x(), mapping.mirrorX); + scissor.offset.y = MapDefaultFramebufferRectAxis(scissor.offset.y, static_cast(scissor.extent.height), + framebufferExtent.y(), mapping.flipY); + return scissor; + } + static VkRect2D MakeDefaultFramebufferScissorRect(const IntVec4& scissorBox, const IntVec2& framebufferExtent, VkSurfaceTransformFlagBitsKHR preTransform) { if (!IsQuarterTurnPreTransform(preTransform)) { - return MakeClampedScissorRect(scissorBox, framebufferExtent); + return MapScissorRectToDefaultFramebuffer(MakeClampedScissorRect(scissorBox, framebufferExtent), + framebufferExtent, preTransform); } const IntVec2 logicalExtent = ResolveDefaultFramebufferLogicalExtent(preTransform, framebufferExtent); @@ -542,7 +608,9 @@ namespace MobileGL::MG_Backend::DirectVulkan { static_cast(std::max(0, rawX1 - rawX0)), static_cast(std::max(0, rawY1 - rawY0)), }; - return scissor; + // A quarter turn maps to {false, false}, so this is a no-op today; it is here so the + // branch cannot drift away from the identity/180 one when quarter turns are modelled. + return MapScissorRectToDefaultFramebuffer(scissor, framebufferExtent, preTransform); } static void ApplyStencilState(VkCommandBuffer commandBuffer) { @@ -1928,6 +1996,29 @@ void main() { } } + // The same conversion on the READ side, which never had one: a blit whose source is the + // default framebuffer used raw GL offsets against a display-oriented image, so it sampled + // the mirrored band and wrote it upside down. Mapping BOTH endpoints inverts the offset + // pair, and an inverted pair is exactly how VkImageBlit spells "flip this axis" - so the + // band and the row order are corrected in one step. A full-extent blit is unchanged in + // band and gains the row flip it always needed. + static void ApplyNativeBlitDefaultFramebufferSourceTransform(VkSurfaceTransformFlagBitsKHR preTransform, + const BlitImageBinding& srcBinding, + VkImageBlit& blitRegion) { + switch (preTransform) { + case VK_SURFACE_TRANSFORM_IDENTITY_BIT_KHR: + blitRegion.srcOffsets[0].y = srcBinding.extent.y() - blitRegion.srcOffsets[0].y; + blitRegion.srcOffsets[1].y = srcBinding.extent.y() - blitRegion.srcOffsets[1].y; + break; + case VK_SURFACE_TRANSFORM_ROTATE_180_BIT_KHR: + blitRegion.srcOffsets[0].x = srcBinding.extent.x() - blitRegion.srcOffsets[0].x; + blitRegion.srcOffsets[1].x = srcBinding.extent.x() - blitRegion.srcOffsets[1].x; + break; + default: + break; + } + } + static Bool DecodeReadbackPixel(const Uint8* source, VkFormat sourceFormat, Float* rgba) { switch (sourceFormat) { case VK_FORMAT_R8G8B8A8_UNORM: @@ -2033,42 +2124,42 @@ void main() { return static_cast(value * 255.0f + 0.5f); } - // Remap raw swapchain pixels (top-left origin, preTransform-rotated) into - // GL-oriented pixels (bottom-left origin) for the retrace snapshot path. - // Mirrors the removed GetPresentedDumpPixel mapping plus the Y-origin flip - // apitrace's flipped=true Image expects. Only identity/180 share the - // swapchain extent with the default framebuffer; 90/270 swap extents and - // are not handled here. + // Re-order the copied BLOCK - not the whole image - from the default framebuffer's stored + // orientation into GL's. The caller has already aimed the copy at the right place with + // MapDefaultFramebufferRectAxis, so what arrives here is exactly the requested + // rectWidth x rectHeight rect, and all that is left is the order of rows (identity) or of + // columns (180) WITHIN it. + // + // This used to iterate the full swapchain extent and index both sides with that stride, + // which is why its caller could only use it on an exact full-extent read - and why every + // partial glReadPixels of the default framebuffer came back in Vulkan row order. Only + // identity/180 share the swapchain extent with the default framebuffer; 90/270 swap + // extents and are still declined. static Bool RemapDefaultFboReadbackToGLOrientation(const Uint8* rawPixels, - VkExtent2D rawExtent, + Uint32 rectWidth, + Uint32 rectHeight, VkSurfaceTransformFlagBitsKHR preTransform, SizeT texelSize, Uint8* outPixels) { if (IsQuarterTurnPreTransform(preTransform)) { return false; } - const Uint32 w = rawExtent.width; - const Uint32 h = rawExtent.height; - if (w == 0 || h == 0) { + if (rectWidth == 0 || rectHeight == 0 || texelSize == 0) { return false; } - for (Uint32 outY = 0; outY < h; ++outY) { - const Uint32 displayY = h - 1 - outY; // GL bottom-origin -> display top-origin - for (Uint32 outX = 0; outX < w; ++outX) { - const Uint32 displayX = outX; - Uint32 rawX = displayX; - Uint32 rawY = displayY; - switch (preTransform) { - case VK_SURFACE_TRANSFORM_ROTATE_180_BIT_KHR: - rawX = w - 1 - displayX; - rawY = h - 1 - displayY; - break; - default: - break; - } - const Uint8* src = rawPixels + (static_cast(rawY) * w + rawX) * texelSize; - Uint8* dst = outPixels + (static_cast(outY) * w + outX) * texelSize; - Memcpy(dst, src, texelSize); + const DefaultFramebufferRectMapping mapping = GetDefaultFramebufferRectMapping(preTransform); + const SizeT rowBytes = static_cast(rectWidth) * texelSize; + for (Uint32 outY = 0; outY < rectHeight; ++outY) { + const Uint32 srcY = mapping.flipY ? (rectHeight - 1 - outY) : outY; + const Uint8* srcRow = rawPixels + static_cast(srcY) * rowBytes; + Uint8* dstRow = outPixels + static_cast(outY) * rowBytes; + if (!mapping.mirrorX) { + Memcpy(dstRow, srcRow, rowBytes); + continue; + } + for (Uint32 outX = 0; outX < rectWidth; ++outX) { + Memcpy(dstRow + static_cast(outX) * texelSize, + srcRow + static_cast(rectWidth - 1 - outX) * texelSize, texelSize); } } return true; @@ -7680,11 +7771,19 @@ void main() { blitRegion.dstSubresource.layerCount = dstBinding.layerCount; blitRegion.dstOffsets[0] = {dstX0, dstY0, 0}; blitRegion.dstOffsets[1] = {dstX1, dstY1, 1}; + if (readIsDefaultFbo) { + ApplyNativeBlitDefaultFramebufferSourceTransform(m_swapchainObject.GetPreTransform(), srcBinding, + blitRegion); + } if (drawIsDefaultFbo) { ApplyNativeBlitDefaultFramebufferTransform(m_swapchainObject.GetPreTransform(), dstBinding, blitRegion); } if (srcBinding.sampleCount != VK_SAMPLE_COUNT_1_BIT && dstBinding.sampleCount == VK_SAMPLE_COUNT_1_BIT) { + // NOTE: vkCmdResolveImage cannot flip, and this region is still built from the raw GL + // offsets. A multisample-resolve blit whose source or destination is the default + // framebuffer therefore keeps the pre-fix behaviour; it needs a resolve-then-blit + // (or blit-then-resolve) split, which is its own change. // GL multisample resolve blits are 1:1 by spec; vkCmdBlitImage cannot read a // multisampled source. VkImageResolve resolveRegion{}; @@ -7888,6 +7987,19 @@ void main() { copyRegion.srcSubresource.mipLevel = srcBinding.mipLevel; copyRegion.srcSubresource.baseArrayLayer = srcBinding.baseArrayLayer; copyRegion.srcSubresource.layerCount = srcBinding.layerCount; + // KNOWN GAP, deliberately not half-fixed here: when the read framebuffer is the default + // one this samples GL rows [y, y+h) counted from the TOP of a display-oriented image, so + // it takes the mirrored band AND writes it into the (GL-oriented) destination texture + // upside down. Correcting only the offset would swap one wrong answer for another, + // because vkCmdCopyImage cannot reverse rows: this path has to become a vkCmdBlitImage + // with an inverted source Y pair, the way BlitFramebuffer above now does it. Tracked + // separately; the four sites behind the 1,759-case orientation defect are the viewport, + // the scissor, the ReadPixels copy offset and the readback remap. + if (readIsDefaultFbo) { + MGLOG_I("DirectVulkan::CopyTexSubImage2D: copying from the DEFAULT framebuffer still uses the raw GL " + "Y origin (x=%d y=%d w=%d h=%d); the result is the mirrored band, stored flipped", + x, y, width, height); + } copyRegion.srcOffset = {x, y, 0}; copyRegion.dstSubresource.aspectMask = dstBinding.aspectMask; copyRegion.dstSubresource.mipLevel = dstBinding.mipLevel; @@ -8248,7 +8360,21 @@ void main() { copyRegion.imageSubresource.mipLevel = srcBinding.mipLevel; copyRegion.imageSubresource.baseArrayLayer = srcBinding.baseArrayLayer; copyRegion.imageSubresource.layerCount = 1; - copyRegion.imageOffset = {x, y, static_cast(srcBinding.depthOffset)}; + // The GL rect, aimed at the default framebuffer's stored orientation. Using the GL y + // verbatim copied rows [y, y+h) counted from the TOP of the image, i.e. the wrong band for + // every read that was not full-height. + Int32 copyOffsetX = x; + Int32 copyOffsetY = y; + if (readIsDefaultFbo) { + const VkExtent2D defaultFboExtent = m_swapchainObject.GetExtent(); + const DefaultFramebufferRectMapping mapping = + GetDefaultFramebufferRectMapping(m_swapchainObject.GetPreTransform()); + copyOffsetX = MapDefaultFramebufferRectAxis(x, width, static_cast(defaultFboExtent.width), + mapping.mirrorX); + copyOffsetY = MapDefaultFramebufferRectAxis(y, height, static_cast(defaultFboExtent.height), + mapping.flipY); + } + copyRegion.imageOffset = {copyOffsetX, copyOffsetY, static_cast(srcBinding.depthOffset)}; copyRegion.imageExtent = {static_cast(width), static_cast(height), 1}; vkCmdCopyImageToBuffer(frame.commandBuffer, srcBinding.image, VK_IMAGE_LAYOUT_TRANSFER_SRC_OPTIMAL, readback.GetHandle(), 1, ©Region); @@ -8286,23 +8412,23 @@ void main() { return; } if (readIsDefaultFbo) { - const VkExtent2D swapchainExtent = m_swapchainObject.GetExtent(); const VkSurfaceTransformFlagBitsKHR preTransform = m_swapchainObject.GetPreTransform(); - if (static_cast(width) == swapchainExtent.width && - static_cast(height) == swapchainExtent.height) { - Vector remapped(static_cast(width) * static_cast(height) * sourceTexelSize); - if (RemapDefaultFboReadbackToGLOrientation(mapped, swapchainExtent, preTransform, - sourceTexelSize, - remapped.data())) { - PackReadbackToClientOrPbo(remapped.data(), srcFormat, width, height, 1, format, type, pixels, - /*applyPackImageParams=*/false, /*applyReadColorClamp=*/true); - return; - } + // No full-extent gate any more: the remap works on the copied rect, and the copy was + // already aimed with the same mapping. The gate is exactly what made every partial + // read of the default framebuffer come back in Vulkan row order. + Vector remapped(static_cast(width) * static_cast(height) * sourceTexelSize); + if (RemapDefaultFboReadbackToGLOrientation(mapped, static_cast(width), + static_cast(height), preTransform, sourceTexelSize, + remapped.data())) { + PackReadbackToClientOrPbo(remapped.data(), srcFormat, width, height, 1, format, type, pixels, + /*applyPackImageParams=*/false, /*applyReadColorClamp=*/true); + return; } - MGLOG_W("DirectVulkan::ReadPixels: default-FBO remap skipped (w=%d h=%d swapchain=%ux%u preTransform=%d); " - "falling back to raw readback", - width, height, swapchainExtent.width, swapchainExtent.height, - static_cast(preTransform)); + // Only a quarter-turn pre-transform reaches this, and nothing in this renderer models + // one. MGLOG_I because the INFO builds are the ones that run conformance. + MGLOG_I("DirectVulkan::ReadPixels: default-FBO remap declined (w=%d h=%d preTransform=%d); falling back " + "to raw readback", + width, height, static_cast(preTransform)); } PackReadbackToClientOrPbo(mapped, srcFormat, width, height, 1, format, type, pixels, /*applyPackImageParams=*/false, /*applyReadColorClamp=*/true); diff --git a/MobileGL/MG_IntegrationTest/Harness/HeadlessGL.cpp b/MobileGL/MG_IntegrationTest/Harness/HeadlessGL.cpp index 3f37b9b3..7526028f 100644 --- a/MobileGL/MG_IntegrationTest/Harness/HeadlessGL.cpp +++ b/MobileGL/MG_IntegrationTest/Harness/HeadlessGL.cpp @@ -601,9 +601,13 @@ namespace MGITest { } Image ReadPixels(int width, int height) { + return ReadPixelsRect(0, 0, width, height); + } + + Image ReadPixelsRect(int x, int y, int width, int height) { Image image(width, height); glPixelStorei(GL_PACK_ALIGNMENT, 1); - glReadPixels(0, 0, width, height, GL_RGBA, GL_UNSIGNED_BYTE, image.Data()); + glReadPixels(x, y, width, height, GL_RGBA, GL_UNSIGNED_BYTE, image.Data()); return image; } diff --git a/MobileGL/MG_IntegrationTest/Harness/HeadlessGL.h b/MobileGL/MG_IntegrationTest/Harness/HeadlessGL.h index 355b453b..e3a36c3f 100644 --- a/MobileGL/MG_IntegrationTest/Harness/HeadlessGL.h +++ b/MobileGL/MG_IntegrationTest/Harness/HeadlessGL.h @@ -184,11 +184,18 @@ namespace MGITest { void ClearTo(float r, float g, float b, float a); - // Reads back the whole currently bound READ framebuffer. width/height must - // be the target's full size - DirectVulkan's default-framebuffer readback - // only re-orients a full-extent read. + // Reads back the whole currently bound READ framebuffer. Image ReadPixels(int width, int height); + // A PARTIAL glReadPixels. Row 0 of the returned image is GL row `y` of the + // framebuffer, i.e. the bottom row of the requested rect - the same + // convention ReadPixels uses, just with an origin. This is the shape the + // conformance suite reads in (a random sub-rect of the default + // framebuffer), and the shape DirectVulkan's default-FBO readback used to + // hand back in Vulkan row order because its re-orientation only ran on an + // exact full-extent read. + Image ReadPixelsRect(int x, int y, int width, int height); + // Drains any GL error queue and returns the first error, or 0. unsigned int FirstGLError(); const char* GLErrorName(unsigned int error); diff --git a/MobileGL/MG_IntegrationTest/Scenarios/OrientationScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/OrientationScenario.cpp index 5181b9d1..cb5b0c3a 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/OrientationScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/OrientationScenario.cpp @@ -55,6 +55,7 @@ #include #include +#include #include #include @@ -101,6 +102,39 @@ void main() { // "every single pixel" an achievable (and therefore useful) demand. constexpr int kQuadrantInset = 2; + // A deliberately asymmetric sub-rect of the 128x96 surface: neither centred nor + // full-extent in either axis, mirroring the conformance suite's randomised + // sub-viewport geometry (glcShaderRenderCase.cpp:735-741). Asymmetry is the whole + // point - y == H - y - h is exactly the case an unconverted Y origin gets right by + // accident, and it is the only case the shipped code ever exercised. + // correct band = GL rows [13, 55) + // mirrored band = GL rows [41, 83) (what H-y-h produces) + constexpr int kSubX = 17; + constexpr int kSubY = 13; + constexpr int kSubW = 60; + constexpr int kSubH = 42; + + Image CropRect(const Image& source, int x0, int y0, int width, int height) { + Image out(width, height); + const std::size_t rowBytes = static_cast(width) * 4; + for (int y = 0; y < height; ++y) { + const std::uint8_t* sourceRow = + source.Data() + (static_cast(y0 + y) * source.Width() + x0) * 4; + std::memcpy(out.Data() + static_cast(y) * rowBytes, sourceRow, rowBytes); + } + return out; + } + + Image VFlip(const Image& source) { + Image out(source.Width(), source.Height()); + const std::size_t rowBytes = static_cast(source.Width()) * 4; + for (int y = 0; y < source.Height(); ++y) { + std::memcpy(out.Data() + static_cast(y) * rowBytes, + source.Data() + static_cast(source.Height() - 1 - y) * rowBytes, rowBytes); + } + return out; + } + struct Vertex { float x, y; float r, g, b; @@ -377,5 +411,174 @@ void main() { } } + // ------------------------------------------------------------------ sub-rect / M-1 ---- + // + // Everything above reads the FULL extent of its target, which is the one case + // DirectVulkan's default-framebuffer readback ever re-oriented: the remap at + // VulkanRenderer.cpp:2042 had no rect parameters at all, so :8278 gated it on + // `width == swapchainExtent.width && height == swapchainExtent.height` and fell back to a + // raw copy otherwise. Meanwhile the viewport (:422), the scissor (:506-546) and the + // ReadPixels copy offset (:8238) all used the GL bottom-origin Y verbatim as a Vulkan + // top-origin Y. + // + // In the conformance suite those defects CANCEL in placement - the draw lands in Vulkan + // rows [y, y+h) and the readback copies the same rows back - and compose into an exact + // vertical flip of a correct image. That is 1,759 of Magma's 1,793 non-pass cases, and + // image forensics over all 861 gl33 failures found 861 vertical flips and nothing else. + // Taken apart, they are two independent user-visible bugs, so they are tested apart: + // SubViewportDraw pins placement with a full-extent read, SubRectReadback pins the + // readback rect after a full-viewport draw, and SubViewportSubRectRoundTrip is the CTS + // shape where the two cancel. + + // Placement: a sub-viewport draw must land in GL rows [y0, y0+h), not mirrored about the + // surface centre. Read back full-extent, which is the path that already worked, so a + // failure here can only be the viewport's Y origin. + TEST_F(OrientationScenario, SubViewportDrawLandsWhereGLPutsIt) { + BindDefaultFramebuffer(); + ClearTo(0.0f, 0.0f, 0.0f, 1.0f); + glViewport(kSubX, kSubY, kSubW, kSubH); + DrawQuadrants(); + glViewport(0, 0, Gl().Width(), Gl().Height()); + + const Image whole = ReadPixels(Gl().Width(), Gl().Height()); + EXPECT_EQ(FirstGLError(), GLenum(GL_NO_ERROR)); + + const Image placed = CropRect(whole, kSubX, kSubY, kSubW, kSubH); + EXPECT_EQ(placed.QuadrantSignature(), kUprightSignature) + << "the sub-viewport draw is not upright inside its own rect"; + ExpectUprightQuadrants(placed, "sub-viewport draw, cropped out of a full-extent read"); + + // Nothing may have been painted outside the viewport. This is what catches the + // mirrored placement: the drawn band would sit at GL rows [41, 83) instead. + EXPECT_TRUE(RegionIsMostly(whole, 0, Gl().Width() - 1, 0, kSubY - 2, "black", 0.0, + "below the sub-viewport")); + EXPECT_TRUE(RegionIsMostly(whole, 0, Gl().Width() - 1, kSubY + kSubH + 1, Gl().Height() - 1, "black", + 0.0, "above the sub-viewport")); + } + + // Readback: a full-viewport draw read back through a sub-rect must return the requested + // band, in GL row order. Band and orientation are asserted separately so that fixing only + // one of the two cannot pass this case. + TEST_F(OrientationScenario, SubRectReadbackReturnsTheRequestedBandUpright) { + BindDefaultFramebuffer(); + ClearTo(0.0f, 0.0f, 0.0f, 1.0f); + DrawQuadrants(); + + const Image whole = ReadPixels(Gl().Width(), Gl().Height()); + ASSERT_EQ(whole.QuadrantSignature(), kUprightSignature) + << "the full-extent read is already wrong, so nothing below can be trusted"; + + const Image sub = ReadPixelsRect(kSubX, kSubY, kSubW, kSubH); + EXPECT_EQ(FirstGLError(), GLenum(GL_NO_ERROR)); + ASSERT_EQ(sub.Width(), kSubW); + ASSERT_EQ(sub.Height(), kSubH); + + const Image requestedBand = CropRect(whole, kSubX, kSubY, kSubW, kSubH); + const Image mirroredBand = CropRect(whole, kSubX, Gl().Height() - kSubY - kSubH, kSubW, kSubH); + + // The geometry has to be able to see both mistakes; if a future surface size made the + // band symmetric these assertions would be vacuous, so say so loudly instead. + ASSERT_FALSE(requestedBand == VFlip(requestedBand)) + << "the chosen sub-rect is vertically symmetric - it cannot detect a row flip"; + ASSERT_FALSE(requestedBand == mirroredBand) + << "the chosen sub-rect equals its mirror band - it cannot detect a wrong band"; + + EXPECT_FALSE(sub == VFlip(requestedBand)) + << "ORIENTATION: the requested band came back with its rows in Vulkan (top-first) order"; + EXPECT_FALSE(sub == mirroredBand || sub == VFlip(mirroredBand)) + << "BAND: the read returned GL rows [H-y-h, H-y) instead of [y, y+h)"; + EXPECT_TRUE(sub == requestedBand) + << "the sub-rect readback differs from the same rect of the full-extent read in " + << sub.ByteDiffCount(requestedBand) << " bytes"; + } + + // The exact conformance-suite shape: an asymmetric sub-viewport draw read back through the + // very same sub-rect. The placement and readback errors cancel, leaving an image that is + // correct in every pixel VALUE and vertically flipped - which is precisely the 861-case + // signature. One assertion, and it pins all of them. + TEST_F(OrientationScenario, SubViewportSubRectRoundTripIsUpright) { + BindDefaultFramebuffer(); + ClearTo(0.0f, 0.0f, 0.0f, 1.0f); + glViewport(kSubX, kSubY, kSubW, kSubH); + DrawQuadrants(); + const Image sub = ReadPixelsRect(kSubX, kSubY, kSubW, kSubH); + glViewport(0, 0, Gl().Width(), Gl().Height()); + + EXPECT_EQ(FirstGLError(), GLenum(GL_NO_ERROR)); + EXPECT_EQ(sub.QuadrantSignature(), kUprightSignature) + << "sub-viewport draw + same-rect readback came back flipped - this is the shape " + "behind KHR-GL33/GL40.shaders.* (861 cases each)"; + ExpectUprightQuadrants(sub, "sub-viewport draw read back through the same sub-rect"); + } + + // The same conversion, on the other rect consumer that reads the default framebuffer. + // glBlitFramebuffer already converted its DESTINATION rect when the draw framebuffer was + // the default one (ApplyNativeBlitDefaultFramebufferTransform), but never its SOURCE rect, + // so a blit OUT of the default framebuffer took the mirrored band and wrote it upside + // down. Blitting a sub-rect and comparing against the same sub-rect of a direct read pins + // both halves at once. + TEST_F(OrientationScenario, BlitOutOfTheDefaultFramebufferKeepsBandAndOrientation) { + BindDefaultFramebuffer(); + ClearTo(0.0f, 0.0f, 0.0f, 1.0f); + DrawQuadrants(); + const Image whole = ReadPixels(Gl().Width(), Gl().Height()); + ASSERT_EQ(whole.QuadrantSignature(), kUprightSignature) + << "the full-extent read is already wrong, so nothing below can be trusted"; + + BindFbo(m_offscreen); + ClearTo(0.0f, 0.0f, 0.0f, 1.0f); + glBindFramebuffer(GL_READ_FRAMEBUFFER, 0); + glBindFramebuffer(GL_DRAW_FRAMEBUFFER, m_offscreen.fbo); + glBlitFramebuffer(kSubX, kSubY, kSubX + kSubW, kSubY + kSubH, kSubX, kSubY, kSubX + kSubW, + kSubY + kSubH, GL_COLOR_BUFFER_BIT, GL_NEAREST); + const unsigned int blitError = FirstGLError(); + if (blitError != GL_NO_ERROR) { + GTEST_SKIP() << "this backend refused the default-framebuffer blit: " + << GLErrorName(blitError); + } + + glBindFramebuffer(GL_FRAMEBUFFER, m_offscreen.fbo); + const Image blitted = ReadPixels(m_offscreen.width, m_offscreen.height); + EXPECT_EQ(FirstGLError(), GLenum(GL_NO_ERROR)); + + const Image landed = CropRect(blitted, kSubX, kSubY, kSubW, kSubH); + const Image expected = CropRect(whole, kSubX, kSubY, kSubW, kSubH); + EXPECT_FALSE(landed == VFlip(expected)) + << "ORIENTATION: the blitted band arrived upside down"; + EXPECT_TRUE(landed == expected) + << "the blitted sub-rect differs from the same sub-rect of a direct read in " + << landed.ByteDiffCount(expected) << " bytes"; + } + + // Negative control. A non-default framebuffer is already self-consistent - no + // gl_Position.y negation, GL row 0 IS Vulkan row 0 - so none of the fixes above may touch + // it. If this ever starts failing, the default-FBO remap has leaked into the FBO path. + TEST_F(OrientationScenario, FboSubRectReadbackAndSubViewportAreUnaffected) { + BindFbo(m_offscreen); + ClearTo(0.0f, 0.0f, 0.0f, 1.0f); + DrawQuadrants(); + + const Image whole = ReadPixels(m_offscreen.width, m_offscreen.height); + const Image sub = ReadPixelsRect(kSubX, kSubY, kSubW, kSubH); + EXPECT_EQ(FirstGLError(), GLenum(GL_NO_ERROR)); + EXPECT_TRUE(sub == CropRect(whole, kSubX, kSubY, kSubW, kSubH)) + << "an FBO sub-rect readback differs from the same rect of its full-extent read in " + << sub.ByteDiffCount(CropRect(whole, kSubX, kSubY, kSubW, kSubH)) << " bytes"; + + BindFbo(m_offscreen); + ClearTo(0.0f, 0.0f, 0.0f, 1.0f); + glViewport(kSubX, kSubY, kSubW, kSubH); + DrawQuadrants(); + glViewport(0, 0, m_offscreen.width, m_offscreen.height); + const Image placedWhole = ReadPixels(m_offscreen.width, m_offscreen.height); + EXPECT_EQ(FirstGLError(), GLenum(GL_NO_ERROR)); + EXPECT_EQ(CropRect(placedWhole, kSubX, kSubY, kSubW, kSubH).QuadrantSignature(), kUprightSignature) + << "an FBO sub-viewport draw must land in GL rows [y0, y0+h) upright"; + EXPECT_TRUE(RegionIsMostly(placedWhole, 0, m_offscreen.width - 1, 0, kSubY - 2, "black", 0.0, + "below an FBO sub-viewport")); + EXPECT_TRUE(RegionIsMostly(placedWhole, 0, m_offscreen.width - 1, kSubY + kSubH + 1, + m_offscreen.height - 1, "black", 0.0, "above an FBO sub-viewport")); + } + } // namespace } // namespace MGITest