Re: [PATCH v2 4/6] wined3d: Handle typeless resolve in the FBO blitter.

[Henri Verbeet]

On Tue, 9 Mar 2021 at 10:26, Jan Sikorski <jsikorski(a)codeweavers.com> wrote:
> +    if (resolve_format_id != WINED3DFMT_UNKNOWN)
> +    {
> I think we only care about the resolve format if we're doing a resolve
> operation.
> +        switch (resolve_format_id)
> +        {
> +            case WINED3DFMT_R8G8B8A8_UNORM_SRGB:
> +            case WINED3DFMT_B8G8R8A8_UNORM_SRGB:
> +            case WINED3DFMT_B8G8R8X8_UNORM_SRGB:
> +                gl_info->gl_ops.gl.p_glEnable(GL_FRAMEBUFFER_SRGB);
> +                context_invalidate_state(context, STATE_RENDER(WINED3D_RS_SRGBWRITEENABLE));
> +                resolve_internal = resolve_format_gl->srgb_internal;
> +                break;
> +            default:
> +                resolve_internal = resolve_format_gl->internal;
> +                break;
> +        }
> +
> Don't hardcode formats like that, it's very fragile. You'll probably
> want to do something similar to needs_srgb_write(). Should we enable
> GL_FRAMEBUFFER_SRGB for blits more broadly instead of only for
> typeless resolves? If yes, this belongs in a separate commit, and
> something like wined3d_context_gl_apply_fbo_state_blit() may be a more
> appropriate place.
> +        if (src_location == WINED3D_LOCATION_TEXTURE_SRGB)
> +            src_internal = src_format_gl->srgb_internal;
> +        else if (src_texture->resource.bind_flags & WINED3D_BIND_RENDER_TARGET && wined3d_resource_is_offscreen(&src_texture->resource))
> +            src_internal = src_format_gl->rt_internal;
> +        else
> +            src_internal = src_format_gl->internal;
> +
> +        if (dst_location == WINED3D_LOCATION_TEXTURE_SRGB)
> +            dst_internal = dst_format_gl->srgb_internal;
> +        else if (dst_texture->resource.bind_flags & WINED3D_BIND_RENDER_TARGET && wined3d_resource_is_offscreen(&dst_texture->resource))
> +            dst_internal = dst_format_gl->rt_internal;
> +        else
> +            dst_internal = dst_format_gl->internal;
> +
> That could use a helper function.
> +        /* In case of typeless resolve the texture type may not match the resolve type.
> +         * To handle that, allocate intermediate texture(s) to resolve from/to.
> +         * A possible performance improvement would be to resolve using a shader instead. */
> +        if (src_internal != resolve_internal)
> +        {
> +            struct wined3d_resource_desc desc;
> +            unsigned src_level;
> +            HRESULT hr;
> +
> +            src_level = src_sub_resource_idx % src_texture->level_count;
> +            desc.resource_type = WINED3D_RTYPE_TEXTURE_2D;
> +            desc.format = resolve_format_id;
> +            desc.multisample_type = src_texture->resource.multisample_type;
> +            desc.multisample_quality = src_texture->resource.multisample_quality;
> +            desc.usage = WINED3DUSAGE_PRIVATE;
> +            desc.bind_flags = 0;
> +            desc.access = WINED3D_RESOURCE_ACCESS_GPU;
> +            desc.width = wined3d_texture_get_level_width(src_texture, src_level);
> +            desc.height = wined3d_texture_get_level_height(src_texture, src_level);
> +            desc.depth = 1;
> +            desc.size = 0;
> +
> +            hr = wined3d_texture_create(device, &desc, 1, 1, 0, NULL, NULL, &wined3d_null_parent_ops, &src_staging_texture);
> +            if (FAILED(hr))
> +            {
> +                ERR("Failed to create staging texture, hr %#x.\n", hr);
> +                return;
> +            }
> +
> +            device->blitter->ops->blitter_blit(device->blitter, WINED3D_BLIT_OP_RAW_BLIT, context,
> +                    src_texture, src_sub_resource_idx, src_location, src_rect,
> +                    src_staging_texture, 0, src_location, src_rect,
> +                    NULL, WINED3D_TEXF_NONE, WINED3DFMT_UNKNOWN);
> +
> +            src_texture = src_staging_texture;
> +            src_sub_resource_idx = 0;
> +        }
> +
> Note that that only works if the source location isn't
> WINED3D_LOCATION_DRAWABLE. That should always hold in practice, but we
> may want to be explicit about that.
> +        if (dst_internal != resolve_internal)
> +        {
> +            struct wined3d_resource_desc desc;
> +            unsigned dst_level;
> +            HRESULT hr;
> +
> +            dst_level = dst_sub_resource_idx % dst_texture->level_count;
> +            desc.resource_type = WINED3D_RTYPE_TEXTURE_2D;
> +            desc.format = resolve_format_id;
> +            desc.multisample_type = dst_texture->resource.multisample_type;
> +            desc.multisample_quality = dst_texture->resource.multisample_quality;
> +            desc.usage = WINED3DUSAGE_PRIVATE;
> +            desc.bind_flags = 0;
> +            desc.access = WINED3D_RESOURCE_ACCESS_GPU;
> +            desc.width = wined3d_texture_get_level_width(dst_texture, dst_level);
> +            desc.height = wined3d_texture_get_level_height(dst_texture, dst_level);
> +            desc.depth = 1;
> +            desc.size = 0;
> +
> +            hr = wined3d_texture_create(device, &desc, 1, 1, 0, NULL, NULL, &wined3d_null_parent_ops, &dst_staging_texture);
> +            if (FAILED(hr))
> +            {
> +                ERR("Failed to create staging texture, hr %#x.\n", hr);
> +                if (src_staging_texture)
> +                    wined3d_texture_decref(src_staging_texture);
> +                return;
> +            }
> +
> +            wined3d_texture_load_location(dst_staging_texture, 0, context, dst_location);
> +
> +            dst_texture = dst_staging_texture;
> +            dst_sub_resource_idx = 0;
> +        }
> +    }
> +
> Likewise.