1109945 - Rewrite shader/program handling

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

Attachment: 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.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: 0002-Rewrite-shader-validation-handling-to-allow-bypass.patch

So, uh, this is big.

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: 0003-Fix-conf-misc-bad-arguments-test.patch

This was fixed upstream here:
https://github.com/KhronosGroup/WebGL/pull/803

Comment 3

[Kelsey Gilbert [:jgilbert]]

Attachment: 0004-Rebased.patch

Fixes needed after rebase.

Comment 4

[Kelsey Gilbert [:jgilbert]]

Attachment: testcase for getProgramInfoLog returning null on error

Comment 5

[Kelsey Gilbert [:jgilbert]]

Info on nsWrapperCache which should be added to a comment somewhere:
https://pastebin.mozilla.org/7917861

Comment 6

[u480271]

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

Comment 7

[u480271]

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.

Comment 8

[Kelsey Gilbert [:jgilbert]]

(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 9

[Kelsey Gilbert [:jgilbert]]

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.

Comment 10

[Kelsey Gilbert [:jgilbert]]

Attachment: 0005-Review-fixups.patch

Comment 11

[Kelsey Gilbert [:jgilbert]]

Attachment: 0005-Review-fixups.patch

Forgot to save a file.

Comment 12

[u480271]

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

Comment 13

[u480271]

Attachment: Compilation Fixes

Jeff, I needed these changes to get the patches to compile.

Comment 14

[u480271]

Bzexport stole the assignment.

Comment 15

[u480271]

Attachment: Fix SubstringStartsWith

ArraySize for a char[] includes the trailing \0. We don't want to match on that.

Comment 16

[u480271]

Attachment: Enable BypassShaderValidation in WebGL contexts

Enable bypassing ANGLE checks if WebGL2 context.

Comment 17

[u480271]

Attachment: Fix version replacement

Ditto. erase shouldn't include trailing '\0' in length calculation.

Comment 18

[u480271]

Attachment: Reinstate GL_ACTIVE_UNIFORM_BLOCKS and GL_ACTIVE_UNIFORM_BLOCK_MAX_NAME_LENGTH

In the refactor, these cases disappeared.

Comment 19

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8537531 [details] [diff] [review]
Enable BypassShaderValidation in WebGL contexts

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

I guess this is OK.

Comment 20

[Kelsey Gilbert [:jgilbert]]

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.

Comment 21

[u480271]

(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?

Comment 22

[Kelsey Gilbert [:jgilbert]]

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

Comment 23

[Kelsey Gilbert [:jgilbert]]

Attachment: 0005-Review-fixups.patch

Comment 24

[Kelsey Gilbert [:jgilbert]]

Attachment: 0006-Fixups-from-kamidphish.patch

Rolled up your changes when merging into my branch.

Comment 25

[Kelsey Gilbert [:jgilbert]]

 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7c4af1f4c34c

Comment 26

[u480271]

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?

Comment 27

[u480271]

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.

Comment 28

[Kelsey Gilbert [:jgilbert]]

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

Comment 29

[Kelsey Gilbert [:jgilbert]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/ca411b1cf001

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/6dab2820469a for dt2 failures:
https://treeherder.mozilla.org/logviewer.html#?job_id=5302773&repo=mozilla-inbound

Comment 31

[Kelsey Gilbert [:jgilbert]]

Attachment: 0009-Fix-devtools-tests.patch

Comment 32

[Kelsey Gilbert [: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

Comment 33

[Kelsey Gilbert [:jgilbert]]

Attachment: bypass-angle.diff

Comment 34

[Carsten Book [:Tomcat]]

sorry had to back this out for test failure like https://treeherder.mozilla.org/logviewer.html#?job_id=5386777&repo=mozilla-inbound

Comment 35

[Armen [:armenzg]]

https://hg.mozilla.org/projects/cedar/rev/83c4b180a815

Comment 36

[Kelsey Gilbert [:jgilbert]]

  https://hg.mozilla.org/integration/mozilla-inbound/rev/ca0fbdaf25e0

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ca0fbdaf25e0