968374 - Implement WEBGL_debug_shaders

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

We should implement this, even if we aren't going to have it be user-facing.
This is useful for both WebGL devs and us implementors, for debugging shaders.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 0: Add pref for 'protected' extensions

Let's make these easy to turn on for developers.

Comment 2

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8370956 [details] [diff] [review]
patch 0: Add pref for 'protected' extensions

Maybe we should call these 'debug' extensions?

Comment 3

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 1: Add test assuring that chrome-only extensions stay that way.

Comment 4

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 2: Fix conformance test to match WEBGL_debug_shaders spec

Comment 5

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3: Implement WEBGL_debug_shaders

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

What is WEBGL_debug_shaders good for? I used to disregard it because it seems to only be useful to debug ANGLE... There are multiple translation layers anyway and this seems to only expose only one arbitrary one among them. Are we going to expose generated HLSL code?

Comment 7

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #6)
> What is WEBGL_debug_shaders good for? I used to disregard it because it
> seems to only be useful to debug ANGLE... There are multiple translation
> layers anyway and this seems to only expose only one arbitrary one among
> them. Are we going to expose generated HLSL code?

To developers, sure. By default, it's chrome-code-only, like WEBGL_debug_renderer_info.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

I wasn't saying that like that would be a bad thing. What I want to understand is, among all the intermediate stages of shader transformation, which ones we're going to expose, and how? (Are we just going to return a big free-form string with all the intermediate forms of the shader?)

Comment 9

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #8)
> I wasn't saying that like that would be a bad thing. What I want to
> understand is, among all the intermediate stages of shader transformation,
> which ones we're going to expose, and how? (Are we just going to return a
> big free-form string with all the intermediate forms of the shader?)

This is a really cool idea, but for now, I'm just exposing what we get out of ANGLE via ShGetObjectCode().

Comment 10

[Kelsey Gilbert [:jgilbert]]

That is, it's exactly what we feed into glShaderSource.

Comment 11

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 4: Fix null-term staying on the string we return to JS

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

Myeah, so that is exactly the arbitrary choice of one particular intermediate form that I am afraid limits the usefulness of this extension. But at least, once we have your patches in, it will be easy to add all the other information that we think is useful.

Comment 13

[Kelsey Gilbert [:jgilbert]]

I think even just this intermediary step is a really good step. It'd be good to get ANGLE's HLSL as well, but even just giving devs an easy way to see what mangling ANGLE's done is progress.

Comment 14

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3: Implement WEBGL_debug_shaders

Forgot to hg add the extension CPP file.

Comment 15

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8370956 [details] [diff] [review]
patch 0: Add pref for 'protected' extensions

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

::: content/canvas/src/WebGLContextExtensions.cpp
@@ +52,5 @@
>  }
>  
>  bool WebGLContext::IsExtensionSupported(JSContext *cx, WebGLExtensionID ext) const
>  {
> +    bool allowProtectedExts = false;

How about we stick to the 'privileged' terminology?

Comment 16

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8370997 [details] [diff] [review]
patch 4: Fix null-term staying on the string we return to JS

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +3308,2 @@
>              ShGetObjectCode(compiler, translatedSrc.BeginWriting());
> +            translatedSrc.SetLength(lenWithNull-1);

No, I looked at the implementation of SetLength, and it does take care of adding +1 char at the end for the terminating zero. That means that you can right away call translatedSrc.SetLength(lenWithNull-1); before calling ShGetObjectCode. There is no reason why you would want to call SetLength twice, is there?

SetLength calls SetCapacity calls MutatePrep which does:

http://dxr.mozilla.org/mozilla-central/source/xpcom/string/src/nsTSubstring.cpp#98

Comment 17

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8371104 [details] [diff] [review]
patch 3: Implement WEBGL_debug_shaders

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +3304,5 @@
>  
>              nsAutoCString translatedSrc;
>              translatedSrc.SetLength(len);
>              ShGetObjectCode(compiler, translatedSrc.BeginWriting());
> +            

Trailing \w

@@ +3325,5 @@
>              // we just pass the raw untranslated shader source. We then can't use ANGLE idenfier mapping.
>              // that's really bad, as that means we can't be 100% conformant. We should work towards always
>              // using ANGLE identifier mapping.
>              gl->fShaderSource(shadername, 1, &s, nullptr);
> +            

Trailing \w

::: content/canvas/src/WebGLExtensionDebugShaders.cpp
@@ +26,5 @@
> +WebGLExtensionDebugShaders::GetTranslatedShaderSource(WebGLShader* shader,
> +                                                      nsAString& retval)
> +{
> +    mContext->GetShaderTranslatedSource(shader, retval);
> +    

Trailing \w

Comment 18

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #15)
> Comment on attachment 8370956 [details] [diff] [review]
> patch 0: Add pref for 'protected' extensions
> 
> Review of attachment 8370956 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContextExtensions.cpp
> @@ +52,5 @@
> >  }
> >  
> >  bool WebGLContext::IsExtensionSupported(JSContext *cx, WebGLExtensionID ext) const
> >  {
> > +    bool allowProtectedExts = false;
> 
> How about we stick to the 'privileged' terminology?

Alright.

Comment 19

[Kelsey Gilbert [:jgilbert]]

Attachment: patch -1: Strip some WS

Comment 20

[Kelsey Gilbert [:jgilbert]]

Attachment: pref-chrome-shaders

r=bjacob

Comment 21

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 0: Add pref for 'privileged' exts

r=bjacob

Comment 22

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 1: Add test assuring that chrome-only extensions stay that way.

r=bjacob

Comment 23

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 2: Fix conformance test to match WEBGL_debug_shaders spec

r=bjacob

Comment 24

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3: Implement WEBGL_debug_shaders

r=bjacob

Comment 25

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 4: Fix null-term staying on the string we return to JS

Comment 26

[Kelsey Gilbert [:jgilbert]]

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=4ef3916ba004

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08313e3b06d3
https://hg.mozilla.org/integration/mozilla-inbound/rev/d74025eb0a71
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0840291ff97
https://hg.mozilla.org/integration/mozilla-inbound/rev/c93990b12047
https://hg.mozilla.org/integration/mozilla-inbound/rev/18c979cfd41f
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f066a76715b

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/08313e3b06d3
https://hg.mozilla.org/mozilla-central/rev/d74025eb0a71
https://hg.mozilla.org/mozilla-central/rev/b0840291ff97
https://hg.mozilla.org/mozilla-central/rev/c93990b12047
https://hg.mozilla.org/mozilla-central/rev/18c979cfd41f
https://hg.mozilla.org/mozilla-central/rev/9f066a76715b

Comment 29

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/API/WEBGL_debug_shaders
https://developer.mozilla.org/en-US/docs/Web/API/WEBGL_debug_shaders/getTranslatedShaderSource