From 3b3b6e5b8b3c11fefac2ce2a44964703a19c8e59 Mon Sep 17 00:00:00 2001 From: BZLZHH Date: Wed, 5 Aug 2026 03:41:23 -0400 Subject: [PATCH] [Fix] (MG_Backend): read back the stencil half, and clear an sRGB target to the value asked for Two reasons a framebuffer's contents came back wrong, both on the read/clear side rather than the write side. Stencil, on both backends. The CTS reads stencil with glReadPixels(GL_STENCIL_INDEX, GL_INT), which is as legal as the unsigned widths, and neither backend accepted it: DirectGLES's ReadPixelsStencilViaNative rejected every signed type, after which the call fell through to a native ES read the driver refuses and nothing was written at all, so the caller kept its zeros; DirectVulkan's pack switch had no GL_INT case, and of the cases it did have only GL_UNSIGNED_INT sourced the stencil plane - GL_FLOAT and GL_UNSIGNED_SHORT emitted a depth value, which is meaningless for a stencil-only image. Both now take the signed and float widths, and DirectVulkan decides "this is a stencil read" once rather than per type. DirectGLES also gains the GL_FLOAT_32_UNSIGNED_INT_24_8_REV fallback a DEPTH32F_STENCIL8 attachment needs, which rejects the 24_8 packed type. sRGB, on DirectVulkan. Every other write path goes through the UNORM twin view while GL_FRAMEBUFFER_SRGB is off, storing the raw value GL asked for, but a deferred clear is materialised with vkCmdClearColorImage - which names the image, so the driver applied the sRGB transfer function and a clear to 0.25 landed at 0.537. PreCompensateSrgbClearColor hands it the linear colour whose encoding is the requested value instead. It is a no-op for non-sRGB destinations, for integer clear encodings, and when GL_FRAMEBUFFER_SRGB is on and GL really does want the encode. Takes renderbuffers_storage from failing to passing on both backends, plus renderbuffers_storage_multisample and framebuffers_blit on Espryt. --- MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp | 37 ++++++++++++++++--- .../DirectVulkan/Renderer/VkClearManager.cpp | 21 +++++++++++ .../DirectVulkan/Renderer/VkClearManager.h | 9 +++++ .../DirectVulkan/Renderer/VulkanRenderer.cpp | 32 ++++++++++++---- 4 files changed, 86 insertions(+), 13 deletions(-) diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index 53f378ea..17382b48 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -4566,14 +4566,27 @@ namespace MobileGL::MG_Backend::DirectGLES { Vector packed(outStencil.size(), 0); ClearGLErrors(); g_GLESFuncs.glReadPixels(x, y, width, height, GL_DEPTH_STENCIL, GL_UNSIGNED_INT_24_8, packed.data()); + if (g_GLESFuncs.glGetError() == GL_NO_ERROR) { + for (SizeT i = 0; i < outStencil.size(); ++i) { + outStencil[i] = static_cast(packed[i] & 0xFFu); + } + return true; + } + + // A DEPTH32F_STENCIL8 attachment rejects the 24_8 type: its packed layout is a float depth + // followed by a padded stencil byte, eight bytes per pixel with the index at offset 4. + Vector packed32f(outStencil.size() * 8u, 0); + ClearGLErrors(); + g_GLESFuncs.glReadPixels(x, y, width, height, GL_DEPTH_STENCIL, GL_FLOAT_32_UNSIGNED_INT_24_8_REV, + packed32f.data()); const GLenum packedError = g_GLESFuncs.glGetError(); if (packedError != GL_NO_ERROR) { - MGLOG_E("ReadPixels: neither GL_STENCIL_INDEX nor GL_DEPTH_STENCIL readback is available: %s", + MGLOG_E("ReadPixels: no stencil readback path is available: %s", MG_Util::ConvertGLEnumToString(packedError).c_str()); return false; } for (SizeT i = 0; i < outStencil.size(); ++i) { - outStencil[i] = static_cast(packed[i] & 0xFFu); + outStencil[i] = packed32f[i * 8u + 4u]; } return true; } @@ -4583,11 +4596,20 @@ namespace MobileGL::MG_Backend::DirectGLES { // values themselves are always 8 bits. static Bool ReadPixelsStencilViaNative(GLint x, GLint y, GLsizei width, GLsizei height, GLenum type, void* pixels) { + // GL 4.6 core 18.2.8: a stencil index is written unconverted into whichever integer width + // the client asked for, and converted to a float value for GL_FLOAT. The signed widths are + // as legal as the unsigned ones - the CTS reads stencil with GL_INT - and rejecting them + // here used to let the call fall through to a native ES read the driver refuses, after + // which nothing was written at all and the caller kept its zeros. SizeT dstPixelBytes = 0; switch (type) { - case GL_UNSIGNED_BYTE: dstPixelBytes = sizeof(Uint8); break; - case GL_UNSIGNED_SHORT: dstPixelBytes = sizeof(Uint16); break; - case GL_UNSIGNED_INT: dstPixelBytes = sizeof(Uint32); break; + case GL_UNSIGNED_BYTE: + case GL_BYTE: dstPixelBytes = sizeof(Uint8); break; + case GL_UNSIGNED_SHORT: + case GL_SHORT: dstPixelBytes = sizeof(Uint16); break; + case GL_UNSIGNED_INT: + case GL_INT: dstPixelBytes = sizeof(Uint32); break; + case GL_FLOAT: dstPixelBytes = sizeof(GLfloat); break; default: return false; } if (width <= 0 || height <= 0) { @@ -4621,11 +4643,16 @@ namespace MobileGL::MG_Backend::DirectGLES { for (GLsizei col = 0; col < width; ++col) { switch (type) { case GL_UNSIGNED_BYTE: + case GL_BYTE: rowBuf[static_cast(col)] = srcRow[col]; break; case GL_UNSIGNED_SHORT: + case GL_SHORT: reinterpret_cast(rowBuf.data())[col] = srcRow[col]; break; + case GL_FLOAT: + reinterpret_cast(rowBuf.data())[col] = static_cast(srcRow[col]); + break; default: reinterpret_cast(rowBuf.data())[col] = srcRow[col]; break; diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VkClearManager.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VkClearManager.cpp index 629dc488..c4c589a4 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VkClearManager.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VkClearManager.cpp @@ -8,9 +8,13 @@ #include "VkClearManager.h" +#include "MG_State/GLState/Core.h" #include "MG_Util/Converters/MGToStr/FramebufferEnumConverter.h" #include "MG_Util/Converters/MGToStr/TextureEnumConverter.h" +#include +#include + namespace MobileGL::MG_Backend::DirectVulkan { static Bool IsCubeMapFaceUploadTarget(TextureUploadTarget target) { return target >= TextureUploadTarget::CubeMapPositiveX && @@ -42,6 +46,23 @@ namespace MobileGL::MG_Backend::DirectVulkan { return clearValue; } + void PreCompensateSrgbClearColor(ClearAttachmentPayload& payload, VkFormat destinationFormat) { + if (payload.colorEncoding != ClearColorEncoding::Float) return; + // With GL_FRAMEBUFFER_SRGB enabled GL performs the encoding itself, so the driver doing it + // is exactly right and there is nothing to undo. + if (MG_State::pGLContext->IsCapabilityEnabled(MobileGL::CapabilityInput::FramebufferSrgb)) return; + if (ResolveSrgbAttachmentWriteFormat(destinationFormat, false) == destinationFormat) return; + + // sRGB -> linear (GL 4.6 core 8.24), applied to the colour channels only: alpha is stored + // linearly in an sRGB format and must pass through untouched. + const auto toLinear = [](Float encoded) { + const Float value = std::clamp(encoded, 0.0f, 1.0f); + return value <= 0.04045f ? value / 12.92f : std::pow((value + 0.055f) / 1.055f, 2.4f); + }; + payload.color = FloatVec4(toLinear(payload.color.x()), toLinear(payload.color.y()), + toLinear(payload.color.z()), payload.color.w()); + } + void ForceOpaqueClearAlpha(ClearAttachmentPayload& payload) { switch (payload.colorEncoding) { case ClearColorEncoding::Int: diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VkClearManager.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/VkClearManager.h index bdcd7d8a..ea6d920d 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VkClearManager.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VkClearManager.h @@ -50,6 +50,15 @@ namespace MobileGL::MG_Backend::DirectVulkan { // the destination is known. void ForceOpaqueClearAlpha(ClearAttachmentPayload& payload); + // vkCmdClearColorImage names the image, so the driver applies the destination format's transfer + // function to whatever value it is handed. Every other write path in this backend goes through + // the UNORM twin view while GL_FRAMEBUFFER_SRGB is off (ResolveSrgbAttachmentWriteFormat) and + // therefore stores the raw value GL asked for. Rewrites `payload` to the linear colour whose + // encoding is that raw value, so a direct image clear of an sRGB destination agrees with them. + // A no-op for every other format, for integer clear encodings, and when GL is doing the + // encoding itself. + void PreCompensateSrgbClearColor(ClearAttachmentPayload& payload, VkFormat destinationFormat); + struct PendingClearKey { MG_State::GLState::ITextureObject* texture = nullptr; Uint64 textureLifetimeId = 0; diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp index 259378c8..248296c9 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp @@ -5914,9 +5914,10 @@ void main() { subresourceRange.baseArrayLayer = pendingClear.key.baseArrayLayer; subresourceRange.layerCount = pendingClear.key.layerCount; - const auto& clearPayload = pendingClear.payload; + auto clearPayload = pendingClear.payload; if ((resource->aspect & VK_IMAGE_ASPECT_COLOR_BIT) != 0) { subresourceRange.aspectMask = VK_IMAGE_ASPECT_COLOR_BIT; + PreCompensateSrgbClearColor(clearPayload, resource->format); const VkClearColorValue clearValue = MakeVkClearColorValue(clearPayload, ColorFormatLacksAlpha(&texture)); vkCmdClearColorImage(commandBuffer, resource->image, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, @@ -6000,6 +6001,7 @@ void main() { VkImageLayout steadyLayout; if ((resource->aspect & VK_IMAGE_ASPECT_COLOR_BIT) != 0) { subresourceRange.aspectMask = VK_IMAGE_ASPECT_COLOR_BIT; + PreCompensateSrgbClearColor(clearPayload, resource->format); // RGB renderbuffers are backed by an RGBA image; the missing alpha reads as 1. const VkClearColorValue clearValue = MakeVkClearColorValue( clearPayload, @@ -7632,13 +7634,16 @@ void main() { switch (type) { case GL_FLOAT: case GL_UNSIGNED_INT: + case GL_INT: case GL_UNSIGNED_INT_24_8: dstPixelBytes = 4; break; case GL_UNSIGNED_SHORT: + case GL_SHORT: dstPixelBytes = 2; break; case GL_UNSIGNED_BYTE: + case GL_BYTE: dstPixelBytes = 1; break; case GL_FLOAT_32_UNSIGNED_INT_24_8_REV: @@ -7649,29 +7654,40 @@ void main() { return; } + // GL 4.6 core 18.2.8: a GL_STENCIL_INDEX read reports the index itself, unconverted, in + // whatever width the client asked for. Only the packed types mix depth in. Deciding this + // once - rather than per type, where GL_FLOAT and GL_UNSIGNED_SHORT used to emit a depth + // value that is meaningless for a stencil-only image - is what makes the CTS's + // (GL_STENCIL_INDEX, GL_INT) read return 7 instead of nothing. + const Bool stencilOnly = format == GL_STENCIL_INDEX; + Vector packed(pixelCount * dstPixelBytes); for (SizeT i = 0; i < pixelCount; ++i) { Uint8* dst = packed.data() + i * dstPixelBytes; switch (type) { case GL_FLOAT: { - const Float value = depthValueAt(i); + const Float value = stencilOnly ? static_cast(stencilSrc[i]) : depthValueAt(i); Memcpy(dst, &value, sizeof(value)); break; } - case GL_UNSIGNED_SHORT: { - const Uint16 value = - static_cast(std::lround(static_cast(depthValueAt(i)) * 65535.0)); + case GL_UNSIGNED_SHORT: + case GL_SHORT: { + const Uint16 value = stencilOnly + ? static_cast(stencilSrc[i]) + : static_cast(std::lround(static_cast(depthValueAt(i)) * 65535.0)); Memcpy(dst, &value, sizeof(value)); break; } - case GL_UNSIGNED_INT: { - const Uint32 value = format == GL_STENCIL_INDEX + case GL_UNSIGNED_INT: + case GL_INT: { + const Uint32 value = stencilOnly ? stencilSrc[i] : static_cast(static_cast(depthValueAt(i)) * 4294967295.0); Memcpy(dst, &value, sizeof(value)); break; } - case GL_UNSIGNED_BYTE: { + case GL_UNSIGNED_BYTE: + case GL_BYTE: { dst[0] = stencilSrc[i]; break; }