Re: [PATCH 0/7] wined3d: blit set_shader texture_target activation cleanup
Am 19.05.2010 um 00:38 schrieb Roderick Colenbrander:
This patch series removes the responsibility for enabling/disabling the texture_target from the 'set_shader' stage of the blit_shader. I disagree with this patch. glEnable(GL_TEXTURE_2D) is the fixed function pipline's way of putting the right texture target into the "TEX dst, src, sampler, dimension" instruction. They are in the ARB blitter only because of my laziness - it is using the fixed function pipeline instead of an equivalent shader.
So if you want to get rid of the glEnable / glDisable calls you should instead use ARB shaders which match the fixed function pipeline's functionality, e.g. TEX result.color, fragment.texcoord[0], texture[0], 2D; (for the 2D case) ffp_blit will keep calling glEnable / glDisable GL_TEXTURE_...
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?
On 19 May 2010 09:56, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote:
Am 19.05.2010 um 00:38 schrieb Roderick Colenbrander:
This patch series removes the responsibility for enabling/disabling the texture_target from the 'set_shader' stage of the blit_shader. I disagree with this patch. glEnable(GL_TEXTURE_2D) is the fixed function pipline's way of putting the right texture target into the "TEX dst, src, sampler, dimension" instruction. They are in the ARB blitter only because of my laziness - it is using the fixed function pipeline instead of an equivalent shader.
Yup, as explained yesterday, this is something that should be internal to a specific blit implementation. This series is a change in the opposite direction. Technical issues with this series aside, I think it's too close to 1.2 for making architectural changes. IMO the focus should be mainly on fixing regressions and minor bugs.
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
On Wed, May 19, 2010 at 11:30 AM, Roderick Colenbrander <thunderbird2k(a)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
On 19 May 2010 14:46, Roderick Colenbrander <thunderbird2k(a)gmail.com> wrote:
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 ... What do you think?
Is there a bug report / demo application that clearly shows the problem? Maybe I can give it a look.
On Wed, May 19, 2010 at 7:02 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 19 May 2010 14:46, Roderick Colenbrander <thunderbird2k(a)gmail.com> wrote:
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 ... What do you think?
Is there a bug report / demo application that clearly shows the problem? Maybe I can give it a look.
The bug in question is and a test case is StarCraft: http://bugs.winehq.org/show_bug.cgi?id=22575 I have made a draft patch (this would have to be cut in a lot of pieces and changes a few more things than just the issue). It might be an acceptable path. The idea is to move the 'p8 texture format' to the formats table including the case when no shaders/ext_paletted_texture is around (this case needs to be added in some form anyway since fixup up an empty format desc is not that nice). The format is overridden in case of color keying / drawpixels. So at this stage the 'blit_supported' is implicitly performed at formats table initialization. This might be acceptable for now. At a later stage I can imagine that we want to ask the 'blit_shader' for the format to use as that what this is basically about. If I add the 'converted p8' format to the formats table, I would still have to return 'CONVERT_PALETTED' in d3dfmt_get_conv and somehow inspect the format. The cleanest way (I could check conv_bytes) I think is to add a flag WINED3DFMT_FLAG_CONVERTED. I want to set this flag also for converted textures and thus checks like 'convert != NO_CONVERSION || desc.convert' can be adjusted as well and d3dfmt_convert_surface is now a bit nicer as well. Though not all parts might be needed right now, I think at least the Flags changes are a step in the right direction. Is the 'p8 solution' acceptable for now? Roderick
On Wed, May 19, 2010 at 8:27 PM, Roderick Colenbrander <thunderbird2k(a)gmail.com> wrote:
On Wed, May 19, 2010 at 7:02 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 19 May 2010 14:46, Roderick Colenbrander <thunderbird2k(a)gmail.com> wrote:
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 ... What do you think?
Is there a bug report / demo application that clearly shows the problem? Maybe I can give it a look.
The bug in question is and a test case is StarCraft: http://bugs.winehq.org/show_bug.cgi?id=22575
I have made a draft patch (this would have to be cut in a lot of pieces and changes a few more things than just the issue). It might be an acceptable path.
The idea is to move the 'p8 texture format' to the formats table including the case when no shaders/ext_paletted_texture is around (this case needs to be added in some form anyway since fixup up an empty format desc is not that nice). The format is overridden in case of color keying / drawpixels. So at this stage the 'blit_supported' is implicitly performed at formats table initialization. This might be acceptable for now. At a later stage I can imagine that we want to ask the 'blit_shader' for the format to use as that what this is basically about.
If I add the 'converted p8' format to the formats table, I would still have to return 'CONVERT_PALETTED' in d3dfmt_get_conv and somehow inspect the format. The cleanest way (I could check conv_bytes) I think is to add a flag WINED3DFMT_FLAG_CONVERTED. I want to set this flag also for converted textures and thus checks like 'convert != NO_CONVERSION || desc.convert' can be adjusted as well and d3dfmt_convert_surface is now a bit nicer as well.
Though not all parts might be needed right now, I think at least the Flags changes are a step in the right direction. Is the 'p8 solution' acceptable for now?
Roderick
Before I clean this stuff up and flood wine-patches, would this be an acceptable solution for 1.2? Essentially it is about adding a WINED3DFMT_FLAG_CONVERTED flag to converted formats and adding a generic p8 format (which uses conversion) to the formats table, so the table acts as a 'blit_supported' for now. Even in the long run I think it makes sense to have these flags and the main thing which has to be fixed is the format selection itself. Roderick
On 26 May 2010 14:51, Roderick Colenbrander <thunderbird2k(a)gmail.com> wrote:
Before I clean this stuff up and flood wine-patches, would this be an acceptable solution for 1.2? The "converted" and "color key" flags are probably ok, but I doubt we want that in 1.2. If this is about fixing the regression, it's probably better to just reintroduce the extension check in d3dfmt_get_conv().
participants (3)
-
Henri Verbeet -
Roderick Colenbrander -
Stefan Dösinger