Implement the backend for a WebGL shader editor

RESOLVED FIXED in Firefox 27

Status

DevTools
WebGL Shader Editor
RESOLVED FIXED
5 years ago
6 days ago

People

(Reporter: vporof, Assigned: vporof)

Tracking

unspecified
Firefox 27
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 797571 [details] [diff] [review]
webgl-backend.patch

Of course this needs tests. I will write tests. But for now, let's get this rolling.
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attachment #797571 - Flags: review?(dcamp)
(Assignee)

Updated

5 years ago
Blocks: 910955
(Assignee)

Comment 2

5 years ago
Created attachment 797575 [details]
This is how you use it

Comment 3

5 years ago
Comment on attachment 797571 [details] [diff] [review]
webgl-backend.patch

I can't confidently sign off on the gl code, but I'm happy with the actor code.
Attachment #797571 - Flags: review?(dcamp) → review+
Attachment #797571 - Flags: review?(vladimir)
(Assignee)

Comment 4

5 years ago
Comment on attachment 797571 [details] [diff] [review]
webgl-backend.patch

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

::: toolkit/devtools/server/actors/webgl.js
@@ +262,5 @@
> +
> +      // Make sure the shader data array always contains the vertex shader
> +      // first, and the fragment shader second. There are no guarantees that
> +      // the compilation order of shaders in the debuggee is always the same.
> +      if (shaderText.contains("gl_FragColor")) {

This is a brain fart. I should be using gl.getShaderParameter(..., SHADER_TYPE) instead.
Comment on attachment 797571 [details] [diff] [review]
webgl-backend.patch

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

I don't think the one issue below should block landing, but it's a papercut that would be nice to at least do the simple bandaid (indicate whether the shader is in a last-link-failed state)

::: toolkit/devtools/server/actors/webgl.js
@@ +478,5 @@
> +   * @param void glResult
> +   *        The returned value of the original function call.
> +   */
> +  linkProgram: function(glArgs, glResult) {
> +    let { gl, cache } = this;

glResult is/should just be undefined, most of these functions always return void.

This is a little tricky, and I'm not sure if this matters -- but you really want to check the link status here.  Calling getProgramParameter with GL_LINK_STATUS will return a boolean indicating whether the link is successful or not.

For a next step for this inspector, this step should also get the PROGRAM_INFO_LOG (both on successful and unsuccessful link; some drivers put some info there on successful link) and make it available to the user.

But, I'm not sure what should be done if the link fails.  It's still valid to query the source of the attached shaders, but the actual in-use program will still be any previous one in case link was called twice with different shaders (with the second set causing a link error).

This is probably not a dealbreaker, but there should be caveats.  But, at least reporting that the last link on a program was unsuccessful would probably prevent some WTF moments.

The same applies to compileShader, actually, which isn't wrapped; it would be really handy to query the compile status and fetch the shader info log.

@@ +502,5 @@
> +   *        The returned value of the original function call.
> +   */
> +  getAttribLocation: function(glArgs, glResult) {
> +    let [program, name] = glArgs;
> +    this.cache.call("addAttribute", program, name, glResult);

Not sure if it matters, but after linkProgram, you could just walk all the attribs and get their info instead.  But I suppose if you only care about the attrib locations that the program actually uses, this is simpler :)  (There's some stuff you ahve to deal with arrays that make walking the list complex.)
Attachment #797571 - Flags: review?(vladimir) → review+
(Assignee)

Updated

5 years ago
Blocks: 917226
(Assignee)

Comment 6

5 years ago
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #5)

Thanks for the review!

> But, I'm not sure what should be done if the link fails.  It's still valid
> to query the source of the attached shaders, but the actual in-use program
> will still be any previous one in case link was called twice with different
> shaders (with the second set causing a link error).
> 

You're right. I haven't really seen this happening in the wild too much, but it's a valid concern that should be looked into. Since it mostly affects presentation than internal logic, I'll file a followup bug for this, fixable along with or after bug 910955.

> > +   *        The returned value of the original function call.
> > +   */
> > +  getAttribLocation: function(glArgs, glResult) {
> > +    let [program, name] = glArgs;
> > +    this.cache.call("addAttribute", program, name, glResult);
> 
> Not sure if it matters, but after linkProgram, you could just walk all the
> attribs and get their info instead.  But I suppose if you only care about
> the attrib locations that the program actually uses, this is simpler :) 
> (There's some stuff you ahve to deal with arrays that make walking the list
> complex.)

The current logic really doesn't care about unused attributes or uniforms for now. If this (or some other WebGL tool) will need this functionality, I'll implement the required backend support then. It'd certainly be nice to have better shaders inspection capabilities that would present every possible detail about them.
(Assignee)

Comment 7

5 years ago
Created attachment 811974 [details] [diff] [review]
webgl-backend + tests

Added a bunch of tests and fixed some things I found while writing said tests.
Attachment #797575 - Attachment is obsolete: true
Attachment #811974 - Flags: review?(dcamp)
(In reply to Victor Porof [:vp] from comment #6)
> The current logic really doesn't care about unused attributes or uniforms
> for now. If this (or some other WebGL tool) will need this functionality,
> I'll implement the required backend support then. It'd certainly be nice to
> have better shaders inspection capabilities that would present every
> possible detail about them.

The thing is, they can actually be important -- things like attribs/uniforms will have default values, and if they're not set it could be the source of bugs.  So being able to see a uniform with a bogus default value could be what tells you why your shader is being wonky :)

But, same as before, that's for a followup.
(Assignee)

Updated

5 years ago
Attachment #811974 - Flags: review?(dcamp) → review?(rcampbell)
Comment on attachment 811974 [details] [diff] [review]
webgl-backend + tests

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

we should deal with the makefile.in changes before landing this.

::: browser/devtools/webgleditor/test/Makefile.in
@@ +20,5 @@
> +	head.js \
> +	doc_multiple-contexts.html \
> +	doc_shader-order.html \
> +	doc_simple-canvas.html \
> +	$(NULL)

as per recent mailing list instructions, we're now going to be defining mochitest browser test files in browser.ini.

read: https://ci.mozilla.org/job/mozilla-central-docs/Build_Documentation/test_manifests.html.

Refer also to the thread on dev-platform, https://groups.google.com/forum/#!topic/mozilla.dev.platform/F-uESi8ByPE.
Attachment #811974 - Flags: review?(rcampbell)
(Assignee)

Comment 11

5 years ago
Created attachment 814997 [details] [diff] [review]
webgl-backend.patch + tests

That was an easy rebase.
Attachment #811974 - Attachment is obsolete: true
Attachment #814997 - Flags: review?(rcampbell)
(Assignee)

Comment 12

5 years ago
Comment on attachment 814997 [details] [diff] [review]
webgl-backend.patch + tests

Everyone wants a piece of dis.
Attachment #814997 - Flags: review?(rcampbell) → review?(dcamp)
Comment on attachment 814997 [details] [diff] [review]
webgl-backend.patch + tests

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

::: browser/devtools/shadereditor/test/head.js
@@ +210,5 @@
> +    yield target.makeRemote();
> +
> +    let front = new WebGLFront(target.client, target.form);
> +    throw new Task.Result([target, debuggee, front]);
> +  });

lovely stuff.
Attachment #814997 - Flags: review?(dcamp) → review+

Updated

5 years ago
Attachment #814997 - Flags: review+
Comment on attachment 814997 [details] [diff] [review]
webgl-backend.patch + tests

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

::: browser/devtools/shadereditor/test/head.js
@@ +209,5 @@
> +
> +    yield target.makeRemote();
> +
> +    let front = new WebGLFront(target.client, target.form);
> +    throw new Task.Result([target, debuggee, front]);

Task.jsm supports ES6 generators now, so you can do this a bit more cleanly:

> return Task.spawn(function*() {
>   /* ... */
>   return [target, debugger, front];
> }
(Assignee)

Comment 15

5 years ago
(In reply to Brandon Benvie [:bbenvie] from comment #14)
> 
> Task.jsm supports ES6 generators now, so you can do this a bit more cleanly:
> 
> > return Task.spawn(function*() {
> >   /* ... */
> >   return [target, debugger, front];
> > }

Nice.
(Assignee)

Updated

5 years ago
Flags: sec-review?(mgoodwin)
(Assignee)

Updated

5 years ago
Attachment #797571 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Component: Developer Tools → Developer Tools: WebGL Shader Editor
https://hg.mozilla.org/mozilla-central/rev/273efc178016
https://hg.mozilla.org/mozilla-central/rev/9ba511439633
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
Flags: sec-review?(mgoodwin)

Updated

6 days ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.