[PATCH 5/5] Rename color_fixup_supported to blit_supported support and add additional checks.

[Roderick Colenbrander]

On Fri, Apr 2, 2010 at 12:15 PM, Henri Verbeet <hverbeet at gmail.com> wrote:
> On 1 April 2010 23:58, Roderick Colenbrander <thunderbird2k at 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