From ed919f0d64eb31ec35236cecb1d4361ac35847f0 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Sun, 20 Sep 2026 19:17:17 -0400 Subject: [PATCH] [MG_Impl] guard the monolith mip generate arm against immutable storage and views EnsureGeneratedMipmapStorageAllocated branched on the transport. The split arm planned the generation window from BASE_LEVEL/MAX_LEVEL and refused to touch immutable storage (70c8230b); the monolith arm rebuilt the chain from the bound object's level-0 extent and then truncated it. Through a view that is destructive in a way the split arm had already been fixed for: a view's level-0 extent is one slice of the owner's layers, so a two-layer view of a four-layer array resized every owner level the generation touched to two layers - the storage behind the owner's other layers was gone - and redefined the owner's levels out of the view's window as well. Both arms now take one path. An immutable texture or a view returns before any allocation: it already owns every level generation can write. A mutable texture gets levels from the plan's Base..End window and nothing else moves - the chain is not truncated, because neither MAX_LEVEL nor a generate is a truncation (a level-0 respecification drops the stale tail itself, in DiscardMipmapChainOnBaseRespecification). MipmapGenerationPlan.h is no longer behind MOBILEGL_BUILD_DISAGGREGATED for the same reason: the window is a property of the GL call, not of how the backend is reached, and the plan the wire carrier publishes (MGPMipPlan::LevelCount, end-exclusive) is this one. The window rule was asserted only by the split-only F1WireScenario, whose fixture skips under every other transport - so the monolith arm, which is what a non-disaggregated library runs for every application, had no coverage at all. TextureViewScenario.GenerateMipmapThroughAViewLeavesTheOwnerShapeAlone runs in an integration-gpu lane under both transports and pins the owner's layer counts, its own level extents, and the pixels of the levels and layers outside the view's window. It is DirectGLES-only, and the skip names why: Magma reallocates the chain through the bound object in its own generate path (DirectVulkan/Renderer/VulkanRenderer.cpp's EnsureGenerateMipmapStorageAllocated), and a CPU sub-image into the layers of mip 2 or 3 does not read back through Magma at all - a run of the same scenario with the generate removed fails identically - so the per-level colours this case is built on are unobservable on that backend rather than merely wrong. --- .../MG_Impl/GLImpl/Texture/GL_Texture.cpp | 79 ++++------ .../GLImpl/Texture/MipmapGenerationPlan.h | 5 +- .../Scenarios/TextureViewScenario.cpp | 136 ++++++++++++++++++ 3 files changed, 164 insertions(+), 56 deletions(-) diff --git a/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp b/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp index 1efd6b8e..c6472853 100644 --- a/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp +++ b/MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp @@ -508,65 +508,36 @@ namespace MobileGL::MG_Impl::GLImpl { return size; } + // glGenerateMipmap defines levels BASE_LEVEL+1 up to the level the base image's extent and + // MAX_LEVEL admit, and leaves every other level exactly as it was (GL 4.6 core 8.17). Both + // transports take this one path: the window, and the refusal to redefine the levels around + // it, are properties of the GL call rather than of how the backend is reached. Bool EnsureGeneratedMipmapStorageAllocated( MG_State::GLState::TextureObjectMipmap& texture, TextureUploadTarget uploadTarget) { -#if MOBILEGL_BUILD_DISAGGREGATED - if (MG_Config::Transport != MG_Config::TransportMode::Monolith) { - const auto plan = ComputeMipmapGenerationRange(texture, uploadTarget); - if (plan.End <= plan.Base) return false; - // Immutable textures and views already own every generated level. - // Allocating through a view would redefine its owner's complete - // level, destroying layers and levels outside the view window. - if (texture.IsImmutable()) return true; - const IntVec3 baseSize = texture.GetMipmapTexelSize(uploadTarget, plan.Base); - const SizeT baseBytes = texture.GetMipmapByteSize(uploadTarget, plan.Base); - const SizeT texels = static_cast(baseSize.x()) * baseSize.y() * baseSize.z(); - if (!baseBytes || !texels || baseBytes % texels) return false; - const SizeT pixelBytes = baseBytes / texels; - const Int axes = MipShrinkingAxisCount(texture.GetTarget()); - for (Uint level = plan.Base + 1; level < plan.End; ++level) { - const auto size = ComputeMipmapTexelSize(baseSize, level - plan.Base, axes); - const SizeT bytes = pixelBytes * static_cast(size.x()) * size.y() * size.z(); - texture.AllocateStorage(uploadTarget, level, {size, bytes}); - texture.MarkStorageDirty(uploadTarget, level, false); - } - // Levels before BASE_LEVEL and after the generated end remain - // valid images. In particular, MAX_LEVEL is not a truncate call. - texture.BumpContentVersion(); - return true; - } -#endif - const Uint existingLevelCount = texture.GetMipmapLevelCount(); - if (existingLevelCount == 0) { - return false; - } - - const IntVec3 baseTexelSize = texture.GetMipmapTexelSize(uploadTarget, 0); - const SizeT baseByteSize = texture.GetMipmapByteSize(uploadTarget, 0); - const SizeT baseTexelCount = static_cast(baseTexelSize.x()) * - static_cast(baseTexelSize.y()) * - static_cast(baseTexelSize.z()); - if (baseTexelSize.x() <= 0 || baseTexelSize.y() <= 0 || baseTexelSize.z() <= 0 || - baseByteSize == 0 || baseTexelCount == 0 || (baseByteSize % baseTexelCount) != 0) { - return false; - } - - const SizeT bytesPerTexel = baseByteSize / baseTexelCount; - const Int shrinkingAxes = MipShrinkingAxisCount(texture.GetTarget()); - const Uint requiredLevelCount = ComputeFullMipmapLevelCount(baseTexelSize, shrinkingAxes); - for (Uint level = 1; level < requiredLevelCount; ++level) { - const IntVec3 levelTexelSize = ComputeMipmapTexelSize(baseTexelSize, level, shrinkingAxes); - const SizeT levelByteSize = bytesPerTexel * static_cast(levelTexelSize.x()) * - static_cast(levelTexelSize.y()) * - static_cast(levelTexelSize.z()); - texture.AllocateStorage(uploadTarget, level, {levelTexelSize, levelByteSize}); + const auto plan = ComputeMipmapGenerationRange(texture, uploadTarget); + if (plan.End <= plan.Base) return false; + // Immutable textures and views already own every level generation can write. + // Allocating through a view would redefine its owner's levels at the VIEW's base + // extent - one slice of the owner's layers - destroying the storage of every layer and + // level outside the view window. + if (texture.IsImmutable()) return true; + const IntVec3 baseSize = texture.GetMipmapTexelSize(uploadTarget, plan.Base); + const SizeT baseBytes = texture.GetMipmapByteSize(uploadTarget, plan.Base); + const SizeT texels = static_cast(baseSize.x()) * baseSize.y() * baseSize.z(); + if (!baseBytes || !texels || baseBytes % texels) return false; + const SizeT pixelBytes = baseBytes / texels; + const Int axes = MipShrinkingAxisCount(texture.GetTarget()); + for (Uint level = plan.Base + 1; level < plan.End; ++level) { + const auto size = ComputeMipmapTexelSize(baseSize, level - plan.Base, axes); + const SizeT bytes = pixelBytes * static_cast(size.x()) * size.y() * size.z(); + texture.AllocateStorage(uploadTarget, level, {size, bytes}); texture.MarkStorageDirty(uploadTarget, level, false); } - // glGenerateMipmap defines exactly levels 0..requiredLevelCount-1. AllocateStorage only - // grows, so a previously longer chain (a bigger base image before respecification) would - // otherwise keep a tail of stale levels here and read as incomplete. - texture.TruncateMipmapLevels(uploadTarget, requiredLevelCount); + // Levels before BASE_LEVEL and after the generated end remain valid images and are left + // alone: neither MAX_LEVEL nor a generate truncates the chain. AllocateStorage only + // grows, so a longer chain a level-0 respecification left behind is dropped there + // (DiscardMipmapChainOnBaseRespecification) rather than here. // Mip generation grows/regenerates the level set on the GPU without marking any CPU // level dirty (MarkStorageDirty(...,false) above). Bump the content version so the // backend re-syncs: a cached sampled VkImageView built for the pre-generate level diff --git a/MobileGL/MG_Impl/GLImpl/Texture/MipmapGenerationPlan.h b/MobileGL/MG_Impl/GLImpl/Texture/MipmapGenerationPlan.h index 4b1c298f..59677d12 100644 --- a/MobileGL/MG_Impl/GLImpl/Texture/MipmapGenerationPlan.h +++ b/MobileGL/MG_Impl/GLImpl/Texture/MipmapGenerationPlan.h @@ -8,7 +8,9 @@ #pragma once #include -#if MOBILEGL_BUILD_DISAGGREGATED +// The plan is the GL call's own window, not a transport's, so both arms of +// EnsureGeneratedMipmapStorageAllocated compute it here and the wire carrier +// (MGPMipPlan::LevelCount) publishes the same end-exclusive number. #include #include @@ -42,4 +44,3 @@ namespace MobileGL::MG_Impl::GLImpl { return result; } } -#endif diff --git a/MobileGL/MG_IntegrationTest/Scenarios/TextureViewScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/TextureViewScenario.cpp index e84fd14e..0db36acb 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/TextureViewScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/TextureViewScenario.cpp @@ -904,5 +904,141 @@ void main() { "the view must still reach its storage after the original's name was deleted"); DestroyColorFbo(destination); } + + // ------------------------------------------------------------------------------------ + // glGenerateMipmap THROUGH a view: the view's window is a write target, never a shape. + // + // THE DEFECT this pins. Generating through a view used to (re)allocate the storage + // OWNER from the VIEW's own level-zero extent. A view's level-zero extent is one slice + // of the owner's layers, so a two-layer view of a four-layer array resized every owner + // level it touched to two layers - the storage behind the owner's other layers was gone - + // and redefined the owner's other levels out of the view's window. GL 4.6 core 8.18 makes + // a view a window INTO storage it does not own: level sizes and layer counts belong to + // the owner, and generation through the view is a GPU-side write into that window. + // + // IT IS ASSERTED HERE, in the transport-agnostic view scenario, because the arm that + // reallocated is the MONOLITH one - what every application of a non-disaggregated library + // runs, and what the split-only F1WireScenario never reaches (its fixture skips under any + // other transport, which is why its copy of this rule runs nowhere). + // + // DirectGLES ONLY, for two facts about the other backend and neither of them this arm's: + // Magma reallocates the chain through the bound object in its OWN generate path + // (DirectVulkan/Renderer/VulkanRenderer.cpp's EnsureGenerateMipmapStorageAllocated), which + // through a view means its owner's levels - the same defect on the far side of the door this + // fix closes; and a CPU sub-image into layer 1 or 2 of mip 2 or 3 does not read back through + // Magma at all - a run with the generate removed fails identically - so the per-level colours + // this case is built on are unobservable there rather than wrong. + // ------------------------------------------------------------------------------------ + TEST_F(TextureViewScenario, GenerateMipmapThroughAViewLeavesTheOwnerShapeAlone) { + if (!Ready() || IsSkipped()) return; + if (Gl().BackendName() != "DirectGLES") { + GTEST_SKIP() << "the DirectGLES monolith arm is the one that reallocated through the " + "view; Magma does its own reallocation there and reads no such sub-image"; + } + + constexpr int kLevels = 5; + constexpr int kBase = 16; + constexpr int kLayers = 4; + constexpr int kViewMinLevel = 1; + constexpr int kViewNumLevels = 3; + constexpr int kViewMinLayer = 1; + constexpr int kViewNumLayers = 2; + // One colour per level, so a level that was redefined from the view's base extent reads + // as another level's colour rather than as a plausible one. + const Rgba8 levelColour[kLevels] = {{255, 0, 0, 255}, {0, 0, 255, 255}, {255, 255, 0, 255}, + {255, 0, 255, 255}, {0, 255, 255, 255}}; + + const GLuint storage = MakeTexture(); + glBindTexture(GL_TEXTURE_2D_ARRAY, storage); + glTexStorage3D(GL_TEXTURE_2D_ARRAY, kLevels, GL_RGBA8, kBase, kBase, kLayers); + glPixelStorei(GL_UNPACK_ALIGNMENT, 1); + for (int level = 0; level < kLevels; ++level) { + const int side = kBase >> level; + std::vector texels(static_cast(side) * side * kLayers * 4); + for (std::size_t at = 0; at < texels.size(); at += 4) { + texels[at + 0] = levelColour[level].r; + texels[at + 1] = levelColour[level].g; + texels[at + 2] = levelColour[level].b; + texels[at + 3] = levelColour[level].a; + } + glTexSubImage3D(GL_TEXTURE_2D_ARRAY, level, 0, 0, 0, side, side, kLayers, GL_RGBA, + GL_UNSIGNED_BYTE, texels.data()); + } + ASSERT_EQ(glGetError(), static_cast(GL_NO_ERROR)) << "seeding the owner raised an error"; + + // Levels 0..2 of the view alias the owner's 1..3. Its nonzero BASE_LEVEL makes the + // owner's level 2 the source of the generation, and MAX_LEVEL 2 ends it at the owner's + // level 3 - both expressed in the VIEW's coordinates. + const GLuint view = MakeTexture(); + glTextureView(view, GL_TEXTURE_2D_ARRAY, storage, GL_RGBA8, kViewMinLevel, kViewNumLevels, + kViewMinLayer, kViewNumLayers); + ASSERT_EQ(glGetError(), static_cast(GL_NO_ERROR)); + glTextureParameteri(view, GL_TEXTURE_BASE_LEVEL, 1); + glTextureParameteri(view, GL_TEXTURE_MAX_LEVEL, 2); + + // The generation's source is the view's base level, the owner's level 2. Writing it in + // GREEN on the GPU gives the source and the level generated from it the same colour, so + // losing either one is visible below. + const GLuint fbo = MakeFbo(); + glBindFramebuffer(GL_FRAMEBUFFER, fbo); + glDisable(GL_DEPTH_TEST); + glDisable(GL_STENCIL_TEST); + glDisable(GL_SCISSOR_TEST); + glViewport(0, 0, kBase >> 2, kBase >> 2); + for (int layer = kViewMinLayer; layer < kViewMinLayer + kViewNumLayers; ++layer) { + glFramebufferTextureLayer(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, storage, 2, layer); + ASSERT_EQ(glCheckFramebufferStatus(GL_FRAMEBUFFER), static_cast(GL_FRAMEBUFFER_COMPLETE)) + << "the source level's layer " << layer << " is not attachable"; + glClearColor(0.0f, 1.0f, 0.0f, 1.0f); + glClear(GL_COLOR_BUFFER_BIT); + } + ASSERT_EQ(glGetError(), static_cast(GL_NO_ERROR)) << "clearing the source level raised an error"; + + glGenerateTextureMipmap(view); + ASSERT_EQ(glGetError(), static_cast(GL_NO_ERROR)) + << "glGenerateTextureMipmap on a view raised an error"; + + // The view addresses two layers of the owner; the owner keeps its own four. + GLint viewDepth = 0; + glBindTexture(GL_TEXTURE_2D_ARRAY, view); + glGetTexLevelParameteriv(GL_TEXTURE_2D_ARRAY, 0, GL_TEXTURE_DEPTH, &viewDepth); + glBindTexture(GL_TEXTURE_2D_ARRAY, storage); + EXPECT_EQ(viewDepth, kViewNumLayers); + for (int level = 0; level < kLevels; ++level) { + const int side = kBase >> level; + GLint width = 0, height = 0, depth = 0; + glGetTexLevelParameteriv(GL_TEXTURE_2D_ARRAY, level, GL_TEXTURE_WIDTH, &width); + glGetTexLevelParameteriv(GL_TEXTURE_2D_ARRAY, level, GL_TEXTURE_HEIGHT, &height); + glGetTexLevelParameteriv(GL_TEXTURE_2D_ARRAY, level, GL_TEXTURE_DEPTH, &depth); + EXPECT_EQ(width, side) << "the owner's mip " << level << " must keep its own width"; + EXPECT_EQ(height, side) << "the owner's mip " << level << " must keep its own height"; + ASSERT_EQ(depth, kLayers) + << "generation through a " << kViewNumLayers << "-layer view must not resize its owner's mip " + << level; + for (int layer = 0; layer < kLayers; ++layer) { + // Per-layer FBO readback rather than glGetTexImage: the question is what the + // GPU holds, and a layer outside the view's window proves it by still holding + // its own pixels. + glBindFramebuffer(GL_FRAMEBUFFER, fbo); + glFramebufferTextureLayer(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, storage, level, layer); + glReadBuffer(GL_COLOR_ATTACHMENT0); + ASSERT_EQ(glCheckFramebufferStatus(GL_FRAMEBUFFER), static_cast(GL_FRAMEBUFFER_COMPLETE)) + << "the owner's mip " << level << ", layer " << layer << " lost its storage"; + const Image image = ReadPixels(side, side); + // Generation covers levels base+1..max of the view, i.e. the owner's levels 2 + // and 3, over the two layers the view names. Everything else keeps its own + // image: the owner's level 4 is past the view's max level, and its layers 0 and + // 3 are outside the view's layer window. + const bool inWindow = + level >= 2 && level <= 3 && layer >= kViewMinLayer && layer < kViewMinLayer + kViewNumLayers; + const Rgba8 expected = inWindow ? Rgba8{0, 255, 0, 255} : levelColour[level]; + const std::string what = + "the owner's mip " + std::to_string(level) + ", layer " + std::to_string(layer) + + (inWindow ? ", inside the view's window" : ", outside the view's window"); + ExpectRegion(image, 0, side - 1, 0, side - 1, expected, 2, what.c_str()); + } + } + ASSERT_EQ(glGetError(), static_cast(GL_NO_ERROR)); + } } // namespace } // namespace MGITest