Re: DDRAW: Surface locking patch, take 2

[Christian Costa]

Hi Matthew,

This seems fine.
There are some comments though, see below.
> Index: dlls/ddraw/dsurface/dib.c
>
> RCS file: /home/wine/wine/dlls/ddraw/dsurface/dib.c,v
> retrieving revision 1.46
> diff -u -r1.46 dib.c
> --- dlls/ddraw/dsurface/dib.c	10 Mar 2005 11:13:11 -0000	1.46
> +++ dlls/ddraw/dsurface/dib.c	20 Mar 2005 16:49:40 -0000
> @@ -528,9 +528,15 @@
>     DD_STRUCT_INIT(&sdesc);
> sdesc.dwSize = sizeof(sdesc);
> - if (src) IDirectDrawSurface7_Lock(src, NULL, &sdesc, DDLOCK_READONLY, 0);
> ddesc.dwSize = sizeof(ddesc);
> - IDirectDrawSurface7_Lock(iface,NULL,&ddesc,DDLOCK_WRITEONLY,0);
> -
> -
>
> if (src == iface) {
> -
>    IDirectDrawSurface7_Lock(iface, NULL, &ddesc, 0, 0);
> -
>    DD_STRUCT_COPY_BYSIZE(&sdesc, &ddesc);
> -
>
> } else {
> -
>    if (src) IDirectDrawSurface7_Lock(src, NULL, &sdesc, DDLOCK_READONLY, 0);
> -
>    IDirectDrawSurface7_Lock(iface,NULL,&ddesc,DDLOCK_WRITEONLY,0);
> -
>
> }
>
> if ((sdesc.u4.ddpfPixelFormat.dwFlags & DDPF_FOURCC) &&
> (ddesc.u4.ddpfPixelFormat.dwFlags & DDPF_FOURCC)) {
>
> @@ -581,7 +587,8 @@
>     (xsrc.right > sdesc.dwWidth) || (xsrc.right < 0) ||
>     (xsrc.right < xsrc.left) || (xsrc.bottom < xsrc.top))) {
>         WARN("Application gave us bad source rectangle for Blt.\n");
> - return DDERR_INVALIDRECT;
> - ret = DDERR_INVALIDRECT;
> - goto release;
> }
> /* For the Destination rect, it can be out of bounds on the condition that a clipper
>   is set for the given surface.
>
> @@ -593,7 +600,8 @@
>     (xdst.right > ddesc.dwWidth) || (xdst.right < 0) ||
>     (xdst.right < xdst.left) || (xdst.bottom < xdst.top))) {
>         WARN("Application gave us bad destination rectangle for Blt without a clipper set.\n");
> - return DDERR_INVALIDRECT;
> -
>
> ret = DDERR_INVALIDRECT;
> -
>
> goto release;
> }
>
> /* Now handle negative values in the rectangles. Warning: only supported for now
>
> @@ -958,7 +966,7 @@
>
> release:
>     IDirectDrawSurface7_Unlock(iface,NULL);
> - if (src) IDirectDrawSurface7_Unlock(src,NULL);
> - if (src && src != iface) IDirectDrawSurface7_Unlock(src,NULL);
> return DD_OK;
>
> }
>
> @@ -975,7 +983,7 @@
>     HRESULT		ret = DD_OK;
>     LPBYTE		sbuf, dbuf;
>     RECT		rsrc2;
> - RECT                lock_src, lock_dst;
> -
>
> RECT                lock_src, lock_dst, lock_union;
>
> if (TRACE_ON(ddraw)) {
> TRACE("(%p)->(%ld,%ld,%p,%p,%08lx)\n",
>
> @@ -1043,11 +1051,28 @@
>     lock_dst.right = dstx + w;
>     lock_dst.bottom = dsty + h;
> - bpp = GET_BPP(This->surface_desc);
> - /* We need to lock the surfaces, or we won't get refreshes when done. */
> - sdesc.dwSize = sizeof(sdesc);
> - IDirectDrawSurface7_Lock(src, &lock_src, &sdesc, DDLOCK_READONLY, 0);
> - ddesc.dwSize = sizeof(ddesc);
> - IDirectDrawSurface7_Lock(iface, &lock_dst, &ddesc, DDLOCK_WRITEONLY, 0);
> - if (src == iface) {
> -
>    int pitch;
> -
> -
>    UnionRect(&lock_union, &lock_src, &lock_dst);
> -
> -
>    /* Lock the union of the two rectangles */
> -
>    IDirectDrawSurface7_Lock(iface, &lock_union, &ddesc, 0, 0);
> -
> -
>    pitch = This->surface_desc.u1.lPitch;
> -

ddesc has not been properly initialized.
you should also copy sdesc from ddesc.
> -
>    /* Since sdesc was originally copied from this surface's description, we can just reuse it */
> -
>    sdesc.lpSurface = (BYTE *)This->surface_desc.lpSurface + lock_src.top * pitch + lock_src.left * bpp; 
> -
>    ddesc.lpSurface = (BYTE *)This->surface_desc.lpSurface + lock_dst.top * pitch + lock_dst.left * bpp; 
> -
>
> } else {
> -
>    sdesc.dwSize = sizeof(sdesc);
> -
>    IDirectDrawSurface7_Lock(src, &lock_src, &sdesc, DDLOCK_READONLY, 0);
> -
>    ddesc.dwSize = sizeof(ddesc);
> -
>    IDirectDrawSurface7_Lock(iface, &lock_dst, &ddesc, DDLOCK_WRITEONLY, 0);
> -
>
> }
>
> /* Handle first the FOURCC surfaces... */
> if ((sdesc.u4.ddpfPixelFormat.dwFlags & DDPF_FOURCC) && (ddesc.u4.ddpfPixelFormat.dwFlags & DDPF_FOURCC)) {
>
> @@ -1069,7 +1094,6 @@
>    goto error;
>     }
> - bpp = GET_BPP(This->surface_desc);
> sbuf = (BYTE *) sdesc.lpSurface;
> dbuf = (BYTE *) ddesc.lpSurface;
>
> @@ -1141,8 +1165,13 @@
>     }
>
> error:
> - IDirectDrawSurface7_Unlock(iface, &lock_dst);
> - IDirectDrawSurface7_Unlock(src, &lock_src);
> - if (src == iface) {
> -
>    IDirectDrawSurface7_Unlock(iface, &lock_union);
> - } else {
> -
>    IDirectDrawSurface7_Unlock(iface, &lock_dst);
> -
>    IDirectDrawSurface7_Unlock(src, &lock_src);

You should check if src != NULL before trying to unlocking it.
> - }
> - return ret;
>
> }

DDRAW: Surface locking patch, take 2

[Matthew Mastracci]

Christian Costa wrote:
> Hi Matthew,
>
> This seems fine.
> There are some comments though, see below.

...
> > - if (src == iface) {
> > -
> >    int pitch;
> > -
> > -
> >    UnionRect(&lock_union, &lock_src, &lock_dst);
> > -
> > -
> >    /* Lock the union of the two rectangles */
> > -
> >    IDirectDrawSurface7_Lock(iface, &lock_union, &ddesc, 0, 0);
> > -
> > -
> >    pitch = This->surface_desc.u1.lPitch;
> > -
>
> ddesc has not been properly initialized.
> you should also copy sdesc from ddesc.

It actually has.  If you look higher up in the function, ddesc and sdesc 
are copied from This->surface_desc:

/* Get the surface description without locking to first compute the 
width / height */
     ddesc = This->surface_desc;
     sdesc = (ICOM_OBJECT(IDirectDrawSurfaceImpl, IDirectDrawSurface7, 
src))->surface_desc;

AFAICT, ddesc and sdesc will always be valid after this initialization.
> > - if (src == iface) {
> > -
> >    IDirectDrawSurface7_Unlock(iface, &lock_union);
> > - } else {
> > -
> >    IDirectDrawSurface7_Unlock(iface, &lock_dst);
> > -
> >    IDirectDrawSurface7_Unlock(src, &lock_src);
>
> You should check if src != NULL before trying to unlocking it.

BltFast() doesn't accept a NULL src, so this isn't necessary.  The 
function should probably check for NULL src before the surface lock 
check.  If I can figure out what the error return code should be (I 
think it's DDERR_INVALIDPARAM), I can check for a NULL src at the start 
of the function in a later patch.

Thanks for the review,
Matt.

DDRAW: Surface locking patch, take 2

[Christian Costa]

Matthew Mastracci wrote:
> Christian Costa wrote:
> > Hi Matthew,
> >
> > This seems fine.
> > There are some comments though, see below.
>
> ...
> > > - if (src == iface) {
> > > -
> > >    int pitch;
> > > -
> > > -
> > >    UnionRect(&lock_union, &lock_src, &lock_dst);
> > > -
> > > -
> > >    /* Lock the union of the two rectangles */
> > > -
> > >    IDirectDrawSurface7_Lock(iface, &lock_union, &ddesc, 0, 0);
> > > -
> > > -
> > >    pitch = This->surface_desc.u1.lPitch;
> > > -
> >
> > ddesc has not been properly initialized.
> > you should also copy sdesc from ddesc.
>
> It actually has.  If you look higher up in the function, ddesc and 
> sdesc are copied from This->surface_desc:
> /* Get the surface description without locking to first compute 
>
> the width / height */
>     ddesc = This->surface_desc;
>     sdesc = (ICOM_OBJECT(IDirectDrawSurfaceImpl, IDirectDrawSurface7, 
> src))->surface_desc;
>
> AFAICT, ddesc and sdesc will always be valid after this initialization.

Sorry! I didn't see that in the diff. I should have looked at dib.c too.
> > > - if (src == iface) {
> > > -
> > >    IDirectDrawSurface7_Unlock(iface, &lock_union);
> > > - } else {
> > > -
> > >    IDirectDrawSurface7_Unlock(iface, &lock_dst);
> > > -
> > >    IDirectDrawSurface7_Unlock(src, &lock_src);
> >
> > You should check if src != NULL before trying to unlocking it.
>
> BltFast() doesn't accept a NULL src, so this isn't necessary.  The 
> function should probably check for NULL src before the surface lock 
> check.  If I can figure out what the error return code should be (I 
> think it's DDERR_INVALIDPARAM), I can check for a NULL src at the 
> start of the function in a later patch.

Ok! Fine.
> Thanks for the review,
> Matt.