Use specialization constants for WR features

[kvark]

Vulkan specialization constants appear to be the best-fit mechanism for supporting multiple shader configurations driven by features. Metal supports those natively, and there is hope that Vulkan drivers can generate a more efficient code (or save time on shader parsing) when using specialization.

[zakorgy]

I started to take a look at this, but still have some things to figure out:

• Some WR feature(dual source belnding, dithering, color target) are used in the global scope in #define blocks e.g.:
  

  webrender/webrender/res/shared.glsl

  Lines 53 to 58 in a218a92

  	#ifdef WR_FEATURE_DUAL_SOURCE_BLENDING
  	layout(location = 0, index = 0) out vec4 oFragColor;
  	layout(location = 0, index = 1) out vec4 oFragBlend;
  	#else
  	out vec4 oFragColor;
  	#endif

  
  

  webrender/webrender/res/shared.glsl

  Lines 140 to 142 in a218a92

  	#ifdef WR_FEATURE_DITHERING
  	uniform sampler2D sDither;
  	#endif

  
  

  webrender/webrender/res/cs_blur.glsl

  Lines 83 to 89 in a218a92

  	#if defined WR_FEATURE_COLOR_TARGET
  	#define SAMPLE_TYPE vec4
  	#define SAMPLE_TEXTURE(uv) texture(sCacheRGBA8, uv)
  	#else
  	#define SAMPLE_TYPE float
  	#define SAMPLE_TEXTURE(uv) texture(sCacheA8, uv).r
  	#endif

  
  Since we can't use specialization constant values for #if preprocessors, I can't find any solution in these cases (except for the last one, in that case we can duplicate the code in the main to cover both cases), just to leave out these features from the specialization constants. I will appreciate any advice/idea for this.

• Will this kind of change also work with dx12? I have a preliminary implementation for this here: https://github.com/zakorgy/webrender/commits/spec-const, and I have way more failing wrench test with dx12 (around 30), than with vulkan (12), the expected is 6.
  NOTE: I still have to hunt down some new Vulkan validation layer error, caused by the changes in our shader set. Maybe the difference is caused by dx12 being more strict in these cases.

[kvark]

@zakorgy dx12 specialization constant support may be less complete still. cc @msiglreith

For global scopes, I think it's solvable in each individual case:
For WR_FEATURE_DUAL_SOURCE_BLENDING, we could possibly declare the oFragBlend unconditionally. It's not going to be used when the blend mode isn't interested in it anyway.

For WR_FEATURE_DITHERING, we can easily reserve a texture+sampler in the pipeline layout, but only really use it when the specialization constant tells us to do so.

For WR_FEATURE_COLOR_TARGET, the code you provided is only for cs_blur, so the solution could be custom as well. For example the shader could have 2 entry points: for color and alpha. I'm not entirely sure how to convince glslang though that we have multiple entry points...

Overall, given the concerns raised and the state of specialization constants in DX12 at this point, I think we should postpone this feature to the next year.

[msiglreith]

@kvark specialization constant implementation is almost identical for all backends, so should be working fine.

[zakorgy]

I have revisited this and tried to figure out what happens on the DX12 side: we have 33 failing tests, but there should be only 7 on windows.
After capturing the reftest\blend\large.yaml test with RenderDoc I found that something is wrong with the brush_image vertex shader, the return value of vMaskSwizzle is wrong. The variable is evaluated in a switch statement:
https://github.com/zakorgy/webrender/blob/4458fd4b4603ba0315bdf33261f742032542731c/webrender/res/brush_image.glsl#L149-L182
For some reason we have the value from the default branch instead of the COLOR_MODE_COLOR_BITMAP branch, which would be the correct choice for this test.
I have checked the value of color_mode and it is 9 which is COLOR_MODE_COLOR_BITMAP
After I have changed this part of the switch-case from

        case COLOR_MODE_SUBPX_CONST_COLOR:
        case COLOR_MODE_SUBPX_BG_PASS0:
        case COLOR_MODE_COLOR_BITMAP:
            vMaskSwizzle = vec2(1.0, 0.0);
            vColor = vec4(image_data.color.a);
            break;

to

        case COLOR_MODE_SUBPX_BG_PASS2:
            vMaskSwizzle = vec2(1.0, 0.0);
            vColor = image_data.color;
            break;
        case COLOR_MODE_SUBPX_DUAL_SOURCE:
            vMaskSwizzle = vec2(1.0, 0.0);
            vColor = image_data.color;
            break;
        case COLOR_MODE_IMAGE:
            vMaskSwizzle = vec2(1.0, 0.0);
            vColor = image_data.color;
            break;

vMaskSwizzle received the correct value and the test passed.

Unfortunatelly I haven't found any related driver/d3d12 issue for this. It is strange that we only have this issue with push constants and only with DX12.
Here is my branch: https://github.com/zakorgy/webrender/commits/spec-const
The vertex shader (hlsl) from RenderDoc before push constants: https://gist.github.com/zakorgy/5eab335ab97c7e852a15ec0b3a28dd9c
The vertex shader with push constants:
https://gist.github.com/zakorgy/a5751d17a5d40052782d2cb95af2a423
The bug can be reproduced by running cargo run --features=dx12 --release show reftests\blend\large.yaml from the wrench folder.
It can be compared to the szeged/master branch which works fine.
cc @kvark @msiglreith

[kvark]

@zakorgy it would be good to confirm if the generated SPIR-V is wrong, or just spirv_cross messing with us.

Also, I think at this point it's worth it creating an entry in WR driver bug wiki, and providing a PR upstream to fix this switch statement.

[zakorgy]

Meanwhile I have checked the test (blend\large.yaml) with a debug build, and it passes 😕 (My previous attempt was with release). It looks like the issue is not that simple as I described, and maybe the above mentioned solution just fixes the symptom not the real problem. I will take a look at the generated SPIR-V, hope that shows us something.

[msiglreith]

The way data is transmuted here looks really scary: zakorgy@4458fd4#diff-4d269bbd98df6af055b56ab19362567fR1149

Rather transmute it like:

&unsafe { mem::transmute::<_, [u8; 4]>(color_target as u32) }

[zakorgy]

@msiglreith thanks, that made the test results consistent with debug and release builds.