On Wed, May 19, 2010 at 11:30 AM, Roderick Colenbrander thunderbird2k@gmail.com wrote:
At a later stage I might disallow calling set_shader when we don't need the shader.
How does the caller know when a shader is needed?
In the end set_shader will only be called by blit_shader->blit_surface.
The main issue I'm facing is p8 uploads which are completely broken right now (a regression). At the beginning of the week I wanted to add BLIT_OP_COMPLEX_UPLOAD to fix this but Henri didn't like that. This 'fixed' the bug where d3dfmt_get_conv was not allowed by blit_supported to offer the 8-bit shader. This fix would only apply to the LoadLocation part which is where the 8-bit upload takes place.
The suggested direction was to fix blit_supported to allow 8-bit blits under 'special circumstances'. Such a change would not only allow 8-bit uploads but also allow 8-bit -> 8-bit Blt/BltFast (blit_supported can't distinguish between the two uses since it has too vague information). Allowing the 8-bit -> 8-Blt/BltFast led to these changes (I'm not that happy about allowing those).
At this point BltOverride (and some of the other helpers derived from it) blindly call set_shader when it is not needed which wouldn't work here. The set_shader call at this point lacks the knowledge (it has only one of the surfaces) to not apply the shader in this case. I don't think I can add the missing surface because not all places which call set_shader have it. For this reason I wanted to take the texture target activation out of 'set_shader' and lift this to 'blit_surface' which the current callers are supposed to become at some point. Some places (e.g. swapchain_blit) imply that that the shader is needed when a complex source fixup is detected and the same for blit_to_drawable, so in BltOverride/arbfp_blit_surface I can just not call set_shader.
Right now I don't know anymore how it should be fixed at this stage without re-adding the ugly extension checks which existed before in d3dfmt_get_conv or other or other hacks like checking for 'is_complex_fixup' in d3dfmt_get_conv :(
Roderick
So the main issue is that a p8 sysmem -> drawable upload on the same surface is detected as a 'destination fixup' by blit_supported. The blit_supported call is used for p8 in d3dfmt_get_conv basically for format selection before it were GL extension checks.
What I could try to do is take out the blit_supported call and rely on the 'implicit blit_supported' for now in the form of the formats table. When the table is loaded it already sets the P8 texturing format we want (it is not that nice). Together with this change I would also have to rewrite d3dfmt_get_conv to really get rid of the format fixups as they are a part of the issue.
Issues: 1) sysmem -> drawable uploads on the same surface don't work 2) when shaders nor paletted_texture is around, d3dfmt_get_conv receives an 'empty' description which it fills with the gl info (bad and seems to cause some issues already) ( 3) format desc fixups are ugly )
I would propose to make three changes: 1) format desc changes Add some format color keying flags to the format_desc (would be set for all CK formats) and a conversion flag (this one would be set for P8 and perhaps also for all CK formats)
2) add separate formats table for color keyed formats which d3dfmt_get_conv would select from. This could work like:
HRESULT d3dfmt_get_conv(..) { if (color_keying_active) { /* search color key format list */ /* At the moment this is performed in software for p8 */ return desc; }
/* In case of P8 we need a different format when we use drawpixels * compared to texturing when ext_paletted_texture or shaders are around */ if (!use_textures && p8) { return p8_drawpixels_desc; }
/* In case of P8 this when we arrive here we return the default format which is in the formats table, * so if paletted textures are around then that format is returned. This would be an implicit * blit_supported check. At a later stage the blit_shader should perhaps be asked what format is needed. */ return default_desc; }
3) Get rid of CONVERT_TYPES and look at the flags in the format_desc before calling d3dfmt_convert_surface (this call would also check these flags to figure out the right conversion).
This change addresses some ugly things (format desc fixups), fixes issue '2' and would also address the 8-bit 'complex upload' case. I admit that the last one would be fixed in a 'sneaky' way. At a later point the blit_shader could perhaps be asked for a format.
What do you think?
Roderick