diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index 1a2b7003..07cb2a66 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp @@ -2467,13 +2467,18 @@ namespace MobileGL::MG_Backend::DirectGLES { // makes it stale too. Both level comparisons sit INSIDE the >= 0 guard: a program // with a control stage of its own - which is nearly all of them - still pays only // the one integer test. + // + // Compared by BIT PATTERN, matching what DirectVulkan hashes into its module key. + // A float compare here would never settle for a NaN level - NaN != NaN - and every + // draw of that program would re-transpile, re-compile and re-link a byte-identical + // shader. glPatchParameterfv accepts NaN by design. (twin->GetPassthroughTessControlPatchVertices() >= 0 && (twin->GetPassthroughTessControlPatchVertices() != static_cast(MG_State::pGLContext->GetPatchVertices()) || - twin->GetPassthroughTessControlOuterLevel() != - MG_State::pGLContext->GetPatchDefaultOuterLevel() || - twin->GetPassthroughTessControlInnerLevel() != - MG_State::pGLContext->GetPatchDefaultInnerLevel()))) { + !BitwiseEqual(twin->GetPassthroughTessControlOuterLevel(), + MG_State::pGLContext->GetPatchDefaultOuterLevel()) || + !BitwiseEqual(twin->GetPassthroughTessControlInnerLevel(), + MG_State::pGLContext->GetPatchDefaultInnerLevel())))) { twin->SyncToBackend(currentProgram); } g_currentDrawFrontendProgram = currentProgram.get(); @@ -3538,8 +3543,14 @@ namespace MobileGL::MG_Backend::DirectGLES { // 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. + // + // firstIndex counts ELEMENTS, so it survives a widened copy untouched; what does not + // survive is the type and the element size, which are re-taken from the substitution + // below for both the native and the CPU-unrolled path. const ScopedRestartIndexSubstitution restart(type, /*count=*/0, /*indices=*/nullptr); if (!restart.DrawIsValid()) return; + type = restart.IndexType(); + indexSize = MG_Util::GetGLTypeSize(type); const Bool useNative = drawIndirectBuffer != nullptr && SupportsNativeIndirectDraws(); if (useNative) { // gl_BaseInstance must observe GPU-written command fields; expose the indirect @@ -3852,8 +3863,7 @@ namespace MobileGL::MG_Backend::DirectGLES { // 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). + // disagree the index DATA is rewritten into a scratch element array buffer. // // 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 @@ -3875,63 +3885,82 @@ namespace MobileGL::MG_Backend::DirectGLES { // 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) { + // The index type one step wider than this one, or 0 when there is none. Widening is how + // an all-ones value that is a REAL vertex index keeps its meaning while the all-ones + // value of the destination type serves as the restart sentinel: a source that cannot + // spell 0xFFFF cannot collide with a 16-bit sentinel, and likewise 8 -> 16. + GLenum WiderIndexType(GLenum indexType) { switch (indexType) { - case GL_UNSIGNED_BYTE: return 1; - case GL_UNSIGNED_SHORT: return 2; - case GL_UNSIGNED_INT: return 4; + case GL_UNSIGNED_BYTE: return GL_UNSIGNED_SHORT; + case GL_UNSIGNED_SHORT: return GL_UNSIGNED_INT; 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; + Uint32 ReadIndex(const Uint8* source, SizeT i, SizeT indexSize) { + switch (indexSize) { + case 1: return source[i]; + case 2: { + Uint16 narrow = 0; + std::memcpy(&narrow, source + i * 2, sizeof(narrow)); + return narrow; + } + default: { + Uint32 wide = 0; + std::memcpy(&wide, source + i * 4, sizeof(wide)); + return wide; + } } } - // 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; + void WriteIndex(Uint8* destination, SizeT i, SizeT indexSize, Uint32 value) { + switch (indexSize) { + case 1: destination[i] = static_cast(value); break; + case 2: { + const Uint16 narrow = static_cast(value); + std::memcpy(destination + i * 2, &narrow, sizeof(narrow)); + break; } - 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); - } + default: std::memcpy(destination + i * 4, &value, sizeof(value)); break; + } + } + + // True when any index in the range already holds the type's all-ones value, i.e. when + // that value is doing double duty as a real vertex index and so cannot also be the + // restart sentinel. Only asked on the rare substitution path. + Bool ContainsFixedRestartIndex(const Uint8* source, SizeT indexCount, SizeT indexSize, + Uint32 fixedMax) { + for (SizeT i = 0; i < indexCount; ++i) { + if (ReadIndex(source, i, indexSize) == fixedMax) return true; + } + return false; + } + + // Copies index data, replacing every occurrence of the application's restart index with + // the all-ones value of the DESTINATION type - the only one GLES restarts on. The + // destination may be wider than the source, which is what makes the copy lossless: a + // source index equal to the source's all-ones value zero-extends to something the wider + // sentinel can never equal, so it stays the vertex it was. + // + // Same width in and out is the degenerate case, used when the source contains no + // all-ones index at all (nothing to protect) or when there is no wider type to move to. + // In that last case only - a GL_UNSIGNED_INT stream that really does use index + // 0xFFFFFFFF while asking to restart on a different one - a legal index has to be + // nudged to 0xFFFFFFFE, because 32 bits cannot hold both meanings. The caller logs it; + // it is the one input this feature cannot represent. + void RewriteRestartIndices(const Uint8* source, SizeT indexCount, SizeT sourceIndexSize, + SizeT destinationIndexSize, Uint32 applicationRestartIndex, + Uint32 destinationFixedMax, Vector& output) { + output.resize(indexCount * destinationIndexSize); + for (SizeT i = 0; i < indexCount; ++i) { + Uint32 value = ReadIndex(source, i, sourceIndexSize); + if (value == applicationRestartIndex) { + value = destinationFixedMax; + } else if (value == destinationFixedMax) { + // Only reachable when no widening was possible; see above. + value = destinationFixedMax - 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; + WriteIndex(output.data(), i, destinationIndexSize, value); } } @@ -3975,14 +4004,21 @@ namespace MobileGL::MG_Backend::DirectGLES { } } // namespace - Bool NeedsArbitraryRestartSubstitution(GLenum indexType) { + RestartSubstitutionKind ResolveRestartSubstitution(GLenum indexType) { if (!MG_State::pGLContext->IsCapabilityEnabled(CapabilityInput::PrimitiveRestart) || MG_State::pGLContext->IsCapabilityEnabled(CapabilityInput::PrimitiveRestartFixedIndex)) { - return false; + return RestartSubstitutionKind::None; } - const Uint32 fixedMax = FixedRestartIndexFor(indexType); - if (fixedMax == 0) return false; - return MG_State::pGLContext->GetPrimitiveRestartIndex() != fixedMax; + const Uint32 fixedMax = MG_Util::FixedRestartIndexForGLType(indexType); + if (fixedMax == 0) return RestartSubstitutionKind::None; + const Uint32 restartIndex = MG_State::pGLContext->GetPrimitiveRestartIndex(); + if (restartIndex == fixedMax) return RestartSubstitutionKind::None; + // Strictly greater, never truncated. GL 4.6 core 10.3.6 compares the fetched index + // zero-extended against the full 32-bit state, so an index this type cannot hold matches + // nothing. Truncating instead - glPrimitiveRestartIndex(0x100) over GL_UNSIGNED_BYTE data + // becoming "restart on 0" - turns the most common index in any mesh into a restart. + if (restartIndex > fixedMax) return RestartSubstitutionKind::SuppressRestart; + return RestartSubstitutionKind::RewriteIndices; } void OnRestartSubstitutionContextDestroyed() { @@ -3991,22 +4027,43 @@ namespace MobileGL::MG_Backend::DirectGLES { g_restartStaging.shrink_to_fit(); } + ScopedSuppressedPrimitiveRestart::ScopedSuppressedPrimitiveRestart(RestartSubstitutionKind kind) { + if (kind != RestartSubstitutionKind::SuppressRestart) return; + // SyncRenderState turned the driver's fixed-index restart on because GL_PRIMITIVE_RESTART + // is enabled; for this draw's index type it would restart on a value the application + // never named. Toggled directly rather than through the render-state shadow, and put back + // in the destructor, so the shadow stays true and the next draw pays nothing. + g_GLESFuncs.glDisable(GL_PRIMITIVE_RESTART_FIXED_INDEX); + m_suppressed = true; + } + + ScopedSuppressedPrimitiveRestart::~ScopedSuppressedPrimitiveRestart() { + if (!m_suppressed) return; + g_GLESFuncs.glEnable(GL_PRIMITIVE_RESTART_FIXED_INDEX); + } + ScopedRestartIndexSubstitution::ScopedRestartIndexSubstitution(GLenum indexType, GLsizei count, const void* indices) - : m_indices(indices) { - if (!NeedsArbitraryRestartSubstitution(indexType)) { + : m_kind(ResolveRestartSubstitution(indexType)), m_capOverride(m_kind), m_indices(indices), + m_indexType(indexType) { + if (m_kind != RestartSubstitutionKind::RewriteIndices) { return; } - const SizeT indexSize = RestartIndexTypeSize(indexType); + const SizeT sourceIndexSize = MG_Util::GetGLTypeSize(indexType); + const Uint32 fixedMax = MG_Util::FixedRestartIndexForGLType(indexType); const Uint32 applicationRestartIndex = MG_State::pGLContext->GetPrimitiveRestartIndex(); const auto& indexBuffer = BoundElementArrayBuffer(); + const Uint8* source = nullptr; + SizeT indexCount = 0; + SizeT sourceByteOffset = 0; + 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. + // keeps its position: an indirect draw's firstIndex lives in GPU memory and cannot be + // adjusted from here. It is an ELEMENT index, so it survives widening unchanged. const SizeT sizeBytes = indexBuffer->GetSize(); - if (sizeBytes == 0) { + if (sizeBytes < sourceIndexSize) { return; // Nothing to restart on; let the driver see the draw unchanged. } if (sizeBytes > kMaxRestartRewriteBytes) { @@ -4022,36 +4079,64 @@ namespace MobileGL::MG_Backend::DirectGLES { // shader write may have moved past it since the last sync. indexBuffer->SyncPersistentMappedRange(); indexBuffer->SyncGpuWrites(); - const Uint8* bytes = indexBuffer->MappedData(); - if (bytes == nullptr) { + source = indexBuffer->MappedData(); + if (source == 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); + indexCount = sizeBytes / sourceIndexSize; + sourceByteOffset = reinterpret_cast(indices); } 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) { + if (count <= 0 || indices == nullptr || sourceIndexSize == 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) { + if (static_cast(count) * sourceIndexSize > kMaxRestartRewriteBytes) { MGLOG_E_ONCE("Draw skipped: GL_PRIMITIVE_RESTART index rewrite of %zu bytes is past the %zu-byte " "ceiling.", - sizeBytes, kMaxRestartRewriteBytes); + static_cast(count) * sourceIndexSize, kMaxRestartRewriteBytes); m_valid = false; return; } - RewriteRestartIndices(indices, sizeBytes, indexType, applicationRestartIndex, g_restartStaging); + source = static_cast(indices); + indexCount = static_cast(count); } + // Widen only when the source really does use the all-ones value as a vertex index - + // otherwise the sentinel is free and the copy stays the caller's width, which keeps the + // common substitution allocation-for-allocation identical to the narrow form. + GLenum destinationType = indexType; + SizeT destinationIndexSize = sourceIndexSize; + if (ContainsFixedRestartIndex(source, indexCount, sourceIndexSize, fixedMax)) { + const GLenum wider = WiderIndexType(indexType); + // An element-array offset that is not a whole number of indices cannot be rescaled + // into the widened copy, so such a draw keeps the narrow (lossy) form. + const Bool offsetIsWholeIndices = sourceIndexSize != 0 && (sourceByteOffset % sourceIndexSize) == 0; + if (wider != 0 && offsetIsWholeIndices && + indexCount * MG_Util::GetGLTypeSize(wider) <= kMaxRestartRewriteBytes) { + destinationType = wider; + destinationIndexSize = MG_Util::GetGLTypeSize(wider); + } else { + MGLOG_E_ONCE("GL_PRIMITIVE_RESTART with restart index %u over index data that also uses the " + "all-ones index %u: this index type cannot spell both, so every all-ones index is " + "drawn as %u instead. Use GL_PRIMITIVE_RESTART_FIXED_INDEX, or keep the all-ones " + "value out of the index data.", + applicationRestartIndex, fixedMax, fixedMax - 1); + } + } + + const Uint32 destinationFixedMax = MG_Util::FixedRestartIndexForGLType(destinationType); + RewriteRestartIndices(source, indexCount, sourceIndexSize, destinationIndexSize, applicationRestartIndex, + destinationFixedMax, g_restartStaging); + 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."); @@ -4061,9 +4146,13 @@ namespace MobileGL::MG_Backend::DirectGLES { 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; + m_indexType = destinationType; + // The rewritten copy starts at byte 0 of the scratch buffer and holds one + // destination-width element per source element, so an EBO-sourced draw keeps its ELEMENT + // offset (rescaled to the new width) and a client-memory draw reads from the front. + m_indices = indexBuffer + ? reinterpret_cast((sourceByteOffset / sourceIndexSize) * destinationIndexSize) + : nullptr; } ScopedRestartIndexSubstitution::~ScopedRestartIndexSubstitution() { @@ -4080,7 +4169,7 @@ namespace MobileGL::MG_Backend::DirectGLES { const ScopedRestartIndexSubstitution restart(type, count, indices); if (!restart.DrawIsValid()) return; ForEachViewportRoutingPass([&] { - g_GLESFuncs.glDrawElements(mode, count, type, restart.Indices()); + g_GLESFuncs.glDrawElements(mode, count, restart.IndexType(), restart.Indices()); }); } @@ -4112,7 +4201,7 @@ namespace MobileGL::MG_Backend::DirectGLES { if (!restart.DrawIsValid()) return; SetCurrentBaseVertex(basevertex); ForEachViewportRoutingPass([&] { - g_GLESFuncs.glDrawElementsBaseVertex(mode, count, type, restart.Indices(), basevertex); + g_GLESFuncs.glDrawElementsBaseVertex(mode, count, restart.IndexType(), restart.Indices(), basevertex); }); SetCurrentBaseVertex(0); } @@ -4381,7 +4470,8 @@ namespace MobileGL::MG_Backend::DirectGLES { if (!restart.DrawIsValid()) return; SetCurrentBaseVertex(basevertex); ForEachViewportRoutingPass([&] { - g_GLESFuncs.glDrawRangeElementsBaseVertex(mode, start, end, count, type, restart.Indices(), basevertex); + g_GLESFuncs.glDrawRangeElementsBaseVertex(mode, start, end, count, restart.IndexType(), restart.Indices(), + basevertex); }); SetCurrentBaseVertex(0); } @@ -4392,7 +4482,7 @@ namespace MobileGL::MG_Backend::DirectGLES { const ScopedRestartIndexSubstitution restart(type, count, indices); if (!restart.DrawIsValid()) return; ForEachViewportRoutingPass([&] { - g_GLESFuncs.glDrawRangeElements(mode, start, end, count, type, restart.Indices()); + g_GLESFuncs.glDrawRangeElements(mode, start, end, count, restart.IndexType(), restart.Indices()); }); } @@ -4420,11 +4510,12 @@ namespace MobileGL::MG_Backend::DirectGLES { SetCurrentBaseVertex(basevertex); ForEachViewportRoutingPass([&] { if (UseNativeBaseInstance()) { - g_GLESFuncs.glDrawElementsInstancedBaseVertexBaseInstanceEXT(mode, count, type, restart.Indices(), - instancecount, basevertex, baseinstance); + g_GLESFuncs.glDrawElementsInstancedBaseVertexBaseInstanceEXT(mode, count, restart.IndexType(), + restart.Indices(), instancecount, + basevertex, baseinstance); } else { - g_GLESFuncs.glDrawElementsInstancedBaseVertex(mode, count, type, restart.Indices(), instancecount, - basevertex); + g_GLESFuncs.glDrawElementsInstancedBaseVertex(mode, count, restart.IndexType(), restart.Indices(), + instancecount, basevertex); } }); SetCurrentBaseVertex(0); @@ -4455,10 +4546,10 @@ namespace MobileGL::MG_Backend::DirectGLES { SetCurrentBaseInstance(baseinstance); ForEachViewportRoutingPass([&] { if (UseNativeBaseInstance()) { - g_GLESFuncs.glDrawElementsInstancedBaseInstanceEXT(mode, count, type, restart.Indices(), instancecount, - baseinstance); + g_GLESFuncs.glDrawElementsInstancedBaseInstanceEXT(mode, count, restart.IndexType(), restart.Indices(), + instancecount, baseinstance); } else { - g_GLESFuncs.glDrawElementsInstanced(mode, count, type, restart.Indices(), instancecount); + g_GLESFuncs.glDrawElementsInstanced(mode, count, restart.IndexType(), restart.Indices(), instancecount); } }); SetCurrentBaseInstance(0); @@ -4470,7 +4561,7 @@ namespace MobileGL::MG_Backend::DirectGLES { const ScopedRestartIndexSubstitution restart(type, count, indices); if (!restart.DrawIsValid()) return; ForEachViewportRoutingPass([&] { - g_GLESFuncs.glDrawElementsInstanced(mode, count, type, restart.Indices(), instancecount); + g_GLESFuncs.glDrawElementsInstanced(mode, count, restart.IndexType(), restart.Indices(), instancecount); }); } diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.h b/MobileGL/MG_Backend/DirectGLES/Managers.h index f100ebdb..7473e1d4 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.h +++ b/MobileGL/MG_Backend/DirectGLES/Managers.h @@ -102,28 +102,64 @@ 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); - // 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); + // What an indexed draw has to do about primitive restart before it can be issued. + // + // Desktop GL restarts 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 enables for BOTH caps. + // That leaves three cases, and the difference between the last two is not cosmetic - one + // adds restarts, the other has to take away restarts the driver would otherwise make. + enum class RestartSubstitutionKind : Uint8 { + // Nothing to do: restart is off, the fixed-index cap is on, or the application's + // restart index already IS the type's all-ones value. The overwhelmingly common answer. + None, + // The application's index is representable in this index type and differs from the + // all-ones value: the index DATA has to be rewritten so the driver restarts where the + // application asked. + RewriteIndices, + // The application's index cannot be held by this index type at all. GL 4.6 core 10.3.6 + // compares the fetched index, zero-extended, against the full 32-bit + // PRIMITIVE_RESTART_INDEX, so no index can match and the draw restarts NOWHERE - but the + // render-state push has already enabled the driver's fixed-index restart, so the + // all-ones value has to be un-restarted for the duration of the draw. + SuppressRestart, + }; + RestartSubstitutionKind ResolveRestartSubstitution(GLenum indexType); + + // Turns the driver's fixed-index restart off for one draw and back on afterwards, for the + // SuppressRestart case above. Separate from the substitution below because the multi-draw + // tiers need it on its own: they rewrite the index stream themselves and only ever need the + // cap half. Inert for every other kind, and it never touches the render-state shadow - it + // puts the driver back exactly where SyncRenderState left it. + class ScopedSuppressedPrimitiveRestart { + public: + explicit ScopedSuppressedPrimitiveRestart(RestartSubstitutionKind kind); + ~ScopedSuppressedPrimitiveRestart(); + ScopedSuppressedPrimitiveRestart(const ScopedSuppressedPrimitiveRestart&) = delete; + ScopedSuppressedPrimitiveRestart& operator=(const ScopedSuppressedPrimitiveRestart&) = delete; + + private: + Bool m_suppressed = false; + }; // 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. + // (and free) unless ResolveRestartSubstitution asks for it. 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. + // + // The copy may be WIDER than the source (see IndexType): when the source already contains + // the type's all-ones value as an ordinary vertex index, that value cannot double as the + // restart sentinel, and widening is the only way to keep both meanings. Callers must + // therefore take the index type from this object, not from their own argument. 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. + // a GPU-resident firstIndex - an ELEMENT index, so it survives widening too - still + // addresses the index it named. ScopedRestartIndexSubstitution(GLenum indexType, GLsizei count, const void* indices); ~ScopedRestartIndexSubstitution(); ScopedRestartIndexSubstitution(const ScopedRestartIndexSubstitution&) = delete; @@ -136,9 +172,19 @@ namespace MobileGL::MG_Backend::DirectGLES { // 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; } + // The index type the draw must be issued with. Identical to the constructor's unless + // the copy had to be widened to keep an all-ones vertex index distinguishable from the + // restart sentinel. + GLenum IndexType() const { return m_indexType; } private: + // Declared before m_capOverride so it is initialised first (members initialise in + // declaration order): the whole decision is made once, and both the cap override and the + // constructor body read the same answer. + RestartSubstitutionKind m_kind = RestartSubstitutionKind::None; + ScopedSuppressedPrimitiveRestart m_capOverride; const void* m_indices = nullptr; + GLenum m_indexType = 0; Uint m_previousBinding = 0; Bool m_substituted = false; Bool m_valid = true; diff --git a/MobileGL/MG_Backend/DirectGLES/MultiDraw.cpp b/MobileGL/MG_Backend/DirectGLES/MultiDraw.cpp index 02625dd6..d3dde2ce 100644 --- a/MobileGL/MG_Backend/DirectGLES/MultiDraw.cpp +++ b/MobileGL/MG_Backend/DirectGLES/MultiDraw.cpp @@ -29,20 +29,21 @@ namespace MobileGL::MG_Backend::DirectGLES::MultiDrawImpl { } } - // 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. + // The index value this batch restarts on, compared at 32 bits against the zero-extended + // source index. Normally the all-ones value of the source 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. + // + // No truncation, deliberately, and the same rule ResolveRestartSubstitution applies: a + // restart index the source type cannot hold simply matches nothing, so returning it + // verbatim is already "this batch restarts nowhere". Uint32 RestartSentinelFor(GLenum type) { - if (NeedsArbitraryRestartSubstitution(type)) { + if (ResolveRestartSubstitution(type) != RestartSubstitutionKind::None) { return MG_State::pGLContext->GetPrimitiveRestartIndex(); } - switch (type) { - case GL_UNSIGNED_BYTE: return 0xFFu; - case GL_UNSIGNED_SHORT: return 0xFFFFu; - default: return 0xFFFFFFFFu; - } + return MG_Util::FixedRestartIndexForGLType(type); } Bool RestartActive() { @@ -502,6 +503,16 @@ namespace MobileGL::MG_Backend::DirectGLES::MultiDrawImpl { const Bool restartActive = RestartActive(); const Uint32 restartSentinel = RestartSentinelFor(type); + // Widening to GL_UNSIGNED_INT gives a UBYTE/USHORT source a sentinel it can never + // spell, so those batches are lossless. A UINT source that already uses 0xFFFFFFFF as + // a real vertex index while restarting on a different one is the one shape 32 bits + // cannot express - the same corner the single-draw substitution reports. + if (restartActive && indexSize == 4 && restartSentinel != 0xFFFFFFFFu) { + MGLOG_E_ONCE("GL_PRIMITIVE_RESTART with restart index %u over GL_UNSIGNED_INT multi-draw indices: " + "any index that is already 0xFFFFFFFF will restart too, because the rewritten stream " + "has no wider sentinel to move to.", + restartSentinel); + } g_indexStaging.resize(total); SizeT cursor = 0; for (GLsizei i = 0; i < drawcount; ++i) { @@ -868,8 +879,12 @@ void main() { if (drawcount <= 0 || !count || !indices) return; // 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); + // index stream (see ResolveTierForBatch). A restart index this index type cannot hold + // needs no rewrite at all - nothing can match it - but it does need the driver's own + // fixed-index restart held off for the batch, which is what the scope below does. + const RestartSubstitutionKind restartKind = ResolveRestartSubstitution(type); + const Bool arbitraryRestart = restartKind == RestartSubstitutionKind::RewriteIndices; + const ScopedSuppressedPrimitiveRestart restartCapOverride(restartKind); const Bool hasIndexBuffer = BoundIndexBuffer() != nullptr; diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp index ba6cfd4f..cb4a3996 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp @@ -3985,14 +3985,13 @@ void main() { const RenderStateParameters& rsp = MG_State::pGLContext->GetRenderStateParameters(); if (rsp.PrimitiveRestartEnabled && !rsp.PrimitiveRestartFixedIndexEnabled) { const Uint32 restartIndex = rsp.PrimitiveRestartIndex; - Uint32 fixedMax = 0; - switch (vkIndexType) { - case VK_INDEX_TYPE_UINT8: fixedMax = 0xFFu; break; - case VK_INDEX_TYPE_UINT16: fixedMax = 0xFFFFu; break; - case VK_INDEX_TYPE_UINT32: fixedMax = 0xFFFFFFFFu; break; - default: break; - } - substituteRestart = restartIndex != fixedMax; + const Uint32 fixedMax = MG_Util::FixedRestartIndexForGLType(pIndexBufferView->indexType); + // STRICTLY less, and never truncated. Equal needs no rewrite (the driver already + // restarts there); GREATER means the index type cannot hold the application's restart + // index, so GL 4.6 core 10.3.6 says nothing matches it and the draw restarts nowhere - + // which is exactly what ResolvePrimitiveRestartEnable told the pipeline, so rewriting + // here would put restarts into a stream the pipeline was built not to restart on. + substituteRestart = restartIndex < fixedMax; substituteRestartIndex = restartIndex; } @@ -4794,13 +4793,42 @@ void main() { return program.HasLinkedShaderStage(ShaderStage::Geometry); } + // GL primitive restart is defined on the INDEX STREAM (GL 4.6 core 10.3.6): it splits + // primitives when a fetched index matches PRIMITIVE_RESTART_INDEX. Two consequences the + // capability bits alone cannot express, both resolved here because only the caller knows them: + // + // - A non-indexed draw has no index stream, so restart is a no-op for it. Leaving the + // pipeline's primitiveRestartEnable on for a glDrawArrays is what made the list-topology + // guard below refuse those draws, so an application that enables GL_PRIMITIVE_RESTART once + // at init lost every glDrawArrays on a device without the extension. + // - The comparison is against the full 32-bit restart index with the fetched index + // zero-extended, so a restart index the type cannot hold (0x100FF against UNSIGNED_BYTE + // data) matches no index and that draw restarts NOWHERE. UploadAndBindIndexBuffer makes the + // same call for the rewrite, and the two must agree or the pipeline says "restart" over + // index data nothing rewrote. + Bool VulkanRenderer::ResolvePrimitiveRestartEnable(Flags aspects, + const IndexBufferView* pIndexBufferView) const { + if (!(aspects & DrawSetupAspect::IndexBuffer) || pIndexBufferView == nullptr) { + return false; + } + const RenderStateParameters& rsp = MG_State::pGLContext->GetRenderStateParameters(); + if (rsp.PrimitiveRestartFixedIndexEnabled) { + return true; + } + if (!rsp.PrimitiveRestartEnabled) { + return false; + } + return rsp.PrimitiveRestartIndex <= MG_Util::FixedRestartIndexForGLType(pIndexBufferView->indexType); + } + VkPipeline VulkanRenderer::GetOrCreatePipeline( GLenum mode, const MG_State::GLState::ProgramObject& program, const ProgramFactory::VkProgramObject& programObj, ProgramFactory::CompileOptionFlags transformFlags, const MG_State::GLState::VertexArrayObject& vao, - const RenderPassEntry& renderPassEntry) { + const RenderPassEntry& renderPassEntry, + Bool primitiveRestartEnable) { Bool invertClockwise = transformFlags & ProgramFactory::CompileOptionBit::PositionYFlip; if (programObj.stages.empty()) { MGLOG_D("GetOrCreatePipeline skipped: program has no shader stages"); @@ -4842,6 +4870,7 @@ void main() { entry.programHash == programObj.hash && entry.vertexInputHash == vertexLayoutHash && entry.renderPassHash == renderPassHash && entry.pipelineStateHash == pipelineStateHash && + entry.primitiveRestartEnable == primitiveRestartEnable && entry.transformFlags == transformFlags) { return entry.pipeline; } @@ -5040,9 +5069,22 @@ void main() { : VK_POLYGON_MODE_FILL; const VkPrimitiveTopology vkTopology = MG_Util::ConvertPrimitiveModeToVkEnum(mode); - const Bool primitiveRestartEnabled = - MG_State::pGLContext->IsCapabilityEnabled(CapabilityInput::PrimitiveRestart) || - MG_State::pGLContext->IsCapabilityEnabled(CapabilityInput::PrimitiveRestartFixedIndex); + // Resolved by the caller (ResolvePrimitiveRestartEnable), which knows whether the draw is + // indexed and with what index type; the capability bits alone answer neither. + Bool primitiveRestartEnabled = primitiveRestartEnable; + + // GL applies restart to PATCHES only when PRIMITIVE_RESTART_FOR_PATCHES_SUPPORTED is true + // (GL 4.6 core 10.3.6). MobileGL supports no such thing - neither backend has a way to + // restart a patch stream - and GL_FALSE is a legal answer to that query, so a patch draw + // simply never restarts here. Doing this BEFORE the feature guard below is what keeps a + // perfectly ordinary GL_PATCHES draw from being refused on a device that lacks + // VK_EXT_primitive_topology_list_restart. (When GL_PRIMITIVE_RESTART_FOR_PATCHES_SUPPORTED + // is eventually added to glGetIntegerv it has to report GL_FALSE to stay consistent with + // this.) + if (vkTopology == VK_PRIMITIVE_TOPOLOGY_PATCH_LIST) { + primitiveRestartEnabled = false; + } + // Primitive restart on a *list* topology requires the primitiveTopologyListRestart 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 @@ -5053,11 +5095,16 @@ void main() { // 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. + // + // Reached only when this draw's index stream really does restart. Testing the raw + // capability bits here instead - which is what it did - refused every NON-INDEXED + // list-topology draw as well, so an application that enables GL_PRIMITIVE_RESTART once at + // init and then calls glDrawArrays(GL_TRIANGLES, ...) rendered nothing at all. const auto isListTopology = [](VkPrimitiveTopology t) { return t == VK_PRIMITIVE_TOPOLOGY_POINT_LIST || t == VK_PRIMITIVE_TOPOLOGY_LINE_LIST || t == VK_PRIMITIVE_TOPOLOGY_TRIANGLE_LIST || t == VK_PRIMITIVE_TOPOLOGY_LINE_LIST_WITH_ADJACENCY || - t == VK_PRIMITIVE_TOPOLOGY_TRIANGLE_LIST_WITH_ADJACENCY || t == VK_PRIMITIVE_TOPOLOGY_PATCH_LIST; + t == VK_PRIMITIVE_TOPOLOGY_TRIANGLE_LIST_WITH_ADJACENCY; }; if (primitiveRestartEnabled && !m_primitiveTopologyListRestartFeatureEnabled && isListTopology(vkTopology)) { MGLOG_E_ONCE("Draw skipped: primitive restart on a list topology (0x%x) requires the " @@ -5425,6 +5472,7 @@ void main() { entry.vertexInputHash = vertexLayoutHash; entry.renderPassHash = renderPassHash; entry.pipelineStateHash = pipelineStateHash; + entry.primitiveRestartEnable = primitiveRestartEnable; entry.transformFlags = transformFlags; entry.pipeline = pipeline; m_pipelineMemoNext = (m_pipelineMemoNext + 1) % kPipelineMemoSize; @@ -5809,7 +5857,11 @@ void main() { return false; } SetupDrawSnapshot& snap = *snapPtr; - if (snap.aspects != aspects.GetRaw() || snap.mode != mode) { + // Resolved once for the whole function: it guards the snapshot, keys the pipeline memo + // probe below, and is handed to GetOrCreatePipeline on a miss - all three must agree. + const Bool drawPrimitiveRestartEnable = ResolvePrimitiveRestartEnable(aspects, pIndexBufferView); + if (snap.aspects != aspects.GetRaw() || snap.mode != mode || + snap.primitiveRestartEnable != drawPrimitiveRestartEnable) { return false; } if (m_clearManager->HasAnyPendingClears()) { @@ -6079,6 +6131,7 @@ void main() { entry.programHash == programObj.hash && entry.vertexInputHash == vaoLayoutHash && entry.renderPassHash == snap.renderPassHash && entry.pipelineStateHash == m_pipelineStateHash && + entry.primitiveRestartEnable == drawPrimitiveRestartEnable && entry.transformFlags == memoTransformFlags) { pipeline = entry.pipeline; break; @@ -6096,7 +6149,7 @@ void main() { } pipeline = GetOrCreatePipeline(mode, program, programObj, ProgramFactory::CompileOptionFlags(snap.resolvedTransformFlags), - vao, renderPassEntry); + vao, renderPassEntry, drawPrimitiveRestartEnable); if (pipeline == VK_NULL_HANDLE) { return false; } @@ -6533,7 +6586,8 @@ void main() { } } - auto pipeline = GetOrCreatePipeline(mode, program, programObj, transformFlags, vao, *renderPassEntry); + auto pipeline = GetOrCreatePipeline(mode, program, programObj, transformFlags, vao, *renderPassEntry, + ResolvePrimitiveRestartEnable(aspects, pIndexBufferView)); // GetOrCreatePipeline documents a VK_NULL_HANDLE return (empty stages, or a driver that // rejected vkCreateGraphicsPipelines). Binding it dereferences null inside the driver - // 9 of the 15 CTS process deaths were exactly this vkCmdBindPipeline. A draw that has no @@ -6596,6 +6650,7 @@ void main() { if (nowActiveRenderPass != nullptr && !programObj.hasStorageImages) { snap.valid = true; snap.aspects = aspects.GetRaw(); + snap.primitiveRestartEnable = ResolvePrimitiveRestartEnable(aspects, pIndexBufferView); snap.mode = mode; snap.programLifetimeId = program.GetLifetimeId(); snap.programVersion = program.GetBackendStateVersion(); diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.h index d03c82ec..850e8dba 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.h @@ -733,6 +733,12 @@ namespace MobileGL::MG_Backend::DirectVulkan { // values the memo already holds. Uint64 pipelineStateHash = 0; ProgramFactory::CompileOptionFlags transformFlags = {}; + // Baked into the pipeline (PipelineFactory::ComputeHash mixes it), and NOT derivable + // from anything else in this key: it depends on whether the draw is indexed and on the + // index type, neither of which the mode/program/state hashes carry. Without it an + // indexed and a non-indexed draw over the same program and state collide on one entry + // and the second one gets the first one's restart setting. + Bool primitiveRestartEnable = false; VkPipeline pipeline = VK_NULL_HANDLE; }; static constexpr Uint32 kPipelineMemoSize = 8; @@ -865,6 +871,12 @@ namespace MobileGL::MG_Backend::DirectVulkan { Uint64 bindGeneration = 0; Uint32 baseTransformFlags = 0; Uint32 resolvedTransformFlags = 0; + // What ResolvePrimitiveRestartEnable answered for the draw this snapshot was taken + // from, i.e. what its pipeline's primitiveRestartEnable was built with. `aspects` + // already separates indexed from non-indexed draws, but not one index TYPE from + // another, and a restart index that fits GL_UNSIGNED_INT but not GL_UNSIGNED_SHORT + // makes those two draws want different pipelines. + Bool primitiveRestartEnable = false; Uint64 renderPassHash = 0; Uint32 imageIndex = 0; Uint64 textureEraseEpoch = 0; @@ -1168,13 +1180,23 @@ namespace MobileGL::MG_Backend::DirectVulkan { void CreateSwapchain(); void CreateCommandPool(); + // Whether THIS draw's primitive stream restarts, and therefore what + // VkPipelineInputAssemblyStateCreateInfo::primitiveRestartEnable must be. Resolved by the + // caller because it needs two facts a pipeline cannot see: whether the draw is indexed at + // all (GL primitive restart acts on the index stream, so it is a no-op for glDrawArrays), + // and the index TYPE (an application restart index that does not fit the type matches no + // index, so that draw restarts nowhere - see UploadAndBindIndexBuffer). + Bool ResolvePrimitiveRestartEnable(Flags aspects, + const IndexBufferView* pIndexBufferView) const; + VkPipeline GetOrCreatePipeline( GLenum mode, const MG_State::GLState::ProgramObject& program, const ProgramFactory::VkProgramObject& programObj, ProgramFactory::CompileOptionFlags transformFlags, const MG_State::GLState::VertexArrayObject& vao, - const RenderPassEntry& renderPassEntry); + const RenderPassEntry& renderPassEntry, + Bool primitiveRestartEnable); VkPipeline GetOrCreateComputePipeline(const ProgramFactory::VkProgramObject& programObj); void DestroyComputePipelines(); // Takes the frame rather than a command buffer: a first-time storage-usage upgrade has to diff --git a/MobileGL/MG_IntegrationTest/Scenarios/PrimitiveRestartScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/PrimitiveRestartScenario.cpp index 0b6f999c..2cf33b88 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/PrimitiveRestartScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/PrimitiveRestartScenario.cpp @@ -297,5 +297,108 @@ void main() << "the second draw restarted on an index that is not in its data"; } + // A NON-indexed draw has no index stream, so GL primitive restart cannot affect it - and a + // list topology is the shape DirectVulkan has to refuse when the device lacks + // VK_EXT_primitive_topology_list_restart. Deriving the pipeline's primitiveRestartEnable + // from the capability bits alone conflated the two: an application that enables + // GL_PRIMITIVE_RESTART once at init and then draws its UI with glDrawArrays(GL_TRIANGLES) + // had every one of those draws silently dropped on such a device. + TEST_F(PrimitiveRestartScenario, ANonIndexedListTopologyDrawIsUnaffectedByTheCap) { + if (!Ready()) GTEST_SKIP(); + + glEnable(GL_PRIMITIVE_RESTART); + glPrimitiveRestartIndex(kRestartIndex); + ASSERT_EQ(glGetError(), static_cast(GL_NO_ERROR)); + + glClearColor(0.0f, 0.0f, 0.0f, 1.0f); + glClear(GL_COLOR_BUFFER_BIT); + // Vertices 0,1,2 are the left triangle; GL_TRIANGLES is a list topology. + glDrawArrays(GL_TRIANGLES, 0, 3); + EXPECT_EQ(glGetError(), static_cast(GL_NO_ERROR)); + + std::vector pixels(static_cast(kSurface) * kSurface); + glReadPixels(0, 0, kSurface, kSurface, GL_RGBA, GL_UNSIGNED_BYTE, pixels.data()); + EXPECT_TRUE(IsGreen(At(pixels, kLeftX, kLeftY))) + << "primitive restart has no meaning for glDrawArrays, so the draw must render " + "normally whatever the device supports"; + DrainErrors(); + } + + // GL 4.6 core 10.3.6 compares the fetched index, zero-extended, against the full 32-bit + // PRIMITIVE_RESTART_INDEX. A restart index the index type cannot hold therefore matches + // nothing and the draw restarts NOWHERE - it does not restart on the truncated value, and + // it does not restart on the type's all-ones value either, which is what the driver's own + // fixed-index restart would have done if it had been left enabled. + TEST_F(PrimitiveRestartScenario, ARestartIndexTooLargeForTheIndexTypeRestartsNowhere) { + if (!Ready()) GTEST_SKIP(); + + // 16-bit indices with a restart index of 0x10007: the low half (7) IS a real index in + // the data, so a truncating comparison would split the strip exactly where a correct + // one leaves it whole. + const GLushort shortIndices[] = {0, 1, 2, static_cast(kRestartIndex), 3, 4, 5}; + glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, m_ebo); + glBufferData(GL_ELEMENT_ARRAY_BUFFER, sizeof(shortIndices), shortIndices, GL_STATIC_DRAW); + + glEnable(GL_PRIMITIVE_RESTART); + glPrimitiveRestartIndex(0x10000u + kRestartIndex); + ASSERT_EQ(glGetError(), static_cast(GL_NO_ERROR)); + + glClearColor(0.0f, 0.0f, 0.0f, 1.0f); + glClear(GL_COLOR_BUFFER_BIT); + glDrawElements(GL_TRIANGLE_STRIP, static_cast(std::size(shortIndices)), GL_UNSIGNED_SHORT, + nullptr); + EXPECT_EQ(glGetError(), static_cast(GL_NO_ERROR)); + + std::vector pixels(static_cast(kSurface) * kSurface); + glReadPixels(0, 0, kSurface, kSurface, GL_RGBA, GL_UNSIGNED_BYTE, pixels.data()); + EXPECT_TRUE(IsGreen(At(pixels, kLeftX, kLeftY))); + EXPECT_TRUE(IsGreen(At(pixels, kGapX, kGapY))) + << "no 16-bit index can equal 0x10007, so nothing restarts and the strip is " + "continuous - truncating the restart index to 7 would split it here"; + + glBufferData(GL_ELEMENT_ARRAY_BUFFER, sizeof(kIndices), kIndices, GL_STATIC_DRAW); + DrainErrors(); + } + + // The all-ones value of an index type is an ordinary vertex index whenever the array uses + // the type's full range, which is exactly why an application picks an arbitrary restart + // index in the first place. Substituting the sentinel in place would either steal that + // vertex or spuriously restart on it, so the copy widens instead - and the draw has to be + // issued with the widened type, which is the part that is easy to forget. + TEST_F(PrimitiveRestartScenario, AnAllOnesVertexIndexSurvivesTheSubstitution) { + if (!Ready()) GTEST_SKIP(); + + // The buffer carries the 16-bit all-ones value as an ordinary element. It sits past + // the seven indices this draw reads, because the vertex array has only eight entries + // and fetching index 65535 would be out of range - what is under test is that its + // mere PRESENCE forces the widened copy, and that the draw still finds its own + // indices at the right offsets in a copy whose element width has changed underneath + // it. Narrowly substituting in place instead would rewrite this element to 0xFFFE. + const GLushort shortIndices[] = {0, 1, 2, static_cast(kRestartIndex), 3, 4, 5, 0xFFFFu}; + glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, m_ebo); + glBufferData(GL_ELEMENT_ARRAY_BUFFER, sizeof(shortIndices), shortIndices, GL_STATIC_DRAW); + + glEnable(GL_PRIMITIVE_RESTART); + glPrimitiveRestartIndex(kRestartIndex); + ASSERT_EQ(glGetError(), static_cast(GL_NO_ERROR)); + + glClearColor(0.0f, 0.0f, 0.0f, 1.0f); + glClear(GL_COLOR_BUFFER_BIT); + // Only the first seven indices are drawn, so the 0xFFFF element is never fetched - what + // is under test is that its PRESENCE does not break the substitution or the offsets. + glDrawElements(GL_TRIANGLE_STRIP, 7, GL_UNSIGNED_SHORT, nullptr); + EXPECT_EQ(glGetError(), static_cast(GL_NO_ERROR)); + + std::vector pixels(static_cast(kSurface) * kSurface); + glReadPixels(0, 0, kSurface, kSurface, GL_RGBA, GL_UNSIGNED_BYTE, pixels.data()); + 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 restart still has to happen once the copy has been widened"; + + glBufferData(GL_ELEMENT_ARRAY_BUFFER, sizeof(kIndices), kIndices, GL_STATIC_DRAW); + DrainErrors(); + } + } // namespace } // namespace MGITest diff --git a/MobileGL/MG_Util/Metrics/BufferMetrics.cpp b/MobileGL/MG_Util/Metrics/BufferMetrics.cpp index b964b688..6d58dd6b 100644 --- a/MobileGL/MG_Util/Metrics/BufferMetrics.cpp +++ b/MobileGL/MG_Util/Metrics/BufferMetrics.cpp @@ -10,6 +10,15 @@ namespace MobileGL { namespace MG_Util { + Uint32 FixedRestartIndexForGLType(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; + } + } + SizeT GetGLTypeSize(GLenum type) { switch (type) { // Scalars diff --git a/MobileGL/MG_Util/Metrics/BufferMetrics.h b/MobileGL/MG_Util/Metrics/BufferMetrics.h index 08191c45..f152ba8a 100644 --- a/MobileGL/MG_Util/Metrics/BufferMetrics.h +++ b/MobileGL/MG_Util/Metrics/BufferMetrics.h @@ -12,5 +12,19 @@ namespace MobileGL { namespace MG_Util { SizeT GetGLTypeSize(GLenum type); + + // The largest value an index of this type can hold, which is also the value + // GL_PRIMITIVE_RESTART_FIXED_INDEX (and its GLES/Vulkan equivalents) restart on. Zero for a + // type that cannot index at all. + // + // Shared rather than re-derived per backend on purpose: three places have to agree about + // what an index of this type can be - whether a rewrite is needed at all, what the rewrite + // compares against, and whether the driver should be told to restart. GL 4.6 core 10.3.6 + // compares the FETCHED index, zero-extended, against the full 32-bit + // PRIMITIVE_RESTART_INDEX, so a restart index greater than this value matches no index and + // the draw restarts nowhere. Truncating it to the type's width instead - which one of these + // three places used to do - turns a legal vertex index into a restart. + Uint32 FixedRestartIndexForGLType(GLenum indexType); + } // namespace MG_Util } // namespace MobileGL