[PATCH v2 2/6] ddraw: Create a separate texture for drawing with bindable sysmem surfaces.

[Henri Verbeet]

On Tue, 2 Mar 2021 at 18:03, Paul Gofman <pgofman at codeweavers.com> wrote:
> On 3/2/21 19:16, Henri Verbeet wrote:
> >> @@ -6951,6 +6951,12 @@ static HRESULT d3d_device_init(struct d3d_device *device, struct ddraw *ddraw, c
> >>      device->hardware_device = IsEqualGUID(&IID_IDirect3DTnLHalDevice, guid)
> >>              || IsEqualGUID(&IID_IDirect3DHALDevice, guid);
> >>
> >> +    if (device->hardware_device && !(target->surface_desc.ddsCaps.dwCaps & DDSCAPS_VIDEOMEMORY))
> >> +    {
> >> +        WARN("Surface %p is not in video memory.\n", target);
> >> +        return D3DERR_SURFACENOTINVIDMEM;
> >> +    }
> >> +
> >>      if (outer_unknown)
> >>          device->outer_unknown = outer_unknown;
> >>      else
> >> @@ -7038,12 +7044,6 @@ HRESULT d3d_device_create(struct ddraw *ddraw, const GUID *guid, struct ddraw_su
> >>          return DDERR_OUTOFMEMORY;
> >>      }
> >>
> >> -    if (!(target->surface_desc.ddsCaps.dwCaps & DDSCAPS_VIDEOMEMORY))
> >> -    {
> >> -        WARN("Surface %p is not in video memory.\n", target);
> >> -        return D3DERR_SURFACENOTINVIDMEM;
> >> -    }
> >> -
> > Was there any particular reason to move this? It's fine if there's a
> > reason, but on first sight it seems somewhat arbitrary.
>
> Checking the guids seemed a bit more reasonable to me in
> d3d_device_init() which fills all the fields of the struct d3d_device,
> including hardware_device, so I moved the check after the estimation of
> that field. Otherwise, I could check guids in d3d_device_create() and
> pass hardware_device BOOL to d3d_device_init. Should I change that?
>
Oh, I see, we don't have device->hardware_device yet in
d3d_device_create(). That's fine, no need to change it.

> >> +static void ddraw_surface_create_draw_texture(struct ddraw_surface *surface)
> >> +{
> >> +    DDSURFACEDESC2 *desc = &surface->surface_desc;
> >> +    struct wined3d_resource *draw_texture_resource;
> >> +    struct wined3d_resource_desc wined3d_desc;
> >> +    unsigned int i, layer_count, level_count;
> >> +    struct wined3d_texture *draw_texture;
> >> +    struct ddraw_surface *parent;
> >> +    unsigned int bind_flags;
> >> +    HRESULT hr;
> >> +
> >> +    if (!(desc->ddsCaps.dwCaps & DDSCAPS_SYSTEMMEMORY))
> >> +        return;
> >> +
> > Shouldn't we also check whether we have a hardware device or not here?
> Do you mean checking if we have a hardware renderer at all and skip the
> attempt to create draw_texture if we don't? But wouldn't it be easier to
> just let it fail without adding an extra check?

No, the other way around, there's no need to create the extra texture
if we do have a hardware device. Granted, that only makes a difference
if applications create system memory surfaces with the relevant caps
on a hardware device, but well, ddraw applications.

> >> @@ -6695,7 +6810,7 @@ struct wined3d_rendertarget_view *ddraw_surface_get_rendertarget_view(struct ddr
> >>      if (surface->wined3d_rtv)
> >>          return surface->wined3d_rtv;
> >>
> >> -    if (FAILED(hr = wined3d_rendertarget_view_create_from_sub_resource(surface->wined3d_texture,
> >> +    if (FAILED(hr = wined3d_rendertarget_view_create_from_sub_resource(ddraw_surface_get_draw_texture(surface, 0),
> >>              surface->sub_resource_idx, surface, &ddraw_view_wined3d_parent_ops, &surface->wined3d_rtv)))
> >>      {
> >>          ERR("Failed to create rendertarget view, hr %#x.\n", hr);
> > The ddraw_surface_get_draw_texture() here seems a little awkward,
> > since typically that function implies synchronising texture contents,
> > but we don't care about that here. Not wrong though.
>
> But there are flags which are 0. Do you think it is cleaner to just
> select the texture with '?' in this case?
>
Perhaps, I haven't quite made up my mind yet. My first instinct would
be to write

    if (surface->draw_texture)
        wined3d_texture = surface->draw_texture;
    else
        wined3d_texture = surface->wined3d_texture;

but there's something to be said for always going through
ddraw_surface_get_draw_texture() too.