Closed Bug 1088345 Opened 5 years ago Closed 5 years ago

Support MOZ_GL_DEBUG_ABORT_ON_ERROR with WebGL

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

Attachments

(7 files, 4 obsolete files)

15.29 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
58.00 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
8.10 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
1.18 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
2.03 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
2.64 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
1.60 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
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.
Attachment #8510649 - Flags: review?(dglastonbury)
Attachment #8510650 - Flags: review?(dglastonbury)
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
Attachment #8510649 - Flags: review?(dglastonbury) → review+
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.
Attachment #8510650 - Flags: review?(dglastonbury) → review+
(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.
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.
Attachment #8510778 - Flags: review?(dglastonbury)
Attached patch 0004-Fix-OPT-building.patch (obsolete) — Splinter Review
Attachment #8511257 - Flags: review?(dglastonbury)
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-.
Attachment #8510778 - Flags: review?(dglastonbury) → review+
Attachment #8511257 - Flags: review?(dglastonbury) → review+
Attachment #8510649 - Attachment is obsolete: true
Attachment #8512374 - Flags: review?(dglastonbury)
Sorry, but since I changed the API, I want you to take a quick look at this again.
Attachment #8510650 - Attachment is obsolete: true
Attachment #8512375 - Flags: review?(dglastonbury)
I couldn't decide on a better name for this.
Attachment #8510778 - Attachment is obsolete: true
Attachment #8512376 - Flags: review?(dglastonbury)
Attachment #8511257 - Attachment is obsolete: true
Attachment #8512377 - Flags: review?(dglastonbury)
Attachment #8512378 - Flags: review?(dglastonbury)
This is a patch against ANGLE. On r+, I'll push it into our repo.
Attachment #8512380 - Flags: review?(dglastonbury)
Try is looking good, but Windows isn't done yet.
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=88dcde703ab6
Attachment #8512379 - Flags: review?(dglastonbury)
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
Attachment #8512374 - Flags: review?(dglastonbury) → review+
(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 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.
Attachment #8512375 - Flags: review?(dglastonbury) → review+
Attachment #8512376 - Flags: review?(dglastonbury) → review+
Attachment #8512377 - Flags: review?(dglastonbury) → review+
Attachment #8512378 - Flags: review?(dglastonbury) → review+
Attachment #8512379 - Flags: review?(dglastonbury) → review+
Attachment #8512380 - Flags: review?(dglastonbury) → review+
You need to log in before you can comment on or make changes to this bug.