From 88ee75be0e954c3646a8e418e8bd12e9d02087b5 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 27 Aug 2026 05:11:53 -0400 Subject: [PATCH] [Fix] (DirectVulkan): resolve renderbuffer VkFormats through the shared texture table and police copy size-compatibility --- .../Renderer/VkRenderPassManager.cpp | 53 +++++-------------- .../Renderer/VkTextureManager.cpp | 9 +--- .../DirectVulkan/Renderer/VkTextureManager.h | 25 +++++++++ .../DirectVulkan/Renderer/VulkanRenderer.cpp | 29 ++++++++++ 4 files changed, 67 insertions(+), 49 deletions(-) diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VkRenderPassManager.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VkRenderPassManager.cpp index ce588ccf..7396fa2e 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VkRenderPassManager.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VkRenderPassManager.cpp @@ -355,47 +355,18 @@ namespace MobileGL::MG_Backend::DirectVulkan { } const auto internalFormat = renderbuffer->GetInternalFormat(); - // Three-channel color formats widen to their RGBA twin exactly like textures do - // (VkTextureManager::ResolveTextureFormatInfo): blits/resolves between a - // renderbuffer and a texture of the same GL format then see one VkFormat. - const VkFormat format = [&]() -> VkFormat { - switch (internalFormat) { - case TextureInternalFormat::RGB: - case TextureInternalFormat::RGB8: - case TextureInternalFormat::R3G3B2: - case TextureInternalFormat::RGB4: - case TextureInternalFormat::RGB5: - return VK_FORMAT_R8G8B8A8_UNORM; - case TextureInternalFormat::SRGB8: - return VK_FORMAT_R8G8B8A8_SRGB; - case TextureInternalFormat::RGB8Snorm: - return VK_FORMAT_R8G8B8A8_SNORM; - case TextureInternalFormat::RGB10: - case TextureInternalFormat::RGB12: - case TextureInternalFormat::RGB16: - return VK_FORMAT_R16G16B16A16_UNORM; - case TextureInternalFormat::RGB16Snorm: - return VK_FORMAT_R16G16B16A16_SNORM; - case TextureInternalFormat::RGB16F: - return VK_FORMAT_R16G16B16A16_SFLOAT; - case TextureInternalFormat::RGB32F: - return VK_FORMAT_R32G32B32A32_SFLOAT; - case TextureInternalFormat::RGB8I: - return VK_FORMAT_R8G8B8A8_SINT; - case TextureInternalFormat::RGB8UI: - return VK_FORMAT_R8G8B8A8_UINT; - case TextureInternalFormat::RGB16I: - return VK_FORMAT_R16G16B16A16_SINT; - case TextureInternalFormat::RGB16UI: - return VK_FORMAT_R16G16B16A16_UINT; - case TextureInternalFormat::RGB32I: - return VK_FORMAT_R32G32B32A32_SINT; - case TextureInternalFormat::RGB32UI: - return VK_FORMAT_R32G32B32A32_UINT; - default: - return MG_Util::ConvertTextureInternalFormatToVkEnum(internalFormat); - } - }(); + // ONE resolver, shared with textures (VkTextureManager::ResolveTextureFormatInfo), so a + // renderbuffer and a texture of the same GL format cannot disagree about their VkFormat. + // `expandRgbToRgba` / `componentByteCount` / `alphaBytes` describe how to reshape a SHADOW + // UPLOAD, and a renderbuffer has none, so only `.format` is taken. + // + // This used to be a hand-maintained second copy of that table, and it was missing exactly + // four rows: RGBA2 and RGBA12 fell through to ConvertTextureInternalFormatToVkEnum's + // VK_FORMAT_UNDEFINED (no image at all - bound as a draw buffer the attachment became + // VK_ATTACHMENT_UNUSED and every draw into it was dropped), while RGBA4 and RGB5A1 fell + // through to the 16-bit packed formats and then faced 32-bit R8G8B8A8_UNORM textures across + // a size-incompatible vkCmdCopyImage. + const VkFormat format = ResolveTextureFormatInfo(internalFormat).format; const VkImageAspectFlags aspect = ResolveImageAspectMaskForFormat(format); // Renderbuffers are never sampled (GL has no way to bind one to a sampler), so the // usage set is attachment + transfer: transfer covers readback (vkCmdCopyImageToBuffer), diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VkTextureManager.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VkTextureManager.cpp index 63ae6165..222e7cd9 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VkTextureManager.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VkTextureManager.cpp @@ -46,13 +46,6 @@ namespace MobileGL::MG_Backend::DirectVulkan { return mipLevelCount; } - struct TextureFormatInfo { - VkFormat format = VK_FORMAT_UNDEFINED; - Bool expandRgbToRgba = false; - Uint32 componentByteCount = 0; - Array alphaBytes = {0, 0, 0, 0}; - }; - struct TextureShapeInfo { VkImageType imageType = VK_IMAGE_TYPE_2D; VkImageViewType viewType = VK_IMAGE_VIEW_TYPE_2D; @@ -380,7 +373,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { return true; } - static TextureFormatInfo ResolveTextureFormatInfo(TextureInternalFormat format) { + TextureFormatInfo ResolveTextureFormatInfo(TextureInternalFormat format) { switch (format) { case TextureInternalFormat::RGB: case TextureInternalFormat::RGB8: diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VkTextureManager.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/VkTextureManager.h index 434b39bd..117b8424 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VkTextureManager.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VkTextureManager.h @@ -24,6 +24,31 @@ class ITextureObject; namespace MobileGL::MG_Backend::DirectVulkan { enum class SamplerNumericDomain : Uint8; +// What VkFormat a GL internal format is BACKED with, and how a shadow upload has to be reshaped to +// fit it. This is not the same question as "is there an exact VkFormat for this GL format", which is +// what ConvertTextureInternalFormatToVkEnum answers: several GL formats have no Vulkan twin at all +// (RGBA2, RGBA12) and several three-channel ones are deliberately widened to their four-channel twin +// because Vulkan devices rarely support the 3-channel layouts. +// +// SHARED, and it must stay the only answer to that question. A renderbuffer and a texture of the +// same GL format have to resolve to the SAME VkFormat or every blit, resolve and glCopyImageSubData +// between them crosses a size-incompatible pair, which vkCmdCopyImage leaves undefined +// (VUID-vkCmdCopyImage-srcImage-01548). The renderbuffer path used to carry a hand-maintained second +// copy of this table that was missing four rows - RGBA2, RGBA4, RGB5A1 and RGBA12 - so those four +// renderbuffer formats either got no image at all or a 16-bit-packed one facing a 32-bit texture. +struct TextureFormatInfo { + VkFormat format = VK_FORMAT_UNDEFINED; + // The GL format has three channels and is carried in a four-channel image; a shadow upload has + // to be expanded, inserting `alphaBytes` after every `componentByteCount * 3` source bytes. + Bool expandRgbToRgba = false; + Uint32 componentByteCount = 0; + Array alphaBytes = {0, 0, 0, 0}; +}; + +// Callers that only need the backing VkFormat (a renderbuffer has no shadow upload to reshape) take +// `.format` and ignore the rest. +TextureFormatInfo ResolveTextureFormatInfo(TextureInternalFormat format); + // A GL 1D-ARRAY level keeps its LAYER COUNT in the state-side HEIGHT: that is what // glTexImage2D(GL_TEXTURE_1D_ARRAY, width, layers) means, and the frontend records the level // as {width, layers, 1} (see GL_Texture.cpp's AllocateStorage and the completeness walk in diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp index 1b16e137..8f979802 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp @@ -9136,6 +9136,12 @@ void main() { VkExtent2D extent = {0, 0}; Uint32 depth = 1; Uint32 arrayLayers = 1; + // Both resources carry a format; this copy used to decline to read it, which is why a + // four-row drift between the texture and renderbuffer format tables turned into + // corrupted texels with nothing in the log. vkCmdCopyImage requires size-compatible + // formats whenever they differ (VUID-vkCmdCopyImage-srcImage-01548) and there is no + // downstream check - a mismatched pair is a promise the driver takes at face value. + VkFormat format = VK_FORMAT_UNDEFINED; }; Bool TryResolveCopyImageSliceMapping(TextureTarget target, const CopyImageVkImage& image, Uint32 mipLevel, @@ -9255,6 +9261,7 @@ void main() { out.extent = resource->extent; out.depth = 1; out.arrayLayers = 1; + out.format = resource->format; return out.image != VK_NULL_HANDLE; } // An endpoint that named nothing is the frontend validator's INVALID_VALUE and never @@ -9270,6 +9277,7 @@ void main() { out.extent = resource->extent; out.depth = resource->depth; out.arrayLayers = resource->arrayLayers; + out.format = resource->format; return true; }; CopyImageVkImage srcImage{}; @@ -9310,6 +9318,27 @@ void main() { srcLevel, srcImage.mipLevels, dstLevel, dstImage.mipLevels); return; } + // Size compatibility, the guard whose absence let a table drift two files away reach the + // driver as a promise. glCopyImageSubData is a raw texel-block move (GL 4.6 core 18.3.2), and + // Vulkan says as much: when the two formats differ they must be size-compatible - the same + // texel block size - or vkCmdCopyImage is undefined (VUID-vkCmdCopyImage-srcImage-01548). + // Nothing else on this path asks: the three checks around it cover the mip range, the region + // bounds and the slice range, and none of them ever looked at a format. + // + // A decline rather than a MOBILEGL_ASSERT, for the reason the neighbouring guards spell out: + // assertions compile out of the release build that the CTS and shipping both run, which is + // exactly where the corruption was observed. + if (srcImage.format != dstImage.format) { + const Uint32 srcBlockSize = vkuGetFormatInfo(srcImage.format).texel_block_size; + const Uint32 dstBlockSize = vkuGetFormatInfo(dstImage.format).texel_block_size; + if (srcBlockSize == 0 || dstBlockSize == 0 || srcBlockSize != dstBlockSize) { + MGLOG_E_ONCE("%s: source format %d and destination format %d are not size-compatible " + "(%u vs %u bytes per texel block); declining the copy", + __func__, static_cast(srcImage.format), static_cast(dstImage.format), + srcBlockSize, dstBlockSize); + return; + } + } const VkImageAspectFlags copyAspectMask = srcImage.aspect & dstImage.aspect & (VK_IMAGE_ASPECT_COLOR_BIT | VK_IMAGE_ASPECT_DEPTH_BIT | VK_IMAGE_ASPECT_STENCIL_BIT);