Open kvark opened 6 years ago
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.:
https://github.com/szeged/webrender/blob/a218a929325d0059ffc67b006be6bf841ee26c7a/webrender/res/shared.glsl#L53-L58
https://github.com/szeged/webrender/blob/a218a929325d0059ffc67b006be6bf841ee26c7a/webrender/res/shared.glsl#L140-L142
https://github.com/szeged/webrender/blob/a218a929325d0059ffc67b006be6bf841ee26c7a/webrender/res/cs_blur.glsl#L83-L89
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.
@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.
@kvark specialization constant implementation is almost identical for all backends, so should be working fine.
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
@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.
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.
The way data is transmuted here looks really scary: https://github.com/zakorgy/webrender/commit/4458fd4b4603ba0315bdf33261f742032542731c#diff-4d269bbd98df6af055b56ab19362567fR1149
Rather transmute it like:
&unsafe { mem::transmute::<_, [u8; 4]>(color_target as u32) }
@msiglreith thanks, that made the test results consistent with debug and release builds.
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.