diff --git a/MobileGL/MG_Pipe/PipeApply.cpp b/MobileGL/MG_Pipe/PipeApply.cpp index 871f717d..69f480cf 100644 --- a/MobileGL/MG_Pipe/PipeApply.cpp +++ b/MobileGL/MG_Pipe/PipeApply.cpp @@ -502,14 +502,33 @@ namespace MobileGL::MG_Pipe { } // A sub-data record's own discriminator, and it is DELIBERATELY NOT the table selector - // above: MGPSubData::Target is the UPLOAD target - a cube face is one, and those are - // not MGPipeResourceTarget enumerators - so the only thing it can be asked is the one - // question that has an answer for every value. kMGPipeResourceTargetBuffer is 0 and no - // texture upload target is, which is the contract the emitter is held to. + // above: MGPSubData::Target carries the UPLOAD target as well - a cube face is one, and + // those are not MGPipeResourceTarget enumerators - so the buffer question is asked of + // the whole field, which is the one question that has an answer for every value. + // kMGPipeResourceTargetBuffer is 0, a buffer has no upload target to pack beside it, and + // no texture emission leaves the whole field 0: that is the contract the emitter is + // held to. Bool SubDataNamesABuffer(const MGPSubData& record) { 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. + // + // 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(record.Target & 0x00FFu); + } + // WHY A DEAD HANDLE IS NOT A TRIP WIRE HERE, and the bounds faults below are - and // why it is nonetheless COUNTED rather than silently dropped. // @@ -785,9 +804,13 @@ namespace MobileGL::MG_Pipe { // buffer_subdata_resident differ only in which backend hook takes the bytes and in the // fact that one of them is allowed to be absent, so a second copy of this arithmetic // would be a second place to get it wrong. - void ApplyBufferWrite(const char* call, const MGPSubData& record, const void* bytes, Bool resident) { + // RETURNS THE ACCEPTANCE, for D-D5 step 1's reason: the emitter clears its own dirty + // state on the strength of this answer and neither refusal path is otherwise visible + // from the call site (a dead handle is a counted no-op, a corrupt record is a Fatal + // that deliberately moves no counter). + Bool ApplyBufferWrite(const char* call, const MGPSubData& record, const void* bytes, Bool resident) { MGPipeResourceRecord* stored = ResolveResource(call, record.Res); - if (stored == nullptr) return; + if (stored == nullptr) return false; const Uint64 offset = MGPipeSubDataBufferOffset(record); const Uint64 size = MGPipeSubDataBufferSize(record); @@ -803,7 +826,7 @@ namespace MobileGL::MG_Pipe { call, record.Res.Slot, record.Res.Gen, stored->Desc.GlNameForDiag, fault, static_cast(offset), static_cast(size), stored->Desc.Width); - return; + return false; } PinNoLiveHostWrites(*stored, record.Res, call); @@ -813,7 +836,12 @@ namespace MobileGL::MG_Pipe { // draw would re-upload what it had just landed. ++stored->Serial; - if (g_resourceOps == nullptr) return; + // THE RANGE IS LANDED FROM HERE ON, so every path below returns true: whether a + // backend table is registered, and whether it implements the optional resident + // hook, is a property of the BUILD and not of the record. An emitter that read + // "not accepted" off an unregistered table would keep re-sending a write the + // applier has already taken responsibility for. + if (g_resourceOps == nullptr) return true; if (resident) { // kOptional, and the frontend already checks the same way for the table this // one replaces: a backend that does not implement the resident path leaves the @@ -821,9 +849,10 @@ namespace MobileGL::MG_Pipe { if (g_resourceOps->SubDataResident != nullptr) { g_resourceOps->SubDataResident(record.Res, record, bytes); } - return; + return true; } if (g_resourceOps->SubData != nullptr) g_resourceOps->SubData(record.Res, record, bytes); + return true; } // The texture half of resource_subdata, and it DISPATCHES TO NOBODY. Nothing in this @@ -831,10 +860,10 @@ namespace MobileGL::MG_Pipe { // and Espryt uploads it at its own sync point, out of the accumulated set below. So the // whole of this function is the gate, the accumulation and the serial - which is also // why MGPipeResourceOps did not have to grow a member for it. - void ApplyTextureUpload(const MGPSubData& record, const void* bytes, const MGPSubRegion* regions) { + Bool ApplyTextureUpload(const MGPSubData& record, const void* bytes, const MGPSubRegion* regions) { MGPipeResourceRecord* stored = ResolveResourceIn(g_applier.TextureResources, "resource_subdata", record.Res); - if (stored == nullptr) return; + if (stored == nullptr) return false; const char* fault = SubDataTextureFault(record, regions); // A whole-level upload declares no regions and a non-empty box; a record that @@ -854,7 +883,7 @@ namespace MobileGL::MG_Pipe { record.Target, record.Level, record.UnionBox.X, record.UnionBox.Y, record.UnionBox.Z, record.UnionBox.W, record.UnionBox.H, record.UnionBox.D, record.RegionCount); - return; + return false; } if (!AccumulatePendingUpload(*stored, record, regions)) { MGP_TRIP_WIRE_REPORT("MGPipe: " MGP_TRIP_WIRE_TAG("ProtocolCorruption") @@ -863,14 +892,16 @@ namespace MobileGL::MG_Pipe { "have (%u)", record.Res.Slot, record.Res.Gen, stored->Desc.GlNameForDiag, kMGPipeMaxPendingUploads); - return; + return false; } // The serial moves for the buffer half's reason: the twin stamps its own synced // serial from inside the sync that reads this record, so a bump afterwards would // leave it one mutation behind and the next draw would re-upload what it had just // landed. The ACCEPTANCE is what the client reads to clear its own dirty flag - the - // record was accumulated, so the texels are the server's now. + // record was accumulated, so the texels are the server's now, and that is the true + // this returns. ++stored->Serial; + return true; } // ---------------------------------------------------------------------------- @@ -1455,7 +1486,8 @@ namespace MobileGL::MG_Pipe { } } - void MGPipeApplyResourceRespecify(const MGPResourceDesc& desc, const void* initialBytes) { + void MGPipeApplyResourceRespecify(const MGPResourceDesc& desc, const void* initialBytes, + const MGPRespecifiedLevel* level) { Vector* table = ResourceTableForTarget(desc.Target); if (table == nullptr) { MGP_TRIP_WIRE_REPORT("MGPipe: " MGP_TRIP_WIRE_TAG("ProtocolCorruption") @@ -1475,14 +1507,38 @@ namespace MobileGL::MG_Pipe { record->Desc = desc; ++record->Serial; - // A RESPECIFY REDEFINES THE STORE, SO THE PENDING UPLOADS AGAINST THE OLD ONE GO WITH - // IT. They are boxes and rects in a level's coordinate system, and the level that space - // belonged to has just been replaced - a box kept across a shrink would have Espryt - // upload past the end of the new level. Nothing is lost by it: the frontend entry - // points that respecify a texture re-mark the levels they define - // (AllocateStorage then MarkStorageDirty), so what is still owed is re-emitted against - // the storage that now exists. A buffer never has one, so this is inert for P3a's half. - record->PendingUploads.clear(); + // A RESPECIFY REDEFINES A STORE, SO THE PENDING UPLOADS AGAINST THE STORE IT REPLACES + // GO WITH IT - AND ONLY THOSE. They are boxes and rects in a level's coordinate system + // and that space has just been replaced, so a box kept across a shrink would have + // Espryt upload past the end of the new level. But WHICH storage a respecify replaces + // is the call's to say and the descriptor's to say nothing about: + // + // - `level == nullptr` is a WHOLE-RESOURCE redefinition - every glBufferData / + // glBufferStorage, every glTexStorage* (which defines every level at once), every + // texture view - and every key goes. + // - a non-null `level` is ONE glTexImage*D on a mutable texture, which redefines that + // (uploadTarget, level) AND NOTHING ELSE. Dropping the other keys here would lose + // texels the client no longer owes: MG_State's AllocateStorage / MarkStorageDirty + // are per (uploadTarget, level) too, so the re-mark this call causes covers ITS + // level only, while every other level's dirty flag was cleared at its own emission + // (D-D5 step 1). The canonical sequence that loses them is glTexImage2D(0, data) -> + // draw (the shape is emitted and accumulated; Espryt bails because the texture is + // not yet mipmap-complete, Managers.cpp's incomplete-texture early return, which is + // 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) { + record->PendingUploads.clear(); + } else { + // The keys are unique by AccumulatePendingUpload's construction - it looks for the + // pair before it appends - so this erases at most one entry and stops. + for (auto it = record->PendingUploads.begin(); it != record->PendingUploads.end(); ++it) { + if (it->UploadTarget != level->UploadTarget || it->Level != level->Level) continue; + record->PendingUploads.erase(it); + break; + } + } // resource_respecify is the catalogue's only kNeedsAck call, and the per-record half // of that flag is MGPipeResourceRespecifyNeedsAck(desc): glBufferStorage is a real @@ -1499,7 +1555,7 @@ namespace MobileGL::MG_Pipe { } } - void MGPipeApplyResourceSubData(const MGPSubData& record, const void* bytes, + Bool MGPipeApplyResourceSubData(const MGPSubData& record, const void* bytes, const MGPSubRegion* regions) { // ONE CALL, TWO HALVES, and the branch is one comparison. For a buffer the applier // stores NOTHING per record - the contents are the backend's, and the range is the @@ -1509,10 +1565,27 @@ namespace MobileGL::MG_Pipe { if (SubDataNamesABuffer(record)) { MOBILEGL_ASSERT(regions == nullptr, "resource_subdata: the buffer half declares no sub-regions and carries none"); - ApplyBufferWrite("resource_subdata", record, bytes, /*resident=*/false); - return; + return ApplyBufferWrite("resource_subdata", record, bytes, /*resident=*/false); } - ApplyTextureUpload(record, bytes, regions); + // AND THE TEXTURE HALF IS NOT "EVERYTHING ELSE". The table selector's own argument - + // acting on the wrong table would have this applier act outside the storage the record + // names - is exactly the argument for refusing a resource-target half that names no + // texture, and Renderbuffer is the reachable one: it is enumerator 10 and therefore a + // perfectly well-formed MGPSubData::Target, so without this a renderbuffer record would + // 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); + if (resourceTarget == kMGPipeResourceTargetBuffer || + resourceTarget == static_cast(MGPipeResourceTarget::Renderbuffer) || + resourceTarget >= static_cast(MGPipeResourceTarget::Count)) { + 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); + return false; + } + return ApplyTextureUpload(record, bytes, regions); } void MGPipeApplyBufferSubDataResident(const MGPSubData& record, const void* bytes) { diff --git a/MobileGL/MG_Pipe/PipeApply.h b/MobileGL/MG_Pipe/PipeApply.h index 97caa4c0..8fa78a8a 100644 --- a/MobileGL/MG_Pipe/PipeApply.h +++ b/MobileGL/MG_Pipe/PipeApply.h @@ -229,6 +229,22 @@ namespace MobileGL::MG_Pipe { // cannot be recomputed after emission, so the tracker retains the pre-clear set and // the comparator compares the emitted (UnionBox, RegionCount, Regions[]) against it // field by field. + // + // THE SET IS KEYED (UploadTarget, Level) AND EVERY KEY IS INDEPENDENT OF EVERY OTHER. + // That is not a detail: a respecify redefines ONE level when it arrives from + // glTexImage*D (MGPipeApplyResourceRespecify's trailing MGPRespecifiedLevel*), so it + // may only drop that one key - the frontend's AllocateStorage / MarkStorageDirty are + // per (uploadTarget, level) too, and the other levels' dirty flags were cleared at + // THEIR emission, so nothing anywhere still owes them. + // + // THE ACCUMULATED RECT LIST MAY OVERLAP, AND A CONSUMER MUST TOLERATE THAT. Behind one + // level the frontend's own model is pairwise disjoint (MipmapStorage keeps it so), but + // this list CONCATENATES the lists of successive emissions and the applier's gate only + // asks that each rect be inside the record's own union box - so two emissions that + // touch the same texels leave two rects that do. Staging N rects therefore uploads + // those texels twice, which is a cost and never a correctness problem; nothing here + // de-duplicates and nothing downstream may assume "the frontend's model" means disjoint + // once the shapes have been accumulated. struct PendingUpload { Uint16 UploadTarget = 0; Uint16 Level = 0; @@ -607,6 +623,20 @@ namespace MobileGL::MG_Pipe { // the backend and the gates compile against, and the records above are what they write // into; the bodies land in the two commits that follow this one on the same branch. + // The scope of one resource_respecify, and it is an APPLIER-SIDE ARGUMENT and not a wire + // record: it is not in PipeFields.def, it crosses no payload, and the transport reads the + // scope off the call it is replaying rather than off a field. The two members mirror + // MGPipeResourceRecord::PendingUpload's key exactly, which is the only thing the applier + // does with them - so UploadTarget is MGPSubData::Target VERBATIM, the whole packed field + // (ID-12: low byte = MGPipeResourceTarget, high byte = the cube-face upload target), the + // same value the emission of that level put in the record. A per-face respecify therefore + // drops the face it redefines and leaves the other five standing, and a caller that packs + // the pair differently here than it packs it there simply matches nothing. + struct MGPRespecifiedLevel { + Uint16 UploadTarget = 0; + Uint16 Level = 0; + }; + // 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. @@ -614,7 +644,25 @@ namespace MobileGL::MG_Pipe { // 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. - void MGPipeApplyResourceRespecify(const MGPResourceDesc& desc, const void* initialBytes); + // + // P4a: `level` IS THE SCOPE OF THE REDEFINITION, and MGPResourceDesc cannot carry it - the + // descriptor describes the resource, and a mutable texture redefines its levels ONE + // glTexImage*D AT A TIME. Null means "this respecify redefines the WHOLE resource" - every + // glBufferData / glBufferStorage, every glTexStorage*, every texture view - and drops every + // pending upload, which is right because every level's coordinate system has just been + // replaced. Non-null names the single (uploadTarget, level) the call redefines and drops + // ONLY that key: the frontend's AllocateStorage / MarkStorageDirty are per + // (uploadTarget, level) as well (MG_State/GLState/TextureState/TextureObject.h), so a + // glTexImage2D(level 1) re-marks level 1 AND NOTHING ELSE, while the levels already + // emitted had their client dirty flags cleared at THEIR emission (D-D5 step 1) and nothing + // anywhere still owes them. Clearing the whole set here would lose exactly those texels, + // silently, in every build - the loss the server-side set exists to prevent. + // + // 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, + 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. // The applier stores nothing per record - contents are the backend's - and bumps Serial. @@ -625,7 +673,24 @@ namespace MobileGL::MG_Pipe { // applier's pending-upload set is (UnionBox, RegionCount, Regions[]) and the verify lane's // retain mode compares all three. The buffer half declares no regions, so P3a's one call // site and every existing case are unchanged by the default. - void MGPipeApplyResourceSubData(const MGPSubData& record, const void* bytes, + // + // THE RETURN IS THE ACCEPTANCE SIGNAL D-D5 STEP 1 NAMES: true when the record was stored - + // the buffer half landed its range, or the texture half accumulated the shape onto the + // record - and false when it was refused. THE EMITTER MUST GATE ITS DIRTY-FLAG CLEAR ON IT + // ("only for levels whose record the applier ACCEPTED"), because the two refusal paths are + // otherwise invisible to it: a dead or stale handle is a counted no-op and a corrupt record + // is a Fatal that does NOT move RefusedResourceCalls, so in a shipped push build a refused + // upload and an accumulated one are indistinguishable from the call site. A client that + // clears on the strength of having emitted drops those texels for good. + // + // THE RESOURCE-TARGET HALF OF record.Target PICKS THE HALF. MGPSubData::Target is PACKED + // (ID-12): low byte = MGPipeResourceTarget, high byte = the cube-face upload target. The + // buffer half is the whole field being 0 - the encoding the emitter is held to, since a + // buffer has no upload target - and the texture half additionally requires the low byte to + // name a TEXTURE target: Buffer, Renderbuffer and anything at or above + // MGPipeResourceTarget::Count are Fatal{ProtocolCorruption} rather than an upload onto + // whatever object holds that slot in the texture slot space. + Bool MGPipeApplyResourceSubData(const MGPSubData& record, const void* bytes, const MGPSubRegion* regions = nullptr); // buffer_subdata_resident: same shape; `bytes` is the application's staging store and is // valid for the duration of the call only. The op-table entry may be null.