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

[Paul Gofman]

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?
> > @@ -121,7 +122,9 @@ HRESULT ddraw_surface_update_frontbuffer(struct ddraw_surface *surface,
> >          return hr;
> >      }
> > - if (FAILED(hr = wined3d_texture_get_dc(surface->wined3d_texture, surface->sub_resource_idx, &surface_dc)))
> > - src_texture = ddraw_surface_get_default_texture(surface, DDRAW_SURFACE_READ);
> >
> > +the
> > - if (FAILED(hr = wined3d_texture_get_dc(src_texture, surface->sub_resource_idx, &surface_dc)))
> > {
> >     ERR("Failed to get surface DC, hr %#x.\n", hr);
> >     return hr;
>
> I don't think that's quite right in the "read" case.

Yes, I will adjust the flags to ddraw_surface_get_default_texture().
> > +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?
> > @@ -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?