search

alsa-project / alsa-lib

The Advanced Linux Sound Architecture (ALSA) - library
GNU Lesser General Public License v2.1
366 stars 177 forks source link

OpenAL fails to build with recent "rawmidi" commit #172

Closed shoober420 closed 3 years ago

shoober420 commented 3 years ago

The recent alsa-lib commit https://github.com/alsa-project/alsa-lib/commit/95eb312fade1908a2c944e9de4626c0ff60b2203 borks OpenAL from building. I thought it was an issue with OpenAL, but he mentions that its an issue with alsa-lib (https://github.com/kcat/openal-soft/issues/592#issuecomment-903211857). Here is the OpenAL build log just in case and a snippet of the OpenAL build error.

https://github.com/kcat/openal-soft/files/7026546/buildopenal.log

OpenAL build error.

[84/97] /usr/bin/x86_64-pc-linux-gnu-g++ -m32 -DALC_API="__attribute__((visibility(\"protected\"))) __attribute__((force_align_arg_pointer))" -DAL_ALEXT_PROTOTYPES -DAL_API="__attribute__((visibility(\"protected\"))) __attribute__((force_align_arg_pointer))" -DAL_BUILD_LIBRARY -DOpenAL_EXPORTS -DRESTRICT=__restrict -I/var/tmp/portage/media-libs/openal-9999/work/openal-9999/include -I/usr/include/pipewire-0.3 -I/usr/include/spa-0.2 -I/var/tmp/portage/media-libs/openal-9999/work/openal-9999_build-abi_x86_32.x86 -I/var/tmp/portage/media-libs/openal-9999/work/openal-9999 -I/var/tmp/portage/media-libs/openal-9999/work/openal-9999/common  -march=native -Ofast -pipe -fno-plt -fno-common -fno-semantic-interposition -fipa-pta -falign-functions=32 -fdevirtualize-at-ltrans -fuse-linker-plugin -floop-nest-optimize -fgraphite-identity -fno-signed-zeros -fno-trapping-math -fassociative-math -freciprocal-math -fno-math-errno -ffinite-math-only -fno-rounding-math -fno-signaling-nans -fcx-limited-range -fexcess-precision=fast -w -flto=auto -fPIC -Winline -Wunused -Wall -Wextra -Wshadow -Wconversion -Wcast-align -Wpedantic -Wold-style-cast -Wnon-virtual-dtor -Woverloaded-virtual -fno-math-errno -fvisibility=hidden -msse2 -mfpmath=sse -pthread -std=gnu++14 -MD -MT CMakeFiles/OpenAL.dir/alc/backends/alsa.cpp.o -MF CMakeFiles/OpenAL.dir/alc/backends/alsa.cpp.o.d -o CMakeFiles/OpenAL.dir/alc/backends/alsa.cpp.o -c /var/tmp/portage/media-libs/openal-9999/work/openal-9999/alc/backends/alsa.cpp
FAILED: CMakeFiles/OpenAL.dir/alc/backends/alsa.cpp.o 
/usr/bin/x86_64-pc-linux-gnu-g++ -m32 -DALC_API="__attribute__((visibility(\"protected\"))) __attribute__((force_align_arg_pointer))" -DAL_ALEXT_PROTOTYPES -DAL_API="__attribute__((visibility(\"protected\"))) __attribute__((force_align_arg_pointer))" -DAL_BUILD_LIBRARY -DOpenAL_EXPORTS -DRESTRICT=__restrict -I/var/tmp/portage/media-libs/openal-9999/work/openal-9999/include -I/usr/include/pipewire-0.3 -I/usr/include/spa-0.2 -I/var/tmp/portage/media-libs/openal-9999/work/openal-9999_build-abi_x86_32.x86 -I/var/tmp/portage/media-libs/openal-9999/work/openal-9999 -I/var/tmp/portage/media-libs/openal-9999/work/openal-9999/common  -march=native -Ofast -pipe -fno-plt -fno-common -fno-semantic-interposition -fipa-pta -falign-functions=32 -fdevirtualize-at-ltrans -fuse-linker-plugin -floop-nest-optimize -fgraphite-identity -fno-signed-zeros -fno-trapping-math -fassociative-math -freciprocal-math -fno-math-errno -ffinite-math-only -fno-rounding-math -fno-signaling-nans -fcx-limited-range -fexcess-precision=fast -w -flto=auto -fPIC -Winline -Wunused -Wall -Wextra -Wshadow -Wconversion -Wcast-align -Wpedantic -Wold-style-cast -Wnon-virtual-dtor -Woverloaded-virtual -fno-math-errno -fvisibility=hidden -msse2 -mfpmath=sse -pthread -std=gnu++14 -MD -MT CMakeFiles/OpenAL.dir/alc/backends/alsa.cpp.o -MF CMakeFiles/OpenAL.dir/alc/backends/alsa.cpp.o.d -o CMakeFiles/OpenAL.dir/alc/backends/alsa.cpp.o -c /var/tmp/portage/media-libs/openal-9999/work/openal-9999/alc/backends/alsa.cpp
In file included from /usr/include/alsa/asoundlib.h:55,
                 from /var/tmp/portage/media-libs/openal-9999/work/openal-9999/alc/backends/alsa.cpp:51:
/usr/include/alsa/rawmidi.h:107:9: error: '__u8' does not name a type
  107 |         __u8 frame_type;
      |         ^~~~
/usr/include/alsa/rawmidi.h:108:9: error: '__u8' does not name a type
  108 |         __u8 length; /* number of valid bytes in data field */
      |         ^~~~
/usr/include/alsa/rawmidi.h:109:9: error: '__u8' does not name a type
  109 |         __u8 reserved[2];
      |         ^~~~
/usr/include/alsa/rawmidi.h:110:9: error: '__u32' does not name a type; did you mean '__m32'?
  110 |         __u32 tv_nsec;          /* nanoseconds */
      |         ^~~~~
      |         __m32
/usr/include/alsa/rawmidi.h:111:9: error: '__u64' does not name a type; did you mean '__m64'?
  111 |         __u64 tv_sec;           /* seconds */
      |         ^~~~~
      |         __m64
/usr/include/alsa/rawmidi.h:112:9: error: '__u8' does not name a type
  112 |         __u8 data[SND_RAWMIDI_FRAMING_DATA_LENGTH];
      |         ^~~~
diwic commented 3 years ago

I guess this is my fault, but the best way to fix it without destroying someone else's build is beyond me. I could either add type_compat.h into asoundlib.h somehow, or I could change __u8 to uint8_t, I'm not sure which one is better. Or perhaps some third option.

@tiwai Do you have any suggestion on what the best path forward is?

perexg commented 3 years ago

@diwic : The structure should not be exported outside. See snd_ctl_event_t for an example.

tiwai commented 3 years ago

Admittedly it's a bit exceptional to expose the struct, but I thought this would fit well for the rawmidi use case, as the application so far used only the direct byte stream read without alsa-lib API.

diwic commented 3 years ago

@perexg The structure must be exposed, it's supposed to be used by applications.

perexg commented 3 years ago

@perexg The structure must be exposed, it's supposed to be used by applications.

The data are supposed to be used by apps, but the use may be controlled by alsa-lib like we do for other APIs. I suggest to create a decoder function like:

ssize_t snd_rawmidi_decode_frame(snd_rawmidi_t *rmidi, void *buffer, size_t size,
                                 struct timespec *tstamp, void **frame, size_t *frame_size);

Where buffer and size and the undecoded buffer area returned by snd_rawmidi_read. The return value can be an error code or size of frame (replace SND_RAWMIDI_FRAMING_DATA_LENGTH) and frame will return pointer to the midi data chunk with the frame_size length. tstamp should be allocated by the caller and filled by the decoder.

diwic commented 3 years ago

@perexg I find that to be both more complicated and worse performance, than just reading the struct where it is in memory.

aiChaoSONG commented 3 years ago

@diwic @perexg I find another issue which caused by the recent rawmidi change. I got alsa-utils build failure on Ubuntu 20.04. After reset to 23a191a, the alsa-utils build fine.

checking for libasound headers version >= 1.2.5 (1.2.5)... not present.
configure: error: Sufficiently new version of libasound not found.
tiwai commented 3 years ago

Let's fix the build problem at first. The current definition looks also buggy due to the lack of packed attribute, too.

perexg commented 3 years ago

My objection against the export of this structure persists. The performance issue is negligible with my proposal and I would prefer to follow the other alsa-lib conventions to hide those internal structures. And it's more safe to verify the data consistency in the library rather to open-code the checks (frame_type, length) in applications.

perexg commented 3 years ago

https://github.com/alsa-project/alsa-lib/pull/173

perexg commented 3 years ago

I redesigned the API in https://github.com/alsa-project/alsa-lib/pull/179 .