Closed Bug 978418 Opened 6 years ago Closed 6 years ago

Split WebGL GetError flags into 'webgl' and 'driver' errors

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

Attachments

(1 file, 3 obsolete files)

WebGL generates errors in two ways:
1. We detect something wrong, and generate an error ourselves.
2. The driver tells us we can't do something, and we pass this along.

Our logic is made simpler if we pass this reality along to the client, a la 'worst is better'.

Users should already be calling GetError until it returns 0 if they want to clear errors, instead of assuming that one call to GetError will clear the error flag.

I don't think there's a good path to detecting a user doing this wrong, or I'd write that a patch to give them a warning.
Attached patch patch (obsolete) — Splinter Review
Attachment #8384078 - Flags: review?(dglastonbury)
Attachment #8384078 - Flags: feedback?(bjacob)
Attached patch patch (obsolete) — Splinter Review
Changed the name, so it's more obvious what it returns.
Attachment #8384078 - Attachment is obsolete: true
Attachment #8384078 - Flags: review?(dglastonbury)
Attachment #8384078 - Flags: feedback?(bjacob)
Attachment #8384079 - Flags: review?(dglastonbury)
Attachment #8384079 - Flags: feedback?(bjacob)
Comment on attachment 8384079 [details] [diff] [review]
patch

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

The code changes look fine to me. My only concern is that we potential throw away all but the first error from GL driver. Should we have a FIFO for errors instead of a single value?
Comment on attachment 8384079 [details] [diff] [review]
patch

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

The code changes look fine to me. My only concern is that we potential throw away all but the first error from GL driver. Should we have a FIFO for errors instead of a single value?
Attachment #8384079 - Flags: review?(dglastonbury) → review+
Comment on attachment 8384079 [details] [diff] [review]
patch

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

::: content/canvas/src/WebGLContext.h
@@ +853,2 @@
>      GLenum mWebGLError;
> +    GLenum mDriverError;

Call this mOpenGLError: there is no driver, there is just "the GL" ;-)

(except, of course, where we are WorkingAroundDriverBugs, since "the GL" is, of course, perfect, so any problem is to be attributed to a pesky driver)

@@ +853,3 @@
>      GLenum mWebGLError;
> +    GLenum mDriverError;
> +    GLenum GetAndFlushDriverGLErrors();

...and same here.
Attachment #8384079 - Flags: feedback?(bjacob) → feedback+
(In reply to Dan Glastonbury :djg :kamidphish from comment #4)
> Comment on attachment 8384079 [details] [diff] [review]
> patch
> 
> Review of attachment 8384079 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The code changes look fine to me. My only concern is that we potential throw
> away all but the first error from GL driver. Should we have a FIFO for
> errors instead of a single value?

Yeah, but this is what the spec says to do. I agree that it's dumb, but for better error reporting, we should port KHR_debug into a WebGL extension.
(In reply to Benoit Jacob [:bjacob] from comment #5)
> Comment on attachment 8384079 [details] [diff] [review]
> patch
> 
> Review of attachment 8384079 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContext.h
> @@ +853,2 @@
> >      GLenum mWebGLError;
> > +    GLenum mDriverError;
> 
> Call this mOpenGLError: there is no driver, there is just "the GL" ;-)
> 
> (except, of course, where we are WorkingAroundDriverBugs, since "the GL" is,
> of course, perfect, so any problem is to be attributed to a pesky driver)
> 
> @@ +853,3 @@
> >      GLenum mWebGLError;
> > +    GLenum mDriverError;
> > +    GLenum GetAndFlushDriverGLErrors();
> 
> ...and same here.

I see what you mean, but I think it's more obvious having 'Driver' vs 'WebGL', instead of the more similar 'OpenGL' and 'WebGL', which are traditionally a pair prone to confusion in conversations. (namespace near-collisions leading to accidentally saying/using the wrong one)
Attached patch distrib-error (obsolete) — Splinter Review
Attachment #8384079 - Attachment is obsolete: true
Attachment #8386539 - Flags: review?(dglastonbury)
Blocks: 980178
Comment on attachment 8386539 [details] [diff] [review]
distrib-error

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

nit

::: content/canvas/src/WebGLContextUtils.cpp
@@ +236,5 @@
>  {
>      // get and clear GL error in ALL cases
>      GLenum error = gl->GetAndClearError();
> +
> +    // only store in mDriverGLError if is hasn't already recorded an error

Comment is out-of-date.
Attachment #8386539 - Flags: review?(dglastonbury) → review+
Attached patch final patchSplinter Review
r=kamidphish
Attachment #8386539 - Attachment is obsolete: true
Attachment #8387839 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/69cb6eddfc77
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.