[PATCH 5/5] wined3d: Read back onscreen surfaces before downloading.

[Henri Verbeet]

On Sun, 21 Mar 2021 at 22:06, Matteo Bruni <matteo.mystral at gmail.com> wrote:
> On Thu, Mar 18, 2021 at 2:33 PM Henri Verbeet <hverbeet at gmail.com> wrote:
> > On Wed, 17 Mar 2021 at 13:35, Matteo Bruni <mbruni at codeweavers.com> wrote:
> > > @@ -1666,6 +1666,16 @@ HRESULT texture2d_blt(struct wined3d_texture *dst_texture, unsigned int dst_sub_
> > >              TRACE("Not doing download because of partial download (src).\n");
> > >          else if (!wined3d_texture_is_full_rect(dst_texture, dst_sub_resource_idx % dst_texture->level_count, &dst_rect))
> > >              TRACE("Not doing download because of partial download (dst).\n");
> > > +        else if (src_sub_resource->locations == WINED3D_LOCATION_DRAWABLE)
> > > +        {
> > > +            context = context_acquire(device, src_texture, src_sub_resource_idx);
> > > +            texture2d_read_from_framebuffer(src_texture, src_sub_resource_idx, context,
> > > +                    WINED3D_LOCATION_DRAWABLE, dst_texture->resource.map_binding);
> > > +            wined3d_texture_validate_location(src_texture, src_sub_resource_idx, dst_texture->resource.map_binding);
> > > +            context_release(context);
> > > +            return texture2d_blt(dst_texture, dst_sub_resource_idx, dst_box,
> > > +                    src_texture, src_sub_resource_idx, src_box, flags, fx, filter);
> > > +        }
> >
> > It's perhaps not immediately obvious from the function name, but
> > texture2d_read_from_framebuffer() is specific to the GL backend.
> > Calling it from common code like texture2d_blt() is problematic.
>
> You're right :/
> For some background, this patch was also related to backbuffer ORM:
> IIRC after this the d3d tests start to give sensible results (mostly
> broken, of course, but at least backbuffer readback works so SOME
> tests pass).
>
I didn't write the patch, but my guess would be that the issue was
that wined3d_texture_download_from_texture() doesn't handle resources
in WINED3D_LOCATION_DRAWABLE. Perhaps the most straightforward way to
address that would be to simply implement downloading from
WINED3D_LOCATION_DRAWABLE in wined3d_texture_download_from_texture()
and wined3d_texture_gl_download_data(). Alternatively:
  - We could detect that case here, skip
wined3d_texture_download_from_texture(), and let the blitter figure it
out. That should work, although it may introduce more copies between
the DRAWABLE/TEXTURE/SYSMEM locations than desirable.
  - Instead of skipping wined3d_texture_download_from_texture(), we
could call wined3d_texture_load_location() before calling it. The main
disadvantage of that approach is that for on-screen resources like the
back-buffer that ends up doing a flip in system memory first. In that
context, it may be worth considering using the "AlwaysOffscreen"
approach for backbuffer ORM as well. That would introduce an extra
blit for presents, but would simplify some other, potentially more
expensive things, like the issue we're discussing here.