From ba18d9627d934d27357f4cbc40a9e409475f91d4 Mon Sep 17 00:00:00 2001 From: Jamie Nicol Date: Thu, 11 Mar 2021 15:07:40 +0000 Subject: [PATCH] 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 --- gfx/wr/Cargo.lock | 4 +-- gfx/wr/glsl-to-cxx/src/hir.rs | 30 +++++++++++++++++-- gfx/wr/swgl/src/glsl.h | 15 ++++++++++ gfx/wr/webrender/Cargo.toml | 2 +- gfx/wr/webrender/res/brush_blend.glsl | 18 +++++------ gfx/wr/webrender/res/brush_yuv_image.glsl | 11 ++++--- gfx/wr/webrender/res/cs_svg_filter.glsl | 2 +- .../tests/angle_shader_validation.rs | 17 +++++++++-- 8 files changed, 75 insertions(+), 24 deletions(-) diff --git a/gfx/wr/Cargo.lock b/gfx/wr/Cargo.lock index bde6cbcee7e6..a5f09131f10a 100644 --- a/gfx/wr/Cargo.lock +++ b/gfx/wr/Cargo.lock @@ -1105,9 +1105,9 @@ dependencies = [ [[package]] name = "mozangle" -version = "0.3.2" +version = "0.3.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d77438925edbca894202489c32351b3cce7e0ab25ae7358af83824440ef5e8e2" +checksum = "ef98797da14500fb5eaabc90dc91cf7c42710c11330351521d72f4aa268d689c" dependencies = [ "cc", "gl_generator 0.13.1", diff --git a/gfx/wr/glsl-to-cxx/src/hir.rs b/gfx/wr/glsl-to-cxx/src/hir.rs index d714abc8186d..efa14803b675 100644 --- a/gfx/wr/glsl-to-cxx/src/hir.rs +++ b/gfx/wr/glsl-to-cxx/src/hir.rs @@ -1942,6 +1942,32 @@ fn is_vector(ty: &Type) -> bool { } } +fn index_vector(ty: &Type) -> Option { + 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 { use TypeKind::*; if ty.array_sizes != None { @@ -2502,8 +2528,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/gfx/wr/swgl/src/glsl.h b/gfx/wr/swgl/src/glsl.h index df23f00fd627..af67a5536d48 100644 --- a/gfx/wr/swgl/src/glsl.h +++ b/gfx/wr/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/gfx/wr/webrender/Cargo.toml b/gfx/wr/webrender/Cargo.toml index b03b91d89991..32cdd16215f1 100644 --- a/gfx/wr/webrender/Cargo.toml +++ b/gfx/wr/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/gfx/wr/webrender/res/brush_blend.glsl b/gfx/wr/webrender/res/brush_blend.glsl index 565b4f7067ff..9b9dd2f80fef 100644 --- a/gfx/wr/webrender/res/brush_blend.glsl +++ b/gfx/wr/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 @@ -241,7 +239,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 +252,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/gfx/wr/webrender/res/brush_yuv_image.glsl b/gfx/wr/webrender/res/brush_yuv_image.glsl index 9debf6fa0aa3..0a497411da8d 100644 --- a/gfx/wr/webrender/res/brush_yuv_image.glsl +++ b/gfx/wr/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/gfx/wr/webrender/res/cs_svg_filter.glsl b/gfx/wr/webrender/res/cs_svg_filter.glsl index ae5262e6ba6e..c36a22eb181c 100644 --- a/gfx/wr/webrender/res/cs_svg_filter.glsl +++ b/gfx/wr/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 diff --git a/gfx/wr/webrender/tests/angle_shader_validation.rs b/gfx/wr/webrender/tests/angle_shader_validation.rs index 0a099d0b045d..d6fe618de096 100644 --- a/gfx/wr/webrender/tests/angle_shader_validation.rs +++ b/gfx/wr/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(_) => {