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  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.