Closed Bug 968374 Opened 10 years ago Closed 10 years ago

Implement WEBGL_debug_shaders

Categories

(Core :: Graphics: CanvasWebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: jgilbert, Assigned: jgilbert)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(6 files, 7 obsolete files)

905 bytes, patch
bjacob
: review+
Details | Diff | Splinter Review
2.12 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
1.67 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
1.50 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
12.86 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
1.48 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
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.
Let's make these easy to turn on for developers.
Attachment #8370956 - Flags: review?(bjacob)
Comment on attachment 8370956 [details] [diff] [review]
patch 0: Add pref for 'protected' extensions

Maybe we should call these 'debug' extensions?
Attachment #8370956 - Attachment description: pref-chrome-shaders → patch 0: Add pref for 'protected' extensions
Attachment #8370967 - Flags: review?(bjacob)
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?
(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.
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?)
(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().
That is, it's exactly what we feed into glShaderSource.
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.
Blocks: webgl1ext
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.
Forgot to hg add the extension CPP file.
Attachment #8371104 - Flags: review?(bjacob)
Attachment #8370967 - Attachment is obsolete: true
Attachment #8370967 - Flags: review?(bjacob)
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?
Attachment #8370956 - Flags: review?(bjacob) → review+
Attachment #8370959 - Flags: review?(bjacob) → review+
Attachment #8370965 - Flags: review?(bjacob) → review+
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
Attachment #8370997 - Flags: review?(bjacob) → review-
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
Attachment #8371104 - Flags: review?(bjacob) → review+
(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.
Attachment #8371889 - Flags: review?(bjacob)
Attached patch pref-chrome-shaders (obsolete) — Splinter Review
r=bjacob
Attachment #8371890 - Flags: review+
r=bjacob
Attachment #8370959 - Attachment is obsolete: true
Attachment #8371890 - Attachment is obsolete: true
Attachment #8371891 - Flags: review+
Attachment #8370956 - Attachment is obsolete: true
r=bjacob
Attachment #8370965 - Attachment is obsolete: true
Attachment #8371895 - Flags: review+
r=bjacob
Attachment #8371104 - Attachment is obsolete: true
Attachment #8371897 - Flags: review+
Attachment #8370997 - Attachment is obsolete: true
Attachment #8371898 - Flags: review?(bjacob)
Attachment #8371889 - Flags: review?(bjacob) → review+
Attachment #8371898 - Flags: review?(bjacob) → review+
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.