Implement WEBGL_debug_shaders

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jgilbert, Assigned: jgilbert)

Tracking

({dev-doc-complete})

unspecified
mozilla30
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(6 attachments, 7 obsolete attachments)

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

Description

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

Comment 1

5 years ago
Let's make these easy to turn on for developers.
Attachment #8370956 - Flags: review?(bjacob)
(Assignee)

Comment 2

5 years ago
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
(Assignee)

Comment 5

5 years ago
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?
(Assignee)

Comment 7

5 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.
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?)
(Assignee)

Comment 9

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

Comment 10

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: webgl1ext
(Assignee)

Comment 13

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

Comment 14

5 years ago
Forgot to hg add the extension CPP file.
Attachment #8371104 - Flags: review?(bjacob)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 18

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

Comment 19

5 years ago
Attachment #8371889 - Flags: review?(bjacob)
(Assignee)

Comment 20

5 years ago
Posted patch pref-chrome-shaders (obsolete) — Splinter Review
r=bjacob
Attachment #8371890 - Flags: review+
(Assignee)

Comment 21

5 years ago
r=bjacob
Attachment #8370959 - Attachment is obsolete: true
Attachment #8371890 - Attachment is obsolete: true
Attachment #8371891 - Flags: review+
(Assignee)

Updated

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

Comment 23

5 years ago
r=bjacob
Attachment #8370965 - Attachment is obsolete: true
Attachment #8371895 - Flags: review+
(Assignee)

Comment 24

5 years ago
r=bjacob
Attachment #8371104 - Attachment is obsolete: true
Attachment #8371897 - Flags: review+
(Assignee)

Comment 25

5 years ago
Attachment #8370997 - Attachment is obsolete: true
Attachment #8371898 - Flags: review?(bjacob)
Attachment #8371889 - Flags: review?(bjacob) → review+
Attachment #8371898 - Flags: review?(bjacob) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.