Re: ddraw: Allow creating back buffer for pre-DirectX 7 interfaces. (try 2)

[Oldřich Jedlička]

Hi all,

I've made one discovery - the explicit BACKBUFFER creation (as shown in logs) 
works only for IID_IDirectDraw interface, not for any newer (on my Windows 
XP). I will do some more testing, add tests into a separate patch and send it 
afterwards.

Regards,
Oldrich.

On Wednesday 21 July 2010 20:51:49 Oldřich Jedlička wrote:
> Old DirectX interfaces allowed creation of explicit back buffers, so move
> the restrictive check to DirectX 7 implementation.
> This fixes bug #9008.
>
> dlls/ddraw/ddraw.c |   35 ++++++++++++++++++++++++++++-------
>  1 files changed, 28 insertions(+), 7 deletions(-)
>
> diff --git a/dlls/ddraw/ddraw.c b/dlls/ddraw/ddraw.c
> index e55ac5b..9f13c8b 100644
> --- a/dlls/ddraw/ddraw.c
> +++ b/dlls/ddraw/ddraw.c
> @@ -2967,7 +2967,7 @@ static HRESULT
> ddraw_create_gdi_swapchain(IDirectDrawImpl *ddraw, IDirectDrawSur * 
> DDERR_* if an error occurs
> -
>
> ***/ -static HRESULT WINAPI ddraw7_CreateSurface(IDirectDraw7 *iface,
> +static HRESULT WINAPI CreateSurface(IDirectDraw7 *iface,
>          DDSURFACEDESC2 *DDSD, IDirectDrawSurface7 **Surf, IUnknown
> *UnkOuter) {
>      IDirectDrawImpl *This = (IDirectDrawImpl *)iface;
> @@ -3030,8 +3030,8 @@ static HRESULT WINAPI
> ddraw7_CreateSurface(IDirectDraw7 *iface, return DDERR_NOEXCLUSIVEMODE;
>      }
> - if(DDSD->ddsCaps.dwCaps & (DDSCAPS_FRONTBUFFER | DDSCAPS_BACKBUFFER))
>
> { -        WARN("Application tried to create an explicit front or back
> buffer\n"); +    if(DDSD->ddsCaps.dwCaps & DDSCAPS_FRONTBUFFER) {
> -
>    WARN("Application tried to create an explicit front buffer\n");
>    LeaveCriticalSection(&ddraw_cs);
>    return DDERR_INVALIDCAPS;
> }
>
> @@ -3391,6 +3391,27 @@ static HRESULT WINAPI
> ddraw7_CreateSurface(IDirectDraw7 *iface, return hr;
>  }
>
> +static HRESULT WINAPI ddraw7_CreateSurface(IDirectDraw7 *iface,
> -
>    DDSURFACEDESC2 *surface_desc, IDirectDrawSurface7 **surface,
>
> IUnknown *outer_unknown) +{
> - TRACE("iface %p, surface_desc %p, surface %p, outer_unknown %p.\n",
> -
>        iface, surface_desc, surface, outer_unknown);
> -
> - if(surface_desc->ddsCaps.dwCaps & (DDSCAPS_FRONTBUFFER |
>
> DDSCAPS_BACKBUFFER)) +    {
> -
>    if (TRACE_ON(ddraw))
> -
>    {
> -
>        TRACE(" (%p) Requesting surface desc :\n", iface);
> -
>        DDRAW_dump_surface_desc(surface_desc);
> -
>    }
> -
> -
>    WARN("Application tried to create an explicit front or back
>
> buffer\n"); +        return DDERR_INVALIDCAPS;
> - }
> -
> - return CreateSurface(iface, surface_desc, surface, outer_unknown);
>
> +}
> -
>
> static HRESULT WINAPI ddraw4_CreateSurface(IDirectDraw4 *iface,
>          DDSURFACEDESC2 *surface_desc, IDirectDrawSurface4 **surface,
> IUnknown *outer_unknown) {
> @@ -3401,7 +3422,7 @@ static HRESULT WINAPI
> ddraw4_CreateSurface(IDirectDraw4 *iface, TRACE("iface %p, surface_desc
> %p, surface %p, outer_unknown %p.\n", iface, surface_desc, surface,
> outer_unknown);
> - hr = ddraw7_CreateSurface((IDirectDraw7 *)ddraw, surface_desc,
>
> (IDirectDrawSurface7 **)surface, outer_unknown); +    hr =
> CreateSurface((IDirectDraw7 *)ddraw, surface_desc, (IDirectDrawSurface7
> **)surface, outer_unknown); impl = (IDirectDrawSurfaceImpl *)*surface;
>      if (SUCCEEDED(hr) && impl)
>      {
> @@ -3425,7 +3446,7 @@ static HRESULT WINAPI
> ddraw3_CreateSurface(IDirectDraw3 *iface, TRACE("iface %p, surface_desc
> %p, surface %p, outer_unknown %p.\n", iface, surface_desc, surface,
> outer_unknown);
> - hr = ddraw7_CreateSurface((IDirectDraw7 *)ddraw, (DDSURFACEDESC2
>
> *)surface_desc, &surface7, outer_unknown); +    hr =
> CreateSurface((IDirectDraw7 *)ddraw, (DDSURFACEDESC2 *)surface_desc,
> &surface7, outer_unknown); if (FAILED(hr))
>      {
>          *surface = NULL;
> @@ -3453,7 +3474,7 @@ static HRESULT WINAPI
> ddraw2_CreateSurface(IDirectDraw2 *iface, TRACE("iface %p, surface_desc
> %p, surface %p, outer_unknown %p.\n", iface, surface_desc, surface,
> outer_unknown);
> - hr = ddraw7_CreateSurface((IDirectDraw7 *)ddraw, (DDSURFACEDESC2
>
> *)surface_desc, &surface7, outer_unknown); +    hr =
> CreateSurface((IDirectDraw7 *)ddraw, (DDSURFACEDESC2 *)surface_desc,
> &surface7, outer_unknown); if (FAILED(hr))
>      {
>          *surface = NULL;
> @@ -3483,7 +3504,7 @@ static HRESULT WINAPI
> ddraw1_CreateSurface(IDirectDraw *iface, /* Remove front buffer flag, this
> causes failure in v7, and its added to normal * primaries anyway. */
>      surface_desc->ddsCaps.dwCaps &= ~DDSCAPS_FRONTBUFFER;
> - hr = ddraw7_CreateSurface((IDirectDraw7 *)ddraw, (DDSURFACEDESC2
>
> *)surface_desc, &surface7, outer_unknown); +    hr =
> CreateSurface((IDirectDraw7 *)ddraw, (DDSURFACEDESC2 *)surface_desc,
> &surface7, outer_unknown); if (FAILED(hr))
>      {
>          *surface = NULL;

ddraw: Allow creating back buffer for pre-DirectX 7 interfaces. (try 2)

[Roderick Colenbrander]

Hi Oldřich,

Not to discourage you, but in Wine tests serve as proof to show that a
patch is right, so you should first get all the tests you want in. The
bug should be fixed afterward. It is likely that the test will reveal
new issues.

Like I mentioned yesterday, checking memory location related
capabilities in the surface descriptor of the created surface might be
useful. For instance It might show that a standalone backbuffer is not
created in video memory. If that's the case then it is something
different from a backbuffer on the swapchain.

Roderick

2010/7/21 Oldřich Jedlička :
> Hi all,
>
> I've made one discovery - the explicit BACKBUFFER creation (as shown in logs)
> works only for IID_IDirectDraw interface, not for any newer (on my Windows
> XP). I will do some more testing, add tests into a separate patch and send it
> afterwards.
>
> Regards,
> Oldrich.
>
> On Wednesday 21 July 2010 20:51:49 Oldřich Jedlička wrote:
> > Old DirectX interfaces allowed creation of explicit back buffers, so move
> > the restrictive check to DirectX 7 implementation.
> > This fixes bug #9008.
> >
> > dlls/ddraw/ddraw.c |   35 ++++++++++++++++++++++++++++-------
> >  1 files changed, 28 insertions(+), 7 deletions(-)
> >
> > diff --git a/dlls/ddraw/ddraw.c b/dlls/ddraw/ddraw.c
> > index e55ac5b..9f13c8b 100644
> > --- a/dlls/ddraw/ddraw.c
> > +++ b/dlls/ddraw/ddraw.c
> > @@ -2967,7 +2967,7 @@ static HRESULT
> > ddraw_create_gdi_swapchain(IDirectDrawImpl *ddraw, IDirectDrawSur *
> > DDERR_* if an error occurs
> >   *
> >
> > ***/ -static HRESULT WINAPI ddraw7_CreateSurface(IDirectDraw7 *iface,
> > +static HRESULT WINAPI CreateSurface(IDirectDraw7 *iface,
> >          DDSURFACEDESC2 *DDSD, IDirectDrawSurface7 **Surf, IUnknown
> > *UnkOuter) {
> >      IDirectDrawImpl *This = (IDirectDrawImpl *)iface;
> > @@ -3030,8 +3030,8 @@ static HRESULT WINAPI
> > ddraw7_CreateSurface(IDirectDraw7 *iface, return DDERR_NOEXCLUSIVEMODE;
> >      }
> > - if(DDSD->ddsCaps.dwCaps & (DDSCAPS_FRONTBUFFER | DDSCAPS_BACKBUFFER))
> >
> > { -        WARN("Application tried to create an explicit front or back
> > buffer\n"); +    if(DDSD->ddsCaps.dwCaps & DDSCAPS_FRONTBUFFER) {
> > - WARN("Application tried to create an explicit front buffer\n");
> >
> > LeaveCriticalSection(&ddraw_cs);
> >          return DDERR_INVALIDCAPS;
> >      }
> > @@ -3391,6 +3391,27 @@ static HRESULT WINAPI
> > ddraw7_CreateSurface(IDirectDraw7 *iface, return hr;
> >  }
> >
> > +static HRESULT WINAPI ddraw7_CreateSurface(IDirectDraw7 *iface,
> > - DDSURFACEDESC2 *surface_desc, IDirectDrawSurface7 **surface,
> >
> > IUnknown *outer_unknown) +{
> > - TRACE("iface %p, surface_desc %p, surface %p, outer_unknown %p.\n",
> > - iface, surface_desc, surface, outer_unknown);
> > -
> > - if(surface_desc->ddsCaps.dwCaps & (DDSCAPS_FRONTBUFFER |
> >
> > DDSCAPS_BACKBUFFER)) +    {
> > - if (TRACE_ON(ddraw))
> > - {
> > - TRACE(" (%p) Requesting surface desc :\n", iface);
> > - DDRAW_dump_surface_desc(surface_desc);
> > - }
> > -
> > - WARN("Application tried to create an explicit front or back
> >
> > buffer\n"); +        return DDERR_INVALIDCAPS;
> > - }
> > -
> > - return CreateSurface(iface, surface_desc, surface, outer_unknown);
> >
> > +}
> > -
> >
> > static HRESULT WINAPI ddraw4_CreateSurface(IDirectDraw4 *iface,
> >          DDSURFACEDESC2 *surface_desc, IDirectDrawSurface4 **surface,
> > IUnknown *outer_unknown) {
> > @@ -3401,7 +3422,7 @@ static HRESULT WINAPI
> > ddraw4_CreateSurface(IDirectDraw4 *iface, TRACE("iface %p, surface_desc
> > %p, surface %p, outer_unknown %p.\n", iface, surface_desc, surface,
> > outer_unknown);
> > - hr = ddraw7_CreateSurface((IDirectDraw7 *)ddraw, surface_desc,
> >
> > (IDirectDrawSurface7 **)surface, outer_unknown); +    hr =
> > CreateSurface((IDirectDraw7 *)ddraw, surface_desc, (IDirectDrawSurface7
> > **)surface, outer_unknown); impl = (IDirectDrawSurfaceImpl *)*surface;
> >      if (SUCCEEDED(hr) && impl)
> >      {
> > @@ -3425,7 +3446,7 @@ static HRESULT WINAPI
> > ddraw3_CreateSurface(IDirectDraw3 *iface, TRACE("iface %p, surface_desc
> > %p, surface %p, outer_unknown %p.\n", iface, surface_desc, surface,
> > outer_unknown);
> > - hr = ddraw7_CreateSurface((IDirectDraw7 *)ddraw, (DDSURFACEDESC2
> >
> > *)surface_desc, &surface7, outer_unknown); +    hr =
> > CreateSurface((IDirectDraw7 *)ddraw, (DDSURFACEDESC2 *)surface_desc,
> > &surface7, outer_unknown); if (FAILED(hr))
> >      {
> >          *surface = NULL;
> > @@ -3453,7 +3474,7 @@ static HRESULT WINAPI
> > ddraw2_CreateSurface(IDirectDraw2 *iface, TRACE("iface %p, surface_desc
> > %p, surface %p, outer_unknown %p.\n", iface, surface_desc, surface,
> > outer_unknown);
> > - hr = ddraw7_CreateSurface((IDirectDraw7 *)ddraw, (DDSURFACEDESC2
> >
> > *)surface_desc, &surface7, outer_unknown); +    hr =
> > CreateSurface((IDirectDraw7 *)ddraw, (DDSURFACEDESC2 *)surface_desc,
> > &surface7, outer_unknown); if (FAILED(hr))
> >      {
> >          *surface = NULL;
> > @@ -3483,7 +3504,7 @@ static HRESULT WINAPI
> > ddraw1_CreateSurface(IDirectDraw *iface, /* Remove front buffer flag, this
> > causes failure in v7, and its added to normal * primaries anyway. */
> >      surface_desc->ddsCaps.dwCaps &= ~DDSCAPS_FRONTBUFFER;
> > - hr = ddraw7_CreateSurface((IDirectDraw7 *)ddraw, (DDSURFACEDESC2
> >
> > *)surface_desc, &surface7, outer_unknown); +    hr =
> > CreateSurface((IDirectDraw7 *)ddraw, (DDSURFACEDESC2 *)surface_desc,
> > &surface7, outer_unknown); if (FAILED(hr))
> >      {
> >          *surface = NULL;

ddraw: Allow creating back buffer for pre-DirectX 7 interfaces. (try 2)

[Oldřich Jedlička]

Hi Roderick,

On Thursday 22 July 2010 00:23:46 Roderick Colenbrander wrote:
> Hi Oldřich,
>
> Not to discourage you, but in Wine tests serve as proof to show that a
> patch is right, so you should first get all the tests you want in. The
> bug should be fixed afterward. It is likely that the test will reveal
> new issues.

No problem. I've started with this implementation to test if the game works 
and to check if the way how I've fixed it can be done that way according to 
wine developpers. That would be no fun otherwise.

So if you have nothing against the way how the patch tries to fix the 
BACKBUFFER creation, we can focus more on the background (see if the fix is 
sufficient).
> Like I mentioned yesterday, checking memory location related
> capabilities in the surface descriptor of the created surface might be
> useful. For instance It might show that a standalone backbuffer is not
> created in video memory. If that's the case then it is something
> different from a backbuffer on the swapchain.

The function GetSurfaceDesc for DDSCAPS_BACKBUFFER-only surface on Windows XP 
SP3 returns

DDSCAPS_BACKBUFFER | DDSCAPS_VIDEOMEMORY | DDSCAPS_LOCALVIDMEM

This conforms to logic in method ddraw_create_surface that adds the two flags 
if they are missing.

I have prepared one test on which I tried the DirectX CreateSurface logic 
specifically for BACKBUFFERs on different DirectX interface versions. I used 
similar commands as in SurfaceCapsTest() in ddraw/tests/dsurface.c.

Is there something more you see that needs tests to verify if it is the same 
as on Windows? As I wrote I'm a newbie in the DirectX programming, so I will 
definitely need some help.

This is the only thing that blocks the game from running, so from the bug 
perspective only the CreateSurface for DirectX1 interface needs fixing, because 
the other operation - blitting - works (and other flip-chain modification 
methods are not used and are unsupported on DirectX anyway).

When I have the CreateSurface test ready with all the DirectX interface 
versions, I will send it. There is also more to fix - the usage of SURFACEDESC 
and SURFACEDESC2. Windows don't accept the former version on the newer 
interfaces.

For now it looks I will send 4 patches: CreateSurface BACKBUFFER tests as the 
first one; tests of usage of SURFACEDESC/2 as the second one; fix of BACKBUFFER 
creation as third; added checking of surface descriptor in CreateSurface as 
the last one.

Cheers,
Oldrich.
> Roderick
>
> 2010/7/21 Oldřich Jedlička :
> > Hi all,
> >
> > I've made one discovery - the explicit BACKBUFFER creation (as shown in
> > logs) works only for IID_IDirectDraw interface, not for any newer (on my
> > Windows XP). I will do some more testing, add tests into a separate
> > patch and send it afterwards.
> >
> > Regards,
> > Oldrich.
> >
> > On Wednesday 21 July 2010 20:51:49 Oldřich Jedlička wrote:
> > > Old DirectX interfaces allowed creation of explicit back buffers, so
> > > move the restrictive check to DirectX 7 implementation.
> > > This fixes bug #9008.
> > >
> > > dlls/ddraw/ddraw.c |   35 ++++++++++++++++++++++++++++-------
> > >  1 files changed, 28 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/dlls/ddraw/ddraw.c b/dlls/ddraw/ddraw.c
> > > index e55ac5b..9f13c8b 100644
> > > --- a/dlls/ddraw/ddraw.c
> > > +++ b/dlls/ddraw/ddraw.c
> > > @@ -2967,7 +2967,7 @@ static HRESULT
> > > ddraw_create_gdi_swapchain(IDirectDrawImpl *ddraw, IDirectDrawSur *
> > > DDERR_* if an error occurs
> > > -
> > >
> > > ** ***/ -static HRESULT WINAPI ddraw7_CreateSurface(IDirectDraw7 *iface,
> > > +static HRESULT WINAPI CreateSurface(IDirectDraw7 *iface,
> > >          DDSURFACEDESC2 *DDSD, IDirectDrawSurface7 **Surf, IUnknown
> > > *UnkOuter) {
> > >      IDirectDrawImpl *This = (IDirectDrawImpl *)iface;
> > > @@ -3030,8 +3030,8 @@ static HRESULT WINAPI
> > > ddraw7_CreateSurface(IDirectDraw7 *iface, return DDERR_NOEXCLUSIVEMODE;
> > >      }
> > > - if(DDSD->ddsCaps.dwCaps & (DDSCAPS_FRONTBUFFER |
> > >
> > > DDSCAPS_BACKBUFFER)) { -        WARN("Application tried to create an
> > > explicit front or back buffer\n"); +    if(DDSD->ddsCaps.dwCaps &
> > > DDSCAPS_FRONTBUFFER) { +        WARN("Application tried to create an
> > > explicit front buffer\n"); LeaveCriticalSection(&ddraw_cs);
> > >          return DDERR_INVALIDCAPS;
> > >      }
> > > @@ -3391,6 +3391,27 @@ static HRESULT WINAPI
> > > ddraw7_CreateSurface(IDirectDraw7 *iface, return hr;
> > >  }
> > >
> > > +static HRESULT WINAPI ddraw7_CreateSurface(IDirectDraw7 *iface,
> > > -
> > >    DDSURFACEDESC2 *surface_desc, IDirectDrawSurface7 **surface,
> > >
> > > IUnknown *outer_unknown) +{
> > > - TRACE("iface %p, surface_desc %p, surface %p, outer_unknown %p.\n",
> > > -
> > >        iface, surface_desc, surface, outer_unknown);
> > > -
> > > - if(surface_desc->ddsCaps.dwCaps & (DDSCAPS_FRONTBUFFER |
> > >
> > > DDSCAPS_BACKBUFFER)) +    {
> > > -
> > >    if (TRACE_ON(ddraw))
> > > -
> > >    {
> > > -
> > >        TRACE(" (%p) Requesting surface desc :\n", iface);
> > > -
> > >        DDRAW_dump_surface_desc(surface_desc);
> > > -
> > >    }
> > > -
> > > -
> > >    WARN("Application tried to create an explicit front or back
> > >
> > > buffer\n"); +        return DDERR_INVALIDCAPS;
> > > - }
> > > -
> > > - return CreateSurface(iface, surface_desc, surface, outer_unknown);
> > >
> > > +}
> > > -
> > >
> > > static HRESULT WINAPI ddraw4_CreateSurface(IDirectDraw4 *iface,
> > >          DDSURFACEDESC2 *surface_desc, IDirectDrawSurface4 **surface,
> > > IUnknown *outer_unknown) {
> > > @@ -3401,7 +3422,7 @@ static HRESULT WINAPI
> > > ddraw4_CreateSurface(IDirectDraw4 *iface, TRACE("iface %p, surface_desc
> > > %p, surface %p, outer_unknown %p.\n", iface, surface_desc, surface,
> > > outer_unknown);
> > > - hr = ddraw7_CreateSurface((IDirectDraw7 *)ddraw, surface_desc,
> > >
> > > (IDirectDrawSurface7 **)surface, outer_unknown); +    hr =
> > > CreateSurface((IDirectDraw7 *)ddraw, surface_desc, (IDirectDrawSurface7
> > > **)surface, outer_unknown); impl = (IDirectDrawSurfaceImpl *)*surface;
> > >      if (SUCCEEDED(hr) && impl)
> > >      {
> > > @@ -3425,7 +3446,7 @@ static HRESULT WINAPI
> > > ddraw3_CreateSurface(IDirectDraw3 *iface, TRACE("iface %p, surface_desc
> > > %p, surface %p, outer_unknown %p.\n", iface, surface_desc, surface,
> > > outer_unknown);
> > > - hr = ddraw7_CreateSurface((IDirectDraw7 *)ddraw, (DDSURFACEDESC2
> > >
> > > *)surface_desc, &surface7, outer_unknown); +    hr =
> > > CreateSurface((IDirectDraw7 *)ddraw, (DDSURFACEDESC2 *)surface_desc,
> > > &surface7, outer_unknown); if (FAILED(hr))
> > >      {
> > >          *surface = NULL;
> > > @@ -3453,7 +3474,7 @@ static HRESULT WINAPI
> > > ddraw2_CreateSurface(IDirectDraw2 *iface, TRACE("iface %p, surface_desc
> > > %p, surface %p, outer_unknown %p.\n", iface, surface_desc, surface,
> > > outer_unknown);
> > > - hr = ddraw7_CreateSurface((IDirectDraw7 *)ddraw, (DDSURFACEDESC2
> > >
> > > *)surface_desc, &surface7, outer_unknown); +    hr =
> > > CreateSurface((IDirectDraw7 *)ddraw, (DDSURFACEDESC2 *)surface_desc,
> > > &surface7, outer_unknown); if (FAILED(hr))
> > >      {
> > >          *surface = NULL;
> > > @@ -3483,7 +3504,7 @@ static HRESULT WINAPI
> > > ddraw1_CreateSurface(IDirectDraw *iface, /* Remove front buffer flag,
> > > this causes failure in v7, and its added to normal * primaries anyway.
> > > */ surface_desc->ddsCaps.dwCaps &= ~DDSCAPS_FRONTBUFFER;
> > > - hr = ddraw7_CreateSurface((IDirectDraw7 *)ddraw, (DDSURFACEDESC2
> > >
> > > *)surface_desc, &surface7, outer_unknown); +    hr =
> > > CreateSurface((IDirectDraw7 *)ddraw, (DDSURFACEDESC2 *)surface_desc,
> > > &surface7, outer_unknown); if (FAILED(hr))
> > >      {
> > >          *surface = NULL;