Closed Bug 1425369 Opened 8 years ago Closed 8 years ago

Remove separate non-instanced draw calls

Categories

(Core :: Graphics: CanvasWebGL, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox57 --- wontfix
firefox58 - fixed
firefox59 --- fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

Details

(Whiteboard: gfx-noted)

Attachments

(1 file)

Merge them into the draw instanced calls, since non-instanced is a special case of instanced. (where instanceCount=1)
Comment on attachment 8936993 [details] Bug 1425369 - Treat Draw* as Draw*Instanced(1). - a=gchang https://reviewboard.mozilla.org/r/207732/#review213628 r=me after fixing some profiling labels. ::: dom/canvas/WebGLContext.h:1295 (Diff revision 1) > // ----------------------------------------------------------------------------- > // Vertices Feature (WebGLContextVertices.cpp) > GLenum mPrimRestartTypeBytes; > > public: > - void DrawArrays(GLenum mode, GLint first, GLsizei count); > + void DrawArrays(GLenum mode, GLint first, GLsizei count) { I think you would like move the `{` to a newline. ::: dom/canvas/WebGLContextDraw.cpp:520 (Diff revision 1) > > void > -WebGLContext::DrawArrays(GLenum mode, GLint first, GLsizei vertCount) > +WebGLContext::DrawArraysInstanced(GLenum mode, GLint first, GLsizei vertCount, > + GLsizei instanceCount, const char* const funcName) > { > AUTO_PROFILER_LABEL("WebGLContext::DrawArrays", GRAPHICS); Here should be AUTO_PROFILER_LABEL("WebGLContext::DrawArraysInstanced", xxx) ::: dom/canvas/WebGLContextDraw.cpp:550 (Diff revision 1) > { > ScopedDrawCallWrapper wrapper(*this); > - if (vertCount) { > + if (vertCount && instanceCount) { > AUTO_PROFILER_LABEL("glDrawArrays", GRAPHICS); > + if (HasInstancedDrawing(*this)) { > + gl->fDrawArraysInstanced(mode, first, vertCount, instanceCount); add AUTO_PROFILER_LABEL("glDrawArraysInstanced", GRAPHICS); ::: dom/canvas/WebGLContextDraw.cpp:552 (Diff revision 1) > - if (vertCount) { > + if (vertCount && instanceCount) { > AUTO_PROFILER_LABEL("glDrawArrays", GRAPHICS); > + if (HasInstancedDrawing(*this)) { > + gl->fDrawArraysInstanced(mode, first, vertCount, instanceCount); > + } else { > + MOZ_ASSERT(instanceCount == 1); Move AUTO_PROFILER_LABEL("glDrawArrays", GRAPHICS); to here ::: dom/canvas/WebGLContextDraw.cpp:678 (Diff revision 1) > -void > WebGLContext::DrawElementsInstanced(GLenum mode, GLsizei indexCount, GLenum type, > - WebGLintptr byteOffset, GLsizei instanceCount) > + WebGLintptr byteOffset, GLsizei instanceCount, > + const char* const funcName) > { > - AUTO_PROFILER_LABEL("WebGLContext::DrawElementsInstanced", GRAPHICS); > + AUTO_PROFILER_LABEL("WebGLContext::DrawElements", GRAPHICS); AUTO_PROFILER_LABEL("WebGLContext::DrawElementsInstanced", GRAPHICS); instead of "WebGLContext::DrawElements" ::: dom/canvas/WebGLContextDraw.cpp:713 (Diff revision 1) > } > > if (indexCount && instanceCount) { > - AUTO_PROFILER_LABEL("glDrawElementsInstanced", GRAPHICS); > + AUTO_PROFILER_LABEL("glDrawElements", GRAPHICS); > + if (HasInstancedDrawing(*this)) { > - gl->fDrawElementsInstanced(mode, indexCount, type, > + gl->fDrawElementsInstanced(mode, indexCount, type, Add AUTO_PROFILER_LABEL("glDrawElementsInstanced", GRAPHICS); ::: dom/canvas/WebGLContextDraw.cpp:718 (Diff revision 1) > - gl->fDrawElementsInstanced(mode, indexCount, type, > + gl->fDrawElementsInstanced(mode, indexCount, type, > - reinterpret_cast<GLvoid*>(byteOffset), > + reinterpret_cast<GLvoid*>(byteOffset), > - instanceCount); > + instanceCount); > + } else { > + MOZ_ASSERT(instanceCount == 1); > + gl->fDrawElements(mode, indexCount, type, Move AUTO_PROFILER_LABEL("glDrawElements", GRAPHICS); to here
Comment on attachment 8936993 [details] Bug 1425369 - Treat Draw* as Draw*Instanced(1). - a=gchang https://reviewboard.mozilla.org/r/207732/#review213666 r=me, after some nits.
Attachment #8936993 - Flags: review?(dmu) → review+
Comment on attachment 8936993 [details] Bug 1425369 - Treat Draw* as Draw*Instanced(1). - a=gchang https://reviewboard.mozilla.org/r/207732/#review213674 ::: dom/canvas/WebGLContext.h:1295 (Diff revision 1) > // ----------------------------------------------------------------------------- > // Vertices Feature (WebGLContextVertices.cpp) > GLenum mPrimRestartTypeBytes; > > public: > - void DrawArrays(GLenum mode, GLint first, GLsizei count); > + void DrawArrays(GLenum mode, GLint first, GLsizei count) { This format is within our style. ::: dom/canvas/WebGLContextDraw.cpp:550 (Diff revision 1) > { > ScopedDrawCallWrapper wrapper(*this); > - if (vertCount) { > + if (vertCount && instanceCount) { > AUTO_PROFILER_LABEL("glDrawArrays", GRAPHICS); > + if (HasInstancedDrawing(*this)) { > + gl->fDrawArraysInstanced(mode, first, vertCount, instanceCount); I'm just going to use this for both glDrawArraysInstanced and glDrawArrays, since it's not useful to label which one we're using.
Pushed by jgilbert@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1884f463bb81 Treat Draw* as Draw*Instanced(1). - r=daoshengmu
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
We should consider taking this in 58. We keep having issues (in particular with ANGLE) with DrawArrays not being implemented as DrawArraysInstanced(1). At this point I think it would be riskier to not take this in 58.
Comment on attachment 8936993 [details] Bug 1425369 - Treat Draw* as Draw*Instanced(1). - a=gchang Approval Request Comment [Feature/Bug causing the regression]: ANGLE update in 58 [User impact if declined]: Various bugs including one where bad content can stick ANGLE in an irrecoverable (though safe) disabled state for all pages on that content process. [Is this code covered by automated tests?]: no (many of these bugs will go away when we take a new ANGLE update in 60 at the latest, and this is already landed in 59) [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: low risk [Why is the change risky/not risky?]: It's spec-valid to do this, and we only do it when an app starts using instanced arrays (or webgl2) anyway. [String changes made/needed]:
Attachment #8936993 - Flags: approval-mozilla-beta?
Comment on attachment 8936993 [details] Bug 1425369 - Treat Draw* as Draw*Instanced(1). - a=gchang We are only 1+ week from RC week. It's very late and we should start to stablize. I would prefer let this ride the train on 59 and won't fix in 58. Beta58-.
Attachment #8936993 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
(In reply to Gerry Chang [:gchang] from comment #9) > Comment on attachment 8936993 [details] > Bug 1425369 - Treat Draw* as Draw*Instanced(1). - > > We are only 1+ week from RC week. It's very late and we should start to > stablize. I would prefer let this ride the train on 59 and won't fix in 58. > Beta58-. The potential negative impact to not taking this before release is IMO worse, since WebGL will occasionally stop working. This is a big issue, but I was resisting filing a new bug about it, since we can just take this here.
Flags: needinfo?(gchang)
Comment on attachment 8936993 [details] Bug 1425369 - Treat Draw* as Draw*Instanced(1). - a=gchang Per comment #10, this can fix some webgl issues. Beta58+.
Flags: needinfo?(gchang)
Attachment #8936993 - Flags: approval-mozilla-beta- → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: