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