Re: Regression (SimCity 3000 is Broke )

[Tony Lambregts]

Matthew Mastracci wrote:
> > I tried putting in the patch as you demonstrated and I could not get 
> > it to work (get the proper behavior from SimCity 3000). That is why I 
> > came back with the patch that I did. It is possible I did not 
> > understand your intent so I would be happy to try a real patch from you.
>
> I ran the Sim City 3000 demo and traced the problem down to _Blt() 
> failing to release a lock if an invalid rectangle is passed to it.  The 
> attached patch should fix a number of correctness problems, including 
> this one.  Give it a shot and let me know if it fixes things for you.
>
> I checked both SC3000 and Picasa 2 and both continue to work after this 
> patch is applied.
>
> Matt.

works much better for me (insted of the single BOOL locked). After looking at the code more I was wondering why you could not have 
used lastlock type for locking. If that is not possible I would suggest that instead of locked_read and locked_write you could use 
DWORD locktype.
> Index: ddraw_private.h
>
> RCS file: /home/wine/wine/dlls/ddraw/ddraw_private.h,v
> retrieving revision 1.46
> diff -u -r1.46 ddraw_private.h
> --- ddraw_private.h	7 Mar 2005 12:23:34 -0000	1.46
> +++ ddraw_private.h	16 Mar 2005 03:29:34 -0000
> @@ -277,7 +277,8 @@
>      RECT lastlockrect;
>      DWORD lastlocktype;
>      BOOL dc_in_use;
> - BOOL locked;
> -
>
> BOOL locked_read;
> -
>
> BOOL locked_write;
>
> HRESULT (*duplicate_surface)(IDirectDrawSurfaceImpl* src,
> 		 LPDIRECTDRAWSURFACE7* dst);
> Index: dsurface/dib.c
>
> RCS file: /home/wine/wine/dlls/ddraw/dsurface/dib.c,v
> retrieving revision 1.46
> diff -u -r1.46 dib.c
> --- dsurface/dib.c	10 Mar 2005 11:13:11 -0000	1.46
> +++ dsurface/dib.c	16 Mar 2005 03:29:37 -0000
> @@ -255,7 +255,8 @@
>
> This->surface_desc.dwFlags |= DDSD_LPSURFACE;
> - if (This->surface_desc.u4.ddpfPixelFormat.dwFlags & DDPF_FOURCC) {
> -
>    /* Ensure that DDSD_PITCH is respected for DDPF_FOURCC surfaces too */
> - if (This->surface_desc.u4.ddpfPixelFormat.dwFlags & DDPF_FOURCC && !(This->surface_desc.dwFlags & DDSD_PITCH)) {
>   This->surface_desc.lpSurface
> = VirtualAlloc(NULL, This->surface_desc.u1.dwLinearSize, MEM_COMMIT, PAGE_READWRITE);
>   This->surface_desc.dwFlags |= DDSD_LINEARSIZE;
>
> @@ -383,7 +383,7 @@
>  }
>
> /* This is used to factorize the decompression between the Blt and BltFast code */
> -static void DoDXTCDecompression(const DDSURFACEDESC2 *sdesc, const DDSURFACEDESC2 *ddesc)
> +static BOOL DoDXTCDecompression(const DDSURFACEDESC2 *sdesc, const DDSURFACEDESC2 *ddesc)
>  {
>      DWORD rs,rb,rm;
>      DWORD gs,gb,gm;
> @@ -394,7 +394,7 @@
>    /* FIXME: We may fake this by rendering the texture into the framebuffer using OpenGL functions and reading back
>     *        the framebuffer. This will be slow and somewhat ugly. */ 
>    FIXME("Manual S3TC decompression is not supported in native mode\n");
> - return;
> -
>
> return FALSE;
> }
>
> rm = ddesc->u4.ddpfPixelFormat.u2.dwRBitMask;
>
> @@ -474,6 +474,9 @@
>    	else
>    	    *((DWORD*)(dst+y*pitch+x*(is16?2:4))) = pixel;
>        }
> - } else {
> -
>    /* not handled */
> -
>    return FALSE;
> }
>
> #if 0 /* Usefull for debugging */
>      {
> @@ -486,6 +489,8 @@
>    fclose(f);
>      }
>  #endif
> -
> - return TRUE;
>
> }
>
> HRESULT WINAPI
> @@ -514,7 +519,7 @@
>    }
>      }
> - if ((This->locked) || ((src != NULL) && (((IDirectDrawSurfaceImpl *)src)->locked))) {
> - if ((This->locked_write) || ((src != NULL) && (((IDirectDrawSurfaceImpl *)src)->locked_read))) {
>     WARN(" Surface is busy, returning DDERR_SURFACEBUSY\n");
>     return DDERR_SURFACEBUSY;
> }
>
> @@ -545,10 +550,11 @@
>  if ((sdesc.u4.ddpfPixelFormat.dwFlags & DDPF_FOURCC) &&
>
> (!(ddesc.u4.ddpfPixelFormat.dwFlags & DDPF_FOURCC))) {
> - DoDXTCDecompression(&sdesc, &ddesc);
> - goto release;
> -
>    /* If DoDXTCDecompression handled this blit, we're done */
> -
>    if (DoDXTCDecompression(&sdesc, &ddesc))
> -
>    goto release;
> }
> -
> - if (rdst) {
>  memcpy(&xdst,rdst,sizeof(xdst));
>    } else {
>
> @@ -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
>
> @@ -990,7 +998,7 @@
>      TRACE(" srcrect: NULL\n");
>      }
> - if ((This->locked) || ((src != NULL) && (((IDirectDrawSurfaceImpl *)src)->locked))) {
> - if ((This->locked_write) || ((src != NULL) && (((IDirectDrawSurfaceImpl *)src)->locked_read))) {
>     WARN(" Surface is busy, returning DDERR_SURFACEBUSY\n");
>     return DDERR_SURFACEBUSY;
> }
>
> @@ -1044,8 +1052,11 @@
>      lock_dst.bottom = dsty + h;
>  /* We need to lock the surfaces, or we won't get refreshes when done. */
> - DD_STRUCT_INIT(&ddesc);
> - DD_STRUCT_INIT(&sdesc);
> - sdesc.dwSize = sizeof(sdesc);
> - IDirectDrawSurface7_Lock(src, &lock_src, &sdesc, DDLOCK_READONLY, 0);
> - if (src) IDirectDrawSurface7_Lock(src, &lock_src, &sdesc, DDLOCK_READONLY, 0);
> ddesc.dwSize = sizeof(ddesc);
> IDirectDrawSurface7_Lock(iface, &lock_dst, &ddesc, DDLOCK_WRITEONLY, 0);
> Index: dsurface/main.c
>
> RCS file: /home/wine/wine/dlls/ddraw/dsurface/main.c,v
> retrieving revision 1.64
> diff -u -r1.64 main.c
> --- dsurface/main.c	7 Mar 2005 12:23:34 -0000	1.64
> +++ dsurface/main.c	16 Mar 2005 03:29:39 -0000
> @@ -1095,7 +1095,9 @@
>      }
>  /* If the surface is already locked, return busy */
> - if (This->locked) {
> - if ( ((flags & DDLOCK_READONLY) && This->locked_read)
> -
>    || ((flags & DDLOCK_WRITEONLY) && This->locked_write) 
> -
>    || ((flags & (DDLOCK_READONLY|DDLOCK_WRITEONLY)) == 0 && (This->locked_read || This->locked_write)) ) {
>    WARN(" Surface is busy, returning DDERR_SURFACEBUSY\n");
>    return DDERR_SURFACEBUSY;
> }
>
> @@ -1148,7 +1150,8 @@
>    This->lock_update(This, NULL, flags);
>      }
> - This->locked = TRUE;
> -
>
> This->locked_read = (flags & DDLOCK_WRITEONLY) == 0;
> -
>
> This->locked_write = (flags & DDLOCK_READONLY) == 0;
>
> TRACE("locked surface returning description : \n");
> if (TRACE_ON(ddraw)) DDRAW_dump_surface_desc(pDDSD);
>
> @@ -1425,12 +1428,12 @@
>  TRACE("(%p)->Unlock(%p)\n",This,pRect);
>
> - if (!This->locked) {
> - if (!This->locked_read && !This->locked_write) {
>     WARN("Surface not locked - returing DDERR_NOTLOCKED\n");
>     return DDERR_NOTLOCKED;
> }
> - This->locked = FALSE;
> - This->locked_read = This->locked_write = FALSE;
> This->unlock_update(This, pRect);
> if (This->aux_unlock)
> This->aux_unlock(This->aux_ctx, This->aux_data, pRect);