Closed Bug 1278403 Opened 4 years ago Closed 4 years ago

Default to MOZ_GL_DEBUG_ABORT_ON_ERROR=1 for WebGL on DEBUG builds

Categories

(Core :: Canvas: WebGL, defect)

49 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

Details

Attachments

(9 files)

We should definitely run MOZ_GL_DEBUG_ABORT_ON_ERROR=1 when running CI tests on DEBUG builds.
Review commit: https://reviewboard.mozilla.org/r/58104/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58104/
Attachment #8760521 - Flags: review?(milan)
Attachment #8760522 - Flags: review?(jmuizelaar)
Attachment #8760523 - Flags: review?(jmuizelaar)
Attachment #8760524 - Flags: review?(jmuizelaar)
Attachment #8760525 - Flags: review?(jmuizelaar)
Attachment #8760521 - Flags: review?(milan) → review+
Attachment #8760524 - Flags: review?(jmuizelaar) → review+
(In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> https://reviewboard.mozilla.org/r/58112/#review55772
> 
> Why?

We do all validation in our WebGL layer, we only really generate GL_OOM from the driver, no other errors.

This is opposed to SkiaGL, which may or may not rely on errors returned from the driver. This is why I am not just turning on ABORT_ON_ERROR for all GLContexts.

(Also, later, we should be creating no-error contexts on non-DEBUG builds, so the driver doesn't replicate our validation. ANGLE already supports this)
Flags: needinfo?(jmuizelaar)
(In reply to Jeff Gilbert [:jgilbert] from comment #9)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> > https://reviewboard.mozilla.org/r/58112/#review55772
> > 
> > Why?
> 
> We do all validation in our WebGL layer, we only really generate GL_OOM from
> the driver, no other errors.
> 
> This is opposed to SkiaGL, which may or may not rely on errors returned from
> the driver. This is why I am not just turning on ABORT_ON_ERROR for all
> GLContexts.
> 
> (Also, later, we should be creating no-error contexts on non-DEBUG builds,
> so the driver doesn't replicate our validation. ANGLE already supports this)

This would be valuable information to put in the commit message.
Flags: needinfo?(jmuizelaar)
Attachment #8760523 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8760523 [details]
Bug 1278403 - Choose GLContext::DebugFlags based on ContextCreateFlags. -

https://reviewboard.mozilla.org/r/58108/#review56032
Comment on attachment 8760525 [details]
Bug 1278403 - WebGL doesn't need validation. -

https://reviewboard.mozilla.org/r/58112/#review56038
Attachment #8760525 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8760522 [details]
Bug 1278403 - Spread CreateContextFlags to GLContext::ctor. -

https://reviewboard.mozilla.org/r/58106/#review56074
Attachment #8760522 - Flags: review?(jmuizelaar) → review+
Review commit: https://reviewboard.mozilla.org/r/59440/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59440/
Attachment #8763112 - Flags: review?(jmuizelaar)
Attachment #8763113 - Flags: review?(jmuizelaar)
Comment on attachment 8760521 [details]
Bug 1278403 - Clean up gfxEnv.h -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58104/diff/1-2/
Comment on attachment 8760522 [details]
Bug 1278403 - Spread CreateContextFlags to GLContext::ctor. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58106/diff/1-2/
Comment on attachment 8760523 [details]
Bug 1278403 - Choose GLContext::DebugFlags based on ContextCreateFlags. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58108/diff/1-2/
Comment on attachment 8760524 [details]
Bug 1278403 - Misc code cleanup. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58110/diff/1-2/
Comment on attachment 8760525 [details]
Bug 1278403 - WebGL doesn't need validation. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58112/diff/1-2/
https://reviewboard.mozilla.org/r/59444/#review56906

Why don't we want to investigate/fix these? They seem like the kind of thing this patchset was designed to catch.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #23)
> https://reviewboard.mozilla.org/r/59444/#review56906
> 
> Why don't we want to investigate/fix these? They seem like the kind of thing
> this patchset was designed to catch.

Yes and no. This is primarily to prevent regressing from our current state. It's also handy to identify these, but these should be investigated after this bug lands. (relatively low priority, though)
Comment on attachment 8763113 [details]
Bug 1278403 - Disable tests on DEBUG with suspected spurious-error-reporting drivers. -

https://reviewboard.mozilla.org/r/59444/#review56926
Attachment #8763113 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8763753 [details]
Bug 1278403 - Clarify crash message in case anyone unfamiliar runs into this. -

https://reviewboard.mozilla.org/r/59902/#review56930
Attachment #8763753 - Flags: review?(jmuizelaar) → review+
Attachment #8763112 - Flags: review?(jmuizelaar) → review+
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b2f6a194063
Clean up gfxEnv.h - r=milan
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1b7a4207378
Spread CreateContextFlags to GLContext::ctor. - r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c958e7309e7
Fix formatting.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5383918f68f
Choose GLContext::DebugFlags based on ContextCreateFlags. - r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfb9047aaa22
Misc code cleanup. - r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/43de2b8ce001
WebGL doesn't need validation. - r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e482a936dbc
Mark now-too-slow tests. - r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9cdd2062581
Disable tests on DEBUG with suspected spurious-error-reporting drivers. - r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/496586403d9d
Clarify crash message in case anyone unfamiliar runs into this. - r=jrmuizel
You need to log in before you can comment on or make changes to this bug.