1425369 - Remove separate non-instanced draw calls

Status: RESOLVED FIXED

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

Description

[Kelsey Gilbert [:jgilbert]]

Merge them into the draw instanced calls, since non-instanced is a special case of instanced. (where instanceCount=1)

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1425369 - Treat Draw* as Draw*Instanced(1). - a=gchang

Review commit: https://reviewboard.mozilla.org/r/207732/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207732/

Comment 2

[Daosheng Mu[:daoshengmu]]

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

[Daosheng Mu[:daoshengmu]]

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.

Comment 4

[Kelsey Gilbert [:jgilbert]]

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.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1884f463bb81
Treat Draw* as Draw*Instanced(1). - r=daoshengmu

Comment 6

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/1884f463bb81

Comment 7

[Kelsey Gilbert [:jgilbert]]

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 8

[Kelsey Gilbert [:jgilbert]]

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]:

Comment 9

[Gerry Chang [:gchang]]

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-.

Comment 10

[Kelsey Gilbert [:jgilbert]]

(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.

Comment 11

[Gerry Chang [: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+.

Comment 12

[Kelsey Gilbert [:jgilbert]]

  https://hg.mozilla.org/releases/mozilla-beta/rev/5d7f843128a2877a38288424e568868c74cb2f54

Comment 13

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8936993 [details]
Bug 1425369 - Treat Draw* as Draw*Instanced(1). -  a=gchang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207732/diff/1-2/