Enable the ANGLE shader validator for WebGL 2

RESOLVED FIXED in Firefox 44

Status

()

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

People

(Reporter: jrmuizel, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 attachments, 2 obsolete attachments)

Comment hidden (empty)
(Reporter)

Updated

2 years ago
Depends on: 1207206
(Reporter)

Updated

2 years ago
Blocks: 1207206
No longer depends on: 1207206
(Reporter)

Updated

2 years ago
Depends on: 1179280
(Reporter)

Comment 1

2 years ago
Created attachment 8664413 [details] [diff] [review]
Stop disabling the validator
(Reporter)

Comment 2

2 years ago
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.
Whiteboard: [gfx-noted]
I also did some work in this area when I first grabbed ANGLE.
(Reporter)

Comment 4

2 years ago
(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.
Flags: needinfo?(dglastonbury)
Patches are on my machine in Australia and I'm in Auckland. Will attach anything meaningful I have when I get home.
Flags: needinfo?(dglastonbury)
(Reporter)

Comment 6

2 years ago
(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?
Flags: needinfo?(dglastonbury)
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
Flags: needinfo?(dglastonbury)
(Reporter)

Updated

2 years ago
Attachment #8664413 - Flags: review?(jgilbert)
Attachment #8664413 - Flags: review?(jgilbert) → review+
https://hg.mozilla.org/mozilla-central/rev/3c2089b24cef
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Blocks: 1215279
Created attachment 8675479 [details] [diff] [review]
Use Angle shader transpiler.

This patch configures the shader transpiler to output GLSL/ESSL
version that targets the version supported by GLContext.
Attachment #8675479 - Flags: review?(jmuizelaar)
Attachment #8675479 - Flags: review?(jgilbert)
(Reporter)

Comment 13

2 years ago
Created attachment 8681005 [details] [diff] [review]
My version of Dan's patch
(Reporter)

Comment 14

2 years ago
(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
(Reporter)

Comment 15

2 years ago
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 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/
Attachment #8675479 - Flags: review?(jgilbert) → review-
(Reporter)

Comment 17

2 years ago
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?
(Reporter)

Comment 18

2 years ago
(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?
(Reporter)

Comment 19

2 years ago
(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?
(Reporter)

Comment 20

2 years ago
Created attachment 8693246 [details] [diff] [review]
Dan's patch with some of the review comments addressed.

I didn't understand the review comment that I didn't address.
Attachment #8675479 - Attachment is obsolete: true
Attachment #8681005 - Attachment is obsolete: true
Attachment #8675479 - Flags: review?(jmuizelaar)
Attachment #8693246 - Flags: review?(jgilbert)

Updated

2 years ago
Blocks: 1170849
(Reporter)

Comment 21

2 years ago
This is currently failing on try with:
Assertion failure: false (glGetString(GL_SHADING_LANGUAGE_VERSION) has returned 0)
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?
Attachment #8693246 - Flags: review?(jgilbert) → review+

Updated

2 years ago
Depends on: 1225291
(Reporter)

Updated

2 years ago
Depends on: 1232462
You need to log in before you can comment on or make changes to this bug.