1207288 - Enable the ANGLE shader validator for WebGL 2

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: Stop disabling the validator

Comment 2

[Jeff Muizelaar [:jrmuizel]]

It looks like the teleporter demo doesn't agree with the ANGLE 2466. It might be because of dynamic array indexing. Newer ANGLE seems like it might handle this better.

Comment 3

[u480271]

I also did some work in this area when I first grabbed ANGLE.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #3)
> I also did some work in this area when I first grabbed ANGLE.

Say more.

Comment 5

[u480271]

Patches are on my machine in Australia and I'm in Auckland. Will attach anything meaningful I have when I get home.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #5)
> Patches are on my machine in Australia and I'm in Auckland. Will attach
> anything meaningful I have when I get home.

Can you remember what any of them did so that work isn't duplicated?

Comment 7

[u480271]

From memory:
* Setup the input language depending on the shader
  * Fix bugs in #version detection
* Select the `correct` target GLSL version depending on the version supported by driver

Comment 8

[Pulsebot]

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

Comment 9

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/01d6f8dc4daa - all the Windowses said https://treeherder.mozilla.org/logviewer.html#?job_id=15590480&repo=mozilla-inbound

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c2089b24cef

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3c2089b24cef

Comment 12

[u480271]

Attachment: Use Angle shader transpiler.

This patch configures the shader transpiler to output GLSL/ESSL
version that targets the version supported by GLContext.

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Attachment: My version of Dan's patch

Comment 14

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #13)
> Created attachment 8681005 [details] [diff] [review]
> My version of Dan's patch

It looks like I was wrong about the GLSL3.3 being sufficient. We should probably take something more like Dan's patch.

https://github.com/google/angle/blob/e623bd46a4145e5a25606c895ee4d031e6ba9325/src/compiler/translator/BuiltInFunctionEmulatorGLSL.cpp#L47

Comment 15

[Jeff Muizelaar [:jrmuizel]]

It's worth noting that GL_ARB_shading_language_packing is the extension associated with these functions. However, ANGLE doesn't seem that well set up to support GLSL3.3+GL_ARB_shading_language_packing

Comment 16

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8675479 [details] [diff] [review]
Use Angle shader transpiler.

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

::: dom/canvas/WebGLShaderValidator.cpp
@@ +100,5 @@
> +        return SH_ESSL_OUTPUT;
> +
> +    uint32_t version = gl->ShadingLanguageVersion();
> +    switch (version) {
> +    case 100: return SH_ESSL_OUTPUT;

Surely ES3 returns 300+ here?

@@ +114,5 @@
> +    case 440: return SH_GLSL_440_CORE_OUTPUT;
> +    case 450: return SH_GLSL_450_CORE_OUTPUT;
> +    default:
> +        MOZ_CRASH("Unexpected GLSL version.");
> +    }

It seems that ideal this would be:
if (isESSL) {
  return SH_ESSL_OUTPUT; // Is there a SH_ESSL_300_OUTPUT?
} else {
  // big old switch for GLSL versions
}

::: gfx/gl/GLContext.cpp
@@ +177,5 @@
>      "GL_OES_vertex_array_object"
>  };
>  
>  static bool
> +ParseGLSLVersion(GLContext* gl, uint32_t* out_version)

Also return whether or not this is ES or GL.

@@ +186,5 @@
> +    }
> +
> +    /**
> +     * OpenGL 2.x, 3.x, 4.x specifications:
> +     *  The VERSION and SHADING_LANGUAGE_VERSION strings are laid out as follows:

More string parsing. Goodie.

@@ +224,5 @@
> +        return false;
> +    }
> +
> +    const char kGLESVersionPrefix[] = "OpenGL ES ";
> +    if (strncmp(versionString, kGLESVersionPrefix, strlen(kGLESVersionPrefix)) == 0)

s/strlen/ArrayLength/

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8675479 [details] [diff] [review]
Use Angle shader transpiler.

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

Perhaps consider using mozilla::Tokenizer for the version string parsing instead of open coding it?

Comment 18

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jeff Gilbert [:jgilbert] from comment #16)
> 
> @@ +224,5 @@
> > +        return false;
> > +    }
> > +
> > +    const char kGLESVersionPrefix[] = "OpenGL ES ";
> > +    if (strncmp(versionString, kGLESVersionPrefix, strlen(kGLESVersionPrefix)) == 0)
> 
> s/strlen/ArrayLength/

Why?

Comment 19

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jeff Gilbert [:jgilbert] from comment #16)
> >  static bool
> > +ParseGLSLVersion(GLContext* gl, uint32_t* out_version)
> 
> Also return whether or not this is ES or GL.

Why?

Comment 20

[Jeff Muizelaar [:jrmuizel]]

Attachment: Dan's patch with some of the review comments addressed.

I didn't understand the review comment that I didn't address.

Comment 21

[Jeff Muizelaar [:jrmuizel]]

This is currently failing on try with:
Assertion failure: false (glGetString(GL_SHADING_LANGUAGE_VERSION) has returned 0)

Comment 22

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8693246 [details] [diff] [review]
Dan's patch with some of the review comments addressed.

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

::: gfx/gl/GLContext.cpp
@@ +611,3 @@
>          if (ShouldSpew()) {
>              printf_stderr("OpenGL version detected: %u\n", version);
> +            printf_stderr("OpenGL shading language version detected: %u\n", shadingLangVersion);

I would prefer we display "...shading language version NOT detected..." if ParseGLSLVersion fails.

@@ +619,5 @@
>              mVersion = version;
>          }
> +
> +        if (shadingLangVersion >= mShadingLanguageVersion) {
> +            mShadingLanguageVersion = shadingLangVersion;

Why is this added, but shadingLangVersion starts at 100?

Comment 23

[Pulsebot]

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

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8042563db419

Comment 25

[Pulsebot]

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

Comment 26

[Carsten Book [:Tomcat]]

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