Last Comment Bug 745292 - Log non-empty shader/program infoLogs as WebGL (JS) warnings
: Log non-empty shader/program infoLogs as WebGL (JS) warnings
Status: RESOLVED FIXED
webgl-next
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-13 12:12 PDT by Benoit Jacob [:bjacob] (mostly away)
Modified: 2012-05-31 06:20 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
report shader/program infologs as warnings (12.52 KB, patch)
2012-05-24 07:52 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review-
Details | Diff | Review
report shader/program infologs as warnings (12.51 KB, patch)
2012-05-25 06:29 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
Details | Diff | Review

Description Benoit Jacob [:bjacob] (mostly away) 2012-04-13 12:12:50 PDT
Will become really realistic once bug 743753 is resolved. Would be very helpful. Assumption here is that the infolog is always empty when there is no particular problem i.e. we don't have stupid compilers that write "Success!!!11!" to it.
Comment 1 Benoit Jacob [:bjacob] (mostly away) 2012-05-24 07:52:28 PDT
Created attachment 626806 [details] [diff] [review]
report shader/program infologs as warnings

Did some renaming of the new variable and constant introduced in bug 743753.

See the comment about how in principle we can't get the shader infologs before glLinkProgram has been called.

I wanted to have only one implementation of getting a shader/program's infolog, as that is a bit nontrivial (a shader infolog may come either from ANGLE or from the driver). Unfortunately GetShaderInfoLog converts to UTF16 which is useless for us here as JS warnings are 8bit strings. So I added overloads of Get{Shader,Program}InfoLog not doing the conversion.

When a shader infolog has been reported, we don't report the program infolog as this is then, in most cases, just a less helpful duplicate of the information already given in the shader infolog.
Comment 2 Jeff Gilbert [:jgilbert] 2012-05-25 02:04:16 PDT
Comment on attachment 626806 [details] [diff] [review]
report shader/program infologs as warnings

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

R- pending explanations.

::: content/canvas/src/WebGLContext.h
@@ +1354,5 @@
>      bool mDrawSinceContextLossTimerSet;
>      ContextStatus mContextStatus;
>      bool mContextLostErrorSet;
>  
> +    static const int sMaxWarningsToGenerate = 32;

Yes this is static, but static+const should probably be 'kMaxWarningsToGenerate'.

::: content/canvas/src/WebGLContextUtils.cpp
@@ +63,2 @@
>              JS_ReportWarning(ccx,
>                  "WebGL: no further warnings will be reported for this WebGL context "

Surely 'no' should be capitalized?

::: content/canvas/src/WebGLContextValidate.cpp
@@ +53,5 @@
>              NS_ABORT_IF_FALSE(loc >= 0, "major oops in managing the attributes of a WebGL program");
>              if (loc < mContext->mGLMaxVertexAttribs) {
>                  mAttribsInUse[loc] = true;
>              } else {
> +                mContext->GenerateWarning("program exceeds MAX_VERTEX_ATTRIBS");

Is this actually not an InvalidOperation?
Comment 3 Benoit Jacob [:bjacob] (mostly away) 2012-05-25 06:29:35 PDT
Created attachment 627212 [details] [diff] [review]
report shader/program infologs as warnings

(In reply to Jeff Gilbert [:jgilbert] from comment #2)
> Comment on attachment 626806 [details] [diff] [review]
> report shader/program infologs as warnings
> 
> Review of attachment 626806 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> R- pending explanations.
> 
> ::: content/canvas/src/WebGLContext.h
> @@ +1354,5 @@
> >      bool mDrawSinceContextLossTimerSet;
> >      ContextStatus mContextStatus;
> >      bool mContextLostErrorSet;
> >  
> > +    static const int sMaxWarningsToGenerate = 32;
> 
> Yes this is static, but static+const should probably be
> 'kMaxWarningsToGenerate'.

Actually, found a way to not even have to name this constant.

> 
> ::: content/canvas/src/WebGLContextUtils.cpp
> @@ +63,2 @@
> >              JS_ReportWarning(ccx,
> >                  "WebGL: no further warnings will be reported for this WebGL context "
> 
> Surely 'no' should be capitalized?

OK.

> 
> ::: content/canvas/src/WebGLContextValidate.cpp
> @@ +53,5 @@
> >              NS_ABORT_IF_FALSE(loc >= 0, "major oops in managing the attributes of a WebGL program");
> >              if (loc < mContext->mGLMaxVertexAttribs) {
> >                  mAttribsInUse[loc] = true;
> >              } else {
> > +                mContext->GenerateWarning("program exceeds MAX_VERTEX_ATTRIBS");
> 
> Is this actually not an InvalidOperation?

I don't think so: this WebGLProgram::UpdateInfo() is called upon program linking, this "program exceeds MAX_VERTEX_ATTRIBS" should simply be a program linking error, and program linking errors aren't GL errors.
Comment 4 Jeff Gilbert [:jgilbert] 2012-05-25 13:41:02 PDT
Comment on attachment 627212 [details] [diff] [review]
report shader/program infologs as warnings

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

::: content/canvas/src/WebGLContext.h
@@ +1357,5 @@
>  
> +    int mAlreadyGeneratedWarnings;
> +
> +    bool ShouldGenerateWarnings() const {
> +        return mAlreadyGeneratedWarnings < 32;

I don't love it, but it'll do.
Comment 5 Benoit Jacob [:bjacob] (mostly away) 2012-05-29 11:46:19 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/90292e7ec6fb
Comment 6 Benoit Jacob [:bjacob] (mostly away) 2012-05-29 13:34:38 PDT
Backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5ea5a2ba6f1

50465 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | [conformance/context/context-lost.html] Test failed - gl.getProgramInfoLog(program) should be null (of type object). Was (of type string).

50468 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/webgl/test_webgl_conformance_test_suite.html | [conformance/context/context-lost.html] Test failed - gl.getShaderInfoLog(shader) should be null (of type object). Was (of type string).
Comment 7 Benoit Jacob [:bjacob] (mostly away) 2012-05-29 13:49:28 PDT
New try:
https://tbpl.mozilla.org/?tree=Try&rev=89d986d02cd4

Apparently, CopyASCIItoUTF16 doesn't preserve string voidness (nullity).
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2012-05-30 05:32:10 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/d92215a6eea1
Comment 9 Ed Morley [:emorley] 2012-05-31 06:20:01 PDT
https://hg.mozilla.org/mozilla-central/rev/d92215a6eea1

Note You need to log in before you can comment on or make changes to this bug.