Re: [PATCH 2/6] wined3d: Keep track of FBOs through the GL names (v2).

[Henri Verbeet]

On 28 October 2015 at 15:29, Stefan Dösinger <stefan(a)codeweavers.com> wrote:
> Version 2: Don't use fbo_entry->d3d_* for logging, plan for it to go
> away. The disadvantage is now that we don't know the d3d properties once
> we find out that an FBO doesn't work and need a full TRACE log for this
> information, but I think that's something we can live with.
>
> If there is a prettier way to find the texture target of a GL texture
> name please advise. I couldn't find any.

I think I'll hold off on this until the regression from
1ca9dfc8ee25f4ae188fdacd4d3d56046cef8003 is resolved, but you can
probably split the logging changes into their own patch.
> +            char name[32];
> +            sprintf(name, "Color attachment %u", i);
> +            context_dump_fbo_attachment(gl_info, target, GL_COLOR_ATTACHMENT0 + i, name);
> sprintf() is one of those things that's usually best avoided. In this
> case the string is mostly redundant as well, since you have the
> attachment enum.
> +static inline void context_create_fbo_key(struct wined3d_fbo_entry_key *key,
> +        struct wined3d_surface **render_targets, struct wined3d_surface *depth_stencil,
> +        DWORD color_location, DWORD ds_location, UINT color_buffers)
> The naming is a bit odd. "create" is usually for things that allocate
> something, "color_buffers" would normally be called e.g. "rt_count".
> The "inline" seems speculative. I think modern compilers mostly ignore
> it, but it does make it harder to notice when a function becomes
> unused.
> -    entry = HeapAlloc(GetProcessHeap(), 0, sizeof(*entry));
> -    entry->render_targets = HeapAlloc(GetProcessHeap(), 0, gl_info->limits.buffers * sizeof(*entry->render_targets));
> -    memcpy(entry->render_targets, render_targets, gl_info->limits.buffers * sizeof(*entry->render_targets));
> -    entry->depth_stencil = depth_stencil;
> -    entry->color_location = color_location;
> -    entry->ds_location = ds_location;
> +    UINT object_count = gl_info->limits.buffers + 1;
> +
> +    entry = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY,
> +            FIELD_OFFSET(struct fbo_entry, key.objects[object_count]));
> +    entry->d3d_render_targets = HeapAlloc(GetProcessHeap(), 0,
> +            gl_info->limits.buffers * sizeof(*entry->d3d_render_targets));
> +    context_create_fbo_key(&entry->key, render_targets, depth_stencil, color_location, ds_location,
> +            gl_info->limits.buffers);
> +    memcpy(entry->d3d_render_targets, render_targets, sizeof(*entry->d3d_render_targets) * gl_info->limits.buffers);
> +    entry->d3d_depth_stencil = depth_stencil;
> Why HEAP_ZERO_MEMORY? Do we no longer initialize all fields?
> +    {
> +        struct wined3d_fbo_resource resource = {0};
> +        context_attach_depth_stencil_fbo(context, target, &resource, FALSE, 0, 0);
> "resource" should probably be static const.
> -        if (surface == entry->render_targets[i])
> +        if (surface->container->texture_rgb.name == entry->key.objects[i].object ||
> +                surface->container->texture_srgb.name == entry->key.objects[i].object)
> This is different from the usual style.

[PATCH 2/6] wined3d: Keep track of FBOs through the GL names (v2).

[Stefan Dösinger]

> Am 02.11.2015 um 11:48 schrieb Henri Verbeet <hverbeet(a)gmail.com>:
> I think I'll hold off on this until the regression from
> 1ca9dfc8ee25f4ae188fdacd4d3d56046cef8003 is resolved, but you can
> probably split the logging changes into their own patch.
> Fair enough.
> > +            char name[32];
> > +            sprintf(name, "Color attachment %u", i);
> > +            context_dump_fbo_attachment(gl_info, target, GL_COLOR_ATTACHMENT0 + i, name);
> > sprintf() is one of those things that's usually best avoided. In this
> > case the string is mostly redundant as well, since you have the
> > attachment enum.

I prefer "GL_COLOR_ATTACHMENT0" over "0x8CE0", but I guess wine_dbg_sprintf would also do the job without requring my own sprintf.
> > +    entry = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY,
> > +            FIELD_OFFSET(struct fbo_entry, key.objects[object_count]));
> > +    entry->d3d_render_targets = HeapAlloc(GetProcessHeap(), 0,
> > +            gl_info->limits.buffers * sizeof(*entry->d3d_render_targets));
> > +    context_create_fbo_key(&entry->key, render_targets, depth_stencil, color_location, ds_location,
> > +            gl_info->limits.buffers);
> > +    memcpy(entry->d3d_render_targets, render_targets, sizeof(*entry->d3d_render_targets) * gl_info->limits.buffers);
> > +    entry->d3d_depth_stencil = depth_stencil;
> > Why HEAP_ZERO_MEMORY? Do we no longer initialize all fields?
> > I don't trust the padding of the structure array after the 4 byte DWORD on 64 bit architectures.

[PATCH 2/6] wined3d: Keep track of FBOs through the GL names (v2).

[Henri Verbeet]

On 2 November 2015 at 12:09, Stefan Dösinger <stefandoesinger(a)gmail.com> wrote:
> I prefer "GL_COLOR_ATTACHMENT0" over "0x8CE0", but I guess wine_dbg_sprintf would also do the job without requring my own sprintf.

It would, but you don't even need that. You can just return a static
string, like in debug_fbostatus().
> I don't trust the padding of the structure array after the 4 byte DWORD on 64 bit architectures.

It probably has some padding on 64-bit, but we don't memcmp() struct fbo_entry.