swung0x48/MobileGL
1
0
Fork 0
mirror of https://github.com/MobileGL-Dev/MobileGL synced 2026-09-09 04:38:30 +09:00
Code Issues Packages Projects Releases Wiki Activity

[Fix] (Pipe): read a sub-data record's target halves through the contract's own accessors, answer the emitter whether a create or a respecify was accepted, and apply a respecify that restates the stored storage as a metadata update rather than a redefinition

Browse Source
This commit is contained in:
swung0x48
2026-09-08 15:07:23 -04:00
parent 711d5c61ba
commit 08922b6f78
3 changed files with 197 additions and 39 deletions
Download Patch File Download Diff File Expand all files Collapse all files
MobileGL/MG_Pipe/PipeApply.cpp
+123 -31
View File
@@ -512,21 +512,73 @@ namespace MobileGL::MG_Pipe {
return record.Target == kMGPipeResourceTargetBuffer;
}
// THE VALUE SPACE OF MGPSubData::Target, written down here because the record itself does
// not say it and package B is about to encode cube faces into the same field. It is
// PACKED (integrator ruling ID-12, the DV-3 seam): the LOW byte is the MGPipeResourceTarget
// that owns the storage and the HIGH byte is the TextureUploadTarget the upload names -
// which is what makes the packing necessary at all, since TextureUploadTarget::Texture1D
// is 0 and the bare enumerator would collide with Buffer. Both halves are therefore
// free to take any value their own enum defines and neither may be read without the mask.
// THE VALUE SPACE OF MGPSubData::Target IS THE CONTRACT'S, AND THIS FILE NO LONGER
// RESTATES IT. c0c (ID-12, the DV-3 seam) moved the encoding into MGPipeTypes.h as
// MGPipePackSubDataTarget / MGPipeSubDataResourceTargetOf / MGPipeSubDataUploadTargetOf,
// so wire v2's local `SubDataResourceTargetOf` - written because those helpers were not
// on that round's base - is gone and every half-read below goes through the contract's
// accessor. Nothing here open-codes `& 0xFF`.
//
// MGPipeTypes.h IS SUPPOSED TO OWN THE THREE HELPERS (MGPipePackSubDataTarget /
// MGPipeSubDataResourceTargetOf / MGPipeSubDataUploadTargetOf, contract commit c0c);
// they are NOT on this package's base (feat/disaggregated = c0 + c0b), so this is the
// local decode and it is one line to retire the day c0c lands. The applier still matches
// the WHOLE field for the buffer question above, exactly as ID-12 says it does.
Uint16 SubDataResourceTargetOf(const MGPSubData& record) {
return static_cast<Uint16>(record.Target & 0x00FFu);
// The applier still matches the WHOLE field for the buffer question above, exactly as
// ID-12 says it does, and c0c's own static_assert is what keeps the two readings in
// step: a buffer record's Target is exactly kMGPipeResourceTargetBuffer.
// ID-18 M4: DOES THIS RESPECIFY REDEFINE ANY STORAGE, OR IS IT CARRYING METADATA?
//
// The question exists because a sticky BindMask / ImageBindableHint bit has exactly one
// way onto the wire - a respecify - and an IMMUTABLE texture has no further respecify to
// ride on. So B re-publishes the descriptor when the mask moves, and the applier has to
// tell that call apart from a real redefinition: the first must NOT drop the pending
// uploads standing against the storage (nothing replaced them), the second must.
//
// THE COMPARISON IS FIELD BY FIELD AND NOT A memcmp. MGPResourceDesc carries Pad0/Pad1
// and neither side of this comparison is guaranteed to have written them - the client
// fills a stack descriptor field by field - so a byte comparison of 88 bytes is a coin
// flip on padding, which is the defect clientsp's SamplerParameters cache already paid
// for once.
//
// WHAT IS NOT COMPARED IS THE SHORT LIST AND IT IS THE DEFINITION: BindMask and
// ImageBindableHint (the two this call exists to carry), GlNameForDiag (diagnostics, and
// never an identity - section 4.2.1) and the two padding words. EVERYTHING ELSE IS
// COMPARED, which is deliberately WIDER than "the fields that define an allocation":
// being wide can only mis-classify a metadata update as a redefinition, whose cost is
// one level's pending upload dropped by a call B does not emit; being narrow would
// mis-classify a REDEFINITION as metadata and keep boxes in a coordinate system that no
// longer exists, which is an upload past the end of the new level. The asymmetry decides
// the direction.
//
// A BUFFER IS NEVER METADATA-ONLY, and that is a rule rather than a consequence.
// glBufferData at an unchanged size is the canonical ORPHANING idiom - a real
// reallocation whose whole purpose is that the old store is gone - and glBufferStorage
// is the one entry point in the catalogue allowed a synchronous acknowledgement
// (MGPipeResourceRespecifyNeedsAck). Excluding the target here is what makes ID-18 M4's
// "no reallocation ack" true by construction instead of by a second suppression rule,
// and it costs nothing: the mask that M4 is about is a TEXTURE's (and a renderbuffer's),
// and a buffer has no pending upload for the arm to protect anyway.
Bool RespecifyRedefinesNoStorage(const MGPResourceDesc& stored, const MGPResourceDesc& next) {
if (next.Target == kMGPipeResourceTargetBuffer) return false;
if (stored.Target != next.Target) return false;
if (stored.StorageKind != next.StorageKind) return false;
if (stored.InternalFormat != next.InternalFormat) return false;
if (stored.Width != next.Width || stored.Height != next.Height ||
stored.Depth != next.Depth) {
return false;
}
if (stored.ArrayLayers != next.ArrayLayers || stored.Levels != next.Levels ||
stored.Samples != next.Samples) {
return false;
}
if (stored.FixedSampleLocations != next.FixedSampleLocations) return false;
if (stored.Immutable != next.Immutable) return false;
if (stored.HasDefinedContent != next.HasDefinedContent) return false;
if (stored.Usage != next.Usage || stored.StorageFlags != next.StorageFlags) return false;
// The view and buffer-texture fields: a texture view's storage OWNER and a buffer
// texture's backing range are its storage exactly as an extent is one, and
// re-pointing either is a redefinition however unchanged the extent looks.
if (!(stored.ViewOf == next.ViewOf)) return false;
if (!(stored.BufferForTexBuffer == next.BufferForTexBuffer)) return false;
if (stored.BufOffset != next.BufOffset || stored.BufSize != next.BufSize) return false;
return true;
}
// WHY A DEAD HANDLE IS NOT A TRIP WIRE HERE, and the bounds faults below are - and
@@ -1469,10 +1521,10 @@ namespace MobileGL::MG_Pipe {
// is why they are incremented here rather than carried in a payload.
// ================================================================================
void MGPipeApplyResourceCreate(const MGPResourceDesc& desc) {
Bool MGPipeApplyResourceCreate(const MGPResourceDesc& desc) {
MOBILEGL_ASSERT(desc.Resource.Slot >= kMGPipeFirstAllocatableSlot,
"resource_create named the reserved slot 0");
if (desc.Resource.Slot < kMGPipeFirstAllocatableSlot) return;
if (desc.Resource.Slot < kMGPipeFirstAllocatableSlot) return false;
// P4a: THE TABLE IS CHOSEN BY THE DESCRIPTOR'S TARGET, and getting that wrong is the one
// way this call can damage an object it was not about - slot 7 is a live Buffer, a live
@@ -1483,7 +1535,7 @@ namespace MobileGL::MG_Pipe {
" resource_create {slot=%u, gen=%u, glName=%u}: the descriptor names no "
"resource target (%u)",
desc.Resource.Slot, desc.Resource.Gen, desc.GlNameForDiag, desc.Target);
return;
return false;
}
// A CREATE STARTS THE RECORD OVER rather than editing it. The slot it names may be a
@@ -1499,7 +1551,7 @@ namespace MobileGL::MG_Pipe {
"record table's bound (%u)",
desc.Resource.Slot, desc.Resource.Gen, desc.GlNameForDiag,
kMGPipeMaxResourceSlots);
return;
return false;
}
*record = MGPipeResourceRecord{};
record->Gen = desc.Resource.Gen;
@@ -1514,13 +1566,14 @@ namespace MobileGL::MG_Pipe {
// lazily inside SyncMipmapsToBackend and a renderbuffer's inside its own SyncToBackend,
// so there is no GL-call-time hook to dispatch to and P4a adds none: the record IS the
// publication, and the backend reads it at the sync point it already has.
if (desc.Target != kMGPipeResourceTargetBuffer) return;
if (desc.Target != kMGPipeResourceTargetBuffer) return true;
if (g_resourceOps != nullptr && g_resourceOps->Create != nullptr) {
g_resourceOps->Create(desc.Resource, desc);
}
return true;
}
void MGPipeApplyResourceRespecify(const MGPResourceDesc& desc, const void* initialBytes,
Bool MGPipeApplyResourceRespecify(const MGPResourceDesc& desc, const void* initialBytes,
const MGPRespecifiedLevel* level) {
Vector<MGPipeResourceRecord>* table = ResourceTableForTarget(desc.Target);
if (table == nullptr) {
@@ -1528,17 +1581,28 @@ namespace MobileGL::MG_Pipe {
" resource_respecify {slot=%u, gen=%u, glName=%u}: the descriptor names "
"no resource target (%u)",
desc.Resource.Slot, desc.Resource.Gen, desc.GlNameForDiag, desc.Target);
return;
return false;
}
MGPipeResourceRecord* record = ResolveResourceIn(*table, "resource_respecify", desc.Resource);
if (record == nullptr) return;
if (record == nullptr) return false;
PinNoLiveHostWrites(*record, desc.Resource, "resource_respecify");
// IS THIS A REDEFINITION AT ALL? Asked BEFORE the descriptor is replaced, because the
// stored one is the only thing there is to compare against (ID-18 M4). See
// RespecifyRedefinesNoStorage: true means every storage-defining field is unchanged and
// the call exists to carry a BindMask / ImageBindableHint that moved after the
// allocation - the immutable-texture case, where there is no later respecify to ride on.
const Bool metadataOnly = RespecifyRedefinesNoStorage(record->Desc, desc);
// The descriptor is replaced WHOLE, because that is what a respecify is: the store's
// extent, usage, storage flags, immutability and defined-content flag are all restated
// by the call that redefines it, and the backend reads them from here instead of
// asking a frontend object for them.
// asking a frontend object for them. A metadata update replaces it too - that is how
// the mask arrives - and by construction only the non-storage fields differ.
record->Desc = desc;
// THE SERIAL MOVES EITHER WAY, and for a metadata update it is the entire publication:
// the twin re-derives its storage flags from the new mask at its next sync and decides
// for itself whether the backend needs a recreate.
++record->Serial;
// A RESPECIFY REDEFINES A STORE, SO THE PENDING UPLOADS AGAINST THE STORE IT REPLACES
@@ -1561,8 +1625,21 @@ namespace MobileGL::MG_Pipe {
// the arm this set exists for) -> glTexImage2D(1, data), which under a blanket
// clear destroys level 0's entry before anything ever uploaded it.
//
// A buffer never has a pending upload at all, so both arms are inert for P3a's half.
if (level == nullptr) {
// - and a METADATA update (ID-18 M4) drops NOTHING, whatever `level` says. It is the
// third arm and it refines the first two rather than contradicting them: the rule
// is "the uploads against the storage this call REPLACES go with it", and a call
// whose storage-defining fields all equal the stored descriptor replaces no
// storage, so no level's coordinate system has moved and every pending box is still
// described in the space it was accumulated in. B re-emits the descriptor when a
// sticky bind bit moves, which can land between a glTexSubImage2D and the sync that
// consumes it; eating those texels there would be C1's bug with a different
// trigger, and just as silent.
//
// A buffer never has a pending upload at all, so all three arms are inert for P3a's
// half - which is also why a buffer is never classified as metadata-only (below).
if (metadataOnly) {
// nothing to drop, deliberately.
} else if (level == nullptr) {
record->PendingUploads.clear();
} else {
// The keys are unique by AccumulatePendingUpload's construction - it looks for the
@@ -1583,10 +1660,19 @@ namespace MobileGL::MG_Pipe {
// branch whose arms were identical would be dead code the transport would then have to
// find and remove. PipeCatalogueTest.ResourceRespecifyAcksOnlyImmutableStorage is what
// keeps the predicate honest until the doorbell reads it.
if (desc.Target != kMGPipeResourceTargetBuffer) return;
//
// ID-18 M4 ASKS FOR "NO REALLOCATION ACK" ON A METADATA UPDATE, AND IT IS TRUE HERE BY
// CONSTRUCTION RATHER THAN BY A BRANCH: the predicate is false for every target that is
// not a buffer, and RespecifyRedefinesNoStorage refuses the buffer target outright, so
// MGPipeResourceRespecifyNeedsAck(desc) is false for every record this applier
// classifies as metadata-only. That is why the classification excludes buffers rather
// than suppressing the ack afterwards - a suppression would have been a second rule the
// transport would then have to learn.
if (desc.Target != kMGPipeResourceTargetBuffer) return true;
if (g_resourceOps != nullptr && g_resourceOps->Respecify != nullptr) {
g_resourceOps->Respecify(desc.Resource, desc, initialBytes);
}
return true;
}
Bool MGPipeApplyResourceSubData(const MGPSubData& record, const void* bytes,
@@ -1609,14 +1695,20 @@ namespace MobileGL::MG_Pipe {
// accumulate a pending upload onto whatever TEXTURE holds slot N in the texture slot
// space. Renderbuffers have no sub-data path at all, so no correct client can produce
// one and this is a protocol fault rather than a dropped call.
const Uint16 resourceTarget = SubDataResourceTargetOf(record);
const Uint8 resourceTarget = MGPipeSubDataResourceTargetOf(record.Target);
if (resourceTarget == kMGPipeResourceTargetBuffer ||
resourceTarget == static_cast<Uint16>(MGPipeResourceTarget::Renderbuffer) ||
resourceTarget >= static_cast<Uint16>(MGPipeResourceTarget::Count)) {
resourceTarget == static_cast<Uint8>(MGPipeResourceTarget::Renderbuffer) ||
resourceTarget >= static_cast<Uint8>(MGPipeResourceTarget::Count)) {
// BOTH HALVES GO IN THE LINE. The packed field alone leaves a reader doing the
// arithmetic the encoding exists to stop anyone doing by hand, and the upload half
// is what says whether the emitter packed the pair the wrong way round.
MGP_TRIP_WIRE_REPORT("MGPipe: " MGP_TRIP_WIRE_TAG("ProtocolCorruption")
" resource_subdata {slot=%u, gen=%u}: the record's resource target names "
"no texture to upload into (target=%u, resource target=%u)",
record.Res.Slot, record.Res.Gen, record.Target, resourceTarget);
"no texture to upload into (target=%u, resource target=%u, upload "
"target=%u)",
record.Res.Slot, record.Res.Gen, record.Target,
static_cast<Uint32>(resourceTarget),
static_cast<Uint32>(MGPipeSubDataUploadTargetOf(record.Target)));
return false;
}
return ApplyTextureUpload(record, bytes, regions);
MobileGL/MG_Pipe/PipeApply.h
+49 -2
View File
@@ -774,10 +774,30 @@ namespace MobileGL::MG_Pipe {
Uint16 Level = 0;
};
// THE THREE ACCEPTANCE RETURNS, AND WHY ALL THREE (ID-18 M3, clientfb review M3). D-D5
// step 1 says the client clears a level's dirty flags "for the levels whose record the
// applier ACCEPTED", and the emitter cannot answer that for itself: an `if constexpr` that
// discarded the call, a dead or stale handle (a counted no-op) and a corrupt record (a Fatal
// that deliberately moves no counter) are all invisible from the call site, so a client that
// clears on the strength of having EMITTED drops those texels for good. resource_subdata
// returns it, and so must the two calls that DEFINE the storage a subsequent upload lands
// in - a create or a respecify the applier refused leaves no record for the upload to
// accumulate onto, and B's own bookkeeping (its per-entry descriptor dedupe, its drain list)
// must not advance past a call that never landed.
//
// ALL THREE ARE SOURCE-COMPATIBLE: a Bool return is ignorable, P3a's call sites in
// MG_Impl/Pipe/PipeFill.cpp discard it, and gen_pipe.py never parses this header - the wire
// path calls no MGPipeApply* at all (wire review W1), so PipeCalls.def and
// MobileGL/MG_Pipe/generated do not move.
// resource_create: mints the record and marks the slot Live. Emitted from the buffer
// object's CONSTRUCTOR, so a resource exists before anything can name it; storage is
// defined lazily by the first respecify and a backend tolerates a resource with none.
void MGPipeApplyResourceCreate(const MGPResourceDesc& desc);
//
// Returns true when the record was minted. False for the three refusals: the reserved slot
// 0, a descriptor whose target names no resource kind, and a slot at or above
// kMGPipeMaxResourceSlots.
Bool MGPipeApplyResourceCreate(const MGPResourceDesc& desc);
// resource_respecify: replaces the stored descriptor and bumps Serial. `initialBytes` is
// the shadow when desc.HasDefinedContent, else null. kNeedsAck on the call,
// MGPipeResourceRespecifyNeedsAck(desc) per record - only an immutable store acks.
@@ -798,7 +818,34 @@ namespace MobileGL::MG_Pipe {
// Trailing and defaulted for W1's reason: P3a's buffer call site (PipeFill.cpp:691) and
// every existing case compile unchanged. PACKAGE B PASSES THE PAIR IT JUST ALLOCATED at
// every per-level respecify; it has both halves in hand at the AllocateStorage call site.
void MGPipeApplyResourceRespecify(const MGPResourceDesc& desc, const void* initialBytes,
//
// A METADATA RESPECIFY IS A RESPECIFY THAT REDEFINES NO STORAGE (ID-18 M4). A sticky
// BindMask / ImageBindableHint bit reaches the applier only on a respecify, and an
// IMMUTABLE texture has no further one - that is what immutable means - so the canonical
// order (glTexStorage2D, then glBindImageTexture or an FBO attachment) would leave the
// record's hint at 0 for ever, and the hint is the PREVENTION half of the texture-remint
// stall class. So B re-emits the descriptor when the mask moves, and a record whose
// STORAGE-DEFINING fields all equal the stored descriptor's is applied as a metadata
// update:
//
// - the descriptor is replaced, so BindMask and ImageBindableHint take their new values;
// - NO pending upload is dropped, whatever `level` says. This REFINES the rule above
// rather than contradicting it: that rule drops the uploads against the storage a
// respecify REPLACES, and a call that replaces no storage replaces no coordinate system
// either, so there is nothing to drop. A mask change arriving between a
// glTexSubImage2D and the sync that consumes it must not eat the texels;
// - the serial advances, which is the whole publication - the twin re-derives its storage
// flags from the new mask at its next sync and recreates only where the backend needs
// it (D's side);
// - and MGPipeResourceRespecifyNeedsAck is false for it BY CONSTRUCTION, because a buffer
// is never classified this way (see the body: glBufferData at an unchanged size is a
// real orphaning reallocation, and glBufferStorage is the one entry point allowed a
// synchronous ack).
//
// Returns true when the descriptor was stored - metadata updates included, since the record
// did move - and false when the call was refused: a descriptor whose target names no
// resource kind, or a handle this applier has no live record for at that generation.
Bool MGPipeApplyResourceRespecify(const MGPResourceDesc& desc, const void* initialBytes,
const MGPRespecifiedLevel* level = nullptr);
// resource_subdata, buffer half: the destination range rides in the record's box through
// MGPipeSetSubDataBufferRange, and a false from that helper is where the EMITTER split.
MobileGL/MG_Test/Pipe/TextureEmitTest.cpp
+25 -6
View File
@@ -479,17 +479,32 @@ TEST(TextureEmit, ARespecifyOfOneLevelKeepsThePendingUploadsOfTheOthers) {
const Uint8 texels[4096] = {};
MGPipeApplyResourceCreate(TextureDesc(texture, 0, 121));
// EVERY DESCRIPTOR BELOW CARRIES THE LEVEL COUNT THE CALL IT MODELS WOULD CARRY, and that
// is not decoration. A mutable mip build grows MipmapStorage's level count as it defines
// levels, so package B's descriptor (TextureEmit.h: `desc.Levels =
// mipmap->GetMipmapLevelCount()`) MOVES on the glTexImage2D that adds level 1 - which is
// also why B's own memcmp dedupe emits that respecify at all. A respecify whose
// storage-defining fields are all unchanged is a METADATA update (ID-18 M4) and drops
// nothing whatever level it names, so a case that fed the same descriptor three times would
// be exercising that arm rather than this one.
auto levelDesc = [&](Uint16 levels, Uint32 internalFormat) {
MGPResourceDesc desc = TextureDesc(texture, 64, 121);
desc.Levels = levels;
desc.InternalFormat = internalFormat;
return desc;
};
// glTexImage2D(level 0, data): the respecify names the level it defines, and the drain then
// emits level 0's shape, which the applier accepts.
const MGPRespecifiedLevel levelZero{kTex2D, 0};
MGPipeApplyResourceRespecify(TextureDesc(texture, 64, 121), nullptr, &levelZero);
MGPipeApplyResourceRespecify(levelDesc(1, 0x8058u /*GL_RGBA8*/), nullptr, &levelZero);
ASSERT_TRUE(MGPipeApplyResourceSubData(TextureUpload(texture, 0, MGPBox{0, 0, 0, 64, 64, 1}, 0), texels));
// A SECOND FACE OF THE SAME LEVEL, keyed the way the packed Target keys it (ID-12: high
// byte = the cube-face upload target, low byte = the resource target), so what survives is
// a SET and not one lucky entry - and so that the level number alone cannot be what matched.
const Uint16 secondFace = static_cast<Uint16>((1u << 8) | kTex2D);
const Uint16 secondFace = MGPipePackSubDataTarget(kTex2D, 1u);
const MGPRespecifiedLevel faceOfLevelZero{secondFace, 0};
MGPipeApplyResourceRespecify(TextureDesc(texture, 64, 121), nullptr, &faceOfLevelZero);
MGPipeApplyResourceRespecify(levelDesc(1, 0x8058u), nullptr, &faceOfLevelZero);
MGPSubData otherFace = TextureUpload(texture, 0, MGPBox{0, 0, 0, 64, 64, 1}, 0);
otherFace.Target = secondFace;
ASSERT_TRUE(MGPipeApplyResourceSubData(otherFace, texels));
@@ -498,9 +513,10 @@ TEST(TextureEmit, ARespecifyOfOneLevelKeepsThePendingUploadsOfTheOthers) {
// Espryt BAILS - the texture is not mipmap-complete for its min filter - so both entries
// are still owed when the next GL call arrives.
//
// glTexImage2D(level 1, data): this redefines level 1 of the (kTex2D, *) face only.
// glTexImage2D(level 1, data): this redefines level 1 of the (kTex2D, *) face only, and the
// level count moves 1 -> 2 with it.
const MGPRespecifiedLevel levelOne{kTex2D, 1};
MGPipeApplyResourceRespecify(TextureDesc(texture, 64, 121), nullptr, &levelOne);
MGPipeApplyResourceRespecify(levelDesc(2, 0x8058u), nullptr, &levelOne);
ASSERT_EQ(TextureRecordOf(10).PendingUploads.size(), 2u)
<< "a respecify of level 1 dropped the pending uploads of levels it never redefined - "
@@ -511,9 +527,12 @@ TEST(TextureEmit, ARespecifyOfOneLevelKeepsThePendingUploadsOfTheOthers) {
EXPECT_EQ(TextureRecordOf(10).PendingUploads[1].UploadTarget, secondFace);
// And the key it DOES name goes, because that level's coordinate system has been replaced.
// The redefinition modelled here is glTexImage2D(level 1) with a NEW internal format - a
// legal thing to do to a mutable texture, and a real redefinition of that level's storage,
// so the descriptor moves and the metadata arm does not claim it.
ASSERT_TRUE(MGPipeApplyResourceSubData(TextureUpload(texture, 1, MGPBox{0, 0, 0, 32, 32, 1}, 0), texels));
ASSERT_EQ(TextureRecordOf(10).PendingUploads.size(), 3u);
MGPipeApplyResourceRespecify(TextureDesc(texture, 64, 121), nullptr, &levelOne);
MGPipeApplyResourceRespecify(levelDesc(2, 0x8051u /*GL_RGB8*/), nullptr, &levelOne);
ASSERT_EQ(TextureRecordOf(10).PendingUploads.size(), 2u)
<< "the level the respecify DOES redefine kept its box across the redefinition";
for (const auto& entry : TextureRecordOf(10).PendingUploads) {