Re: [PATCH 3/6] wined3d: Use an rbtree for storing shaders for texture format conversion/blitting

[Stefan Dösinger]

Am 11.02.2014 um 12:14 schrieb Martin Storsjo :
> dlls/wined3d/arb_program_shader.c | 187 +++++++++++++++++++++++++-------------
> 1 file changed, 122 insertions(+), 65 deletions(-)
>
> diff --git a/dlls/wined3d/arb_program_shader.c b/dlls/wined3d/arb_program_shader.c
> index 6d17ea3..8390c9e 100644
> --- a/dlls/wined3d/arb_program_shader.c
> +++ b/dlls/wined3d/arb_program_shader.c
> @@ -6768,22 +6768,68 @@ const struct fragment_pipeline arbfp_fragment_pipeline = {
>     arbfp_fragmentstate_template,
> };
>
> +struct arbfp_blit_type
> +{
> - enum complex_fixup fixup;
> - GLenum textype;
>
> +};
> ...
> +static int arbfp_blit_type_compare(const void *key, const struct wine_rb_entry *entry)
> +{
> - const struct arbfp_blit_type *ka = key;
> - const struct arbfp_blit_type *kb = &WINE_RB_ENTRY_VALUE(entry, const struct arbfp_blit_desc, entry)->type;
> -
> - return memcmp(ka, kb, sizeof(*ka));
>
> +}

You have to watch out for padding space added by the compiler if you memcmp the structure. I think there’s no guarantee that enum complex_fixup is 4 bytes. Either use memset to initialize it to zero, or (preferred) make the fixup member a bitfield and add an explicit padding field for the remaining bits (see some structures in wined3d_private.h for examples) and set the padding to zero manually.
> - desc = HeapAlloc(GetProcessHeap(), 0, sizeof(*desc));
> - if (!desc)
> - {
> -
>    ERR("Out of memory\n");
> -
>    GL_EXTCALL(glBindProgramARB(GL_FRAGMENT_PROGRAM_ARB, 0));
> -
>    checkGLcall("glBindProgramARB(GL_FRAGMENT_PROGRAM_ARB, 0)");
> -
>    GL_EXTCALL(glDeleteProgramsARB(1, &shader));
> -
>    checkGLcall("GL_EXTCALL(glDeleteProgramsARB(1, &shader))");
> -
>    return 0;
> - }
>
> ...
> -
>    ERR("Out of memory\n");
> -
>    GL_EXTCALL(glDeleteProgramsARB(1, &shader));
> -
>    checkGLcall("GL_EXTCALL(glDeleteProgramsARB(1, &shader))");
> -
>    GL_EXTCALL(glBindProgramARB(GL_FRAGMENT_PROGRAM_ARB, 0));
> -
>    checkGLcall("glBindProgramARB(GL_FRAGMENT_PROGRAM_ARB, 0)");
> -
>    HeapFree(GetProcessHeap(), 0, desc);
> -
>    return 0;
> - }

You can use goto (yes, goto) to avoid code duplication in error handling. See e.g. wined3d_device_init_3d in device.c
> -
>        upload_palette(surface, context);

I think you lost that call.

The unfortunate thing about the P8 conversion is that the shader conversion codepath is essentially dead code for various reasons. So it’s tricky to test and likely already broken.

[PATCH 3/6] wined3d: Use an rbtree for storing shaders for texture format conversion/blitting

[Martin Storsjö]

On Tue, 11 Feb 2014, Stefan Dösinger wrote:
> Am 11.02.2014 um 12:14 schrieb Martin Storsjo :
> > dlls/wined3d/arb_program_shader.c | 187 +++++++++++++++++++++++++-------------
> > 1 file changed, 122 insertions(+), 65 deletions(-)
> >
> > diff --git a/dlls/wined3d/arb_program_shader.c b/dlls/wined3d/arb_program_shader.c
> > index 6d17ea3..8390c9e 100644
> > --- a/dlls/wined3d/arb_program_shader.c
> > +++ b/dlls/wined3d/arb_program_shader.c
> > @@ -6768,22 +6768,68 @@ const struct fragment_pipeline arbfp_fragment_pipeline = {
> >     arbfp_fragmentstate_template,
> > };
> >
> > +struct arbfp_blit_type
> > +{
> > - enum complex_fixup fixup;
> > - GLenum textype;
> >
> > +};
> > ...
> > +static int arbfp_blit_type_compare(const void *key, const struct wine_rb_entry *entry)
> > +{
> > - const struct arbfp_blit_type *ka = key;
> > - const struct arbfp_blit_type *kb = &WINE_RB_ENTRY_VALUE(entry, const struct arbfp_blit_desc, entry)->type;
> > -
> > - return memcmp(ka, kb, sizeof(*ka));
> >
> > +}
>
> You have to watch out for padding space added by the compiler if you memcmp the structure. I think there’s no guarantee that enum complex_fixup is 4 bytes. Either use memset to initialize it to zero, or (preferred) make the fixup member a bitfield and add an explicit padding field for the remaining bits (see some structures in wined3d_private.h for examples) and set the padding to zero manually.

Ah, right, yes. Or what about just doing two separate comparisons for 
fixup and textype instead of a memcmp of the struct?

// Martin