Rewrite shader/program handling

RESOLVED FIXED in mozilla38

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jgilbert, Assigned: jgilbert)

Tracking

(Depends on: 1 bug)

unspecified
mozilla38
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 7 obsolete attachments)

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

Description

3 years ago
Created attachment 8534667 [details] [diff] [review]
0001-Initial-patches-from-kamidphish.patch

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

Comment 1

3 years ago
Created attachment 8534668 [details] [diff] [review]
0002-Rewrite-shader-validation-handling-to-allow-bypass.patch

So, uh, this is big.
Attachment #8534668 - Flags: review?(dglastonbury)
(Assignee)

Comment 2

3 years ago
Created attachment 8534670 [details] [diff] [review]
0003-Fix-conf-misc-bad-arguments-test.patch

This was fixed upstream here:
https://github.com/KhronosGroup/WebGL/pull/803
Attachment #8534670 - Flags: review?(dglastonbury)
(Assignee)

Comment 3

3 years ago
Created attachment 8534671 [details] [diff] [review]
0004-Rebased.patch

Fixes needed after rebase.
Attachment #8534671 - Flags: review?(dglastonbury)
(Assignee)

Comment 4

3 years ago
Created attachment 8534717 [details]
testcase for getProgramInfoLog returning null on error
(Assignee)

Comment 5

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

Comment 8

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

Comment 9

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

Comment 10

3 years ago
Created attachment 8535220 [details] [diff] [review]
0005-Review-fixups.patch
Attachment #8535220 - Flags: review?(dglastonbury)
(Assignee)

Comment 11

3 years ago
Created attachment 8535233 [details] [diff] [review]
0005-Review-fixups.patch

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-
Created attachment 8537526 [details] [diff] [review]
Compilation Fixes

Jeff, I needed these changes to get the patches to compile.
Assignee: jgilbert → dglastonbury
Status: NEW → ASSIGNED
Bzexport stole the assignment.
Assignee: dglastonbury → jgilbert
Created attachment 8537529 [details] [diff] [review]
Fix SubstringStartsWith

ArraySize for a char[] includes the trailing \0. We don't want to match on that.
Attachment #8537529 - Flags: review?(jgilbert)
Created attachment 8537531 [details] [diff] [review]
Enable BypassShaderValidation in WebGL contexts

Enable bypassing ANGLE checks if WebGL2 context.
Attachment #8537531 - Flags: review?(jgilbert)
Created attachment 8537533 [details] [diff] [review]
Fix version replacement

Ditto. erase shouldn't include trailing '\0' in length calculation.
Attachment #8537533 - Flags: review?(jgilbert)
Created attachment 8537535 [details] [diff] [review]
Reinstate GL_ACTIVE_UNIFORM_BLOCKS and GL_ACTIVE_UNIFORM_BLOCK_MAX_NAME_LENGTH

In the refactor, these cases disappeared.
Attachment #8537535 - Flags: review?(jgilbert)
(Assignee)

Updated

3 years ago
Attachment #8537529 - Flags: review?(jgilbert) → review+
(Assignee)

Comment 19

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

Comment 20

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

Updated

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

Comment 22

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

Comment 23

3 years ago
Created attachment 8544885 [details] [diff] [review]
0005-Review-fixups.patch
Attachment #8535233 - Attachment is obsolete: true
Attachment #8544885 - Flags: review?(dglastonbury)
(Assignee)

Comment 24

3 years ago
Created attachment 8544886 [details] [diff] [review]
0006-Fixups-from-kamidphish.patch

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

Comment 28

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

Comment 31

3 years ago
Created attachment 8547909 [details] [diff] [review]
0009-Fix-devtools-tests.patch
Flags: needinfo?(jgilbert)
(Assignee)

Comment 32

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

Comment 33

3 years ago
Created attachment 8547937 [details] [diff] [review]
bypass-angle.diff
sorry had to back this out for test failure like https://treeherder.mozilla.org/logviewer.html#?job_id=5386777&repo=mozilla-inbound
https://hg.mozilla.org/mozilla-central/rev/ca0fbdaf25e0
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1125475
(Assignee)

Updated

3 years ago
Blocks: 1097227
You need to log in before you can comment on or make changes to this bug.