Re: [PATCH 1/4] wined3d: only activate blit_shader in arbfp_blit_surface for source fixups
On 18 May 2010 09:30, Roderick Colenbrander thunderbird2k@gmail.com wrote:
- arbfp_blit_set((IWineD3DDevice *)device, src_surface);
- if (is_complex_fixup(src_surface->resource.format_desc->color_fixup) &&
- !is_complex_fixup(dst_surface->resource.format_desc->color_fixup))
- arbfp_blit_set((IWineD3DDevice *)device, src_surface);
- This doesn't do what your subject line says it does. - Why do you think this patch makes sense?
On Tue, May 18, 2010 at 9:56 AM, Henri Verbeet hverbeet@gmail.com wrote:
On 18 May 2010 09:30, Roderick Colenbrander thunderbird2k@gmail.com wrote:
- arbfp_blit_set((IWineD3DDevice *)device, src_surface);
- if (is_complex_fixup(src_surface->resource.format_desc->color_fixup) &&
- !is_complex_fixup(dst_surface->resource.format_desc->color_fixup))
- arbfp_blit_set((IWineD3DDevice *)device, src_surface);
- This doesn't do what your subject line says it does.
Correct, it should be using is_identity_fixup for the destination check.
- Why do you think this patch makes sense?
All 'set_shader' calls perform similar checks to figure out whether the shader should be set or not. The call only has information on the source surface (though if you really want I could you could get more from the device or swapchain). Since 'blit_surface' has all the knowledge I thought it was nicer to avoid the unneeded call but it isn't right since set_shader does useful things as well like glEnable(tex_type). I could pass in the destination surface but have to think what to do about swapchain_blit which doesn't fit into the new design.
Roderick
On 18 May 2010 10:45, Roderick Colenbrander thunderbird2k@gmail.com wrote:
isn't right since set_shader does useful things as well like glEnable(tex_type).
Yeah. In the bigger picture it's also slightly insane that we have a shader based blit backend that doesn't actually use a shader for blitting most of the time, but instead invalidates lots of fixed function state.
On Tue, May 18, 2010 at 10:58 AM, Henri Verbeet hverbeet@gmail.com wrote:
On 18 May 2010 10:45, Roderick Colenbrander thunderbird2k@gmail.com wrote:
isn't right since set_shader does useful things as well like glEnable(tex_type).
Yeah. In the bigger picture it's also slightly insane that we have a shader based blit backend that doesn't actually use a shader for blitting most of the time, but instead invalidates lots of fixed function state.
I think we might be able to get rid of a lot of that code. At this point two out of the three cases which use the blit_shader are routed through draw_textured_quad which for instance already does the glEnable(tex_type) for us. In this case I guess if draw_textured_quad also handles glDisable (it knows the target) this could already be more efficient and get rid of unneeded state changes. The other case is swapchain_blit but as I mentioned before the non-fbo path in this call doesn't fit well into the design, so that's something I should perhaps fix first perhaps by letting it use draw_textured_quad if possible.
In general to what degree should we disable blindly disable texture_2d, cubemaps, texture_rectangle? I thought the policy (though not sure how the other code behaves) is that other code should be smart enough to enable/disable states it needs to avoid unneeded state switches or should functions clean up their own mess?
Roderick
On 18 May 2010 11:38, Roderick Colenbrander thunderbird2k@gmail.com wrote:
I think we might be able to get rid of a lot of that code. At this point two out of the three cases which use the blit_shader are routed through draw_textured_quad which for instance already does the glEnable(tex_type) for us. In this case I guess if draw_textured_quad also handles glDisable (it knows the target) this could already be more efficient and get rid of unneeded state changes. The other case is swapchain_blit but as I mentioned before the non-fbo path in this call doesn't fit well into the design, so that's something I should perhaps fix first perhaps by letting it use draw_textured_quad if possible.
In general to what degree should we disable blindly disable texture_2d, cubemaps, texture_rectangle? I thought the policy (though not sure how the other code behaves) is that other code should be smart enough to enable/disable states it needs to avoid unneeded state switches or should functions clean up their own mess?
Well, texture unit enables are mostly a fixed function concept, if arbfp_blit was actually shader based it wouldn't need to bother with that at all. In the general case you just invalidate the relevant states and depend on the state management to figure it out. There are some cases where that's impossible though, mostly for code that's called by state management functions itself.
-
Henri Verbeet -
Roderick Colenbrander