Re: [PATCH 1/5] wined3d: Fix immediate draws when ARB_vertex_array_bgra is not supported.
On 20 July 2016 at 00:33, Matteo Bruni <mbruni(a)codeweavers.com> wrote:
@@ -5848,7 +5850,7 @@ static void wined3d_adapter_init_ffp_attrib_ops(struct wined3d_adapter *adapter) ops->generic[WINED3D_FFP_EMIT_FLOAT2] = (wined3d_generic_attrib_func)gl_info->gl_ops.ext.p_glVertexAttrib2fv; ops->generic[WINED3D_FFP_EMIT_FLOAT3] = (wined3d_generic_attrib_func)gl_info->gl_ops.ext.p_glVertexAttrib3fv; ops->generic[WINED3D_FFP_EMIT_FLOAT4] = (wined3d_generic_attrib_func)gl_info->gl_ops.ext.p_glVertexAttrib4fv; - if (gl_info->supported[ARB_VERTEX_ARRAY_BGRA]) + if (!gl_info->supported[ARB_VERTEX_ARRAY_BGRA]) ops->generic[WINED3D_FFP_EMIT_D3DCOLOR] = generic_d3dcolor; else ops->generic[WINED3D_FFP_EMIT_D3DCOLOR] = This looks wrong. When ARB_vertex_array_bgra is not supported, shaders swizzle the corresponding input registers (see also shader_glsl_swizzle_to_str() and shader_arb_get_swizzle()), so the attribute upload code shouldn't.
2016-07-20 10:13 GMT+02:00 Henri Verbeet <hverbeet(a)gmail.com>:
On 20 July 2016 at 00:33, Matteo Bruni <mbruni(a)codeweavers.com> wrote:
@@ -5848,7 +5850,7 @@ static void wined3d_adapter_init_ffp_attrib_ops(struct wined3d_adapter *adapter) ops->generic[WINED3D_FFP_EMIT_FLOAT2] = (wined3d_generic_attrib_func)gl_info->gl_ops.ext.p_glVertexAttrib2fv; ops->generic[WINED3D_FFP_EMIT_FLOAT3] = (wined3d_generic_attrib_func)gl_info->gl_ops.ext.p_glVertexAttrib3fv; ops->generic[WINED3D_FFP_EMIT_FLOAT4] = (wined3d_generic_attrib_func)gl_info->gl_ops.ext.p_glVertexAttrib4fv; - if (gl_info->supported[ARB_VERTEX_ARRAY_BGRA]) + if (!gl_info->supported[ARB_VERTEX_ARRAY_BGRA]) ops->generic[WINED3D_FFP_EMIT_D3DCOLOR] = generic_d3dcolor; else ops->generic[WINED3D_FFP_EMIT_D3DCOLOR] = This looks wrong. When ARB_vertex_array_bgra is not supported, shaders swizzle the corresponding input registers (see also shader_glsl_swizzle_to_str() and shader_arb_get_swizzle()), so the attribute upload code shouldn't.
It's a bit more convoluted than that. When ARB_vertex_array_bgra is not supported, FFP draws usually fall back to immediate mode because of the check in context_update_stream_info(). There is no shader swizzling in that case so we need to swizzle when uploading the attribute. If ARB_vertex_array_bgra is supported we never need to swizzle the attribute so the above change seems at least a fix for that case (although if ARB_vertex_array_bgra is supported we generally don't fallback to immediate mode either, so that doesn't matter much in practice). context_update_stream_info() only checks for DIFFUSE and SPECULAR though so I guess if other attributes use D3DCOLOR we're currently screwed. The immediate mode check is in the !VS-only branch so it would not apply to non-FFP draws. Those get the shader swizzle instead and thus work fine. I guess there is an exception here too which is instanced draws when both ARB_vertex_array_bgra and ARB_instanced_arrays are missing AND one of the instance attributes is D3DCOLOR. Eventually for GL ES (which I think doesn't have anything equivalent to ARB_vertex_array_bgra) we'll need to always handle this in the shader since we can't fallback to immediate mode there. I guess I'll try to write a patch going in that direction since it should also clean things up WRT the above cases on desktop OpenGL.
On 20 July 2016 at 16:21, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
It's a bit more convoluted than that. When ARB_vertex_array_bgra is not supported, FFP draws usually fall back to immediate mode because of the check in context_update_stream_info(). There is no shader swizzling in that case [...] Yes, but I'd call that a bug. (As an aside, some of those fall-backs to immediate mode could perhaps be avoided by taking stream_info->swizzle_mask into account. It's probably not worth it because diffuse/specular almost always use D3DCOLOR.)
[...] so we need to swizzle when uploading the attribute. If ARB_vertex_array_bgra is supported we never need to swizzle the attribute [...] You do for immediate mode draws. Data is stored in BGRA order, which is expected to be mapped to "zyxw" in shaders. If ARB_vertex_array_bgra is supported, the shader backend assumes this happens on upload, either through GL_BGRA for array draws or through manual swizzling for immediate mode draws. If ARB_vertex_array_bgra is not supported, the shader backend does it by itself. (The decision whether the shader needs to swizzle ultimately happens in context_stream_info_from_declaration() by setting bits in the "swizzle_map".)
[...] so the above change seems at least a fix for that case (although if ARB_vertex_array_bgra is supported we generally don't fallback to immediate mode either, so that doesn't matter much in practice). context_update_stream_info() only checks for DIFFUSE and SPECULAR though so I guess if other attributes use D3DCOLOR we're currently screwed. But D3DCOLOR / WINED3DFMT_B8G8R8A8_UNORM is not valid for a lot of other attributes for fixed function draws. (See also declaration_element_valid_ffp().) I notice it's also allowed for WINED3D_DECL_USAGE_BLEND_WEIGHT though, which may actually get used these days.
The immediate mode check is in the !VS-only branch so it would not apply to non-FFP draws. Those get the shader swizzle instead and thus work fine. Yeah, that's the reason this mostly works in practice, but I think it's fragile to depend on that. I also think it makes it a little more confusing when you need to swizzle on upload and when you don't. I.e., the current rule is perhaps somewhat under-documented, but I think it's a good thing it's fairly simple.
I guess there is an exception here too which is instanced draws when both ARB_vertex_array_bgra and ARB_instanced_arrays are missing AND one of the instance attributes is D3DCOLOR. You can also end up with immediate mode shader draws when "half_float_conv_needed" is set on the vertex declaration.
participants (2)
-
Henri Verbeet -
Matteo Bruni