Closed Bug 1109945 Opened 5 years ago Closed 5 years ago

Rewrite shader/program handling

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

Attachments

(9 files, 7 obsolete files)

21.12 KB, patch
Details | Diff | Splinter Review
178.19 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
1.94 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
2.70 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
973 bytes, text/html
Details
21.50 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
2.53 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
6.26 KB, patch
Details | Diff | Splinter Review
186.54 KB, patch
Details | Diff | Splinter Review
We need to decouple as much as possible from ANGLE's validator if we want to allow for bypassing it.

We also need to switch to the new ANGLE validator APIs, since the entrypoints we currently use for checking uniforms, etc. are deprecated.
So, uh, this is big.
Attachment #8534668 - Flags: review?(dglastonbury)
This was fixed upstream here:
https://github.com/KhronosGroup/WebGL/pull/803
Attachment #8534670 - Flags: review?(dglastonbury)
Fixes needed after rebase.
Attachment #8534671 - Flags: review?(dglastonbury)
Info on nsWrapperCache which should be added to a comment somewhere:
https://pastebin.mozilla.org/7917861
Comment on attachment 8534668 [details] [diff] [review]
0002-Rewrite-shader-validation-handling-to-allow-bypass.patch

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

I like this refactor *a lot*

::: dom/canvas/WebGLContextGL.cpp
@@ +1333,2 @@
>  
> +    retval.SetIsVoid(false);

Can GetProgramInfoLog do the SetIsVoid(false)?

@@ +2840,5 @@
>  
>      if (!ValidateObject("getShaderInfoLog: shader", shader))
>          return;
>  
> +    shader->GetShaderInfoLog(&retval);

No retval.SetIsVoid(false)?

::: dom/canvas/WebGLProgram.cpp
@@ +266,2 @@
>  {
> +    WebGLRefPtr<WebGLShader>* shaderSlot;

What if shader is NULL? This function will access NULL ptr.

@@ +323,5 @@
> +                                    index, "ACTIVE_UNIFORMS", activeList.size());
> +        return nullptr;
> +    }
> +
> +    nsRefPtr<WebGLActiveInfo> ret =  activeList[index];

Remove extraneous WS after =

@@ +340,5 @@
> +        out->AppendElement(mFragShader);
> +}
> +
> +GLint
> +WebGLProgram::GetAttribLocation(const nsAString& userName_wide) const

_wide == *puke*

::: dom/canvas/WebGLProgram.h
@@ +134,4 @@
>  
> +public:
> +    const GLuint mGLName;
> +private:

Blank line before private:

::: dom/canvas/WebGLShader.h
@@ +56,4 @@
>      }
>  
> +    // Other
> +    size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const;

Is this for memory tracking? That might be a better comment than: // Other

::: dom/canvas/WebGLShaderValidator.cpp
@@ +30,5 @@
> +{
> +    int options = SH_VARIABLES |
> +                  SH_ENFORCE_PACKING_RESTRICTIONS |
> +                  SH_INIT_VARYINGS_WITHOUT_STATIC_USE |
> +                  SH_OBJECT_CODE;

I think this will need updating with code from Bug 1602355 - SH_LIMIT_CALL_STACK_DEPTH.

@@ +113,5 @@
> +    // Tell ANGLE to allow highp in frag shaders. (unless disabled)
> +    // If underlying GLES doesn't have highp in frag shaders, it should complain anyways.
> +    resources.FragmentPrecisionHigh = mDisableFragHighP ? 0 : 1;
> +
> +    //resources.HashFunction = IdentifierHashFunc;

Remove?

::: dom/canvas/WebGLUniformLocation.cpp
@@ +30,5 @@
> +{
> +    // Check the weak-pointer.
> +    if (!mLinkInfo) {
> +        webgl->ErrorInvalidOperation("%s: This uniform location is obsolete because its"
> +                                     " program has been relinked successfully.",

... program has been successfully relinked.

Sounds better to my ear.

@@ +223,5 @@
> +            bool boolBuffer[kMaxElemSize];
> +            for (uint8_t i = 0; i < kMaxElemSize; i++)
> +                boolBuffer[i] = buffer[i];
> +
> +            JS::Rooted<JS::Value> val(js);

JS::RootedValue
Attachment #8534668 - Flags: review?(dglastonbury) → review+
Attachment #8534670 - Flags: review?(dglastonbury) → review+
Comment on attachment 8534671 [details] [diff] [review]
0004-Rebased.patch

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

::: dom/canvas/WebGL2ContextTransformFeedback.cpp
@@ +244,4 @@
>      if (len == 0 || tfsize == 0 || tftype == 0)
>          return nullptr;
>  
> +    MOZ_CRASH("todo");

Don't really like this, but I'll let it land and we can sort out if the demo needs it.
Attachment #8534671 - Flags: review?(dglastonbury) → review+
(In reply to Dan Glastonbury :djg :kamidphish from comment #7)
> Comment on attachment 8534671 [details] [diff] [review]
> 0004-Rebased.patch
> 
> Review of attachment 8534671 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGL2ContextTransformFeedback.cpp
> @@ +244,4 @@
> >      if (len == 0 || tfsize == 0 || tftype == 0)
> >          return nullptr;
> >  
> > +    MOZ_CRASH("todo");
> 
> Don't really like this, but I'll let it land and we can sort out if the demo
> needs it.

I agree, but it would hold things up to get this implemented properly. I don't completely understand transform feedback yet, so I figured it'd be quickest to just leave a loud crash here until we get around to fixing it.
Comment on attachment 8534668 [details] [diff] [review]
0002-Rewrite-shader-validation-handling-to-allow-bypass.patch

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

::: dom/canvas/WebGLContextGL.cpp
@@ +1333,2 @@
>  
> +    retval.SetIsVoid(false);

Yes, but I'd really prefer not to leak this binding detail further into the WebGL machinery.

I changed this so it comes after the (implicitly always successful) `void WebGLProgram::GetProgramInfoLog()` call. I think this better conveys that "oh, since we're successful, before returning back to JS, unset this from being void. (I believe 'isVoid' is just a marker on the var, otherwise this won't work, though the failure would be picked up by the conformance tests)

@@ +2840,5 @@
>  
>      if (!ValidateObject("getShaderInfoLog: shader", shader))
>          return;
>  
> +    shader->GetShaderInfoLog(&retval);

It's inside the function. I have brought it back outside like in GetProgramInfoLog.

::: dom/canvas/WebGLProgram.cpp
@@ +266,2 @@
>  {
> +    WebGLRefPtr<WebGLShader>* shaderSlot;

It's guaranteed by the caller. I added an assert to make this clear.

@@ +323,5 @@
> +                                    index, "ACTIVE_UNIFORMS", activeList.size());
> +        return nullptr;
> +    }
> +
> +    nsRefPtr<WebGLActiveInfo> ret =  activeList[index];

Fixed :(

@@ +340,5 @@
> +        out->AppendElement(mFragShader);
> +}
> +
> +GLint
> +WebGLProgram::GetAttribLocation(const nsAString& userName_wide) const

All things about it are pretty puke-worthy. At least this is very up-front about its puke-worthy-ness.

::: dom/canvas/WebGLProgram.h
@@ +134,4 @@
>  
> +public:
> +    const GLuint mGLName;
> +private:

I was trying to bunch all these member vars closer together, but I can add the WS here.

::: dom/canvas/WebGLShader.h
@@ +56,4 @@
>      }
>  
> +    // Other
> +    size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const;

I changed this to `// Other funcs`. It's in opposition to `// GL funcs` and `// Util [funcs]`.

::: dom/canvas/WebGLShaderValidator.cpp
@@ +30,5 @@
> +{
> +    int options = SH_VARIABLES |
> +                  SH_ENFORCE_PACKING_RESTRICTIONS |
> +                  SH_INIT_VARYINGS_WITHOUT_STATIC_USE |
> +                  SH_OBJECT_CODE;

Yeah, I see that's on inbound. I'll catch this with with the merge conflict.

@@ +113,5 @@
> +    // Tell ANGLE to allow highp in frag shaders. (unless disabled)
> +    // If underlying GLES doesn't have highp in frag shaders, it should complain anyways.
> +    resources.FragmentPrecisionHigh = mDisableFragHighP ? 0 : 1;
> +
> +    //resources.HashFunction = IdentifierHashFunc;

Right.

::: dom/canvas/WebGLUniformLocation.cpp
@@ +30,5 @@
> +{
> +    // Check the weak-pointer.
> +    if (!mLinkInfo) {
> +        webgl->ErrorInvalidOperation("%s: This uniform location is obsolete because its"
> +                                     " program has been relinked successfully.",

I could go either way, so I'll cede.

@@ +223,5 @@
> +            bool boolBuffer[kMaxElemSize];
> +            for (uint8_t i = 0; i < kMaxElemSize; i++)
> +                boolBuffer[i] = buffer[i];
> +
> +            JS::Rooted<JS::Value> val(js);

Ok.
Attached patch 0005-Review-fixups.patch (obsolete) — Splinter Review
Attachment #8535220 - Flags: review?(dglastonbury)
Attached patch 0005-Review-fixups.patch (obsolete) — Splinter Review
Forgot to save a file.
Attachment #8535220 - Attachment is obsolete: true
Attachment #8535220 - Flags: review?(dglastonbury)
Attachment #8535233 - Flags: review?(dglastonbury)
Attachment #8535233 - Flags: review?(dglastonbury) → review+
Comment on attachment 8535233 [details] [diff] [review]
0005-Review-fixups.patch

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

Need to add SH_LIMIT_CALL_STACK_DEPTH to compileOptions in WebGLShaderValidator.cpp
Attachment #8535233 - Flags: review+ → review-
Attached patch Compilation Fixes (obsolete) — Splinter Review
Jeff, I needed these changes to get the patches to compile.
Assignee: jgilbert → dglastonbury
Status: NEW → ASSIGNED
Bzexport stole the assignment.
Assignee: dglastonbury → jgilbert
Attached patch Fix SubstringStartsWith (obsolete) — Splinter Review
ArraySize for a char[] includes the trailing \0. We don't want to match on that.
Attachment #8537529 - Flags: review?(jgilbert)
Enable bypassing ANGLE checks if WebGL2 context.
Attachment #8537531 - Flags: review?(jgilbert)
Attached patch Fix version replacement (obsolete) — Splinter Review
Ditto. erase shouldn't include trailing '\0' in length calculation.
Attachment #8537533 - Flags: review?(jgilbert)
In the refactor, these cases disappeared.
Attachment #8537535 - Flags: review?(jgilbert)
Attachment #8537529 - Flags: review?(jgilbert) → review+
Comment on attachment 8537531 [details] [diff] [review]
Enable BypassShaderValidation in WebGL contexts

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

I guess this is OK.
Attachment #8537531 - Flags: review?(jgilbert) → review+
Comment on attachment 8537533 [details] [diff] [review]
Fix version replacement

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

::: dom/canvas/WebGLShader.cpp
@@ +86,5 @@
>          glesslVersion = 100;
>      }
>  
>      std::string reversionedSource = source;
> +    reversionedSource.erase(versionStrStart, versionStrLen-1);

`versionStrLen` should not include the null-term. Fix it above, not here.
Attachment #8537533 - Flags: review?(jgilbert) → review-
Attachment #8537535 - Flags: review?(jgilbert) → review+
(In reply to Jeff Gilbert [:jgilbert] from comment #20)
> Comment on attachment 8537533 [details] [diff] [review]
> Fix version replacement
> 
> Review of attachment 8537533 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGLShader.cpp
> @@ +86,5 @@
> >          glesslVersion = 100;
> >      }
> >  
> >      std::string reversionedSource = source;
> > +    reversionedSource.erase(versionStrStart, versionStrLen-1);
> 
> `versionStrLen` should not include the null-term. Fix it above, not here.

Should strlen be used instead?
(In reply to Dan Glastonbury :djg :kamidphish from comment #21)
> (In reply to Jeff Gilbert [:jgilbert] from comment #20)
> > Comment on attachment 8537533 [details] [diff] [review]
> > Fix version replacement
> > 
> > Review of attachment 8537533 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/canvas/WebGLShader.cpp
> > @@ +86,5 @@
> > >          glesslVersion = 100;
> > >      }
> > >  
> > >      std::string reversionedSource = source;
> > > +    reversionedSource.erase(versionStrStart, versionStrLen-1);
> > 
> > `versionStrLen` should not include the null-term. Fix it above, not here.
> 
> Should strlen be used instead?

Yeah, we should use it wherever we can. Dealing with string lengths manually is bad.
Attachment #8535233 - Attachment is obsolete: true
Attachment #8544885 - Flags: review?(dglastonbury)
Rolled up your changes when merging into my branch.
Attachment #8537526 - Attachment is obsolete: true
Attachment #8537529 - Attachment is obsolete: true
Attachment #8537531 - Attachment is obsolete: true
Attachment #8537533 - Attachment is obsolete: true
Attachment #8537535 - Attachment is obsolete: true
Attachment #8544886 - Flags: review?(dglastonbury)
Comment on attachment 8544885 [details] [diff] [review]
0005-Review-fixups.patch

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

::: dom/canvas/WebGLProgram.cpp
@@ +77,4 @@
>      infoLocMap->insert(std::make_pair(info->mBaseUserName, info.get()));
>  }
>  
> +#define DUMP_SHADERVAR_MAPPINGS

Do you want to land with this enabled?
Attachment #8544885 - Flags: review?(dglastonbury) → review+
Comment on attachment 8544886 [details] [diff] [review]
0006-Fixups-from-kamidphish.patch

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

Happy to land if you remove the LOCAL_GL_ACTIVE_UNIFORM_BLOCK_MAX_NAME_LENGTH case.

::: dom/canvas/WebGLProgram.cpp
@@ +431,5 @@
>  
> +    if (mContext->IsWebGL2()) {
> +        switch (pname) {
> +        case LOCAL_GL_ACTIVE_UNIFORM_BLOCKS:
> +        case LOCAL_GL_ACTIVE_UNIFORM_BLOCK_MAX_NAME_LENGTH:

This has been removed by WebGL2. https://www.khronos.org/registry/webgl/specs/latest/2.0/#5.9

I don't know what we should do for this in the case of compiling es3 via emscripten. I guess for now remove the MAX_NAME_LENGTH query and in emscripten it'll have to return 256.
Attachment #8544886 - Flags: review?(dglastonbury) → review+
(In reply to Dan Glastonbury :djg :kamidphish from comment #26)
> Comment on attachment 8544885 [details] [diff] [review]
> 0005-Review-fixups.patch
> 
> Review of attachment 8544885 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGLProgram.cpp
> @@ +77,4 @@
> >      infoLocMap->insert(std::make_pair(info->mBaseUserName, info.get()));
> >  }
> >  
> > +#define DUMP_SHADERVAR_MAPPINGS
> 
> Do you want to land with this enabled?

No.
Flags: needinfo?(jgilbert)
https://tbpl.mozilla.org/?tree=Try&rev=b28a1ea474a3
It's compiling on Try, and I reproduced and fixed the devtools tests locally, so it should be good to go.
 
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5c6dd9423cf
https://hg.mozilla.org/mozilla-central/rev/ca0fbdaf25e0
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1125475
Blocks: 1097227
You need to log in before you can comment on or make changes to this bug.