Kelsey Gilbert [:jgilbert] Assignee
	Description • 10 years ago

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.

	Kelsey Gilbert [:jgilbert] Assignee
	Comment 1 • 10 years ago

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

	Kelsey Gilbert [:jgilbert] Assignee
	Comment 2 • 10 years ago

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

Maybe we should call these 'debug' extensions?

	Benoit Jacob [:bjacob] (mostly away)
	Comment 6 • 10 years ago

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?

	Kelsey Gilbert [:jgilbert] Assignee
	Comment 7 • 10 years ago

(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.

	Benoit Jacob [:bjacob] (mostly away)
	Comment 8 • 10 years ago

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?)

	Kelsey Gilbert [:jgilbert] Assignee
	Comment 9 • 10 years ago

(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().

	Kelsey Gilbert [:jgilbert] Assignee
	Comment 10 • 10 years ago

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

	Benoit Jacob [:bjacob] (mostly away)
	Comment 12 • 10 years ago

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.

	Kelsey Gilbert [:jgilbert] Assignee
	Comment 13 • 10 years ago

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.

	Kelsey Gilbert [:jgilbert] Assignee
	Comment 14 • 10 years ago

Forgot to hg add the extension CPP file.

	Benoit Jacob [:bjacob] (mostly away)
	Comment 15 • 10 years ago

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?

	Benoit Jacob [:bjacob] (mostly away)
	Comment 16 • 10 years ago

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

	Benoit Jacob [:bjacob] (mostly away)
	Comment 17 • 10 years ago

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

	Kelsey Gilbert [:jgilbert] Assignee
	Comment 18 • 10 years ago

(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.

	Kelsey Gilbert [:jgilbert] Assignee
	Comment 20 • 10 years ago

r=bjacob

	Kelsey Gilbert [:jgilbert] Assignee
	Comment 21 • 10 years ago

r=bjacob

	Kelsey Gilbert [:jgilbert] Assignee
	Comment 22 • 10 years ago

r=bjacob

	Kelsey Gilbert [:jgilbert] Assignee
	Comment 23 • 10 years ago

r=bjacob

	Kelsey Gilbert [:jgilbert] Assignee
	Comment 24 • 10 years ago

r=bjacob

	Kelsey Gilbert [:jgilbert] Assignee
	Comment 26 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 10 years ago

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

	Florian Scholz (Open Web Docs)
	Comment 29 • 9 years ago

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