Closed
Bug 1425369
Opened 8 years ago
Closed 8 years ago
Remove separate non-instanced draw calls
Categories
(Core :: Graphics: CanvasWebGL, defect, P3)
Core
Graphics: CanvasWebGL
Tracking
()
RESOLVED
FIXED
mozilla59
People
(Reporter: jgilbert, Assigned: jgilbert)
Details
(Whiteboard: gfx-noted)
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
daoshengmu
:
review+
gchang
:
approval-mozilla-beta+
|
Details |
Merge them into the draw instanced calls, since non-instanced is a special case of instanced. (where instanceCount=1)
Comment hidden (mozreview-request) |
Comment 2•8 years ago
|
||
mozreview-review |
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 3•8 years ago
|
||
mozreview-review |
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+
Updated•8 years ago
|
Priority: -- → P3
Assignee | ||
Comment 4•8 years ago
|
||
mozreview-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
Comment 6•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Assignee | ||
Comment 7•7 years ago
|
||
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.
Assignee | ||
Comment 8•7 years ago
|
||
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 9•7 years ago
|
||
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-
Updated•7 years ago
|
Assignee | ||
Comment 10•7 years ago
|
||
(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 11•7 years ago
|
||
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+
Updated•7 years ago
|
Assignee | ||
Comment 12•7 years ago
|
||
uplift |
Comment hidden (mozreview-request) |
You need to log in
before you can comment on or make changes to this bug.
Description
•