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