diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index 68375387..a53752ff 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -3524,6 +3524,12 @@ namespace MobileGL::MG_Backend::DirectGLES { const SharedPtr& drawIndirectBuffer, GLsizei drawcount, GLsizei stride, const char* label) { (void)label; + // An indirect command's firstIndex/count live in GPU memory, so the substitution has + // to rewrite the whole element array buffer rather than this draw's range - which is + // exactly what it does when no CPU-known count is handed to it. Held for the whole + // command loop so every command in the batch reads the rewritten copy. + const ScopedRestartIndexSubstitution restart(type, /*count=*/0, /*indices=*/nullptr); + if (!restart.DrawIsValid()) return; const Bool useNative = drawIndirectBuffer != nullptr && SupportsNativeIndirectDraws(); if (useNative) { // gl_BaseInstance must observe GPU-written command fields; expose the indirect @@ -3829,29 +3835,230 @@ namespace MobileGL::MG_Backend::DirectGLES { } } - // GLES core supports only GL_PRIMITIVE_RESTART_FIXED_INDEX (fixed all-ones value). If the app - // enabled the arbitrary GL_PRIMITIVE_RESTART with a non-fixed index, hard-fail at this draw with - // the reason (a fallback would silently drop restarts and corrupt geometry). - void CheckPrimitiveRestartSupported(GLenum indexType) { + // --------------------------------------------------------------------------- + // Arbitrary-index primitive restart + // + // Desktop GL restarts on whatever index glPrimitiveRestartIndex named; GLES core only + // ever restarts on the all-ones value of the index type. When the two agree - which + // includes every GL_PRIMITIVE_RESTART_FIXED_INDEX user - the render state push at + // SyncRenderState is the whole implementation and nothing here does any work. When they + // disagree the index DATA is rewritten into a scratch element array buffer, which is + // what DirectVulkan has always done (VulkanRenderer's RewriteRestartIndices). + // + // This used to throw instead. A throw here unwinds a C++ exception through the C GL ABI + // and takes the process down - the same hazard GL_Texture.cpp and RenderState.cpp + // already call out - so an application that merely asked for a legal desktop feature + // died rather than got an error. + // --------------------------------------------------------------------------- + + namespace { + struct RestartScratchBuffer { + Uint id = 0; + SizeT capacity = 0; + }; + + RestartScratchBuffer g_restartIndices; + Vector g_restartStaging; + + // Past this the rewrite would stage and re-upload hundreds of megabytes on EVERY + // draw (the copy is not memoised, exactly as on the Vulkan side). Decline instead of + // trying: a draw that renders nothing is recoverable, a stall of that size is not. + constexpr SizeT kMaxRestartRewriteBytes = SizeT{1} << 26; // 64 MiB + + SizeT RestartIndexTypeSize(GLenum indexType) { + switch (indexType) { + case GL_UNSIGNED_BYTE: return 1; + case GL_UNSIGNED_SHORT: return 2; + case GL_UNSIGNED_INT: return 4; + default: return 0; + } + } + + // The value GLES restarts on for this index type. Zero for a type that cannot index + // at all, which the caller treats as "nothing to do" - the driver will reject the + // draw on its own terms. + Uint32 FixedRestartIndexFor(GLenum indexType) { + switch (indexType) { + case GL_UNSIGNED_BYTE: return 0xFFu; + case GL_UNSIGNED_SHORT: return 0xFFFFu; + case GL_UNSIGNED_INT: return 0xFFFFFFFFu; + default: return 0; + } + } + + // Copies index data, replacing every occurrence of the application's arbitrary + // restart index with the fixed all-ones value - the only one GLES restarts on. An + // index that already equals the fixed value would then be indistinguishable from a + // restart, so it is nudged to the next-lowest value: it can only be a real index + // (the application's restart index is a different number), and the vertex it selects + // is outside any well-defined draw anyway, whereas leaving it alone would tear the + // primitive in two. Byte-for-byte the rule DirectVulkan applies. + void RewriteRestartIndices(const void* source, SizeT sizeBytes, GLenum indexType, + Uint32 applicationRestartIndex, Vector& output) { + output.resize(sizeBytes); + if (sizeBytes == 0 || source == nullptr) { + return; + } + std::memcpy(output.data(), source, sizeBytes); + const auto rewrite = [&](auto* indices, auto fixedMax) { + const SizeT count = sizeBytes / sizeof(*indices); + for (SizeT i = 0; i < count; ++i) { + if (indices[i] == static_cast(applicationRestartIndex)) { + indices[i] = fixedMax; + } else if (indices[i] == fixedMax) { + indices[i] = static_cast(fixedMax - 1); + } + } + }; + switch (indexType) { + case GL_UNSIGNED_BYTE: + rewrite(reinterpret_cast(output.data()), static_cast(0xFFu)); + break; + case GL_UNSIGNED_SHORT: + rewrite(reinterpret_cast(output.data()), static_cast(0xFFFFu)); + break; + case GL_UNSIGNED_INT: + rewrite(reinterpret_cast(output.data()), static_cast(0xFFFFFFFFu)); + break; + default: + break; + } + } + + // Whole-buffer respecify through the manager-wide staging target, so binding it + // disturbs no VAO state. glBufferData orphans the previous store, so the upload + // never waits on a draw still reading the old contents out of the same name. + Bool UploadRestartScratch(SizeT bytes, const void* data) { + if (g_restartIndices.id == 0) { + GLuint id = 0; + g_GLESFuncs.glGenBuffers(1, &id); + if (id == 0) return false; + g_restartIndices.id = id; + g_restartIndices.capacity = 0; + } + BufferImpl::BindBufferId(BufferImpl::TempBufferTarget, g_restartIndices.id); + SizeT capacity = g_restartIndices.capacity == 0 ? bytes : g_restartIndices.capacity; + while (capacity < bytes) capacity *= 2; + g_GLESFuncs.glBufferData(BufferImpl::TempBufferTarget, static_cast(capacity), nullptr, + GL_STREAM_DRAW); + g_restartIndices.capacity = capacity; + if (data != nullptr && bytes != 0) { + g_GLESFuncs.glBufferSubData(BufferImpl::TempBufferTarget, 0, static_cast(bytes), data); + } + return true; + } + + const SharedPtr& BoundElementArrayBuffer() { + static const SharedPtr none; + const auto& vao = MG_State::pGLContext->GetBoundVertexArray(); + if (!vao) return none; + return vao->GetIndexBufferBindingSlot().GetBoundObject(); + } + + // The GL name PrepareForDraw left on GL_ELEMENT_ARRAY_BUFFER, i.e. what the + // substitution has to put back. + Uint BoundElementArrayBufferId() { + const auto& ibo = BoundElementArrayBuffer(); + if (!ibo) return 0; + const auto* resource = BufferImpl::EnsureBufferResource(ibo); + return resource ? resource->id : 0; + } + } // namespace + + Bool NeedsArbitraryRestartSubstitution(GLenum indexType) { if (!MG_State::pGLContext->IsCapabilityEnabled(CapabilityInput::PrimitiveRestart) || MG_State::pGLContext->IsCapabilityEnabled(CapabilityInput::PrimitiveRestartFixedIndex)) { + return false; + } + const Uint32 fixedMax = FixedRestartIndexFor(indexType); + if (fixedMax == 0) return false; + return MG_State::pGLContext->GetPrimitiveRestartIndex() != fixedMax; + } + + void OnRestartSubstitutionContextDestroyed() { + g_restartIndices = {}; + g_restartStaging.clear(); + g_restartStaging.shrink_to_fit(); + } + + ScopedRestartIndexSubstitution::ScopedRestartIndexSubstitution(GLenum indexType, GLsizei count, + const void* indices) + : m_indices(indices) { + if (!NeedsArbitraryRestartSubstitution(indexType)) { return; } - Uint32 fixedMax = 0; - switch (indexType) { - case GL_UNSIGNED_BYTE: fixedMax = 0xFFu; break; - case GL_UNSIGNED_SHORT: fixedMax = 0xFFFFu; break; - case GL_UNSIGNED_INT: fixedMax = 0xFFFFFFFFu; break; - default: return; + const SizeT indexSize = RestartIndexTypeSize(indexType); + const Uint32 applicationRestartIndex = MG_State::pGLContext->GetPrimitiveRestartIndex(); + const auto& indexBuffer = BoundElementArrayBuffer(); + + if (indexBuffer) { + // The WHOLE buffer is rewritten, not just this draw's range, so that every index + // keeps its position: an indirect draw's firstIndex lives in GPU memory and + // cannot be adjusted from here. Same reasoning, same shape, as DirectVulkan. + const SizeT sizeBytes = indexBuffer->GetSize(); + if (sizeBytes == 0) { + return; // Nothing to restart on; let the driver see the draw unchanged. + } + if (sizeBytes > kMaxRestartRewriteBytes) { + MGLOG_E_ONCE("Draw skipped: GL_PRIMITIVE_RESTART with restart index %u needs the %zu-byte element " + "array buffer rewritten every draw, which is past the %zu-byte ceiling. Use " + "GL_PRIMITIVE_RESTART_FIXED_INDEX, or set glPrimitiveRestartIndex to the all-ones " + "value of the index type.", + applicationRestartIndex, sizeBytes, kMaxRestartRewriteBytes); + m_valid = false; + return; + } + // The shadow is the source of truth for CPU reads, but a persistent map or a + // shader write may have moved past it since the last sync. + indexBuffer->SyncPersistentMappedRange(); + indexBuffer->SyncGpuWrites(); + const Uint8* bytes = indexBuffer->MappedData(); + if (bytes == nullptr) { + MGLOG_E_ONCE("Draw skipped: GL_PRIMITIVE_RESTART with restart index %u needs a CPU-readable copy of " + "the bound element array buffer and none is available.", + applicationRestartIndex); + m_valid = false; + return; + } + RewriteRestartIndices(bytes, sizeBytes, indexType, applicationRestartIndex, g_restartStaging); + } else { + // No element array buffer: `indices` is a client pointer, so only the draw's own + // range is readable and an indirect draw has nothing to read at all. + if (count <= 0 || indices == nullptr || indexSize == 0) { + MGLOG_E_ONCE("Draw skipped: GL_PRIMITIVE_RESTART with restart index %u needs either a bound element " + "array buffer or a client index array with a CPU-known count.", + applicationRestartIndex); + m_valid = false; + return; + } + const SizeT sizeBytes = static_cast(count) * indexSize; + if (sizeBytes > kMaxRestartRewriteBytes) { + MGLOG_E_ONCE("Draw skipped: GL_PRIMITIVE_RESTART index rewrite of %zu bytes is past the %zu-byte " + "ceiling.", + sizeBytes, kMaxRestartRewriteBytes); + m_valid = false; + return; + } + RewriteRestartIndices(indices, sizeBytes, indexType, applicationRestartIndex, g_restartStaging); } - const Uint32 restartIndex = MG_State::pGLContext->GetPrimitiveRestartIndex(); - if (restartIndex != fixedMax) { - THROW_EXCEPTION("GL_PRIMITIVE_RESTART with an arbitrary restart index (" + std::to_string(restartIndex) + - ") is not supported by the GLES backend, which only restarts on the fixed index value (" + - std::to_string(fixedMax) + - ") for this index type; use GL_PRIMITIVE_RESTART_FIXED_INDEX or set glPrimitiveRestartIndex " - "to that value."); + + if (!UploadRestartScratch(g_restartStaging.size(), g_restartStaging.data())) { + MGLOG_E_ONCE("Draw skipped: could not allocate the scratch element array buffer for GL_PRIMITIVE_RESTART " + "index substitution."); + m_valid = false; + return; } + m_previousBinding = BoundElementArrayBufferId(); + BufferImpl::BindBufferId(GL_ELEMENT_ARRAY_BUFFER, g_restartIndices.id); + m_substituted = true; + // The rewritten copy starts at byte 0 of the scratch buffer, so an EBO-sourced draw + // keeps the very offset it was given and a client-memory draw reads from the front. + m_indices = indexBuffer ? indices : nullptr; + } + + ScopedRestartIndexSubstitution::~ScopedRestartIndexSubstitution() { + if (!m_substituted) return; + BufferImpl::BindBufferId(GL_ELEMENT_ARRAY_BUFFER, m_previousBinding); } void DrawElements(GLenum mode, GLsizei count, GLenum type, const void* indices) { @@ -3860,9 +4067,10 @@ namespace MobileGL::MG_Backend::DirectGLES { #endif DrawSyncFlags syncBit = DrawSyncBit::IndexBuffer; PrepareForDraw(syncBit); - CheckPrimitiveRestartSupported(type); + const ScopedRestartIndexSubstitution restart(type, count, indices); + if (!restart.DrawIsValid()) return; ForEachViewportRoutingPass([&] { - g_GLESFuncs.glDrawElements(mode, count, type, indices); + g_GLESFuncs.glDrawElements(mode, count, type, restart.Indices()); }); } @@ -3890,10 +4098,11 @@ namespace MobileGL::MG_Backend::DirectGLES { #endif DrawSyncFlags syncBit = DrawSyncBit::IndexBuffer; PrepareForDraw(syncBit); - CheckPrimitiveRestartSupported(type); + const ScopedRestartIndexSubstitution restart(type, count, indices); + if (!restart.DrawIsValid()) return; SetCurrentBaseVertex(basevertex); ForEachViewportRoutingPass([&] { - g_GLESFuncs.glDrawElementsBaseVertex(mode, count, type, indices, basevertex); + g_GLESFuncs.glDrawElementsBaseVertex(mode, count, type, restart.Indices(), basevertex); }); SetCurrentBaseVertex(0); } @@ -4158,9 +4367,11 @@ namespace MobileGL::MG_Backend::DirectGLES { const void* indices, GLint basevertex) { DrawSyncFlags syncBit = DrawSyncBit::IndexBuffer; PrepareForDraw(syncBit); + const ScopedRestartIndexSubstitution restart(type, count, indices); + if (!restart.DrawIsValid()) return; SetCurrentBaseVertex(basevertex); ForEachViewportRoutingPass([&] { - g_GLESFuncs.glDrawRangeElementsBaseVertex(mode, start, end, count, type, indices, basevertex); + g_GLESFuncs.glDrawRangeElementsBaseVertex(mode, start, end, count, type, restart.Indices(), basevertex); }); SetCurrentBaseVertex(0); } @@ -4168,8 +4379,10 @@ namespace MobileGL::MG_Backend::DirectGLES { void DrawRangeElements(GLenum mode, GLuint start, GLuint end, GLsizei count, GLenum type, const void* indices) { DrawSyncFlags syncBit = DrawSyncBit::IndexBuffer; PrepareForDraw(syncBit); + const ScopedRestartIndexSubstitution restart(type, count, indices); + if (!restart.DrawIsValid()) return; ForEachViewportRoutingPass([&] { - g_GLESFuncs.glDrawRangeElements(mode, start, end, count, type, indices); + g_GLESFuncs.glDrawRangeElements(mode, start, end, count, type, restart.Indices()); }); } @@ -4191,14 +4404,17 @@ namespace MobileGL::MG_Backend::DirectGLES { DrawSyncFlags syncBit = DrawSyncBit::IndexBuffer | DrawSyncBit::Instancing; const VertexArrayImpl::ScopedFetchBaseInstance fetchScope(EmulatedFetchBaseInstance(baseinstance)); PrepareForDraw(syncBit); + const ScopedRestartIndexSubstitution restart(type, count, indices); + if (!restart.DrawIsValid()) return; SetCurrentBaseInstance(baseinstance); SetCurrentBaseVertex(basevertex); ForEachViewportRoutingPass([&] { if (UseNativeBaseInstance()) { - g_GLESFuncs.glDrawElementsInstancedBaseVertexBaseInstanceEXT(mode, count, type, indices, instancecount, - basevertex, baseinstance); + g_GLESFuncs.glDrawElementsInstancedBaseVertexBaseInstanceEXT(mode, count, type, restart.Indices(), + instancecount, basevertex, baseinstance); } else { - g_GLESFuncs.glDrawElementsInstancedBaseVertex(mode, count, type, indices, instancecount, basevertex); + g_GLESFuncs.glDrawElementsInstancedBaseVertex(mode, count, type, restart.Indices(), instancecount, + basevertex); } }); SetCurrentBaseVertex(0); @@ -4209,9 +4425,12 @@ namespace MobileGL::MG_Backend::DirectGLES { GLsizei instancecount, GLint basevertex) { DrawSyncFlags syncBit = DrawSyncBit::IndexBuffer | DrawSyncBit::Instancing; PrepareForDraw(syncBit); + const ScopedRestartIndexSubstitution restart(type, count, indices); + if (!restart.DrawIsValid()) return; SetCurrentBaseVertex(basevertex); ForEachViewportRoutingPass([&] { - g_GLESFuncs.glDrawElementsInstancedBaseVertex(mode, count, type, indices, instancecount, basevertex); + g_GLESFuncs.glDrawElementsInstancedBaseVertex(mode, count, type, restart.Indices(), instancecount, + basevertex); }); SetCurrentBaseVertex(0); } @@ -4221,13 +4440,15 @@ namespace MobileGL::MG_Backend::DirectGLES { DrawSyncFlags syncBit = DrawSyncBit::IndexBuffer | DrawSyncBit::Instancing; const VertexArrayImpl::ScopedFetchBaseInstance fetchScope(EmulatedFetchBaseInstance(baseinstance)); PrepareForDraw(syncBit); + const ScopedRestartIndexSubstitution restart(type, count, indices); + if (!restart.DrawIsValid()) return; SetCurrentBaseInstance(baseinstance); ForEachViewportRoutingPass([&] { if (UseNativeBaseInstance()) { - g_GLESFuncs.glDrawElementsInstancedBaseInstanceEXT(mode, count, type, indices, instancecount, + g_GLESFuncs.glDrawElementsInstancedBaseInstanceEXT(mode, count, type, restart.Indices(), instancecount, baseinstance); } else { - g_GLESFuncs.glDrawElementsInstanced(mode, count, type, indices, instancecount); + g_GLESFuncs.glDrawElementsInstanced(mode, count, type, restart.Indices(), instancecount); } }); SetCurrentBaseInstance(0); @@ -4236,8 +4457,10 @@ namespace MobileGL::MG_Backend::DirectGLES { void DrawElementsInstanced(GLenum mode, GLsizei count, GLenum type, const void* indices, GLsizei instancecount) { DrawSyncFlags syncBit = DrawSyncBit::IndexBuffer | DrawSyncBit::Instancing; PrepareForDraw(syncBit); + const ScopedRestartIndexSubstitution restart(type, count, indices); + if (!restart.DrawIsValid()) return; ForEachViewportRoutingPass([&] { - g_GLESFuncs.glDrawElementsInstanced(mode, count, type, indices, instancecount); + g_GLESFuncs.glDrawElementsInstanced(mode, count, type, restart.Indices(), instancecount); }); } @@ -9996,6 +10219,7 @@ namespace MobileGL::MG_Backend::DirectGLES { BufferImpl::OnBackendContextDestroyed(); XfbImpl::OnBackendContextDestroyed(); MultiDrawImpl::OnBackendContextDestroyed(); + OnRestartSubstitutionContextDestroyed(); ScratchFBOImpl::OnBackendContextDestroyed(); ReleasePackedWordScratchTexture(); FramebufferImpl::InvalidateFramebufferBindingCache(); diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.h b/MobileGL/MG_Backend/DirectGLES/Managers.h index eb646143..79af2709 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.h +++ b/MobileGL/MG_Backend/DirectGLES/Managers.h @@ -102,9 +102,52 @@ namespace MobileGL::MG_Backend::DirectGLES { // Brings the whole draw-relevant frontend state onto the native ES context and binds // the program; every GL draw entry point calls it exactly once before issuing draws. void PrepareForDraw(DrawSyncFlags syncBits); - // GLES core supports only GL_PRIMITIVE_RESTART_FIXED_INDEX. Throws when the app enabled - // the arbitrary GL_PRIMITIVE_RESTART with a non-fixed index for this index type. - void CheckPrimitiveRestartSupported(GLenum indexType); + // Desktop GL restarts indexed primitives on an application-chosen index + // (glPrimitiveRestartIndex under GL_PRIMITIVE_RESTART); GLES core restarts only on the + // all-ones value of the index type (GL_PRIMITIVE_RESTART_FIXED_INDEX), which the render + // state push already enables for both caps. True when the two disagree for this index + // type, i.e. when the index data itself has to be rewritten for the draw to restart + // where the application asked. False - the overwhelmingly common answer - for a draw + // with restart disabled, with the fixed-index cap, or with a restart index that already + // equals the fixed value. + Bool NeedsArbitraryRestartSubstitution(GLenum indexType); + + // Swaps in a scratch element array buffer holding a copy of the index data in which the + // application's restart index has been replaced by the value GLES restarts on. Inert + // (and free) unless NeedsArbitraryRestartSubstitution says otherwise. The swap lives for + // the object's lifetime, so it covers every pass of a viewport-routed draw, and the + // previous GL_ELEMENT_ARRAY_BUFFER name is restored on destruction - which matters + // beyond tidiness, because the VAO twin memoises that it already synced that binding. + class ScopedRestartIndexSubstitution { + public: + // count/indices describe the draw's index range when the CPU knows it. Pass + // count == 0 for an indirect draw, whose count lives in GPU memory: the whole bound + // element array buffer is rewritten instead, so every element keeps its position and + // a GPU-resident firstIndex still addresses the index it named. + ScopedRestartIndexSubstitution(GLenum indexType, GLsizei count, const void* indices); + ~ScopedRestartIndexSubstitution(); + ScopedRestartIndexSubstitution(const ScopedRestartIndexSubstitution&) = delete; + ScopedRestartIndexSubstitution& operator=(const ScopedRestartIndexSubstitution&) = delete; + + // False only when a substitution was needed and could not be made. The draw must + // then be skipped: issuing it would let the driver silently drop every restart and + // weld the primitives on either side together, which is worse than drawing nothing. + Bool DrawIsValid() const { return m_valid; } + // The element-array offset (or client pointer) the draw must use. Identical to what + // was passed in unless a substitution was made. + const void* Indices() const { return m_indices; } + + private: + const void* m_indices = nullptr; + Uint m_previousBinding = 0; + Bool m_substituted = false; + Bool m_valid = true; + }; + + // Drops the scratch element array buffer the substitution above stages through. Like + // MultiDrawImpl's scratch names it is abandoned rather than deleted: the name belongs to + // the dead ES context, and deleting it would target whatever its successor handed out. + void OnRestartSubstitutionContextDestroyed(); // Feed the current program's gl_BaseInstance / gl_DrawID / gl_BaseVertex emulation // uniforms. All are no-ops when the program does not read the corresponding builtin. void SetCurrentBaseInstance(Uint32 baseInstance); diff --git a/MobileGL/MG_Backend/DirectGLES/MultiDraw.cpp b/MobileGL/MG_Backend/DirectGLES/MultiDraw.cpp index db009d52..02625dd6 100644 --- a/MobileGL/MG_Backend/DirectGLES/MultiDraw.cpp +++ b/MobileGL/MG_Backend/DirectGLES/MultiDraw.cpp @@ -29,11 +29,15 @@ namespace MobileGL::MG_Backend::DirectGLES::MultiDrawImpl { } } - // The all-ones value of an index type, which is what GL restarts on once - // primitive restart is in play. CheckPrimitiveRestartSupported has already - // rejected the arbitrary-index form of GL_PRIMITIVE_RESTART, so an enabled - // restart always restarts here and nowhere else. + // The index value this batch restarts on, in the SOURCE index type's width. Normally + // the all-ones value of that type, which is what GL_PRIMITIVE_RESTART_FIXED_INDEX and + // GLES both restart on; with desktop GL_PRIMITIVE_RESTART it is instead whatever + // glPrimitiveRestartIndex named. The rebased tier turns whichever it is into + // 0xFFFFFFFF in its widened stream, which is what the driver restarts on. Uint32 RestartSentinelFor(GLenum type) { + if (NeedsArbitraryRestartSubstitution(type)) { + return MG_State::pGLContext->GetPrimitiveRestartIndex(); + } switch (type) { case GL_UNSIGNED_BYTE: return 0xFFu; case GL_UNSIGNED_SHORT: return 0xFFFFu; @@ -275,10 +279,20 @@ namespace MobileGL::MG_Backend::DirectGLES::MultiDrawImpl { // its remaining feasibility checks inside its implementation, where the data it // has to walk is already in hand. GLESMultiDrawMode ResolveTierForBatch(Bool programReadsDrawID, Bool perSubDrawBaseVertex, - Bool hasIndexBuffer) { + Bool hasIndexBuffer, Bool arbitraryRestart) { ResolveTierOnce(); GLESMultiDrawMode tier = g_resolvedTier; + // Desktop GL_PRIMITIVE_RESTART restarts on an application-chosen index; the driver + // only ever restarts on the all-ones value. Every tier but the rebased one hands + // the application's own index data to the driver, which would then see no restarts + // at all and weld the primitives together. The rebased tier is the one that + // REWRITES the stream, and RestartSentinelFor already tells it which value to + // translate, so it is the only tier this batch can take. + if (arbitraryRestart) { + return GLESMultiDrawMode::DrawElements; + } + // Batched tiers issue one driver entry for the whole batch, so the emulated // gl_DrawID uniform can only hold one value across every sub-draw. A program // that reads gl_DrawID gets an unrolled tier, which feeds each sub-draw its @@ -852,8 +866,10 @@ void main() { void DrawElementsBatch(GLenum mode, const GLsizei* count, GLenum type, const GLvoid* const* indices, GLsizei drawcount, const GLint* basevertex) { if (drawcount <= 0 || !count || !indices) return; - // State-independent and possibly throwing, so it runs before any GL work. - CheckPrimitiveRestartSupported(type); + // Read before any GL work, because it decides the tier below: a desktop restart index + // the driver does not know about can only be honoured by the tier that rewrites the + // index stream (see ResolveTierForBatch). + const Bool arbitraryRestart = NeedsArbitraryRestartSubstitution(type); const Bool hasIndexBuffer = BoundIndexBuffer() != nullptr; @@ -889,7 +905,8 @@ void main() { // the tier choice and the per-sub-draw feeds use those, not the guess above. const Bool feedDrawID = CurrentProgramReadsDrawID(); const Bool feedBaseVertex = basevertex != nullptr && CurrentProgramReadsBaseVertex(); - const GLESMultiDrawMode tier = ResolveTierForBatch(feedDrawID, feedBaseVertex, hasIndexBuffer); + const GLESMultiDrawMode tier = + ResolveTierForBatch(feedDrawID, feedBaseVertex, hasIndexBuffer, arbitraryRestart); Bool drawn = false; switch (tier) { @@ -921,8 +938,10 @@ void main() { // Every tier above may decline a batch whose shape it cannot express. The two // below are the floor: a base-vertex replay where the driver has one, and the // rewritten index stream where it does not. Both are safe for any batch these - // entry points can receive. - if (!drawn) { + // entry points can receive - except that the base-vertex replay hands the + // application's own indices to the driver, which cannot restart on a desktop + // restart index, so that batch has only the rewriting floor. + if (!drawn && !arbitraryRestart) { drawn = RunBaseVertexLoop(mode, count, type, indices, drawcount, basevertex, feedDrawID, feedBaseVertex); } if (!drawn) { diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp index 12e499dd..a0dd8e4f 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp @@ -5029,8 +5029,15 @@ void main() { MG_State::pGLContext->IsCapabilityEnabled(CapabilityInput::PrimitiveRestart) || MG_State::pGLContext->IsCapabilityEnabled(CapabilityInput::PrimitiveRestartFixedIndex); // Primitive restart on a *list* topology requires the primitiveTopologyListRestart feature; - // strip/fan restart works without it. Silently dropping restarts would corrupt geometry, so - // hard-fail here (at the draw) with the reason when the device lacks the feature. + // strip/fan restart works without it. There is no fallback - silently dropping the restarts + // would weld the primitives on either side of each one together - so the draw is declined + // here with the reason. + // + // Declined, not thrown. This used to THROW_EXCEPTION, which unwinds a C++ exception through + // the C GL ABI and takes the process down (the hazard GL_Texture.cpp and RenderState.cpp + // already name); an application that merely enabled a legal desktop feature died instead of + // getting a draw that rendered nothing. VK_NULL_HANDLE is this function's established + // "skip this draw" answer, used by the no-stages case above. const auto isListTopology = [](VkPrimitiveTopology t) { return t == VK_PRIMITIVE_TOPOLOGY_POINT_LIST || t == VK_PRIMITIVE_TOPOLOGY_LINE_LIST || t == VK_PRIMITIVE_TOPOLOGY_TRIANGLE_LIST || @@ -5038,9 +5045,12 @@ void main() { t == VK_PRIMITIVE_TOPOLOGY_TRIANGLE_LIST_WITH_ADJACENCY || t == VK_PRIMITIVE_TOPOLOGY_PATCH_LIST; }; if (primitiveRestartEnabled && !m_primitiveTopologyListRestartFeatureEnabled && isListTopology(vkTopology)) { - THROW_EXCEPTION("Primitive restart on a list topology requires the primitiveTopologyListRestart device " - "feature (VK_EXT_primitive_topology_list_restart), which this device does not support; use " - "a strip/fan topology or a device that supports it."); + MGLOG_E_ONCE("Draw skipped: primitive restart on a list topology (0x%x) requires the " + "primitiveTopologyListRestart device feature (VK_EXT_primitive_topology_list_restart), " + "which this device does not support; use a strip/fan topology, or disable primitive " + "restart for list-topology draws.", + mode); + return VK_NULL_HANDLE; } PipelineFactory::PipelineCreatePayload payload { diff --git a/MobileGL/MG_IntegrationTest/CMakeLists.txt b/MobileGL/MG_IntegrationTest/CMakeLists.txt index 5de3c0a9..261b6eaa 100644 --- a/MobileGL/MG_IntegrationTest/CMakeLists.txt +++ b/MobileGL/MG_IntegrationTest/CMakeLists.txt @@ -63,6 +63,7 @@ add_executable(MobileGLIntegrationTest Scenarios/PipelineFailureScenario.cpp Scenarios/AdvertisedLimitsScenario.cpp Scenarios/PixelStoreSweepScenario.cpp + Scenarios/PrimitiveRestartScenario.cpp Scenarios/FragCoordOriginScenario.cpp Scenarios/ClearThenReadPixelsScenario.cpp Scenarios/DepthStencilReadbackScenario.cpp diff --git a/MobileGL/MG_IntegrationTest/Scenarios/PrimitiveRestartScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/PrimitiveRestartScenario.cpp new file mode 100644 index 00000000..0b6f999c --- /dev/null +++ b/MobileGL/MG_IntegrationTest/Scenarios/PrimitiveRestartScenario.cpp @@ -0,0 +1,301 @@ +// MobileGL - MobileGL/MG_IntegrationTest/Scenarios/PrimitiveRestartScenario.cpp +// Copyright (c) 2025-2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header +// +// Scenario - DESKTOP GL_PRIMITIVE_RESTART WITH AN APPLICATION-CHOSEN INDEX. +// +// Desktop GL restarts on whatever glPrimitiveRestartIndex named; GLES and Vulkan both restart +// only on the all-ones value of the index type. DirectGLES used to THROW_EXCEPTION on the +// mismatch, and a throw out of a GL entry point unwinds a C++ exception through the C ABI and +// kills the process - which is how KHR-GL4x.geometry_shader.primitive_counter.*_rp took the whole +// conformance runner down, nine bodies at a time, losing every result in the chunk with it. +// +// So the first thing this asserts is simply that the process is still here. The second is that +// the restart actually happened: the substitution rewrites the index data so the driver restarts +// where the application asked, and the difference between "restart honoured" and "restart +// silently dropped" is a triangle strip that welds its two halves together across the gap. +// +// Needs a real context on purpose. The GPU-free suite cannot reach a backend at all, and this is +// entirely about what the backend does with the index buffer. + +#include +#include +#include +#include +#include + +#include "../Harness/HeadlessGL.h" +#include "../Harness/ScenarioFixture.h" + +#ifdef GLAPI +#undef GLAPI +#endif +#define GL_GLEXT_PROTOTYPES +#include +#include +#undef GL_GLEXT_PROTOTYPES + +namespace MGITest { + namespace { + + constexpr GLsizei kSurface = 64; + + const char* const kVertexSource = R"(#version 420 core +layout(location = 0) in vec2 a_position; +void main() +{ + gl_Position = vec4(a_position, 0.0, 1.0); +} +)"; + + const char* const kFragmentSource = R"(#version 420 core +out vec4 fragColor; +void main() +{ + fragColor = vec4(0.0, 1.0, 0.0, 1.0); +} +)"; + + // Two triangles with a gap down the middle, plus two spare vertices parked at the origin. + // + // The spares exist so the restart index is a LEGAL vertex index: if the restart were + // dropped the driver would still fetch a real vertex rather than read out of bounds, so + // the negative case is defined behaviour and the test measures the restart rather than + // whatever robust-buffer-access does. + constexpr GLfloat kVertices[] = { + -0.9f, -0.9f, // 0 - left triangle + -0.1f, -0.9f, // 1 + -0.9f, 0.9f, // 2 + 0.1f, -0.9f, // 3 - right triangle + 0.9f, -0.9f, // 4 + 0.9f, 0.9f, // 5 + 0.0f, 0.0f, // 6 - spare + 0.0f, 0.0f, // 7 - spare, and the application's restart index + }; + constexpr GLuint kRestartIndex = 7; + + // A triangle STRIP, restarted in the middle: honoured, it is exactly the two triangles + // above. Dropped, the strip welds vertices 2, 7 and 3 into extra triangles that spill + // across the gap - which is what the middle probe below catches. + constexpr GLuint kIndices[] = {0, 1, 2, kRestartIndex, 3, 4, 5}; + + struct Pixel { + GLubyte r = 0, g = 0, b = 0, a = 0; + }; + + class PrimitiveRestartScenario : public ScenarioTest { + protected: + void SetUp() override { + ScenarioTest::SetUp(); + if (!Ready()) return; + + glGenVertexArrays(1, &m_vao); + glBindVertexArray(m_vao); + + glGenBuffers(1, &m_vbo); + glBindBuffer(GL_ARRAY_BUFFER, m_vbo); + glBufferData(GL_ARRAY_BUFFER, sizeof(kVertices), kVertices, GL_STATIC_DRAW); + glVertexAttribPointer(0, 2, GL_FLOAT, GL_FALSE, 2 * sizeof(GLfloat), nullptr); + glEnableVertexAttribArray(0); + + glGenBuffers(1, &m_ebo); + glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, m_ebo); + glBufferData(GL_ELEMENT_ARRAY_BUFFER, sizeof(kIndices), kIndices, GL_STATIC_DRAW); + + glGenTextures(1, &m_colorTexture); + glBindTexture(GL_TEXTURE_2D, m_colorTexture); + glTexStorage2D(GL_TEXTURE_2D, 1, GL_RGBA8, kSurface, kSurface); + glGenFramebuffers(1, &m_fbo); + glBindFramebuffer(GL_FRAMEBUFFER, m_fbo); + glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, m_colorTexture, 0); + ASSERT_EQ(glCheckFramebufferStatus(GL_FRAMEBUFFER), + static_cast(GL_FRAMEBUFFER_COMPLETE)); + glViewport(0, 0, kSurface, kSurface); + + m_program = BuildProgram(); + ASSERT_NE(m_program, 0u) << "the flat-colour program did not build: " << m_buildLog; + glUseProgram(m_program); + DrainErrors(); + } + + void TearDown() override { + if (!Ready()) return; + glDisable(GL_PRIMITIVE_RESTART); + glDisable(GL_PRIMITIVE_RESTART_FIXED_INDEX); + glPrimitiveRestartIndex(0); + glUseProgram(0); + if (m_program != 0) glDeleteProgram(m_program); + glBindFramebuffer(GL_FRAMEBUFFER, 0); + if (m_fbo != 0) glDeleteFramebuffers(1, &m_fbo); + if (m_colorTexture != 0) glDeleteTextures(1, &m_colorTexture); + glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, 0); + glBindBuffer(GL_ARRAY_BUFFER, 0); + if (m_ebo != 0) glDeleteBuffers(1, &m_ebo); + if (m_vbo != 0) glDeleteBuffers(1, &m_vbo); + glBindVertexArray(0); + if (m_vao != 0) glDeleteVertexArrays(1, &m_vao); + DrainErrors(); + } + + static void DrainErrors() { + for (int i = 0; i < 16 && glGetError() != GL_NO_ERROR; ++i) { + } + } + + GLuint BuildProgram() { + const GLuint vs = glCreateShader(GL_VERTEX_SHADER); + glShaderSource(vs, 1, &kVertexSource, nullptr); + glCompileShader(vs); + const GLuint fs = glCreateShader(GL_FRAGMENT_SHADER); + glShaderSource(fs, 1, &kFragmentSource, nullptr); + glCompileShader(fs); + const GLuint program = glCreateProgram(); + glAttachShader(program, vs); + glAttachShader(program, fs); + glLinkProgram(program); + GLint linked = 0; + glGetProgramiv(program, GL_LINK_STATUS, &linked); + glDeleteShader(vs); + glDeleteShader(fs); + if (!linked) { + GLint length = 0; + glGetProgramiv(program, GL_INFO_LOG_LENGTH, &length); + std::vector buffer(static_cast(length) + 1, '\0'); + glGetProgramInfoLog(program, length + 1, nullptr, buffer.data()); + m_buildLog = buffer.data(); + glDeleteProgram(program); + return 0; + } + return program; + } + + // The whole surface, so a failure can report the three probes together rather than + // three separate readbacks that might disagree about which draw they saw. + std::vector DrawAndRead() { + glClearColor(0.0f, 0.0f, 0.0f, 1.0f); + glClear(GL_COLOR_BUFFER_BIT); + glDrawElements(GL_TRIANGLE_STRIP, static_cast(std::size(kIndices)), GL_UNSIGNED_INT, + nullptr); + std::vector pixels(static_cast(kSurface) * kSurface); + glReadPixels(0, 0, kSurface, kSurface, GL_RGBA, GL_UNSIGNED_BYTE, pixels.data()); + return pixels; + } + + static const Pixel& At(const std::vector& pixels, int x, int y) { + return pixels[static_cast(y) * kSurface + x]; + } + + static bool IsGreen(const Pixel& p) { return p.g > 128 && p.r < 128; } + + // NDC (-0.5, -0.5): well inside the left triangle whichever way the restart went. + static constexpr int kLeftX = 16, kLeftY = 16; + // NDC (0.6, -0.5): well inside the right triangle, and outside every welded one. + static constexpr int kRightX = 51, kRightY = 16; + // NDC (0.2, -0.5): in the gap between the two triangles, and INSIDE the triangle the + // strip welds out of vertices 7, 3 and 4 when the restart is dropped. This is the + // probe that distinguishes a working restart from a silently ignored one. + static constexpr int kGapX = 38, kGapY = 16; + + GLuint m_vao = 0; + GLuint m_vbo = 0; + GLuint m_ebo = 0; + GLuint m_fbo = 0; + GLuint m_colorTexture = 0; + GLuint m_program = 0; + std::string m_buildLog; + }; + + // THE crash regression. Before the fix this call never returned: DirectGLES threw + // std::runtime_error out of glDrawElements and the process died on the spot. Reaching the + // assertion at all is most of the point. + TEST_F(PrimitiveRestartScenario, AnArbitraryRestartIndexDrawsInsteadOfKillingTheProcess) { + if (!Ready()) GTEST_SKIP(); + + glEnable(GL_PRIMITIVE_RESTART); + glPrimitiveRestartIndex(kRestartIndex); + ASSERT_EQ(glGetError(), static_cast(GL_NO_ERROR)); + + const std::vector pixels = DrawAndRead(); + EXPECT_EQ(glGetError(), static_cast(GL_NO_ERROR)) + << "an arbitrary restart index is legal desktop GL and must raise no error"; + + EXPECT_TRUE(IsGreen(At(pixels, kLeftX, kLeftY))) << "the first strip half did not render"; + EXPECT_TRUE(IsGreen(At(pixels, kRightX, kRightY))) << "the second strip half did not render"; + EXPECT_FALSE(IsGreen(At(pixels, kGapX, kGapY))) + << "the gap between the two halves is covered, so the restart was dropped and the " + "strip welded across it"; + } + + // The other half of the state: an application that sets the restart index TO the fixed + // all-ones value needs no rewriting at all, and the cap must map straight onto the + // driver's own fixed-index restart. Same picture, different path through the backend. + TEST_F(PrimitiveRestartScenario, TheFixedIndexValueTakesTheForwardingPath) { + if (!Ready()) GTEST_SKIP(); + + // Index 0xFFFFFFFF is not a vertex this draw uses, so the strip is the same shape. + const GLuint fixedIndices[] = {0, 1, 2, 0xFFFFFFFFu, 3, 4, 5}; + glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, m_ebo); + glBufferSubData(GL_ELEMENT_ARRAY_BUFFER, 0, sizeof(fixedIndices), fixedIndices); + + glEnable(GL_PRIMITIVE_RESTART); + glPrimitiveRestartIndex(0xFFFFFFFFu); + ASSERT_EQ(glGetError(), static_cast(GL_NO_ERROR)); + + const std::vector pixels = DrawAndRead(); + EXPECT_EQ(glGetError(), static_cast(GL_NO_ERROR)); + EXPECT_TRUE(IsGreen(At(pixels, kLeftX, kLeftY))); + EXPECT_TRUE(IsGreen(At(pixels, kRightX, kRightY))); + EXPECT_FALSE(IsGreen(At(pixels, kGapX, kGapY))); + + // Put the buffer back for whatever runs next in this fixture. + glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, m_ebo); + glBufferSubData(GL_ELEMENT_ARRAY_BUFFER, 0, sizeof(kIndices), kIndices); + DrainErrors(); + } + + // With the cap off, the same index data is just data - nothing restarts, and the strip + // welds across the gap. The negative control for the probe above: without it, a backend + // that lost the whole draw would pass the test by rendering nothing in the gap. + TEST_F(PrimitiveRestartScenario, WithoutTheCapTheStripWeldsAcrossTheGap) { + if (!Ready()) GTEST_SKIP(); + + glDisable(GL_PRIMITIVE_RESTART); + glPrimitiveRestartIndex(kRestartIndex); + ASSERT_EQ(glGetError(), static_cast(GL_NO_ERROR)); + + const std::vector pixels = DrawAndRead(); + EXPECT_EQ(glGetError(), static_cast(GL_NO_ERROR)); + EXPECT_TRUE(IsGreen(At(pixels, kLeftX, kLeftY))) << "the draw itself must still happen"; + EXPECT_TRUE(IsGreen(At(pixels, kGapX, kGapY))) + << "with restart disabled the strip is continuous, so the gap must be covered - if " + "it is not, the probe above proves nothing"; + } + + // A second draw with a DIFFERENT restart index has to be rewritten again. The substitution + // stages through one scratch buffer, so a cached or half-restored element-array binding + // would show up here as the second draw reusing the first one's data. + TEST_F(PrimitiveRestartScenario, ChangingTheRestartIndexBetweenDrawsIsHonoured) { + if (!Ready()) GTEST_SKIP(); + + glEnable(GL_PRIMITIVE_RESTART); + glPrimitiveRestartIndex(kRestartIndex); + const std::vector restarted = DrawAndRead(); + ASSERT_EQ(glGetError(), static_cast(GL_NO_ERROR)); + EXPECT_FALSE(IsGreen(At(restarted, kGapX, kGapY))); + + // 6 is the other spare vertex, and it appears nowhere in the index data - so nothing + // restarts and the strip is continuous again, from the very same buffer. + glPrimitiveRestartIndex(6); + const std::vector notRestarted = DrawAndRead(); + ASSERT_EQ(glGetError(), static_cast(GL_NO_ERROR)); + EXPECT_TRUE(IsGreen(At(notRestarted, kLeftX, kLeftY))); + EXPECT_TRUE(IsGreen(At(notRestarted, kGapX, kGapY))) + << "the second draw restarted on an index that is not in its data"; + } + + } // namespace +} // namespace MGITest