Re: 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.