Re: Feedback requested on an OpenAL audio driver patch

[Leon Freitag]

My first impressions:
1) Doesn't compile here:

audio.c: In function ‘OpenAL_WaveClose’:
audio.c:636: error: void value not ignored as it ought to be

because alcCloseDevice() is declared here as void (my openal version is 
1.0.20051129)

2) It'd be better to convert ALC errors from FIXMEs to ERRs.

3) You don't link the driver to openal somewhy (there's no -lopenal switch 
passed to gcc). Perhaps it's related to the configure.in hack I made, but I 
had to modify dlls/winmm/wineopenal/Makefile.in and add openal there too.

4) You check for al/al.h and OpenAL/al.h within the configure script but you 
include AL/al.h and OpenAL/al.h. This doesn't work on Linux, since Linux 
filesystems are case sensitive.

Leon

Feedback requested on an OpenAL audio driver patch

[Nick Burns]

> From: Leon Freitag 
> Date: Wed, 7 Jun 2006 15:54:49 +0200
>
> My first impressions:
> - Doesn't compile here:
>
> audio.c: In function ‘OpenAL_WaveClose’:
> audio.c:636: error: void value not ignored as it ought to be
>
> because alcCloseDevice() is declared here as void (my openal version is
> 1.0.20051129)

Grr -- OpenAL 1.0 spec does not return anything there (Thanks good catch)
This means I cannot check errors on device closing...

(fixed will e in next patch)
> - It'd be better to convert ALC errors from FIXMEs to ERRs.

Will do -- I was just used to FIXME...
Hehe at first (as I was implementing) they were fixme for real! Now they are 
openal failures.

(fixed will be in next patch)
> - You don't link the driver to openal somewhy (there's no -lopenal switch
>
> passed to gcc). Perhaps it's related to the configure.in hack I made, but I
> had to modify dlls/winmm/wineopenal/Makefile.in and add openal there too.

It should do a -lopenal -- looking at configure.ac
'if test "$ac_cv_header_al_al_h" = "yes"
    then
    dnl OpenAL framework
    AC_SUBST(OPENAL,"-lopenal")
fi'

Makefile.in
'EXTRALIBS = $(LIBUUID) @OPENAL@'

Could be related to 4...

(should work in next patch due to fix in 4)
> - You check for al/al.h and OpenAL/al.h within the configure script but
>
> you
> include AL/al.h and OpenAL/al.h. This doesn't work on Linux, since Linux
> filesystems are case sensitive.

Whoops -- its SUPPOSED to be AL/al.h -- I typoed!
(fixed in configure.ac -- will be sending new patch)
> Leon

Thanks for the feedback! -- all the way from Russia

- Nick