Re: [v7 03/10] d3d9: Call wined3d_device_copy_sub_resource_region() instead of wined3d_device_update_surface() in d3d9_device_UpdateSurface().

[Henri Verbeet]

On 10 November 2015 at 14:09, Riccardo Bortolato  wrote:
> diff --git a/dlls/d3d9/device.c b/dlls/d3d9/device.c
> index 16b27e5..3dca920 100644
> --- a/dlls/d3d9/device.c
> +++ b/dlls/d3d9/device.c
> @@ -1227,16 +1227,32 @@ static HRESULT WINAPI d3d9_device_UpdateSurface(IDirect3DDevice9Ex *iface,
>      struct d3d9_device *device = impl_from_IDirect3DDevice9Ex(iface);
>      struct d3d9_surface *src = unsafe_impl_from_IDirect3DSurface9(src_surface);
>      struct d3d9_surface *dst = unsafe_impl_from_IDirect3DSurface9(dst_surface);
> -
>
> struct wined3d_box src_box;
> HRESULT hr;
>
> TRACE("iface %p, src_surface %p, src_rect %p, dst_surface %p, dst_point %p.\n",
>         iface, src_surface, src_rect, dst_surface, dst_point);
> -
>
> if (src_rect)
> -
>
> {
> -
>    src_box.left = src_rect->left;
> -
>    src_box.top = src_rect->top;
> -
>    src_box.right = src_rect->right;
> -
>    src_box.bottom = src_rect->bottom;
> -
>    src_box.front = 0;
> -
>    src_box.back = 1;
> -
>
> }
> -
>
> wined3d_mutex_lock();
> - hr = wined3d_device_update_surface(device->wined3d_device, src->wined3d_surface, src_rect,
> -
>        dst->wined3d_surface, dst_point);
> - hr = wined3d_device_copy_sub_resource_region(device->wined3d_device,
> -
>        wined3d_texture_get_resource(dst->wined3d_texture), dst->sub_resource_idx, dst_point ? dst_point->x : 0,
> -
>        dst_point ? dst_point->y : 0, 0, wined3d_texture_get_resource(src->wined3d_texture),
> -
>        src->sub_resource_idx, src_rect ? &src_box : NULL);
> wined3d_mutex_unlock();

This is fine.
> - if (FAILED(hr))
> -
>    return D3DERR_INVALIDCALL;
> - return hr;

Is this needed? I.e., would just "return hr;" also work?
> diff --git a/dlls/wined3d/device.c b/dlls/wined3d/device.c
> index 365629b..0f64469 100644
> --- a/dlls/wined3d/device.c
> +++ b/dlls/wined3d/device.c
> @@ -3888,14 +3888,6 @@ HRESULT CDECL wined3d_device_copy_sub_resource_region(struct wined3d_device *dev
>          return WINED3DERR_INVALIDCALL;
>      }
> - if (src_resource->type != dst_resource->type)
> - {
> -
>    WARN("Resource types (%s / %s) don't match.\n",
> -
>            debug_d3dresourcetype(dst_resource->type),
> -
>            debug_d3dresourcetype(src_resource->type));
> -
>    return WINED3DERR_INVALIDCALL;
> - }
> - if (src_resource->format->id != dst_resource->format->id)
>    {
>        WARN("Resource formats (%s / %s) don't match.\n",
>
> @@ -3904,9 +3896,11 @@ HRESULT CDECL wined3d_device_copy_sub_resource_region(struct wined3d_device *dev
>          return WINED3DERR_INVALIDCALL;
>      }
> - if (dst_resource->type != WINED3D_RTYPE_TEXTURE)
> - if ((dst_resource->type != WINED3D_RTYPE_TEXTURE && dst_resource->type != WINED3D_RTYPE_CUBE_TEXTURE)
> -
>        || (src_resource->type != WINED3D_RTYPE_TEXTURE && src_resource->type != WINED3D_RTYPE_CUBE_TEXTURE))
> {
> -
>    FIXME("Not implemented for %s resources.\n", debug_d3dresourcetype(dst_resource->type));
> -
>    FIXME("Not implemented for %s to %s resources.\n",
> -
>            debug_d3dresourcetype(src_resource->type), debug_d3dresourcetype(dst_resource->type));
>    return WINED3DERR_INVALIDCALL;
> }

This is really a separate change. I'd prefer something like the following:

enum wined3d_resource_type src_resource_type, dst_resource_type;
    ...
    src_resource_type = src_resource->type;
    if (src_resource_type == WINED3D_RTYPE_CUBE_TEXTURE)
       src_resource_type = WINED3D_RTYPE_TEXTURE;
    ...
    if (src_resource_type != dst_resource_type)
    {
       ...
    }
    ...

[v7 03/10] d3d9: Call wined3d_device_copy_sub_resource_region() instead of wined3d_device_update_surface() in d3d9_device_UpdateSurface().

[Riccardo Bortolato]

Hi Henri,

the return code fixup fixes a failing test in d3d9 (sometimes
surface_blt will return something different than
WINED3DERR_INVALIDCALL).

As for the resource type check change, I put it into this patch as
(afaik) it's only required by d3d9. I will integrate your suggestion
(in a separate commit if you want).

Ciao,
Riccardo

2015-11-12 11:05 GMT+01:00 Henri Verbeet :
> On 10 November 2015 at 14:09, Riccardo Bortolato  wrote:
> > diff --git a/dlls/d3d9/device.c b/dlls/d3d9/device.c
> > index 16b27e5..3dca920 100644
> > --- a/dlls/d3d9/device.c
> > +++ b/dlls/d3d9/device.c
> > @@ -1227,16 +1227,32 @@ static HRESULT WINAPI d3d9_device_UpdateSurface(IDirect3DDevice9Ex *iface,
> >      struct d3d9_device *device = impl_from_IDirect3DDevice9Ex(iface);
> >      struct d3d9_surface *src = unsafe_impl_from_IDirect3DSurface9(src_surface);
> >      struct d3d9_surface *dst = unsafe_impl_from_IDirect3DSurface9(dst_surface);
> > -
> >
> > struct wined3d_box src_box;
> > HRESULT hr;
> >
> > TRACE("iface %p, src_surface %p, src_rect %p, dst_surface %p, dst_point %p.\n",
> >         iface, src_surface, src_rect, dst_surface, dst_point);
> > -
> >
> > if (src_rect)
> > -
> >
> > {
> > -
> >    src_box.left = src_rect->left;
> > -
> >    src_box.top = src_rect->top;
> > -
> >    src_box.right = src_rect->right;
> > -
> >    src_box.bottom = src_rect->bottom;
> > -
> >    src_box.front = 0;
> > -
> >    src_box.back = 1;
> > -
> >
> > }
> > -
> >
> > wined3d_mutex_lock();
> > - hr = wined3d_device_update_surface(device->wined3d_device, src->wined3d_surface, src_rect,
> > -
> >        dst->wined3d_surface, dst_point);
> > - hr = wined3d_device_copy_sub_resource_region(device->wined3d_device,
> > -
> >        wined3d_texture_get_resource(dst->wined3d_texture), dst->sub_resource_idx, dst_point ? dst_point->x : 0,
> > -
> >        dst_point ? dst_point->y : 0, 0, wined3d_texture_get_resource(src->wined3d_texture),
> > -
> >        src->sub_resource_idx, src_rect ? &src_box : NULL);
> > wined3d_mutex_unlock();
>
> This is fine.
> > - if (FAILED(hr))
> > -
> >    return D3DERR_INVALIDCALL;
> > - return hr;
>
> Is this needed? I.e., would just "return hr;" also work?
> > diff --git a/dlls/wined3d/device.c b/dlls/wined3d/device.c
> > index 365629b..0f64469 100644
> > --- a/dlls/wined3d/device.c
> > +++ b/dlls/wined3d/device.c
> > @@ -3888,14 +3888,6 @@ HRESULT CDECL wined3d_device_copy_sub_resource_region(struct wined3d_device *dev
> >          return WINED3DERR_INVALIDCALL;
> >      }
> > - if (src_resource->type != dst_resource->type)
> > - {
> > -
> >    WARN("Resource types (%s / %s) don't match.\n",
> > -
> >            debug_d3dresourcetype(dst_resource->type),
> > -
> >            debug_d3dresourcetype(src_resource->type));
> > -
> >    return WINED3DERR_INVALIDCALL;
> > - }
> > - if (src_resource->format->id != dst_resource->format->id)
> >    {
> >        WARN("Resource formats (%s / %s) don't match.\n",
> >
> > @@ -3904,9 +3896,11 @@ HRESULT CDECL wined3d_device_copy_sub_resource_region(struct wined3d_device *dev
> >          return WINED3DERR_INVALIDCALL;
> >      }
> > - if (dst_resource->type != WINED3D_RTYPE_TEXTURE)
> > - if ((dst_resource->type != WINED3D_RTYPE_TEXTURE && dst_resource->type != WINED3D_RTYPE_CUBE_TEXTURE)
> > -
> >        || (src_resource->type != WINED3D_RTYPE_TEXTURE && src_resource->type != WINED3D_RTYPE_CUBE_TEXTURE))
> > {
> > -
> >    FIXME("Not implemented for %s resources.\n", debug_d3dresourcetype(dst_resource->type));
> > -
> >    FIXME("Not implemented for %s to %s resources.\n",
> > -
> >            debug_d3dresourcetype(src_resource->type), debug_d3dresourcetype(dst_resource->type));
> >    return WINED3DERR_INVALIDCALL;
> > }
>
> This is really a separate change. I'd prefer something like the following:
> enum wined3d_resource_type src_resource_type, dst_resource_type;
> ...
> src_resource_type = src_resource->type;
> if (src_resource_type == WINED3D_RTYPE_CUBE_TEXTURE)
>    src_resource_type = WINED3D_RTYPE_TEXTURE;
> ...
> if (src_resource_type != dst_resource_type)
> {
>    ...
> }
> ...

[v7 03/10] d3d9: Call wined3d_device_copy_sub_resource_region() instead of wined3d_device_update_surface() in d3d9_device_UpdateSurface().

[Henri Verbeet]

On 12 November 2015 at 11:56, Riccardo Bortolato  wrote:
> the return code fixup fixes a failing test in d3d9 (sometimes
> surface_blt will return something different than
> WINED3DERR_INVALIDCALL).

Is that just the one where you're removing the todo_wine, or is that a
different test?
> As for the resource type check change, I put it into this patch as
> (afaik) it's only required by d3d9. I will integrate your suggestion
> (in a separate commit if you want).

The guideline is more or less that if two changes don't depend on each
other, and they still make sense on their own, they should probably be
in separate patches.