From 7800453ed2e57ce26a6e48528e2bd39a47a26867 Mon Sep 17 00:00:00 2001 From: Michael Ludwig Date: Tue, 31 Mar 2020 22:33:56 +0000 Subject: [PATCH] Revert "Use glDraw.*BaseInstance calls to avoid deferred buffer binding" This reverts commit e8c963d474b7a672bb1292c02b7a774ab29b2cef. Reason for revert: assert failures: https://logs.chromium.org/logs/skia/4b4b7fcb2ff3fe11/+/steps/dm/0/stdout Original change's description: > Use glDraw.*BaseInstance calls to avoid deferred buffer binding > > Change-Id: I968dab317673051acc65f87ea76a0d657d89b3d2 > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/279538 > Commit-Queue: Chris Dalton > Reviewed-by: Michael Ludwig > Reviewed-by: Brian Salomon TBR=bsalomon@google.com,csmartdalton@google.com,michaelludwig@google.com Change-Id: Ic760a56ca9d112e924baf7e833adb09b371928b0 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://skia-review.googlesource.com/c/skia/+/280817 Reviewed-by: Michael Ludwig Commit-Queue: Michael Ludwig --- src/gpu/gl/GrGLCaps.cpp | 15 +-- src/gpu/gl/GrGLCaps.h | 7 +- src/gpu/gl/GrGLGpu.cpp | 24 ----- src/gpu/gl/GrGLGpu.h | 5 - src/gpu/gl/GrGLInterfaceAutogen.cpp | 5 +- src/gpu/gl/GrGLOpsRenderPass.cpp | 129 +++++++++---------------- src/gpu/gl/GrGLOpsRenderPass.h | 4 +- tools/gpu/gl/interface/interface.json5 | 3 + 8 files changed, 61 insertions(+), 131 deletions(-) diff --git a/src/gpu/gl/GrGLCaps.cpp b/src/gpu/gl/GrGLCaps.cpp index 78fe8c8252..1700c0949b 100644 --- a/src/gpu/gl/GrGLCaps.cpp +++ b/src/gpu/gl/GrGLCaps.cpp @@ -37,7 +37,7 @@ GrGLCaps::GrGLCaps(const GrContextOptions& contextOptions, fDrawIndirectSupport = false; fDrawRangeElementsSupport = false; fMultiDrawIndirectSupport = false; - fBaseVertexBaseInstanceSupport = false; + fBaseInstanceSupport = false; fUseNonVBOVertexAndIndexDynamicData = false; fIsCoreProfile = false; fBindFragDataLocationSupport = false; @@ -625,17 +625,18 @@ void GrGLCaps::init(const GrContextOptions& contextOptions, if (GR_IS_GR_GL(standard)) { fDrawIndirectSupport = version >= GR_GL_VER(4,0) || ctxInfo.hasExtension("GL_ARB_draw_indirect"); - fBaseVertexBaseInstanceSupport = version >= GR_GL_VER(4,2) || - ctxInfo.hasExtension("GL_ARB_base_instance"); + fBaseInstanceSupport = version >= GR_GL_VER(4,2); fMultiDrawIndirectSupport = version >= GR_GL_VER(4,3) || - ctxInfo.hasExtension("GL_ARB_multi_draw_indirect"); + (fDrawIndirectSupport && + !fBaseInstanceSupport && // The ARB extension has no base inst. + ctxInfo.hasExtension("GL_ARB_multi_draw_indirect")); fDrawRangeElementsSupport = version >= GR_GL_VER(2,0); } else if (GR_IS_GR_GL_ES(standard)) { fDrawIndirectSupport = version >= GR_GL_VER(3,1); fMultiDrawIndirectSupport = fDrawIndirectSupport && ctxInfo.hasExtension("GL_EXT_multi_draw_indirect"); - fBaseVertexBaseInstanceSupport = ctxInfo.hasExtension("GL_EXT_base_instance") || - ctxInfo.hasExtension("GL_ANGLE_base_vertex_base_instance"); + fBaseInstanceSupport = ctxInfo.hasExtension("GL_EXT_base_instance") || + ctxInfo.hasExtension("GL_ANGLE_base_vertex_base_instance"); fDrawRangeElementsSupport = version >= GR_GL_VER(3,0); } else if (GR_IS_GR_WEBGL(standard)) { // WebGL lacks indirect support, but drawRange was added in WebGL 2.0 @@ -1183,7 +1184,7 @@ void GrGLCaps::onDumpJSON(SkJSONWriter* writer) const { writer->appendBool("Debug support", fDebugSupport); writer->appendBool("Draw indirect support", fDrawIndirectSupport); writer->appendBool("Multi draw indirect support", fMultiDrawIndirectSupport); - writer->appendBool("Base (vertex base) instance support", fBaseVertexBaseInstanceSupport); + writer->appendBool("Base instance support", fBaseInstanceSupport); writer->appendBool("RGBA 8888 pixel ops are slow", fRGBA8888PixelsOpsAreSlow); writer->appendBool("Partial FBO read is slow", fPartialFBOReadIsSlow); writer->appendBool("Bind uniform location support", fBindUniformLocationSupport); diff --git a/src/gpu/gl/GrGLCaps.h b/src/gpu/gl/GrGLCaps.h index b4efd5d910..3468e0493f 100644 --- a/src/gpu/gl/GrGLCaps.h +++ b/src/gpu/gl/GrGLCaps.h @@ -324,9 +324,8 @@ public: /// Is there support for glDrawRangeElements? bool drawRangeElementsSupport() const { return fDrawRangeElementsSupport; } - /// Are the glDraw*Base(VertexBase)Instance methods, and baseInstance fields in indirect draw - //commands supported? - bool baseVertexBaseInstanceSupport() const { return fBaseVertexBaseInstanceSupport; } + /// Are the baseInstance fields supported in indirect draw commands? + bool baseInstanceSupport() const { return fBaseInstanceSupport; } /// Use indices or vertices in CPU arrays rather than VBOs for dynamic content. bool useNonVBOVertexAndIndexDynamicData() const { return fUseNonVBOVertexAndIndexDynamicData; } @@ -529,7 +528,7 @@ private: bool fDrawIndirectSupport : 1; bool fDrawRangeElementsSupport : 1; bool fMultiDrawIndirectSupport : 1; - bool fBaseVertexBaseInstanceSupport : 1; + bool fBaseInstanceSupport : 1; bool fUseNonVBOVertexAndIndexDynamicData : 1; bool fIsCoreProfile : 1; bool fBindFragDataLocationSupport : 1; diff --git a/src/gpu/gl/GrGLGpu.cpp b/src/gpu/gl/GrGLGpu.cpp index 1dc61c4fff..1c7c3e6e2c 100644 --- a/src/gpu/gl/GrGLGpu.cpp +++ b/src/gpu/gl/GrGLGpu.cpp @@ -2274,43 +2274,19 @@ void GrGLGpu::drawRangeElements(GrPrimitiveType primitiveType, GrGLuint minIndex void GrGLGpu::drawArraysInstanced(GrPrimitiveType primitiveType, GrGLint baseVertex, GrGLsizei vertexCount, GrGLsizei instanceCount) { - SkASSERT(this->glCaps().instanceAttribSupport()); SkASSERT(instanceCount <= this->glCaps().maxInstancesPerDrawWithoutCrashing(instanceCount)); GrGLenum glPrimType = this->prepareToDraw(primitiveType); GL_CALL(DrawArraysInstanced(glPrimType, baseVertex, vertexCount, instanceCount)); } -void GrGLGpu::drawArraysInstancedBaseInstance(GrPrimitiveType primitiveType, GrGLint baseVertex, - GrGLsizei vertexCount, GrGLsizei instanceCount, - GrGLuint baseInstance) { - SkASSERT(this->glCaps().instanceAttribSupport()); - SkASSERT(this->glCaps().baseVertexBaseInstanceSupport()); - SkASSERT(instanceCount <= this->glCaps().maxInstancesPerDrawWithoutCrashing(instanceCount)); - GrGLenum glPrimType = this->prepareToDraw(primitiveType); - GL_CALL(DrawArraysInstancedBaseInstance(glPrimType, baseVertex, vertexCount, instanceCount, - baseInstance)); -} - void GrGLGpu::drawElementsInstanced(GrPrimitiveType primitiveType, GrGLsizei indexCount, GrGLenum indexType, const void* indices, GrGLsizei instanceCount) { - SkASSERT(this->glCaps().instanceAttribSupport()); SkASSERT(instanceCount <= this->glCaps().maxInstancesPerDrawWithoutCrashing(instanceCount)); GrGLenum glPrimType = this->prepareToDraw(primitiveType); GL_CALL(DrawElementsInstanced(glPrimType, indexCount, indexType, indices, instanceCount)); } -void GrGLGpu::drawElementsInstancedBaseVertexBaseInstance( - GrPrimitiveType primitiveType, GrGLsizei indexCount, GrGLenum indexType, - const void* indices, GrGLsizei instanceCount, GrGLint baseVertex, GrGLuint baseInstance) { - SkASSERT(this->glCaps().instanceAttribSupport()); - SkASSERT(this->glCaps().baseVertexBaseInstanceSupport()); - SkASSERT(instanceCount <= this->glCaps().maxInstancesPerDrawWithoutCrashing(instanceCount)); - GrGLenum glPrimType = this->prepareToDraw(primitiveType); - GL_CALL(DrawElementsInstancedBaseVertexBaseInstance(glPrimType, indexCount, indexType, indices, - instanceCount, baseVertex, baseInstance)); -} - void GrGLGpu::onResolveRenderTarget(GrRenderTarget* target, const SkIRect& resolveRect, ForExternalIO) { // Some extensions automatically resolves the texture when it is read. diff --git a/src/gpu/gl/GrGLGpu.h b/src/gpu/gl/GrGLGpu.h index 5c52d39981..7fdcdf1e00 100644 --- a/src/gpu/gl/GrGLGpu.h +++ b/src/gpu/gl/GrGLGpu.h @@ -113,13 +113,8 @@ public: GrGLsizei indexCount, GrGLenum indexType, const void* indices); void drawArraysInstanced(GrPrimitiveType, GrGLint baseVertex, GrGLsizei vertexCount, GrGLsizei instanceCount); - void drawArraysInstancedBaseInstance(GrPrimitiveType, GrGLint baseVertex, GrGLsizei vertexCount, - GrGLsizei instanceCount, GrGLuint baseInstance); void drawElementsInstanced(GrPrimitiveType, GrGLsizei indexCount, GrGLenum indexType, const void* indices, GrGLsizei instanceCount); - void drawElementsInstancedBaseVertexBaseInstance( - GrPrimitiveType, GrGLsizei indexCount, GrGLenum indexType, const void* indices, - GrGLsizei instanceCount, GrGLint baseVertex, GrGLuint baseInstance); // The GrGLOpsRenderPass does not buffer up draws before submitting them to the gpu. // Thus this is the implementation of the clear call for the corresponding passthrough function diff --git a/src/gpu/gl/GrGLInterfaceAutogen.cpp b/src/gpu/gl/GrGLInterfaceAutogen.cpp index c465fe618c..bcf8c1febb 100644 --- a/src/gpu/gl/GrGLInterfaceAutogen.cpp +++ b/src/gpu/gl/GrGLInterfaceAutogen.cpp @@ -240,10 +240,7 @@ bool GrGLInterface::validate() const { (GR_IS_GR_GL_ES(fStandard) && ( fExtensions.has("GL_EXT_base_instance") || fExtensions.has("GL_ANGLE_base_vertex_base_instance")))) { - if (!fFunctions.fDrawArraysInstancedBaseInstance || - !fFunctions.fDrawElementsInstancedBaseVertexBaseInstance) { - RETURN_FALSE_INTERFACE; - } + // all functions were marked optional or test_only } if (GR_IS_GR_GL(fStandard) || diff --git a/src/gpu/gl/GrGLOpsRenderPass.cpp b/src/gpu/gl/GrGLOpsRenderPass.cpp index c427158abf..260376ee3c 100644 --- a/src/gpu/gl/GrGLOpsRenderPass.cpp +++ b/src/gpu/gl/GrGLOpsRenderPass.cpp @@ -48,7 +48,9 @@ bool GrGLOpsRenderPass::onBindTextures(const GrPrimitiveProcessor& primProc, const GrSurfaceProxy* const primProcTextures[], const GrPipeline& pipeline) { GrGLProgram* program = fGpu->currentProgram(); - SkASSERT(program); + if (!program) { + return false; + } program->bindTextures(primProc, primProcTextures, pipeline); return true; } @@ -58,7 +60,9 @@ void GrGLOpsRenderPass::onBindBuffers(const GrBuffer* indexBuffer, const GrBuffe GrPrimitiveRestart primitiveRestart) { SkASSERT((primitiveRestart == GrPrimitiveRestart::kNo) || indexBuffer); GrGLProgram* program = fGpu->currentProgram(); - SkASSERT(program); + if (!program) { + return; + } int numAttribs = program->numVertexAttributes() + program->numInstanceAttributes(); fAttribArrayState = fGpu->bindInternalVertexArray(indexBuffer, numAttribs, primitiveRestart); @@ -72,27 +76,32 @@ void GrGLOpsRenderPass::onBindBuffers(const GrBuffer* indexBuffer, const GrBuffe } } - if (!fGpu->glCaps().baseVertexBaseInstanceSupport()) { - // This platform does not support baseInstance. Defer binding of the instance buffer. - fActiveInstanceBuffer = sk_ref_sp(instanceBuffer); - } else { - this->bindInstanceBuffer(instanceBuffer, 0); - } - if (!indexBuffer && fGpu->glCaps().drawArraysBaseVertexIsBroken()) { - // There is a driver bug affecting glDrawArrays. Defer binding of the vertex buffer. - fActiveVertexBuffer = sk_ref_sp(vertexBuffer); - } else if (indexBuffer && !fGpu->glCaps().baseVertexBaseInstanceSupport()) { - // This platform does not support baseVertex with indexed draws. Defer binding of the - // vertex buffer. - fActiveVertexBuffer = sk_ref_sp(vertexBuffer); - } else { - this->bindVertexBuffer(vertexBuffer, 0); - } + // We defer binding of instance and vertex buffers because GL does not (always) support base + // instance and/or base vertex. + fActiveInstanceBuffer = sk_ref_sp(instanceBuffer); + fActiveVertexBuffer = sk_ref_sp(vertexBuffer); } -void GrGLOpsRenderPass::bindInstanceBuffer(const GrBuffer* instanceBuffer, int baseInstance) { +void GrGLOpsRenderPass::setupGeometry(const GrBuffer* vertexBuffer, int baseVertex, + const GrBuffer* instanceBuffer, int baseInstance) { GrGLProgram* program = fGpu->currentProgram(); - SkASSERT(program); + if (!program) { + return; + } + + if (int vertexStride = program->vertexStride()) { + SkASSERT(vertexBuffer); + SkASSERT(vertexBuffer->isCpuBuffer() || + !static_cast(vertexBuffer)->isMapped()); + size_t bufferOffset = baseVertex * static_cast(vertexStride); + for (int i = 0; i < program->numVertexAttributes(); ++i) { + const auto& attrib = program->vertexAttribute(i); + static constexpr int kDivisor = 0; + fAttribArrayState->set(fGpu, attrib.fLocation, vertexBuffer, attrib.fCPUType, + attrib.fGPUType, vertexStride, bufferOffset + attrib.fOffset, + kDivisor); + } + } if (int instanceStride = program->instanceStride()) { SkASSERT(instanceBuffer); SkASSERT(instanceBuffer->isCpuBuffer() || @@ -109,47 +118,20 @@ void GrGLOpsRenderPass::bindInstanceBuffer(const GrBuffer* instanceBuffer, int b } } -void GrGLOpsRenderPass::bindVertexBuffer(const GrBuffer* vertexBuffer, int baseVertex) { - GrGLProgram* program = fGpu->currentProgram(); - SkASSERT(program); - if (int vertexStride = program->vertexStride()) { - SkASSERT(vertexBuffer); - SkASSERT(vertexBuffer->isCpuBuffer() || - !static_cast(vertexBuffer)->isMapped()); - size_t bufferOffset = baseVertex * static_cast(vertexStride); - for (int i = 0; i < program->numVertexAttributes(); ++i) { - const auto& attrib = program->vertexAttribute(i); - static constexpr int kDivisor = 0; - fAttribArrayState->set(fGpu, attrib.fLocation, vertexBuffer, attrib.fCPUType, - attrib.fGPUType, vertexStride, bufferOffset + attrib.fOffset, - kDivisor); - } - } -} - void GrGLOpsRenderPass::onDraw(int vertexCount, int baseVertex) { - SkASSERT(!fActiveVertexBuffer || fGpu->glCaps().drawArraysBaseVertexIsBroken()); if (fGpu->glCaps().drawArraysBaseVertexIsBroken()) { - this->bindVertexBuffer(fActiveVertexBuffer.get(), baseVertex); - baseVertex = 0; + this->setupGeometry(fActiveVertexBuffer.get(), baseVertex, nullptr, 0); + fGpu->drawArrays(fPrimitiveType, 0, vertexCount); + return; } + + this->setupGeometry(fActiveVertexBuffer.get(), 0, nullptr, 0); fGpu->drawArrays(fPrimitiveType, baseVertex, vertexCount); } void GrGLOpsRenderPass::onDrawIndexed(int indexCount, int baseIndex, uint16_t minIndexValue, uint16_t maxIndexValue, int baseVertex) { - if (fGpu->glCaps().baseVertexBaseInstanceSupport()) { - SkASSERT(!fActiveVertexBuffer); - if (baseVertex != 0) { - fGpu->drawElementsInstancedBaseVertexBaseInstance( - fPrimitiveType, indexCount, GR_GL_UNSIGNED_SHORT, - this->offsetForBaseIndex(baseIndex), 1, baseVertex, 0); - return; - } - } else { - this->bindVertexBuffer(fActiveVertexBuffer.get(), baseVertex); - } - + this->setupGeometry(fActiveVertexBuffer.get(), baseVertex, nullptr, 0); if (fGpu->glCaps().drawRangeElementsSupport()) { fGpu->drawRangeElements(fPrimitiveType, minIndexValue, maxIndexValue, indexCount, GR_GL_UNSIGNED_SHORT, this->offsetForBaseIndex(baseIndex)); @@ -161,25 +143,12 @@ void GrGLOpsRenderPass::onDrawIndexed(int indexCount, int baseIndex, uint16_t mi void GrGLOpsRenderPass::onDrawInstanced(int instanceCount, int baseInstance, int vertexCount, int baseVertex) { - SkASSERT(!fActiveVertexBuffer || fGpu->glCaps().drawArraysBaseVertexIsBroken()); - if (fGpu->glCaps().drawArraysBaseVertexIsBroken()) { - // We weren't able to bind the vertex buffer during onBindBuffers because of a driver bug - // affecting glDrawArrays. - this->bindVertexBuffer(fActiveVertexBuffer.get(), 0); - } int maxInstances = fGpu->glCaps().maxInstancesPerDrawWithoutCrashing(instanceCount); for (int i = 0; i < instanceCount; i += maxInstances) { - int instanceCountForDraw = std::min(instanceCount - i, maxInstances); - int baseInstanceForDraw = baseInstance + i; - if (fGpu->glCaps().baseVertexBaseInstanceSupport()) { - SkASSERT(!fActiveInstanceBuffer); - fGpu->drawArraysInstancedBaseInstance(fPrimitiveType, baseVertex, vertexCount, - instanceCountForDraw, baseInstanceForDraw); - } else { - this->bindInstanceBuffer(fActiveInstanceBuffer.get(), baseInstanceForDraw); - fGpu->drawArraysInstanced(fPrimitiveType, baseVertex, vertexCount, - instanceCountForDraw); - } + this->setupGeometry(fActiveVertexBuffer.get(), 0, fActiveInstanceBuffer.get(), + baseInstance + i); + fGpu->drawArraysInstanced(fPrimitiveType, baseVertex, vertexCount, + std::min(instanceCount - i, maxInstances)); } } @@ -187,21 +156,11 @@ void GrGLOpsRenderPass::onDrawIndexedInstanced(int indexCount, int baseIndex, in int baseInstance, int baseVertex) { int maxInstances = fGpu->glCaps().maxInstancesPerDrawWithoutCrashing(instanceCount); for (int i = 0; i < instanceCount; i += maxInstances) { - int instanceCountForDraw = std::min(instanceCount - i, maxInstances); - int baseInstanceForDraw = baseInstance + i; - if (fGpu->glCaps().baseVertexBaseInstanceSupport()) { - SkASSERT(!fActiveInstanceBuffer); - SkASSERT(!fActiveVertexBuffer); - fGpu->drawElementsInstancedBaseVertexBaseInstance( - fPrimitiveType, indexCount, GR_GL_UNSIGNED_SHORT, - this->offsetForBaseIndex(baseIndex), instanceCountForDraw, baseVertex, - baseInstanceForDraw); - } else { - this->bindInstanceBuffer(fActiveInstanceBuffer.get(), baseInstanceForDraw); - this->bindVertexBuffer(fActiveVertexBuffer.get(), baseVertex); - fGpu->drawElementsInstanced(fPrimitiveType, indexCount, GR_GL_UNSIGNED_SHORT, - this->offsetForBaseIndex(baseIndex), instanceCountForDraw); - } + this->setupGeometry(fActiveVertexBuffer.get(), baseVertex, fActiveInstanceBuffer.get(), + baseInstance + i); + fGpu->drawElementsInstanced(fPrimitiveType, indexCount, GR_GL_UNSIGNED_SHORT, + this->offsetForBaseIndex(baseIndex), + std::min(instanceCount - i, maxInstances)); } } diff --git a/src/gpu/gl/GrGLOpsRenderPass.h b/src/gpu/gl/GrGLOpsRenderPass.h index af808560f8..b18623722f 100644 --- a/src/gpu/gl/GrGLOpsRenderPass.h +++ b/src/gpu/gl/GrGLOpsRenderPass.h @@ -40,8 +40,8 @@ public: private: GrGpu* gpu() override { return fGpu; } - void bindInstanceBuffer(const GrBuffer*, int baseInstance); - void bindVertexBuffer(const GrBuffer*, int baseVertex); + void setupGeometry(const GrBuffer* vertexBuffer, int baseVertex, const GrBuffer* instanceBuffer, + int baseInstance); const void* offsetForBaseIndex(int baseIndex) const { if (!fIndexPointer) { diff --git a/tools/gpu/gl/interface/interface.json5 b/tools/gpu/gl/interface/interface.json5 index 284fde8508..9d5390e7d2 100644 --- a/tools/gpu/gl/interface/interface.json5 +++ b/tools/gpu/gl/interface/interface.json5 @@ -179,6 +179,9 @@ "functions": [ "DrawArraysInstancedBaseInstance", "DrawElementsInstancedBaseVertexBaseInstance" ], + "optional": [ + "DrawArraysInstancedBaseInstance", "DrawElementsInstancedBaseVertexBaseInstance" + ], }, { // ES 3.0 has glDrawBuffers but not glDrawBuffer