1088345 - Support MOZ_GL_DEBUG_ABORT_ON_ERROR with WebGL

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

MOZ_GL_DEBUG_ABORT_ON_ERROR makes GLContext fatally assert when we hid a GL error as a result of a GL call. It's useful for debugging.

It doesn't work with WebGL really yet, since we let some WebGL errors fall through to the driver. From WebGL, we should only be submitting valid commands to the driver, since we need to do the validation anyways. The only error we should ever get back is OUT_OF_MEMORY.

I patched things up and ran it against the WebGL 1.0.1 conformance tests on my Linux machine with my Quadro 4000 and 331.38 drivers.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: 0001-Update-local-error-checking.patch

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: 0002-Don-t-run-GL-commands-that-may-cause-non-OOM-errors.patch

Comment 3

[u480271]

Comment on attachment 8510649 [details] [diff] [review]
0001-Update-local-error-checking.patch

Review of attachment 8510649 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/SharedSurfaceGL.cpp
@@ +127,3 @@
>  
> +    if (!texture) {
> +      GLContext::ScopedLocalErrorCheck localError(*prodGL);

indent 4 spaces

Comment 4

[u480271]

Comment on attachment 8510650 [details] [diff] [review]
0002-Don-t-run-GL-commands-that-may-cause-non-OOM-errors.patch

Review of attachment 8510650 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLContext.h
@@ +693,3 @@
>                           const GLfloat* data);
>  
> +    void Uniform3fv(WebGLUniformLocation* loc, const dom::Float32Array& arr) {

{ placement is inconsistent in this file.

Comment 5

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #4)
> Comment on attachment 8510650 [details] [diff] [review]
> 0002-Don-t-run-GL-commands-that-may-cause-non-OOM-errors.patch
> 
> Review of attachment 8510650 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGLContext.h
> @@ +693,3 @@
> >                           const GLfloat* data);
> >  
> > +    void Uniform3fv(WebGLUniformLocation* loc, const dom::Float32Array& arr) {
> 
> { placement is inconsistent in this file.

It's consistent with a hard-limit of 80-char per line. If you move the { to the end of the line, it's either 81 or 82 chars, but the rest of the line is not long enough to be folded at the comma, so it ends up looking like block formatting.

I can relax this, but I sort of like the strict 80-char limit. It makes decisions easy.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Attachment: 0003-Handle-possibly-invalid-enums-for-queries.patch

Without this patch, I had on-error aborts on Mac, but ANGLE and Linux run 1.0.1 cleanly. I'm still running 1.0.2 tests, but we look clean on those too.

Comment 7

[Kelsey Gilbert [:jgilbert]]

remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=db97b1cd8da8

Comment 8

[Kelsey Gilbert [:jgilbert]]

Attachment: 0004-Fix-OPT-building.patch

Comment 9

[Kelsey Gilbert [:jgilbert]]

   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c2a71cb46708

Comment 10

[u480271]

Comment on attachment 8510778 [details] [diff] [review]
0003-Handle-possibly-invalid-enums-for-queries.patch

Review of attachment 8510778 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/GLContext.cpp
@@ +183,5 @@
>           */
>          GLint majorVersion = 0;
>          GLint minorVersion = 0;
>  
> +        const bool ok = (gl->GetPotentialInteger(LOCAL_GL_MAJOR_VERSION,

What is a PotentialInteger?

@@ +244,4 @@
>  
>      const char* itr = versionString;
>      char* end = nullptr;
> +    auto majorVersion = strtol(itr, &end, 10);

Don't know if :mattwoodrow would approve. ;-)

::: gfx/gl/GLContext.h
@@ +712,4 @@
>          }
>      };
>  
> +    bool GetPotentialInteger(GLenum pname, GLint* param) {

I'm not really sold on this name. But it's not enough of a concern for r-.

Comment 11

[Kelsey Gilbert [:jgilbert]]

I'm running into this ANGLE issue: https://code.google.com/p/angleproject/issues/detail?id=812

Comment 12

[Kelsey Gilbert [:jgilbert]]

Attachment: 0001-Improve-glGetError-handling.patch

Comment 13

[Kelsey Gilbert [:jgilbert]]

Attachment: 0002-Don-t-run-GL-commands-that-may-cause-non-OOM-errors.patch

Sorry, but since I changed the API, I want you to take a quick look at this again.

Comment 14

[Kelsey Gilbert [:jgilbert]]

Attachment: 0003-Handle-possibly-invalid-enums-for-queries.patch

I couldn't decide on a better name for this.

Comment 15

[Kelsey Gilbert [:jgilbert]]

Attachment: 0004-Remove-now-passing-tests-from-expected-failures.patch

Comment 16

[Kelsey Gilbert [:jgilbert]]

Attachment: 0005-Handle-AA-better.patch

Comment 17

[Kelsey Gilbert [:jgilbert]]

Attachment: (ANGLE) 0006-Don-t-treat-STENCIL_INDEX8-as-INT-for-MSAA-reasons.patch

This is a patch against ANGLE. On r+, I'll push it into our repo.

Comment 18

[Kelsey Gilbert [:jgilbert]]

Attachment: 0007-Misc-cleanup.patch

Comment 19

[Kelsey Gilbert [:jgilbert]]

Try is looking good, but Windows isn't done yet.
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=88dcde703ab6

Comment 20

[u480271]

Comment on attachment 8512374 [details] [diff] [review]
0001-Improve-glGetError-handling.patch

Review of attachment 8512374 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/GLContext.h
@@ +727,5 @@
> +                              GLErrorToString(err), err);
> +            }
> +
> +            if (err != LOCAL_GL_NO_ERROR &&
> +                !mLocalErrorScope)

single line

Comment 21

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #20)
> Comment on attachment 8512374 [details] [diff] [review]
> 0001-Improve-glGetError-handling.patch
> 
> Review of attachment 8512374 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLContext.h
> @@ +727,5 @@
> > +                              GLErrorToString(err), err);
> > +            }
> > +
> > +            if (err != LOCAL_GL_NO_ERROR &&
> > +                !mLocalErrorScope)
> 
> single line

Historically, I've made multi-condition expressions like these two lines. I think it helps understandability of conditionals. I would like to err on the side of readability, but if you think it actually helps readability to put it on a single line, we can do that.

Comment 22

[u480271]

Comment on attachment 8512375 [details] [diff] [review]
0002-Don-t-run-GL-commands-that-may-cause-non-OOM-errors.patch

Review of attachment 8512375 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLContext.h
@@ +629,3 @@
>      }
> +    void Uniform1iv(WebGLUniformLocation* loc, const dom::Sequence<GLint>& arr)
> +    {

{ goes on line with function name in header files. Like line 626.

Comment 23

[Kelsey Gilbert [:jgilbert]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/5e5b3c89df16
 https://hg.mozilla.org/integration/mozilla-inbound/rev/931498bf7d4f
 https://hg.mozilla.org/integration/mozilla-inbound/rev/ac7a5ece234c
 https://hg.mozilla.org/integration/mozilla-inbound/rev/2014a7834d1c
 https://hg.mozilla.org/integration/mozilla-inbound/rev/585cdb566034
 https://hg.mozilla.org/integration/mozilla-inbound/rev/d9e52f1a7dff
 https://hg.mozilla.org/integration/mozilla-inbound/rev/e1876b193803

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5e5b3c89df16
https://hg.mozilla.org/mozilla-central/rev/931498bf7d4f
https://hg.mozilla.org/mozilla-central/rev/ac7a5ece234c
https://hg.mozilla.org/mozilla-central/rev/2014a7834d1c
https://hg.mozilla.org/mozilla-central/rev/585cdb566034
https://hg.mozilla.org/mozilla-central/rev/d9e52f1a7dff
https://hg.mozilla.org/mozilla-central/rev/e1876b193803