Re: [DSOUND] little patch

[Robert Reif]

Raphael wrote:
> Hi
>
> Changelog:
> - try to have better match caps with hw caps so games can active more hw
>
> support
>
> Regards,
> Raphael
> - This->lock.DebugInfo->Spare[1] = 0;
> - /*This->lock.DebugInfo->Spare[1] = 0;*/
> DeleteCriticalSection(&(This->lock));

You have to zero this out or DeleteCriticalSection will try
to delete that static memory.
> - if (pDS->drvcaps.dwFlags | DSCAPS_PRIMARYMONO)  pDS->pwfx->nChannels = 1;
> - if (pDS->drvcaps.dwFlags | DSCAPS_PRIMARY16BIT) pDS->pwfx->wBitsPerSample = 16;
> - pDS->pwfx->nBlockAlign     = pDS->pwfx->wBitsPerSample * pDS->pwfx->nChannels / 8;
> - pDS->pwfx->nAvgBytesPerSec = pDS->pwfx->nSamplesPerSec * pDS->pwfx->nBlockAlign;
> -

You can't do this and be compatible with windows.
> +#ifndef __WINE_DSOUND_PRIVATE_H
> +#define __WINE_DSOUND_PRIVATE_H

You can't include this more than once because there is only one .h file
so it's not really necessary.
> +#define WINE_SET_CS_NAME(cs, name) (cs)->DebugInfo->Spare[1] = (DWORD) name
> +#define WINE_GET_CS_NAME(cs) (const char*) (cs)->lock.DebugInfo->Spare[1]

This is a wine specific trick and should probably be put somewhere global so
all users can be consistent.  The name should have DEBUG in it somewhere.

[DSOUND] little patch

[Raphael]

Hi

On Friday 11 March 2005 03:40, Robert Reif wrote:
> Raphael wrote:
> > Hi
> >
> > Changelog:
> > - try to have better match caps with hw caps so games can active more hw
> >
> > support
> >
> > Regards,
> > Raphael
> > - This->lock.DebugInfo->Spare[1] = 0;
> > - /*This->lock.DebugInfo->Spare[1] = 0;*/
> > DeleteCriticalSection(&(This->lock));
>
> You have to zero this out or DeleteCriticalSection will try
> to delete that static memory.

How ?
At opposite it produce DebugInfo leaks
as seen in RtlDeleteCriticalSection (dlls/ntdll/critsection.c line 197)
<snip>
    if (crit->DebugInfo)
    {
        /* only free the ones we made in here */
        if (!crit->DebugInfo->Spare[1])
        {
            RtlFreeHeap( GetProcessHeap(), 0, crit->DebugInfo );
            crit->DebugInfo = NULL;
        }
    }
<snip>
i will provide a WINE_DEBUG_UNSET_CS_NAME for this problem
> > - if (pDS->drvcaps.dwFlags | DSCAPS_PRIMARYMONO)  pDS->pwfx->nChannels
> >
> > = 1; +    if (pDS->drvcaps.dwFlags | DSCAPS_PRIMARY16BIT)
> > pDS->pwfx->wBitsPerSample = 16; +    pDS->pwfx->nBlockAlign     =
> > pDS->pwfx->wBitsPerSample * pDS->pwfx->nChannels / 8; +   
> > pDS->pwfx->nAvgBytesPerSec = pDS->pwfx->nSamplesPerSec *
> > pDS->pwfx->nBlockAlign; +
>
> You can't do this and be compatible with windows.

How ?
Since i have this code (ie use better caps from my sound card)
i have few latency problems on dsound (ie fewer buffer underruns)

I don't think windows always use sound cards as 22Kh mono channels
> > +#ifndef __WINE_DSOUND_PRIVATE_H
> > +#define __WINE_DSOUND_PRIVATE_H
>
> You can't include this more than once because there is only one .h file
> so it's not really necessary.

Yes, but it cost nothing to do
and its more robust
> > +#define WINE_SET_CS_NAME(cs, name) (cs)->DebugInfo->Spare[1] = (DWORD)
> > name +#define WINE_GET_CS_NAME(cs) (const char*)
> > (cs)->lock.DebugInfo->Spare[1]
>
> This is a wine specific trick and should probably be put somewhere global
> so all users can be consistent.  The name should have DEBUG in it
> somewhere.

Yes, it should be better
but i prefered to centralize it on dsound before moving it to wine/debug.h

Regards,
Raphael

[DSOUND] little patch

[Robert Reif]

Raphael wrote:
> > > - This->lock.DebugInfo->Spare[1] = 0;
> > > - /*This->lock.DebugInfo->Spare[1] = 0;*/
> > > DeleteCriticalSection(&(This->lock));
> >
> > You have to zero this out or DeleteCriticalSection will try
> > to delete that static memory.
>
> How ?
> At opposite it produce DebugInfo leaks
> as seen in RtlDeleteCriticalSection (dlls/ntdll/critsection.c line 197)
>
> <snip>
>    if (crit->DebugInfo)
>    {
>        /* only free the ones we made in here */
>        if (!crit->DebugInfo->Spare[1])
>        {
>            RtlFreeHeap( GetProcessHeap(), 0, crit->DebugInfo );
>            crit->DebugInfo = NULL;
>        }
>    }

Actually it will leak DebugInfo if DebugInfo->Spare[1] is not 0.
> <snip>
> i will provide a WINE_DEBUG_UNSET_CS_NAME for this problem

...
> > > +#define WINE_SET_CS_NAME(cs, name) (cs)->DebugInfo->Spare[1] = (DWORD)
> > > name +#define WINE_GET_CS_NAME(cs) (const char*)
> > > (cs)->lock.DebugInfo->Spare[1]
> >
> > This is a wine specific trick and should probably be put somewhere global
> > so all users can be consistent.  The name should have DEBUG in it
> > somewhere.
>
> Yes, it should be better
> but i prefered to centralize it on dsound before moving it to wine/debug.h
>
> Regards,
> Raphael

This is Alexandre's trick which I borrowed because it is very helpful in 
debugging lock problems.
It would be nice to formalize this and make it available in a more 
general way but that is Alexandre's call.

[DSOUND] little patch

[Robert Reif]

> > > > +#define WINE_SET_CS_NAME(cs, name) (cs)->DebugInfo->Spare[1] = 
> > > > (DWORD)
> > > > name +#define WINE_GET_CS_NAME(cs) (const char*)
> > > > (cs)->lock.DebugInfo->Spare[1]
> > >
> > > This is a wine specific trick and should probably be put somewhere 
> > > global
> > > so all users can be consistent.  The name should have DEBUG in it
> > > somewhere.
> >
> > Yes, it should be better
> > but i prefered to centralize it on dsound before moving it to 
> > wine/debug.h
> >
> > Regards,
> > Raphael
>
> This is Alexandre's trick which I borrowed because it is very helpful 
> in debugging lock problems.
> It would be nice to formalize this and make it available in a more 
> general way but that is Alexandre's call.

Something like this plus a lot of documentation explaining what is going on.

I think this should go in debug.h but there is only trace stuff in there 
so I put it in library.h.

Compile tested only.

Index: dlls/dsound/buffer.c
===================================================================
RCS file: /home/wine/wine/dlls/dsound/buffer.c,v
retrieving revision 1.47
diff -u -p -r1.47 buffer.c
--- dlls/dsound/buffer.c	16 Feb 2005 16:09:03 -0000	1.47
+++ dlls/dsound/buffer.c	12 Mar 2005 15:20:33 -0000
@@ -32,6 +32,7 @@
 #include "dsound.h"
 #include "dsdriver.h"
 #include "dsound_private.h"
+#include "wine/library.h"

WINE_DEFAULT_DEBUG_CHANNEL(dsound);

@@ -365,7 +366,7 @@ static ULONG WINAPI IDirectSoundBufferIm
     if (!ref) {
    DSOUND_RemoveBuffer(This->dsound, This);

-	This->lock.DebugInfo->Spare[1] = 0;
+        wine_dbg_remove_lock_name(&(This->lock));
    DeleteCriticalSection(&(This->lock));

if (This->hwbuf) {
@@ -1176,7 +1177,7 @@ HRESULT WINAPI IDirectSoundBufferImpl_Cr
    	DSOUND_RecalcVolPan(&(dsb->volpan));

InitializeCriticalSection(&(dsb->lock));
-        dsb->lock.DebugInfo->Spare[1] = (DWORD)"DSOUNDBUFFER_lock";
+        wine_dbg_add_lock_name(&(dsb->lock), "DSOUNDBUFFER_lock");

/* register buffer if not primary */
    if (!(dsbd->dwFlags & DSBCAPS_PRIMARYBUFFER)) {
@@ -1184,7 +1185,7 @@ HRESULT WINAPI IDirectSoundBufferImpl_Cr
    	if (err != DS_OK) {
    		HeapFree(GetProcessHeap(),0,dsb->buffer->memory);
    		HeapFree(GetProcessHeap(),0,dsb->buffer);
-        		dsb->lock.DebugInfo->Spare[1] = 0;
+                        wine_dbg_remove_lock_name(&(dsb->lock));
    		DeleteCriticalSection(&(dsb->lock));
    		HeapFree(GetProcessHeap(),0,dsb->pwfx);
    		HeapFree(GetProcessHeap(),0,dsb);
Index: dlls/dsound/dsound.c
===================================================================
RCS file: /home/wine/wine/dlls/dsound/dsound.c,v
retrieving revision 1.30
diff -u -p -r1.30 dsound.c
--- dlls/dsound/dsound.c	25 Feb 2005 16:50:57 -0000	1.30
+++ dlls/dsound/dsound.c	12 Mar 2005 15:20:34 -0000
@@ -35,6 +35,7 @@
 #include "dsound.h"
 #include "dsdriver.h"
 #include "dsound_private.h"
+#include "wine/library.h"

WINE_DEFAULT_DEBUG_CHANNEL(dsound);

@@ -285,7 +286,7 @@ static ULONG WINAPI IDirectSoundImpl_Rel
    HeapFree(GetProcessHeap(),0,This->tmp_buffer);
    HeapFree(GetProcessHeap(),0,This->buffer);
         RtlDeleteResource(&This->buffer_list_lock);
-        This->mixlock.DebugInfo->Spare[1] = 0;
+        wine_dbg_remove_lock_name(&This->mixlock);
         DeleteCriticalSection(&This->mixlock);
         HeapFree(GetProcessHeap(),0,This);
         dsound = NULL;
@@ -581,13 +582,13 @@ static HRESULT WINAPI IDirectSoundImpl_D
     CopyMemory(dsb->pwfx, pdsb->pwfx, size);

InitializeCriticalSection(&(dsb->lock));
-    dsb->lock.DebugInfo->Spare[1] = (DWORD)"DSOUNDBUFFER_lock";
+    wine_dbg_add_lock_name(&(dsb->lock), "DSOUNDBUFFER_lock");

/* register buffer */
     hres = DSOUND_AddBuffer(This, dsb);
     if (hres != DS_OK) {
         IDirectSoundBuffer8_Release(psb);
-        dsb->lock.DebugInfo->Spare[1] = 0;
+        wine_dbg_remove_lock_name(&(dsb->lock));
         DeleteCriticalSection(&(dsb->lock));
         HeapFree(GetProcessHeap(),0,dsb->buffer);
         HeapFree(GetProcessHeap(),0,dsb->pwfx);
@@ -943,7 +944,7 @@ HRESULT WINAPI IDirectSoundImpl_Create(
     }

InitializeCriticalSection(&(pDS->mixlock));
-    pDS->mixlock.DebugInfo->Spare[1] = (DWORD)"DSOUND_mixlock";
+    wine_dbg_add_lock_name(&(pDS->mixlock), "DSOUND_mixlock");

RtlInitializeResource(&(pDS->buffer_list_lock));

Index: include/wine/library.h
===================================================================
RCS file: /home/wine/wine/include/wine/library.h,v
retrieving revision 1.35
diff -u -p -r1.35 library.h
--- include/wine/library.h	11 Jan 2005 10:46:58 -0000	1.35
+++ include/wine/library.h	12 Mar 2005 15:20:39 -0000
@@ -67,6 +67,19 @@ extern int (*__wine_dbg_vlog)( unsigned 
 extern void wine_dbg_add_option( const char *name, unsigned char set, unsigned char clear );
 extern int wine_dbg_parse_options( const char *str );

+inline static void wine_dbg_add_lock_name( CRITICAL_SECTION *cs, const char * name )
+{
+    cs->DebugInfo->Spare[1] = (DWORD)name;
+} 
+inline static const char * wine_dbg_get_lock_name( CRITICAL_SECTION *cs )
+{
+    return (const char *)cs->DebugInfo->Spare[1];
+}
+inline static void wine_dbg_remove_lock_name( CRITICAL_SECTION *cs )
+{
+    cs->DebugInfo->Spare[1] = 0;
+}
+
 /* portability */

extern void DECLSPEC_NORETURN wine_switch_to_stack( void (*func)(void *), void *arg, void *stack );

[DSOUND] little patch

[Raphael]

> > This is Alexandre's trick which I borrowed because it is very helpful
> > in debugging lock problems.
> > It would be nice to formalize this and make it available in a more
> > general way but that is Alexandre's call.

Hi
> Something like this plus a lot of documentation explaining what is going
> on.
>
> I think this should go in debug.h but there is only trace stuff in there
> so I put it in library.h.

I approve :)
i would like to have a wine_dbgstr_cs() :)

Alexandre ?

Regards,
Raphael

[DSOUND] little patch

[Alexandre Julliard]

Raphael  writes:
> I approve :)
> i would like to have a wine_dbgstr_cs() :)
>
> Alexandre ?

I'm not sure we want to make that thing "official", it's really a
nasty hack at the moment.