[v3 PATCH 1/3] wined3d: Manage source texture resolve in texture2d_blt() for colour blits.

[Henri Verbeet]

On Wed, 11 Mar 2020 at 16:25, Paul Gofman <gofmanp at gmail.com> wrote:
> On 3/11/20 15:22, Henri Verbeet wrote:
> > On Tue, 10 Mar 2020 at 15:01, Paul Gofman <gofmanp at gmail.com> wrote:
> >> @@ -2629,8 +2622,14 @@ HRESULT texture2d_blt(struct wined3d_texture *dst_texture, unsigned int dst_sub_
> >>          dst_location = dst_texture->resource.map_binding;
> >>
> >>      context = context_acquire(device, dst_texture, dst_sub_resource_idx);
> >> +    scale = abs(src_box->right - src_box->left) != abs(dst_box->right - dst_box->left)
> >> +            || abs(src_box->bottom - src_box->top) != abs(dst_box->bottom - dst_box->top);
> > "scale" is already initialised earlier in the function. It's true that
> > this variant would allow flips and mirrors whereas the variant earlier
> > in the texture2d_blt() doesn't, but flipped/mirrored boxes aren't
> > valid for texture2d_blt(). (In particular, wined3d_texture_blt() calls
> > wined3d_texture_check_box_dimensions(); callers would use
> > WINEDDBLTFX_MIRROR* to do flipped/mirrored blits.)
> >
> >> +    src_location = (scale || convert || blit_op != WINED3D_BLIT_OP_COLOR_BLIT)
> >> +            && wined3d_texture_is_multisample_location(src_texture,
> >> +            src_texture->resource.draw_binding) ? WINED3D_LOCATION_RB_RESOLVED
> >> +            : src_texture->resource.draw_binding;
> > is the "blit_op" check needed?
> I think it is not strictly necessary as colour key or alpha test blits
> won't end up in FBO blitter anyway, but I thought it is less fragile and
> more clear to be explicit here. Should I remove that?

To the extent that it makes a difference for those blit types, I think
using the resolved location makes sense there as well.

> >> +static inline BOOL wined3d_texture_is_multisample_location(const struct wined3d_texture *texture,
> >> +        DWORD location)
> >> +{
> >> +    if (location == WINED3D_LOCATION_RB_MULTISAMPLE)
> >> +        return TRUE;
> >> +    if (location != WINED3D_LOCATION_TEXTURE_RGB && location != WINED3D_LOCATION_TEXTURE_SRGB)
> >> +        return FALSE;
> >> +    return texture->resource.multisample_type != WINED3D_MULTISAMPLE_NONE;
> >> +}
> > return location == d3d_info->multisample_draw_location
> >         && texture->resource.multisample_type != WINED3D_MULTISAMPLE_NONE;
> >
> > Otherwise, calling the above would give an incorrect result when
> > called with e.g. WINED3D_LOCATION_TEXTURE_RGB on a multisampled
> > texture when ARB_texture_multisample is not used.
>
> I might have missed something but can't WINED3D_LOCATION_TEXTURE_SRGB be
> a multisample location while d3dinfo->multisample_draw_location will be
> WINED3D_LOCATION_TEXTURE_RGB if ARB_texture_multisample is used? If that
> is the case, maybe this should be
>
> 'return texture->resource.multisample_type != WINED3D_MULTISAMPLE_NONE
> && (d3d_info->multisample_draw_location == WINED3D_LOCATION_TEXTURE_RGB)'?
>
I think you're right, although ARB_texture_multisample without
EXT_texture_sRGB_decode isn't terribly likely. Note that in the caller
that essentially reduces to the following though:

    if (!(dst_texture->resource.access & WINED3D_RESOURCE_ACCESS_GPU))
        dst_location = dst_texture->resource.map_binding;
    else if ((scale || convert) &&
dst_texture->resource.multisample_type != WINED3D_MULTISAMPLE_NONE)
        dst_location = WINED3D_LOCATION_RB_RESOLVED;
    else
        dst_location = dst_texture->resource.draw_binding;