SDL3 Support

[andyvand]

Add SDL3 support

[derselbst]

Wow, that was quick. Thanks a lot! I've only got one comment below. Before merging it, I'll adapt the Window CI pipeline in the next days, to make it compile with SDL3 support.

[derselbst]

Thanks for addressing the review findings. It's getting into shape. I found a few more minor issues.

[derselbst]

Memory allocation during audio playback is not real-time friendly. Is it possible to calculate the maximum len beforehand and use it to preallocate the biggest possible buffer?

[andyvand]

Added it to additional len as buffer.

[derselbst]

Sry, I think you misunderstood me. I was asking to get rid of the malloc from the callback and instead allocate the buffer in new_fluid_sdl3_audio_driver. Also note that dev->write_ptr takes the number of samples (audio frames) to synthesize, not the buffer length in bytes. See here for an example:
https://github.com/libsdl-org/SDL/blob/be991239d9bc6df06b0ca7a9ae9dbb7251e93c12/examples/audio/02-simple-playback-callback/simple-playback-callback.c#L25-L34

[andyvand]

I have replaced the malloc with a static buffer and for dev->write_ptr I use the buffer length divided by dev->frame_size

[FtZPetruska]

Since it's a static buffer, you may remove the check for NULL in the same function since it generates a warning:

if (buffer == NULL)
{
  return;
}

The actual warning:

fluidsynth/src/drivers/fluid_sdl3.c:165:13: warning: comparison of array 'buffer' equal to a null pointer is always false [-Wtautological-pointer-compare]
if (buffer == NULL)
    ^~~~~~    ~~~~

On the topic of warnings, the following call also triggers a -Wsign-compare warning on clang:

fluidsynth/src/drivers/fluid_sdl3.c:163:19: warning: comparison of integers of different signs: 'int' and 'unsigned long' [-Wsign-compare]
buf_len = SDL_min(add_len, SDL_arraysize(buffer));

[andyvand]

Fixed

[derselbst]

Sry for nit-picking again. But you cannot use a buffer with a constant size. The size of the buffer must depend on audio.period-size that you've read in new_fluid_sdl3_audio_driver, otherwise there's no point in reading that setting. I suppose allocating a buffer size of period-size * frame_size would be reasonable.

[andyvand]

Buffer got updated to this.
I've added period_size to the dev structure.

[andyvand]

Reverted to malloc since the buffer can't be preallocated.
I have put the buffer inside the dev structure.
I have put the creation of the buffer in new_fluid_sdl3_audio_driver and the destruction of the buffer inside delete_fluid_sdl3_audio_driver.

[derselbst]

@FtZPetruska I quickly smashed together a Windows CI build that works with SDL3. Even when resolving the cmake issues mentioned above, all the unit tests will be failing because of missing DLL - presumably SDL3.dll, e.g. here

The unit tests inherit their link dependencies from the object lib libfluidsynth-OBJ by linking against it here:

fluidsynth/cmake_admin/FluidUnitTest.cmake

Line 11 in de1daa4

target_link_libraries( ${_test} libfluidsynth-OBJ )

This would carry over all INTERFACE_LINK_LIBRARIES from the object lib to the unit tests. However, when looking at the definition of the sdl3 cmake target, INTERFACE_LINK_LIBRARIES only contains the headers' include dir. The DLL is instead declared by the IMPORTED_LOCATION property, which from my understanding is not used to specify transitive dependencies and therefore isn't carried over from fluidsynth's object lib to the unit test:

add_library(SDL3::SDL3-shared SHARED IMPORTED)
        set_target_properties(SDL3::SDL3-shared
            PROPERTIES
                INTERFACE_LINK_LIBRARIES "SDL3::Headers"
                IMPORTED_IMPLIB "${_sdl3_implib}"
                IMPORTED_LOCATION "${_sdl3_dll}"
                COMPATIBLE_INTERFACE_BOOL "SDL3_SHARED"
                INTERFACE_SDL3_SHARED "ON"
                COMPATIBLE_INTERFACE_STRING "SDL_VERSION"
                INTERFACE_SDL_VERSION "SDL3"
        )

Is my understanding correct? And why isn't _sdl3_dll passed to INTERFACE_LINK_LIBRARIES?

[FtZPetruska]

@FtZPetruska I quickly smashed together a Windows CI build that works with SDL3. Even when resolving the cmake issues mentioned above, all the unit tests will be failing because of missing DLL - presumably SDL3.dll, e.g. here

The unit tests inherit their link dependencies from the object lib libfluidsynth-OBJ by linking against it here:

fluidsynth/cmake_admin/FluidUnitTest.cmake

Line 11 in de1daa4

target_link_libraries( ${_test} libfluidsynth-OBJ )

This would carry over all INTERFACE_LINK_LIBRARIES from the object lib to the unit tests. However, when looking at the definition of the sdl3 cmake target, INTERFACE_LINK_LIBRARIES only contains the headers' include dir. The DLL is instead declared by the IMPORTED_LOCATION property, which from my understanding is not used to specify transitive dependencies and therefore isn't carried over from fluidsynth's object lib to the unit test:

add_library(SDL3::SDL3-shared SHARED IMPORTED)
        set_target_properties(SDL3::SDL3-shared
            PROPERTIES
                INTERFACE_LINK_LIBRARIES "SDL3::Headers"
                IMPORTED_IMPLIB "${_sdl3_implib}"
                IMPORTED_LOCATION "${_sdl3_dll}"
                COMPATIBLE_INTERFACE_BOOL "SDL3_SHARED"
                INTERFACE_SDL3_SHARED "ON"
                COMPATIBLE_INTERFACE_STRING "SDL_VERSION"
                INTERFACE_SDL_VERSION "SDL3"
        )

Is my understanding correct? And why isn't _sdl3_dll passed to INTERFACE_LINK_LIBRARIES?

This SDL3 target looks properly set up:

• IMPORTED_IMPLIB is the .lib that is actually linked with.
• IMPORTED_LOCATION is the .dll that is needed at runtime.
• INTERFACE_LINK_LIBRARIES lists all the dependencies for linking, in this case, only the SDL3::Headers target. Since it is a target, "linking" to it also adds the public include directories.

What is most likely happening is that all the DLLs need to be copied to the same directory as the test executable. In CMake >=3.21 it can be done with:

add_custom_command(TARGET ${_test} POST_BUILD
  COMMAND ${CMAKE_COMMAND} -E copy $<TARGET_RUNTIME_DLLS:${_test}> $<TARGET_FILE_DIR:${_test}>
  COMMAND_EXPAND_LISTS
 )

Otherwise, if you know about the lists of DLLs needed, you can just use:

add_custom_command(TARGET ${_test} POST_BUILD
  COMMAND ${CMAKE_COMMAND} -E copy
    $<TARGET_FILE:SDL3::SDL3>
    $<TARGET_FILE:other::dependency>
    ...
    $<TARGET_FILE_DIR:${_test}>
)

Also on the topic of CI, you may be interested in this Github Action: https://github.com/libsdl-org/setup-sdl

[andyvand]

It is working!
I have done a small test on the built fluidsynth binary.
It plays using the SDL3 audio driver everything beautifully.

`fluidsynth /Users/andyvand/Downloads/fluidsynth/sf2/VintageDreamsWaves-v2.sf2 /Users/andyvand/Downloads/opentyrian2000/extras/music40.mid -a sdl3
FluidSynth runtime version 2.4.3
Copyright (C) 2000-2025 Peter Hanappe and others.
Distributed under the LGPL license.
SoundFont(R) is a registered trademark of Creative Technology Ltd.

fluidsynth: warning: Sample 'SineWave': ROM sample ignored
Type 'help' for help topics.`

[derselbst]

Ok, thanks so far. I'll merge this to a feature branch and will take care of the correct buffer allocation and fixing the CI build.