Bug 1695912 - Ensure shaders use 16 or fewer varying vectors even if …

…driver does not pack them. r=jrmuizel On some Adreno 3xx devices we have observed that the driver does not pack varyings in to vectors as efficiently as the spec mandates. This results in some of our shaders using a greater number of varying vectors than GL_MAX_VARYING_VECTORS (which 16 on this device), leading to shader compilation errors at run time. Work around this by manually packing our varyings in to fewer vectors. Additionally, add a test to ensure that we never use more than 16 vectors even if the driver were to perform no additional packing. Differential Revision: https://phabricator.services.mozilla.com/D107929 [ghsync] From https://hg.mozilla.org/mozilla-central/rev/44347d27b4bb7ca16546dffe1d35905376d76a00
servo · Mar 12, 2021 · b9d515a · b9d515a
1 parent 5671424
commit b9d515a
Show file tree

Hide file tree

Showing 8 changed files with 90 additions and 34 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/glsl-to-cxx/src/hir.rs b/glsl-to-cxx/src/hir.rs
@@ -1942,6 +1942,32 @@ fn is_vector(ty: &Type) -> bool {
     }
 }
 
+fn index_vector(ty: &Type) -> Option<TypeKind> {
+    use TypeKind::*;
+    if ty.array_sizes != None {
+        return None;
+    }
+    Some(match ty.kind {
+        Vec2 => Float,
+        Vec3 => Float,
+        Vec4 => Float,
+        DVec2 => Double,
+        DVec3 => Double,
+        DVec4 => Double,
+        BVec2 => Bool,
+        BVec3 => Bool,
+        BVec4 => Bool,
+        IVec2 => Int,
+        IVec3 => Int,
+        IVec4 => Int,
+        UVec2 => UInt,
+        UVec3 => UInt,
+        UVec4 => UInt,
+        _ => return None,
+    })
+
+}
+
 fn index_matrix(ty: &Type) -> Option<TypeKind> {
     use TypeKind::*;
     if ty.array_sizes != None {
@@ -2378,6 +2404,7 @@ fn translate_expression(state: &mut State, e: &syntax::Expr) -> Expr {
                                         syntax::Expr::Variable(i) => match i.as_str() {
                                             "vec4" => TypeKind::Vec4,
                                             "vec2" => TypeKind::Vec2,
+                                            "int" => TypeKind::Int,
                                             _ => panic!("unexpected type constructor {:?}", i),
                                         },
                                         _ => panic!(),
@@ -2502,8 +2529,8 @@ fn translate_expression(state: &mut State, e: &syntax::Expr) -> Expr {
         }
         syntax::Expr::Bracket(e, specifier) => {
             let e = Box::new(translate_expression(state, e));
-            let ty = if is_vector(&e.ty) {
-                Type::new(TypeKind::Float)
+            let ty = if let Some(ty) = index_vector(&e.ty) {
+                Type::new(ty)
             } else if let Some(ty) = index_matrix(&e.ty) {
                 Type::new(ty)
             } else {

diff --git a/swgl/src/glsl.h b/swgl/src/glsl.h
@@ -1072,6 +1072,21 @@ struct ivec4_scalar {
   friend ivec4_scalar operator&(int32_t a, ivec4_scalar b) {
     return ivec4_scalar{a & b.x, a & b.y, a & b.z, a & b.w};
   }
+
+  int32_t& operator[](int index) {
+    switch (index) {
+      case 0:
+        return x;
+      case 1:
+        return y;
+      case 2:
+        return z;
+      case 3:
+        return w;
+      default:
+        UNREACHABLE;
+    }
+  }
 };
 
 struct ivec4 {

diff --git a/webrender/Cargo.toml b/webrender/Cargo.toml
@@ -62,7 +62,7 @@ etagere = "0.2.4"
 swgl = { path = "../swgl", optional = true }
 
 [dev-dependencies]
-mozangle = "0.3.2"
+mozangle = "0.3.3"
 rand = "0.4"
 
 [target.'cfg(any(target_os = "android", all(unix, not(target_os = "macos"))))'.dependencies]

diff --git a/webrender/res/brush_blend.glsl b/webrender/res/brush_blend.glsl
@@ -38,16 +38,14 @@ flat varying vec4 v_uv_sample_bounds;
 flat varying vec4 v_color_offset;
 
 // Flag to allow perspective interpolation of UV.
-flat varying float v_perspective;
-
-flat varying float v_amount;
+flat varying vec2 v_perspective_amount;
 
 flat varying int v_op;
 flat varying int v_table_address;
 
 flat varying mat4 vColorMat;
 
-flat varying int vFuncs[4];
+flat varying ivec4 vFuncs;
 
 #ifdef WR_VERTEX_SHADER
 
@@ -74,7 +72,7 @@ void brush_vs(
     float perspective_interpolate = (brush_flags & BRUSH_FLAG_PERSPECTIVE_INTERPOLATION) != 0 ? 1.0 : 0.0;
 
     v_uv = uv * inv_texture_size * mix(vi.world_pos.w, 1.0, perspective_interpolate);
-    v_perspective = perspective_interpolate;
+    v_perspective_amount.x = perspective_interpolate;
 
     v_uv_sample_bounds = vec4(uv0 + vec2(0.5), uv1 - vec2(0.5)) * inv_texture_size.xyxy;
 
@@ -89,7 +87,7 @@ void brush_vs(
     float invAmount = 1.0 - amount;
 
     v_op = prim_user_data.y & 0xffff;
-    v_amount = amount;
+    v_perspective_amount.y = amount;
 
     // This assignment is only used for component transfer filters but this
     // assignment has to be done here and not in the component transfer case
@@ -98,10 +96,10 @@ void brush_vs(
     // https://github.com/servo/webrender/wiki/Driver-issues#bug-1505871---assignment-to-varying-flat-arrays-inside-switch-statement-of-vertex-shader-suspected-miscompile-on-windows
     // default: just to satisfy angle_shader_validation.rs which needs one
     // default: for every switch, even in comments.
-    vFuncs[0] = (prim_user_data.y >> 28) & 0xf; // R
-    vFuncs[1] = (prim_user_data.y >> 24) & 0xf; // G
-    vFuncs[2] = (prim_user_data.y >> 20) & 0xf; // B
-    vFuncs[3] = (prim_user_data.y >> 16) & 0xf; // A
+    vFuncs.r = (prim_user_data.y >> 28) & 0xf; // R
+    vFuncs.g = (prim_user_data.y >> 24) & 0xf; // G
+    vFuncs.b = (prim_user_data.y >> 20) & 0xf; // B
+    vFuncs.a = (prim_user_data.y >> 16) & 0xf; // A
 
     if (v_op == FILTER_GRAYSCALE) {
         vColorMat = mat4(
@@ -202,8 +200,10 @@ vec4 ComponentTransfer(vec4 colora) {
     vec4 texel;
     int k;
 
+    // Dynamically indexing a vector is buggy on some platforms, so use a temporary array
+    int[4] funcs = int[4](vFuncs.r, vFuncs.g, vFuncs.b, vFuncs.a);
     for (int i = 0; i < 4; i++) {
-        switch (vFuncs[i]) {
+        switch (funcs[i]) {
             case COMPONENT_TRANSFER_IDENTITY:
                 break;
             case COMPONENT_TRANSFER_TABLE:
@@ -241,7 +241,7 @@ vec4 ComponentTransfer(vec4 colora) {
 }
 
 Fragment brush_fs() {
-    float perspective_divisor = mix(gl_FragCoord.w, 1.0, v_perspective);
+    float perspective_divisor = mix(gl_FragCoord.w, 1.0, v_perspective_amount.x);
     vec2 uv = v_uv * perspective_divisor;
     // Clamp the uvs to avoid sampling artifacts.
     uv = clamp(uv, v_uv_sample_bounds.xy, v_uv_sample_bounds.zw);
@@ -254,13 +254,13 @@ Fragment brush_fs() {
 
     switch (v_op) {
         case FILTER_CONTRAST:
-            color = Contrast(color, v_amount);
+            color = Contrast(color, v_perspective_amount.y);
             break;
         case FILTER_INVERT:
-            color = Invert(color, v_amount);
+            color = Invert(color, v_perspective_amount.y);
             break;
         case FILTER_BRIGHTNESS:
-            color = Brightness(color, v_amount);
+            color = Brightness(color, v_perspective_amount.y);
             break;
         case FILTER_SRGB_TO_LINEAR:
             color = SrgbToLinear(color);

diff --git a/webrender/res/brush_yuv_image.glsl b/webrender/res/brush_yuv_image.glsl
@@ -15,9 +15,8 @@ flat varying vec4 vUvBounds_U;
 varying vec2 vUv_V;
 flat varying vec4 vUvBounds_V;
 
-flat varying float vCoefficient;
 flat varying mat3 vYuvColorMatrix;
-flat varying vec3 vYuvOffsetVector;
+flat varying vec4 vYuvOffsetVector_Coefficient;
 flat varying int vFormat;
 
 #ifdef SWGL_DRAW_SPAN
@@ -53,10 +52,10 @@ void brush_vs(
     vec2 f = (vi.local_pos - local_rect.p0) / local_rect.size;
 
     YuvPrimitive prim = fetch_yuv_primitive(prim_address);
-    vCoefficient = prim.coefficient;
+    vYuvOffsetVector_Coefficient.w = prim.coefficient;
 
     vYuvColorMatrix = get_yuv_color_matrix(prim.color_space);
-    vYuvOffsetVector = get_yuv_offset_vector(prim.color_space);
+    vYuvOffsetVector_Coefficient.xyz = get_yuv_offset_vector(prim.color_space);
     vFormat = prim.yuv_format;
 
 #ifdef SWGL_DRAW_SPAN
@@ -92,8 +91,8 @@ Fragment brush_fs() {
     vec4 color = sample_yuv(
         vFormat,
         vYuvColorMatrix,
-        vYuvOffsetVector,
-        vCoefficient,
+        vYuvOffsetVector_Coefficient.xyz,
+        vYuvOffsetVector_Coefficient.w,
         vUv_Y,
         vUv_U,
         vUv_V,

diff --git a/webrender/res/cs_svg_filter.glsl b/webrender/res/cs_svg_filter.glsl
@@ -17,7 +17,7 @@ flat varying vec4 vFilterData0;
 flat varying vec4 vFilterData1;
 flat varying float vFloat0;
 flat varying mat4 vColorMat;
-flat varying int vFuncs[4];
+flat varying ivec4 vFuncs;
 
 #define FILTER_BLEND                0
 #define FILTER_FLOOD                1
@@ -110,10 +110,10 @@ void main(void) {
     // https://github.com/servo/webrender/wiki/Driver-issues#bug-1505871---assignment-to-varying-flat-arrays-inside-switch-statement-of-vertex-shader-suspected-miscompile-on-windows
     // default: just to satisfy angle_shader_validation.rs which needs one
     // default: for every switch, even in comments.
-    vFuncs[0] = (aFilterGenericInt >> 12) & 0xf; // R
-    vFuncs[1] = (aFilterGenericInt >> 8)  & 0xf; // G
-    vFuncs[2] = (aFilterGenericInt >> 4)  & 0xf; // B
-    vFuncs[3] = (aFilterGenericInt)       & 0xf; // A
+    vFuncs.r = (aFilterGenericInt >> 12) & 0xf; // R
+    vFuncs.g = (aFilterGenericInt >> 8)  & 0xf; // G
+    vFuncs.b = (aFilterGenericInt >> 4)  & 0xf; // B
+    vFuncs.a = (aFilterGenericInt)       & 0xf; // A
 
     switch (aFilterKind) {
         case FILTER_BLEND:
@@ -427,8 +427,10 @@ vec4 ComponentTransfer(vec4 colora) {
     vec4 texel;
     int k;
 
+    // Dynamically indexing a vector is buggy on some devices, so use a temporary array.
+    int[4] funcs = int[4](vFuncs.r, vFuncs.g, vFuncs.b, vFuncs.a);
     for (int i = 0; i < 4; i++) {
-        switch (vFuncs[i]) {
+        switch (funcs[i]) {
             case COMPONENT_TRANSFER_IDENTITY:
                 break;
             case COMPONENT_TRANSFER_TABLE:

diff --git a/webrender/tests/angle_shader_validation.rs b/webrender/tests/angle_shader_validation.rs
@@ -35,8 +35,9 @@ fn validate_shaders() {
                 &|f| webrender::get_unoptimized_shader_source(f, None)
             );
 
-            validate(&vs_validator, shader, vs);
-            validate(&fs_validator, shader, fs);
+            let full_shader_name = format!("{} {}", shader, config);
+            validate(&vs_validator, &full_shader_name, vs);
+            validate(&fs_validator, &full_shader_name, fs);
         }
     }
 }
@@ -49,6 +50,18 @@ fn validate(validator: &ShaderValidator, name: &str, source: String) {
     // Run Angle validator
     match validator.compile_and_translate(&[&source]) {
         Ok(_) => {
+            // Ensure that the shader uses at most 16 varying vectors. This counts the number of
+            // vectors assuming that the driver does not perform additional packing. The spec states
+            // that the driver should pack varyings, however, on some Adreno 3xx devices we have
+            // observed that this is not the case. See bug 1695912.
+            let varying_vectors = validator.get_num_unpacked_varying_vectors();
+            let max_varying_vectors = 16;
+            assert!(
+                varying_vectors <= max_varying_vectors,
+                "Shader {} uses {} varying vectors. Max allowed {}",
+                name, varying_vectors, max_varying_vectors
+            );
+
             println!("Shader translated succesfully: {}", name);
         }
         Err(_) => {