Re: [PATCH 5/5] Rename color_fixup_supported to blit_supported support and add additional checks.
On 1 April 2010 23:58, Roderick Colenbrander thunderbird2k@gmail.com wrote:
- /* In general we don't support any destination fixups except for P8. P8 fixup
- * can occur in three cases:
- * 1) Blit from p8 source to p8 destination, this is a source fixup and the
- * destination fixup can be ignored.
- * 2) LockRect on surface, LoadLocation takes care of the 'destination fixup'.
- * 3) RealizePalette refreshes the palette of a surface. The surface is then
- * both the source (the sysmem / texture copy) and the destination (drawable).
- *
But you don't know all that. All you know is that you've got two surfaces with certain properties. You could argue that since the source and destination fixups are the same they cancel out, so that you can do that blit as long as you either don't stretch or the fixup is linear. However, in that case you also need to make sure the blitter actually behaves like that.
On Fri, Apr 2, 2010 at 12:15 PM, Henri Verbeet hverbeet@gmail.com wrote:
On 1 April 2010 23:58, Roderick Colenbrander thunderbird2k@gmail.com wrote:
- /* In general we don't support any destination fixups except for P8. P8 fixup
- * can occur in three cases:
- * 1) Blit from p8 source to p8 destination, this is a source fixup and the
- * destination fixup can be ignored.
- * 2) LockRect on surface, LoadLocation takes care of the 'destination fixup'.
- * 3) RealizePalette refreshes the palette of a surface. The surface is then
- * both the source (the sysmem / texture copy) and the destination (drawable).
- *
But you don't know all that. All you know is that you've got two surfaces with certain properties. You could argue that since the source and destination fixups are the same they cancel out, so that you can do that blit as long as you either don't stretch or the fixup is linear.
I must say that I'm not happy with the piece of code myself. The problem is that the P8 code has more or less behaved like this the past couple of years and yes that was bad and I don't think all ugliness can be fixed one step.
The check in blt_supported is not so great and I'm not sure if it can be improved well. If blit_supported had access to the source and destination surface, it could just check if they are the same which would handle the needs of corner cases like RealizePalette and LoadLocation. RealizePalette doesn't really care about whether a blit is supported at all, it just wants to know if p8 conversion is done by the gpu, so it can call LoadLocation texture->drawable else LoadLocation is called to perform a sysmem->(surface_conversion texture->) drawable upload. The callers (LoadLocation) do care, since they have to perform conversion or not (it is fun how the conversion is entangled with the blit_shader). The main reason for not passing surfaces to blit_supported was because of CheckSurfaceCapability and CheckDeviceFormat which don't have access to them and need a sort 'color_fixup_supported'. Perhaps we should have two calls one which works on pools/flags and one which works on surfaces (and which just extracts the info and passes it to the other one)?
Another slightly related issue is d3dfmt_get_conv. Right now it loads format info from the formats_table. The formats table contains opengl extension checks and based on this sets the formats for the arbfp p8 shader or the ffp paletted_texture code. Perhaps we should ask the blit_shader (once you know that it supports a certain blit) about the format. and so now and then afor P8_UINT. The blit_shader can fall back to d3dfmt_get_conv (or the formats table) when it doesn't perform an override. The same could be done for color keying but the main issue is that you don't know for what purpose you arrived in LoadLocation, so asking the blit_shader won't make sense all cases.
However, in that case you also need to make sure the blitter actually behaves like that.
Even before the ongoing rewrites this is basically how the P8 code behaved. At some point the yuv fixups were added and then FIXMEs about destination fixups appeared because the p8 code didn't receive a rewrite yet. The blit_supported code works fine for p8 source -> p8 render_target while the p8 framebuffer -> texture path is quite broken (I'm quite sure the fb_copy_ ones don't work; not sure about FBO; read_from_framebuffer works though). But I think the framebuffer -> texture code should be killed since I don't think we should advertise P8 textures at all. This would only leave p8 source -> p8 render_target destination fixups and LoadLocation.
This was a long mail (hope you can make sense of it). To summarize the P8 check in blit_supported is so ugly (and it might not work in all cases) for a part due to CheckSurfaceCapability and CheckDeviceFormat. I think that if I would kill the P8 texture support it might take away your concerns. While I would like to keep the problem as simple as possible I fear that some parts of d3dfmt_get_conv already need a rewrite at this point since it is starting to make life difficult. The main concern is how to perform surface conversion when needed and the color keying (though for that a special location flag could work as you suggested before).
Roderick
On 2 April 2010 13:49, Roderick Colenbrander thunderbird2k@gmail.com wrote:
This was a long mail (hope you can make sense of it). To summarize the P8 check in blit_supported is so ugly (and it might not work in all cases) for a part due to CheckSurfaceCapability and CheckDeviceFormat. I think that if I would kill the P8 texture support it might take away your concerns. While I would like to keep the problem as simple as possible I fear that some parts of d3dfmt_get_conv already need a rewrite at this point since it is starting to make life difficult. The main concern is how to perform surface conversion when needed and the color keying (though for that a special location flag could work as you suggested before).
Can't you handle P8 -> P8 blits as regular ARGB -> ARGB blits as long as you don't stretch? I.e., compared to this patch the main difference would be that you don't set the P8 -> ARGB shader at all in that case. This would be generic for all complex fixups as long as we don't stretch, and for simple / linear fixups even with stretching.
Only slightly related, I think it's reasonable to require non-NULL source / destination rectangles for internal interfaces. I.e., handle NULL rectangles in the public wined3d entry points, but just require them to be non-NULL everywhere else.
On Fri, Apr 2, 2010 at 3:24 PM, Henri Verbeet hverbeet@gmail.com wrote:
On 2 April 2010 13:49, Roderick Colenbrander thunderbird2k@gmail.com wrote:
This was a long mail (hope you can make sense of it). To summarize the P8 check in blit_supported is so ugly (and it might not work in all cases) for a part due to CheckSurfaceCapability and CheckDeviceFormat. I think that if I would kill the P8 texture support it might take away your concerns. While I would like to keep the problem as simple as possible I fear that some parts of d3dfmt_get_conv already need a rewrite at this point since it is starting to make life difficult. The main concern is how to perform surface conversion when needed and the color keying (though for that a special location flag could work as you suggested before).
Can't you handle P8 -> P8 blits as regular ARGB -> ARGB blits as long as you don't stretch? I.e., compared to this patch the main difference would be that you don't set the P8 -> ARGB shader at all in that case. This would be generic for all complex fixups as long as we don't stretch, and for simple / linear fixups even with stretching.
Only slightly related, I think it's reasonable to require non-NULL source / destination rectangles for internal interfaces. I.e., handle NULL rectangles in the public wined3d entry points, but just require them to be non-NULL everywhere else.
P8 -> P8 blits could be handled as ARGB->ARGB blits. Another alternative would be to disable opengl p8->p8 blits and perform the blit in software. It would then follow the same route as LockRect through LoadLocation. I would say that a software blit is more efficient than a software P8->ARGB conversion but if the app doesn't reload the source contents frequently, it would be more efficient if it stays on the GL side. This is more a ddraw-gl redesign point. In games like Jedi Knight, C&C and others this would save a lot of framebuffer read backs if GL is used.
Regarding the non-NULL rects it is what I'm trying to move to. BltOverride doesn't really rely on it anymore (the surface_get_rect lines can be moved back to its callers).
Roderick
Roderick
-
Henri Verbeet -
Roderick Colenbrander