Re: [PATCH] xaudio2: Always set current OpenAL context before making AL calls

[Chris Robinson]

On 06/30/2016 12:07 PM, Andrew Eikum wrote:
> if(!alcIsExtensionPresent(NULL, "ALC_SOFT_loopback") ||
>                  !(palcLoopbackOpenDeviceSOFT = alcGetProcAddress(NULL, "alcLoopbackOpenDeviceSOFT")) ||
> -                !(palcRenderSamplesSOFT = alcGetProcAddress(NULL, "alcRenderSamplesSOFT"))){
> +                !(palcRenderSamplesSOFT = alcGetProcAddress(NULL, "alcRenderSamplesSOFT")) ||
> +                !(palcSetThreadContext = alcGetProcAddress(NULL, "alcSetThreadContext"))){
>              ERR("XAudio2 requires the ALC_SOFT_loopback extension (OpenAL-Soft >= 1.14)\n");
>              return FALSE;
>          }

alcSetThreadContext isn't part of the ALC_SOFT_loopback extension, you 
should check for ALC_EXT_thread_local_context instead.
> +static void context_lock(ALCcontext *ctx)
> +{
> +    EnterCriticalSection(&al_lock);
> +    alcMakeContextCurrent(ctx);
> +}
> +
> +static void context_unlock(void)
> +{
> +    LeaveCriticalSection(&al_lock);
> +}

Since you're relying on ALC_EXT_thread_local_context anyway, it would 
probably be better to use alcSetThreadContext in context_lock, which 
would allow you to get rid of the global lock for normal calls (another 
thread can't change the current thread context on you, and unlike 
OpenGL, the same context can be current on multiple threads at the same 
time).

[PATCH] xaudio2: Always set current OpenAL context before making AL calls

[Andrew Eikum]

On Thu, Jun 30, 2016 at 12:47:52PM -0700, Chris Robinson wrote:
> On 06/30/2016 12:07 PM, Andrew Eikum wrote:
> > if(!alcIsExtensionPresent(NULL, "ALC_SOFT_loopback") ||
> >                 !(palcLoopbackOpenDeviceSOFT = alcGetProcAddress(NULL, "alcLoopbackOpenDeviceSOFT")) ||
> > -                !(palcRenderSamplesSOFT = alcGetProcAddress(NULL, "alcRenderSamplesSOFT"))){
> > +                !(palcRenderSamplesSOFT = alcGetProcAddress(NULL, "alcRenderSamplesSOFT")) ||
> > +                !(palcSetThreadContext = alcGetProcAddress(NULL, "alcSetThreadContext"))){
> >             ERR("XAudio2 requires the ALC_SOFT_loopback extension (OpenAL-Soft >= 1.14)\n");
> >             return FALSE;
> >         }
>
> alcSetThreadContext isn't part of the ALC_SOFT_loopback extension,
> you should check for ALC_EXT_thread_local_context instead.

Ok, I'll fix that. That was just me being lazy and assuming that if we
had loopback, we must have thread local context.
> > +static void context_lock(ALCcontext *ctx)
> > +{
> > +    EnterCriticalSection(&al_lock);
> > +    alcMakeContextCurrent(ctx);
> > +}
> > +
> > +static void context_unlock(void)
> > +{
> > +    LeaveCriticalSection(&al_lock);
> > +}
>
> Since you're relying on ALC_EXT_thread_local_context anyway, it
> would probably be better to use alcSetThreadContext in context_lock,
> which would allow you to get rid of the global lock for normal calls
> (another thread can't change the current thread context on you, and
> unlike OpenGL, the same context can be current on multiple threads
> at the same time).

I hadn't thought of it that way, good idea. I'll change that too,
should make the patch much smaller.

Thanks for the review,
Andrew