From 17db759891a666b4ee139cded73d1cbb90414d15 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Tue, 8 Sep 2026 13:51:39 -0400 Subject: [PATCH] [Fix] (Pipe): state the four seam encodings the packages were each inventing - the sub-data target packing, the depth-stencil aspect numbers, the surface kind constants and the texture target the surface record grew where its padding was --- MobileGL/MG_Pipe/MGPipeTypes.h | 118 +++++++++++++++- MobileGL/MG_Pipe/PipeFields.def | 6 +- MobileGL/MG_Test/Pipe/PipeCatalogueTest.cpp | 148 ++++++++++++++++++++ 3 files changed, 268 insertions(+), 4 deletions(-) diff --git a/MobileGL/MG_Pipe/MGPipeTypes.h b/MobileGL/MG_Pipe/MGPipeTypes.h index f22c4901..dff45c81 100644 --- a/MobileGL/MG_Pipe/MGPipeTypes.h +++ b/MobileGL/MG_Pipe/MGPipeTypes.h @@ -471,6 +471,20 @@ namespace MobileGL::MG_Pipe { }; MGP_ASSERT_POD(MGPTextureParams, 40); + // MGPTextureParams::DepthStencilMode's two legal values, and the ONLY spelling of them + // (P4a, ID-12 / esprytobj DV-2). The frontend keeps a GLenum - GL_DEPTH_COMPONENT 0x1902, + // GL_STENCIL_INDEX 0x1901 - and a Uint8 cannot hold one, so the aspect is NUMBERED here + // rather than truncated there. The GLenum -> byte helper belongs to the client emitter; + // this header owns the two numbers, so the emitter and both backends cannot disagree. + // + // 0 IS DEPTH, AND THAT IS THE WHOLE REASON FOR THIS ORDER RATHER THAN THE ENUM'S LOW BYTE. + // GL_DEPTH_COMPONENT is the GL initial value of GL_DEPTH_STENCIL_TEXTURE_MODE and a + // texture that never asks for the stencil aspect never emits the call at all, so A ZEROED + // RECORD MUST DECODE TO EXACTLY WHAT AN UNTOUCHED TEXTURE ALREADY HAS. Numbering by the + // low byte would have made depth 0x02 and stencil 0x01 and left zero meaning nothing. + inline constexpr Uint8 kMGPipeDepthStencilModeDepth = 0; // GL_DEPTH_COMPONENT + inline constexpr Uint8 kMGPipeDepthStencilModeStencil = 1; // GL_STENCIL_INDEX + // create_shader_state. The reflection blob is the whole LinkArtifacts + SpirvArtifacts // archive; P0.5 extracts those types out of ProgramObject.h so a server can // deserialize into them without dragging in glslang (section 4.5.5). @@ -492,17 +506,57 @@ namespace MobileGL::MG_Pipe { // set_* // --------------------------------------------------------------------------------- + // MGPSurface::Kind's three values (P4a, ID-12 / esprytobj DV-4). MGPipeKind is REUSED + // rather than a second three-value enum minted beside it: it already spells Texture and + // Renderbuffer, its None is 0, and a zero-initialised MGPSurface is therefore ALREADY the + // empty attachment point this record describes - {Res = kMGPipeNullHandle, Kind = None} + // and every other field zero. The static_assert is what keeps that true if MGPipeKind is + // ever reordered. + inline constexpr Uint8 kMGPipeSurfaceKindNone = static_cast(MGPipeKind::None); + inline constexpr Uint8 kMGPipeSurfaceKindTexture = static_cast(MGPipeKind::Texture); + inline constexpr Uint8 kMGPipeSurfaceKindRenderbuffer = + static_cast(MGPipeKind::Renderbuffer); + static_assert(kMGPipeSurfaceKindNone == 0, + "a zero-initialised MGPSurface must already be the empty attachment point"); + + // MGPSurface::TextureTarget for a point that names no texture: the renderbuffer point and + // the empty point both carry it. It is MobileGL::TextureTarget::Unknown, which is -1 and + // therefore 0xFFFF in the field's Uint16 - a value no real target has, so a reader that + // forgets to gate on Kind gets a nonsense target rather than a plausible wrong one. + inline constexpr Uint16 kMGPipeSurfaceNoTextureTarget = 0xFFFF; + static_assert(kMGPipeSurfaceNoTextureTarget == + static_cast(MobileGL::TextureTarget::Unknown), + "kMGPipeSurfaceNoTextureTarget is TextureTarget::Unknown widened to the " + "field, and MG_State moved Unknown off -1"); + // = pipe_surface. internalFormat is INLINE so the four cross-object masks fall out at // push time with no lookup (section 4.5.6). struct MGPSurface { MGPipeHandle Res; Uint32 InternalFormat; - Uint8 Kind; // Texture | Renderbuffer | None + Uint8 Kind; // kMGPipeSurfaceKind{None,Texture,Renderbuffer}, above Uint8 Layered; Uint16 Level; Uint32 Layer; - Uint16 UploadTarget; - Uint16 Pad0; + Uint16 UploadTarget; // static_cast(MobileGL::TextureUploadTarget) + // P4a, ID-12 / esprytobj DV-5: WAS Pad0, and the size did not move - the two bytes + // were already here. static_cast(MobileGL::TextureTarget), and + // kMGPipeSurfaceNoTextureTarget on every point that is not a texture. + // + // THE FOUR CROSS-OBJECT MASKS ARE WHY IT EXISTS. IsSnormFallbackAttachment, + // IsUnormFallbackAttachment and IsAlphaWidenedColorAttachment all reduce to + // (format, TEXTURE TARGET) - ShouldUseCaveatTextureFormat(format, target) and + // BackendTextureFormatAddsAlpha(format, target) - and no TextureUploadTarget -> + // TextureTarget inverse exists anywhere in the tree, so UploadTarget cannot answer + // them. Without this field D-C1's promise that the inline InternalFormat makes the + // masks "fall out at push time with no lookup" is unkeepable and the backend keeps + // reading the frontend attachment objects. + // + // CONSULTED ONLY WHEN Kind == kMGPipeSurfaceKindTexture. A zero-initialised record + // carries 0, which is TextureTarget::Texture1D and not the sentinel; that is not a + // defect, because such a record is Kind == None and names no texture at all. Gating + // on Kind is the reader's contract. + Uint16 TextureTarget; }; MGP_ASSERT_POD(MGPSurface, 24); @@ -823,6 +877,30 @@ namespace MobileGL::MG_Pipe { // decision belongs on the side that pays the GPU cost. Mali prices texture upload by // JOB COUNT: ~100 sprite rects against one union box measured +6 ms/frame. // + // `Target` IS TWO FACTS IN ONE Uint16 (P4a, D-D3 / ID-12), and MGPipePackSubDataTarget + // under the struct is the only spelling of the encoding - nothing may open-code a half: + // + // low byte = MGPipeResourceTarget - WHICH KIND of storage the destination is. + // The applier branches on it: a buffer + // target dispatches into MGPipeResourceOps, + // every other target accumulates a pending + // upload for the texture sync to consume. + // high byte = MobileGL::TextureUploadTarget - WHICH cube face / upload target the level + // belongs to. It is NOT derivable from the + // resource target - six faces share TexCube + // - and 26 enumerators leave a byte ample. + // + // WHY THAT WAY ROUND, AND WHY THE ENCODING LIVES HERE RATHER THAN IN EACH EMITTER. The + // applier's SubDataNamesABuffer tests the WHOLE field == 0, and + // TextureUploadTarget::Texture1D is 0 - so a texture record carrying the bare upload + // enumerator is indistinguishable from a buffer record exactly when its owner is a 1D + // texture, and that texture's upload is dispatched into the buffer path. With the + // resource target in the LOW byte a buffer record's Target stays EXACTLY + // kMGPipeResourceTargetBuffer - P3a's buffer records are unchanged on the wire, their + // upload byte being zero too - while a texture record can never be zero, because no + // texture's MGPipeResourceTarget is. The static_assert under the struct holds that + // invariant, and the applier's whole-field test stays right either way. + // // THE BUFFER HALF. With Target == Buffer there is no level and no box, so the destination // byte range rides in the box's first coordinate and first extent: UnionBox.X is the byte // offset, UnionBox.W the byte size, Y = Z = 0, H = D = 1, Level = 0, RegionCount = 0. @@ -841,6 +919,8 @@ namespace MobileGL::MG_Pipe { // the only spelling of this convention; nothing else reads the box for a buffer. struct MGPSubData { MGPipeHandle Res; + // Target is PACKED - see the block above, and read it only through + // MGPipeSubDataResourceTargetOf / MGPipeSubDataUploadTargetOf below. Uint16 Target, Level; // Replaces the backend's `uploadData == mipData` pointer comparison: are these // bytes an untransformed level shadow? @@ -853,6 +933,38 @@ namespace MobileGL::MG_Pipe { }; MGP_ASSERT_POD(MGPSubData, 72); + // The one spelling of MGPSubData::Target's encoding, stated above the struct. + // + // Uint32 ARGUMENTS RATHER THAN THE TWO ENUM TYPES, and that is deliberate. This header is + // the contract: MGPipeResourceTarget is minted in it, but the upload half is MG_State's + // TextureUploadTarget, and nobody who reads the packed field ever needs that type - the + // applier and both backends read the halves BACK, as bytes, through the two accessors. + // Naming it in a signature would pin the contract's own API to a frontend enum for no + // reader's benefit, and would stop kMGPipeResourceTargetBuffer being passed as it stands. + // Callers pass static_cast(MobileGL::TextureUploadTarget) for `uploadTarget` and + // static_cast(MGPipeResourceTarget) - or kMGPipeResourceTargetBuffer - for + // `resourceTarget`. + constexpr inline Uint16 MGPipePackSubDataTarget(Uint32 resourceTarget, Uint32 uploadTarget) { + return static_cast((resourceTarget & 0xFFu) | ((uploadTarget & 0xFFu) << 8)); + } + // Comparable against static_cast(MGPipeResourceTarget) / kMGPipeResourceTargetBuffer. + constexpr inline Uint8 MGPipeSubDataResourceTargetOf(Uint16 packed) { + return static_cast(packed & 0xFFu); + } + // Comparable against static_cast(MobileGL::TextureUploadTarget). + constexpr inline Uint8 MGPipeSubDataUploadTargetOf(Uint16 packed) { + return static_cast((packed >> 8) & 0xFFu); + } + // THE INVARIANT P3a's records and the applier's buffer test both rest on: a buffer + // record's Target is exactly kMGPipeResourceTargetBuffer, whole field, upload byte and + // all. TextureUploadTarget::Texture1D is 0, so the buffer case is the one place where the + // packed form and a bare enumerator agree - and it has to stay that place. + static_assert(MGPipePackSubDataTarget(kMGPipeResourceTargetBuffer, 0u) == + kMGPipeResourceTargetBuffer, + "a buffer sub-data record's Target must stay exactly " + "kMGPipeResourceTargetBuffer: the applier's SubDataNamesABuffer tests the " + "whole field == 0"); + // Encodes a buffer byte range into the record's box. False, with the record untouched, // when the range does not fit one record: the emitter has to split it. inline Bool MGPipeSetSubDataBufferRange(MGPSubData& record, Uint64 offset, Uint64 size) { diff --git a/MobileGL/MG_Pipe/PipeFields.def b/MobileGL/MG_Pipe/PipeFields.def index 1ba0b1c1..76345e50 100644 --- a/MobileGL/MG_Pipe/PipeFields.def +++ b/MobileGL/MG_Pipe/PipeFields.def @@ -89,8 +89,12 @@ F(Cso) F(StageMask) F(GlobalUboSize) F(ReservedNumSamplesOffset) F(SpirvStatus) F(NativeFloat64) \ F(PointSizeDemoted) F(EnableSpirvValidation) F(Spirv) F(Reflection) +// P4a, ID-12 / esprytobj DV-5: Pad0 became Uint16 TextureTarget. Same trip wire as +// MGPFramebufferState's Target below - PADDING_MEMBER_RE only excludes a member still NAMED +// Pad, so the rename without this row is a pipe-gates failure, and the row without the +// rename is one too. A meaning-carrying byte cannot enter this record silently. #define MGP_FIELDS_MGPSurface(F) \ - F(Res) F(InternalFormat) F(Kind) F(Layered) F(Level) F(Layer) F(UploadTarget) + F(Res) F(InternalFormat) F(Kind) F(Layered) F(Level) F(Layer) F(UploadTarget) F(TextureTarget) // P4a, D-C2: Pad0 became Uint8 Target, and gen_pipe.py's PADDING_MEMBER_RE only excludes a // member still NAMED Pad - so the rename without this row is a pipe-gates failure, which diff --git a/MobileGL/MG_Test/Pipe/PipeCatalogueTest.cpp b/MobileGL/MG_Test/Pipe/PipeCatalogueTest.cpp index a51699d8..cf61a744 100644 --- a/MobileGL/MG_Test/Pipe/PipeCatalogueTest.cpp +++ b/MobileGL/MG_Test/Pipe/PipeCatalogueTest.cpp @@ -236,6 +236,154 @@ TEST(PipeCatalogue, EveryTextureTargetMapsToItsOwnResourceTarget) { MGPipeResourceTargetForTextureTarget(TextureTarget::TextureRectangle)); } +// P4a, D-D3 / ID-12: MGPSubData::Target is TWO facts in one Uint16 - the low byte says which +// KIND of storage the destination is, the high byte which cube face / upload target the level +// belongs to - and the packing is the contract's, not each emitter's. +// +// The property this case exists for is the COLLISION the packing prevents. +// TextureUploadTarget::Texture1D is 0 and the applier's buffer branch tests the WHOLE field +// == 0, so a texture record carrying the bare upload enumerator would be indistinguishable +// from a buffer record exactly when its owner is a 1D texture, and that texture's upload +// would be dispatched into the buffer path. Nothing else in the tree would have said so. +TEST(PipeCatalogue, SubDataTargetPacksAResourceTargetAndAnUploadTarget) { + // Both halves must fit their byte, or the encoding is not an encoding. + static_assert(static_cast(MGPipeResourceTarget::Count) <= 0x100u); + static_assert(static_cast(TextureUploadTarget::TextureUploadTargetCount) <= 0x100u); + + // 0 first, and deliberately: it is the enumerator that makes the collision possible. Then + // the plain 2D upload, the first and last cube face, and the largest enumerator the enum + // has, which is what proves the byte is wide enough in practice and not just in principle. + const Uint32 uploadTargets[] = { + 0u, + static_cast(TextureUploadTarget::Texture2D), + static_cast(TextureUploadTarget::CubeMapPositiveX), + static_cast(TextureUploadTarget::CubeMapNegativeZ), + static_cast(TextureUploadTarget::TextureUploadTargetCount) - 1u, + }; + for (Uint32 resource = 0; resource < static_cast(MGPipeResourceTarget::Count); + ++resource) { + for (const Uint32 upload : uploadTargets) { + const Uint16 packed = MGPipePackSubDataTarget(resource, upload); + EXPECT_EQ(MGPipeSubDataResourceTargetOf(packed), static_cast(resource)) + << "resource target " << resource << " upload target " << upload; + EXPECT_EQ(MGPipeSubDataUploadTargetOf(packed), static_cast(upload)) + << "resource target " << resource << " upload target " << upload; + } + } + + // THE BUFFER INVARIANT, at compile time in MGPipeTypes.h and again here so a failure names + // itself: a buffer record's Target is exactly kMGPipeResourceTargetBuffer, whole field, + // upload byte and all, so P3a's records are unchanged on the wire. + static_assert(MGPipePackSubDataTarget(kMGPipeResourceTargetBuffer, 0u) == + kMGPipeResourceTargetBuffer); + EXPECT_EQ(MGPipePackSubDataTarget(kMGPipeResourceTargetBuffer, 0u), kMGPipeResourceTargetBuffer); + EXPECT_EQ(MGPipePackSubDataTarget(kMGPipeResourceTargetBuffer, + static_cast(TextureUploadTarget::Texture1D)), + kMGPipeResourceTargetBuffer); + MGPSubData zeroed{}; + EXPECT_EQ(zeroed.Target, kMGPipeResourceTargetBuffer); + + // ...and the other side of it: a 1D texture's upload target IS 0, and packed it still + // cannot be mistaken for a buffer, because no texture's resource target is 0. + EXPECT_EQ(static_cast(TextureUploadTarget::Texture1D), 0u); + for (Uint32 resource = 1; resource < static_cast(MGPipeResourceTarget::Count); + ++resource) { + EXPECT_NE(MGPipePackSubDataTarget(resource, 0u), kMGPipeResourceTargetBuffer) + << "resource target " << resource << " collides with a buffer record"; + } + EXPECT_NE(MGPipePackSubDataTarget(MGPipeResourceTargetForTextureTarget(TextureTarget::Texture1D), + static_cast(TextureUploadTarget::Texture1D)), + kMGPipeResourceTargetBuffer); + + // What a real cube-face record reads back as, through the field rather than a local. + MGPSubData record{}; + record.Target = + MGPipePackSubDataTarget(MGPipeResourceTargetForTextureTarget(TextureTarget::TextureCubeMap), + static_cast(TextureUploadTarget::CubeMapNegativeY)); + EXPECT_EQ(MGPipeSubDataResourceTargetOf(record.Target), + static_cast(MGPipeResourceTarget::TexCube)); + EXPECT_EQ(MGPipeSubDataUploadTargetOf(record.Target), + static_cast(TextureUploadTarget::CubeMapNegativeY)); + // Six faces share one resource target: the high byte is the only thing that tells them + // apart, which is why it cannot be dropped. + EXPECT_EQ(MGPipeSubDataResourceTargetOf( + MGPipePackSubDataTarget(static_cast(MGPipeResourceTarget::TexCube), + static_cast(TextureUploadTarget::CubeMapPositiveX))), + MGPipeSubDataResourceTargetOf(record.Target)); + EXPECT_NE(MGPipeSubDataUploadTargetOf( + MGPipePackSubDataTarget(static_cast(MGPipeResourceTarget::TexCube), + static_cast(TextureUploadTarget::CubeMapPositiveX))), + MGPipeSubDataUploadTargetOf(record.Target)); +} + +// P4a, ID-12: the three constants MGPSurface::Kind is spelled with, the texture target the +// record grew where its Pad0 was, and MGPTextureParams::DepthStencilMode's two numbers. +// +// All three were UNSTATED in the contract and were being re-invented on both sides of the +// boundary - which is the way a wire field acquires two meanings. The values themselves are +// unremarkable; what this case pins is that there is exactly one spelling of each. +TEST(PipeCatalogue, SurfaceNamesItsKindItsTextureTargetAndItsDepthStencilAspect) { + // MGPipeKind is REUSED rather than a second three-value enum minted beside the field. + EXPECT_EQ(kMGPipeSurfaceKindNone, static_cast(MGPipeKind::None)); + EXPECT_EQ(kMGPipeSurfaceKindTexture, static_cast(MGPipeKind::Texture)); + EXPECT_EQ(kMGPipeSurfaceKindRenderbuffer, static_cast(MGPipeKind::Renderbuffer)); + EXPECT_NE(kMGPipeSurfaceKindTexture, kMGPipeSurfaceKindRenderbuffer); + // None == 0 is load-bearing: it is what makes a zero-initialised record already BE the + // empty attachment point, which every emitter and every reader relies on. + EXPECT_EQ(kMGPipeSurfaceKindNone, 0u); + + // Pad0 -> Uint16 TextureTarget. THE SIZE DID NOT MOVE - the two bytes were already there - + // and neither did anything in front of it. + EXPECT_EQ(sizeof(MGPSurface), 24u); + EXPECT_EQ(offsetof(MGPSurface, UploadTarget), 20u); + EXPECT_EQ(offsetof(MGPSurface, TextureTarget), 22u); + // The sentinel is TextureTarget::Unknown widened, so it is a value no real target has. + EXPECT_EQ(kMGPipeSurfaceNoTextureTarget, 0xFFFFu); + EXPECT_EQ(kMGPipeSurfaceNoTextureTarget, static_cast(TextureTarget::Unknown)); + for (SizeT i = 0; i < static_cast(TextureTarget::TextureTargetCount); ++i) { + EXPECT_NE(static_cast(i), kMGPipeSurfaceNoTextureTarget); + } + + // A ZEROED MGPSurface CARRIES TextureTarget 0, AND 0 IS TextureTarget::Texture1D, NOT THE + // SENTINEL. That is documented rather than defended, and it is why the field's contract is + // "consulted only when Kind == kMGPipeSurfaceKindTexture": a zeroed record is Kind == None + // and names no texture at all, so a reader that gates on Kind can never see the 0. A + // reader that does not gate would read Texture1D out of an empty attachment point. + MGPSurface empty{}; + EXPECT_EQ(empty.TextureTarget, 0u); + EXPECT_EQ(static_cast(TextureTarget::Texture1D), 0u); + EXPECT_EQ(empty.Kind, kMGPipeSurfaceKindNone); + EXPECT_TRUE(MGPipeHandleIsNull(empty.Res)); + + // A renderbuffer point names no texture and says so with the sentinel, which is what + // distinguishes "not a texture" from "a 1D texture" for a reader that looks anyway. + MGPSurface renderbuffer{}; + renderbuffer.Kind = kMGPipeSurfaceKindRenderbuffer; + renderbuffer.TextureTarget = kMGPipeSurfaceNoTextureTarget; + EXPECT_NE(renderbuffer.TextureTarget, static_cast(TextureTarget::Texture1D)); + + // The half a compiler cannot catch: the PipeFields.def row. MGPSurface still asserts its + // size whether or not the field list names TextureTarget, so a comparator blind to the + // field would pass a target-only divergence under MOBILEGL_PIPE_VERIFY - and the field is + // exactly what the four cross-object masks key on. + MGPSurface a{}; + MGPSurface b{}; + const char* field = nullptr; + EXPECT_TRUE(MGPipeVerify(a, b, &field)); + a.TextureTarget = static_cast(TextureTarget::TextureCubeMap); + EXPECT_FALSE(MGPipeVerify(a, b, &field)); + EXPECT_STREQ(field, "TextureTarget"); + + // DepthStencilMode: 0 = GL_DEPTH_COMPONENT, 1 = GL_STENCIL_INDEX. Depth is 0 because it is + // the GL initial value and a texture that never asks for the stencil aspect never emits + // the call, so a zeroed record has to decode to what an untouched texture already has. + EXPECT_EQ(kMGPipeDepthStencilModeDepth, 0u); + EXPECT_EQ(kMGPipeDepthStencilModeStencil, 1u); + EXPECT_NE(kMGPipeDepthStencilModeDepth, kMGPipeDepthStencilModeStencil); + MGPTextureParams params{}; + EXPECT_EQ(params.DepthStencilMode, kMGPipeDepthStencilModeDepth); +} + // G3's opcode numbering is the wire protocol. Position in PipeCalls.def, 1-based, no holes. TEST(PipeCatalogue, WireOpcodesAreThePositionsInTheCatalogue) { EXPECT_EQ(static_cast(MGPWireOp::GetCaps), 1);