Add GenericAudio Backend, make SDL mixer optional #1100

Merged
merged 24 commits into from Apr 1, 2017

Conversation

Projects
None yet
4 participants
@Ghabry
Member

Ghabry commented Feb 13, 2017

This uses work of @ChristianBreitwieser who wrote a generic audio mixer (originally for libretro, but I took it and made is more suitable for general purpose).

All audio backends (Except 3DS - lack of CPU power - and SDL Mixer - obvious reasons) use the GenericAudio backend now which simplified the code a lot.

Removed SDL Mixer dependency from Wii.
Android still depends on Mixer due to lack of WildMidi (buildsystem upgrade required)

New arguments for configure:

--without-audio
--without-sdlmixer

Fixes #968

@carstene1ns

audio_sdl.cpp needs a platform check to be excluded on sdl-less platforms (3DS, PSVita, ...)

src/audio_decoder.cpp
@@ -143,6 +143,7 @@ std::unique_ptr<AudioDecoder> AudioDecoder::Create(FILE* file, const std::string
if (!strncmp(magic, "MThd", 4)) {
#ifdef HAVE_WILDMIDI
static bool wildmidi_works = true;
+ printf("MIDI\n");

This comment has been minimized.

@carstene1ns

carstene1ns Feb 13, 2017

Member

debug leftover?

@carstene1ns

carstene1ns Feb 13, 2017

Member

debug leftover?

@Ghabry

This comment has been minimized.

Show comment
Hide comment
@Ghabry

Ghabry Feb 13, 2017

Member

Reminder for me:

  • Check for SUPPORT_AUDIO in all Makefiles
  • when none of the linked audio libraries supports the format <- supportS, typo. linked - provided?
Member

Ghabry commented Feb 13, 2017

Reminder for me:

  • Check for SUPPORT_AUDIO in all Makefiles
  • when none of the linked audio libraries supports the format <- supportS, typo. linked - provided?
@Ghabry

This comment has been minimized.

Show comment
Hide comment
@Ghabry

Ghabry Feb 14, 2017

Member

I think it would make sense to enable Wildmidi and FmMidi by default when SDL Mixer is not found (did this in CMake). opinion?

Member

Ghabry commented Feb 14, 2017

I think it would make sense to enable Wildmidi and FmMidi by default when SDL Mixer is not found (did this in CMake). opinion?

@Ghabry

This comment has been minimized.

Show comment
Hide comment
@Ghabry

Ghabry Feb 14, 2017

Member

WildMidi doesn't like my "/etc/timidity++/timidity.cfg" on Arch, silence

Member

Ghabry commented Feb 14, 2017

WildMidi doesn't like my "/etc/timidity++/timidity.cfg" on Arch, silence

@Ghabry

This comment has been minimized.

Show comment
Hide comment
@Ghabry

Ghabry Feb 14, 2017

Member

Submitted a patch to WildMidi to fix playback on Arch: Mindwerks/wildmidi#166

Member

Ghabry commented Feb 14, 2017

Submitted a patch to WildMidi to fix playback on Arch: Mindwerks/wildmidi#166

src/audio_generic.cpp
+ mixer_buffer.data()[ii * output_channels + 1] = valr;
+ } else {
+ mixer_buffer.data()[ii * output_channels + 1] = mixer_buffer.data()[ii *
+ output_channels];

This comment has been minimized.

@scurest

scurest Feb 14, 2017

Contributor

Weird indent (and below too).

@scurest

scurest Feb 14, 2017

Contributor

Weird indent (and below too).

@scurest

This comment has been minimized.

Show comment
Hide comment
@scurest

scurest Feb 14, 2017

Contributor

Also, in GenericAudio::Decode, there are a lot of calls like vec.data()[index]. Excepting the ones where you cast the data pointer, those can be vec[index], right?

Contributor

scurest commented Feb 14, 2017

Also, in GenericAudio::Decode, there are a lot of calls like vec.data()[index]. Excepting the ones where you cast the data pointer, those can be vec[index], right?

@Ghabry

This comment has been minimized.

Show comment
Hide comment
@Ghabry

Ghabry Feb 14, 2017

Member

Thx for ´the feedback. You are right about data(), that's not required.

In general the formating of GenericAudio is not the best, will check.

More notes for me:
Move the "static" variables in Decode in the private part of the class.
Try to reduce amount of Mutex locks in BGM_* and SE_* API.

Member

Ghabry commented Feb 14, 2017

Thx for ´the feedback. You are right about data(), that's not required.

In general the formating of GenericAudio is not the best, will check.

More notes for me:
Move the "static" variables in Decode in the private part of the class.
Try to reduce amount of Mutex locks in BGM_* and SE_* API.

@Ghabry

This comment has been minimized.

Show comment
Hide comment
@Ghabry

Ghabry Feb 15, 2017

Member

About wildmidi: They backport the fix to a new version (0.3.12) unfortunately Arch won't profit from this because they package 0.4 ( this versioning scheme is not optimal ^^'). Will solve itself when they ever finish 0.4.1 though.

Member

Ghabry commented Feb 15, 2017

About wildmidi: They backport the fix to a new version (0.3.12) unfortunately Arch won't profit from this because they package 0.4 ( this versioning scheme is not optimal ^^'). Will solve itself when they ever finish 0.4.1 though.

@carstene1ns

General question: Does the 3DS change not to use CSND anymore and removing libkhax earlier still need a target app for permissions,etc? If not, we should remove the easyrpg-player.xml from builds/3ds folder...

README.md
-
+- SDL2_mixer for audio mixing. Used as a fallback when none of the provided
+ audio libraries support the format. Due to API limitations not all audio
+ effects are possible through SDL2_mixer audio.

This comment has been minimized.

@carstene1ns

carstene1ns Feb 19, 2017

Member

Would be better to move the deprecation notice (line 25) of SDL 1.2 here.

@carstene1ns

carstene1ns Feb 19, 2017

Member

Would be better to move the deprecation notice (line 25) of SDL 1.2 here.

src/3ds_ui.cpp
@@ -55,11 +55,21 @@ static const devoptab_t dotab_null = {
CtrUi::CtrUi(int width, int height) :
BaseUi() {
+

This comment has been minimized.

@carstene1ns

carstene1ns Feb 19, 2017

Member

Stray tabs surrounding this block.

@carstene1ns

carstene1ns Feb 19, 2017

Member

Stray tabs surrounding this block.

src/3ds_ui.cpp
}
CtrUi::~CtrUi() {
sf2d_free_texture(main_texture);
+ sf2d_free_texture(keyboard_texture);

This comment has been minimized.

@carstene1ns

carstene1ns Feb 19, 2017

Member

Texture is freed directly after loading already. This should not matter however, when the lib catches double frees.

@carstene1ns

carstene1ns Feb 19, 2017

Member

Texture is freed directly after loading already. This should not matter however, when the lib catches double frees.

src/audio_3ds.cpp
+ bgm_decoder->GetFormat(frequency, format, channels);
+ bgm_decoder->SetFade(0,volume,fadein);
+ bgm_decoder->SetLooping(true);
+

This comment has been minimized.

@carstene1ns

carstene1ns Feb 19, 2017

Member

Again stray tabs surrounding this block.

(There may be a setting in your IDE/editor to highlight or remove this)

@carstene1ns

carstene1ns Feb 19, 2017

Member

Again stray tabs surrounding this block.

(There may be a setting in your IDE/editor to highlight or remove this)

src/main_data.cpp
@@ -153,6 +155,7 @@ void Main_Data::Init() {
// FIXME: Uses SDL API
char* data_dir = SDL_GetBasePath();
project_path = data_dir;
+

This comment has been minimized.

@carstene1ns

carstene1ns Feb 19, 2017

Member

🕺

+ # Provide fmmidi options
+ option(PLAYER_ENABLE_FMMIDI "Enable internal MIDI sequencer. Will be used when external MIDI library fails." ON)
+ if(${PLAYER_ENABLE_FMMIDI} MATCHES "ON")
+ add_definitions(-DWANT_FMMIDI)

This comment has been minimized.

@carstene1ns

carstene1ns Feb 19, 2017

Member

Does this pass the check in system.h? [1/2]

@carstene1ns

carstene1ns Feb 19, 2017

Member

Does this pass the check in system.h? [1/2]

@@ -28,7 +28,7 @@ CC = $(PREFIX)-gcc
CXX = $(PREFIX)-g++
CFLAGS = -Wl,-q -g -O3 -DPSP2 -DHAVE_LIBSPEEXDSP \
-DWANT_FASTWAV -DSUPPORT_AUDIO -DHAVE_LIBSNDFILE \
- -DHAVE_OGGVORBIS -DHAVE_MPG123 -DWANT_FMMIDI=1 \
+ -DHAVE_OGGVORBIS -DHAVE_MPG123 -DWANT_FMMIDI \

This comment has been minimized.

@carstene1ns

carstene1ns Feb 19, 2017

Member

Does this pass the check in system.h? [2/2]

@carstene1ns

carstene1ns Feb 19, 2017

Member

Does this pass the check in system.h? [2/2]

This comment has been minimized.

@Ghabry

Ghabry Feb 19, 2017

Member

Yes, because defines default to 1

@Ghabry

Ghabry Feb 19, 2017

Member

Yes, because defines default to 1

@Ghabry Ghabry modified the milestone: 0.5.1 Feb 21, 2017

@fdelapena fdelapena removed the Needs Rebase label Mar 2, 2017

@Ghabry

This comment has been minimized.

Show comment
Hide comment
@Ghabry

Ghabry Mar 4, 2017

Member

General question: Does the 3DS change not to use CSND anymore and removing libkhax earlier still need a target app for permissions,etc? If not, we should remove the easyrpg-player.xml from builds/3ds folder...

CSND and khax are gone. The easyrpg-player.xml is for selecting the youtube-channel as a target. This is still required for non-CIA due to size limits of the executable memory.

Member

Ghabry commented Mar 4, 2017

General question: Does the 3DS change not to use CSND anymore and removing libkhax earlier still need a target app for permissions,etc? If not, we should remove the easyrpg-player.xml from builds/3ds folder...

CSND and khax are gone. The easyrpg-player.xml is for selecting the youtube-channel as a target. This is still required for non-CIA due to size limits of the executable memory.

@Ghabry

This comment has been minimized.

Show comment
Hide comment
@Ghabry

Ghabry Mar 4, 2017

Member

I have a one-line-change to change the newlib-heapsize on the 3DS, shall I add this, too?

Member

Ghabry commented Mar 4, 2017

I have a one-line-change to change the newlib-heapsize on the 3DS, shall I add this, too?

@fdelapena

fdelapena approved these changes Mar 5, 2017 edited

- free(myFile);
+ FILE* filehandle = FileFinder::fopenUTF8(file, "rb");
+ if (!filehandle) {
+ Output::Warning("Audio not readable: %s", file.c_str());

This comment has been minimized.

@carstene1ns

carstene1ns Mar 13, 2017

Member

"Music" ?

@carstene1ns

carstene1ns Mar 13, 2017

Member

"Music" ?

src/audio_3ds.cpp
+ }
+ ndspChnSetRate(bgm_channel, frequency);
+
+ Output::Debug("Audio started: %s, samplerate: %u, pitch: %u", file.c_str(),frequency,pitch);

This comment has been minimized.

@carstene1ns

carstene1ns Mar 13, 2017

Member

Debug leftover?

@carstene1ns

carstene1ns Mar 13, 2017

Member

Debug leftover?

@carstene1ns

This comment has been minimized.

Show comment
Hide comment
@carstene1ns

carstene1ns Mar 13, 2017

Member

I get crashes when running player without sdl_mixer:

Thread 9 "SDLAudioDev1" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffe40c7700 (LWP 12380)]
GenericAudio::Decode (this=0xfbfc30, 
    output_buffer=0xf99510 "L\002L\002\002\002\002\002\313\001\313\001\003\002\003\002>\002>\002\371\001\371\001[\001[\001\v\001\v\001Z\001Z\001\361\001\361\001C\002C\002\022\002\022\002\201\001\201", <incomplete sequence \354>, 
    buffer_length=3760) at src/audio_generic.cpp:381
381						mixer_buffer[ii * output_format.channels] = vall;
(gdb) bt f
#0  GenericAudio::Decode (this=0xfbfc30, 
    output_buffer=0xf99510 "L\002L\002\002\002\002\002\313\001\313\001\003\002\003\002>\002>\002\371\001\371\001[\001[\001\v\001\v\001Z\001Z\001\361\001\361\001C\002C\002\022\002\022\002\201\001\201", <incomplete sequence \354>, 
    buffer_length=3760) at src/audio_generic.cpp:381
        vall = 0
        valr = 0
        ii = 15564
        read_bytes = 15525120
        channels = 2
        volume = 0
        samplesize = 2
        frequency = 44100
        sampleformat = AudioDecoder::Format::S16
        is_bgm_channel = <optimized out>
        channel_used = <optimized out>
        i = 1
        channel_active = false
        total_volume = 0
        samples_per_frame = 940
#1  0x00007ffff6f009a2 in ?? () from /usr/lib/libSDL2-2.0.so.0
No symbol table info available.
#2  0x00007ffff6f6343c in ?? () from /usr/lib/libSDL2-2.0.so.0
No symbol table info available.
#3  0x00007ffff6fb48e9 in ?? () from /usr/lib/libSDL2-2.0.so.0
No symbol table info available.
#4  0x00007ffff29422e7 in start_thread () from /usr/lib/libpthread.so.0
No symbol table info available.
#5  0x00007ffff519554f in clone () from /usr/lib/libc.so.6
No symbol table info available.
ASAN:DEADLYSIGNAL
=================================================================
==25830==ERROR: AddressSanitizer: SEGV on unknown address 0x628000080000 (pc 0x0000004ef463 bp 0x000000000002 sp 0x7fd1326b1c00 T8)
    #0 0x4ef462 in GenericAudio::Decode(unsigned char*, int) src/audio_generic.cpp:382
    #1 0x7fd149d159a1  (/usr/lib/libSDL2-2.0.so.0+0x1a9a1)
    #2 0x7fd149d7843b  (/usr/lib/libSDL2-2.0.so.0+0x7d43b)
    #3 0x7fd149dc98e8  (/usr/lib/libSDL2-2.0.so.0+0xce8e8)
    #4 0x7fd14789b2e6 in start_thread (/usr/lib/../lib/libpthread.so.0+0x72e6)
    #5 0x7fd147faa54e in clone (/usr/lib/libc.so.6+0xec54e)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV src/audio_generic.cpp:382 in GenericAudio::Decode(unsigned char*, int)
Thread T8 (SDLAudioDev1) created by T0 here:
    #0 0x7fd14ac20468 in __interceptor_pthread_create /build/gcc-multilib/src/gcc/libsanitizer/asan/asan_interceptors.cc:236
    #1 0x7fd149dc9952  (/usr/lib/libSDL2-2.0.so.0+0xce952)

==25830==ABORTING
Member

carstene1ns commented Mar 13, 2017

I get crashes when running player without sdl_mixer:

Thread 9 "SDLAudioDev1" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffe40c7700 (LWP 12380)]
GenericAudio::Decode (this=0xfbfc30, 
    output_buffer=0xf99510 "L\002L\002\002\002\002\002\313\001\313\001\003\002\003\002>\002>\002\371\001\371\001[\001[\001\v\001\v\001Z\001Z\001\361\001\361\001C\002C\002\022\002\022\002\201\001\201", <incomplete sequence \354>, 
    buffer_length=3760) at src/audio_generic.cpp:381
381						mixer_buffer[ii * output_format.channels] = vall;
(gdb) bt f
#0  GenericAudio::Decode (this=0xfbfc30, 
    output_buffer=0xf99510 "L\002L\002\002\002\002\002\313\001\313\001\003\002\003\002>\002>\002\371\001\371\001[\001[\001\v\001\v\001Z\001Z\001\361\001\361\001C\002C\002\022\002\022\002\201\001\201", <incomplete sequence \354>, 
    buffer_length=3760) at src/audio_generic.cpp:381
        vall = 0
        valr = 0
        ii = 15564
        read_bytes = 15525120
        channels = 2
        volume = 0
        samplesize = 2
        frequency = 44100
        sampleformat = AudioDecoder::Format::S16
        is_bgm_channel = <optimized out>
        channel_used = <optimized out>
        i = 1
        channel_active = false
        total_volume = 0
        samples_per_frame = 940
#1  0x00007ffff6f009a2 in ?? () from /usr/lib/libSDL2-2.0.so.0
No symbol table info available.
#2  0x00007ffff6f6343c in ?? () from /usr/lib/libSDL2-2.0.so.0
No symbol table info available.
#3  0x00007ffff6fb48e9 in ?? () from /usr/lib/libSDL2-2.0.so.0
No symbol table info available.
#4  0x00007ffff29422e7 in start_thread () from /usr/lib/libpthread.so.0
No symbol table info available.
#5  0x00007ffff519554f in clone () from /usr/lib/libc.so.6
No symbol table info available.
ASAN:DEADLYSIGNAL
=================================================================
==25830==ERROR: AddressSanitizer: SEGV on unknown address 0x628000080000 (pc 0x0000004ef463 bp 0x000000000002 sp 0x7fd1326b1c00 T8)
    #0 0x4ef462 in GenericAudio::Decode(unsigned char*, int) src/audio_generic.cpp:382
    #1 0x7fd149d159a1  (/usr/lib/libSDL2-2.0.so.0+0x1a9a1)
    #2 0x7fd149d7843b  (/usr/lib/libSDL2-2.0.so.0+0x7d43b)
    #3 0x7fd149dc98e8  (/usr/lib/libSDL2-2.0.so.0+0xce8e8)
    #4 0x7fd14789b2e6 in start_thread (/usr/lib/../lib/libpthread.so.0+0x72e6)
    #5 0x7fd147faa54e in clone (/usr/lib/libc.so.6+0xec54e)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV src/audio_generic.cpp:382 in GenericAudio::Decode(unsigned char*, int)
Thread T8 (SDLAudioDev1) created by T0 here:
    #0 0x7fd14ac20468 in __interceptor_pthread_create /build/gcc-multilib/src/gcc/libsanitizer/asan/asan_interceptors.cc:236
    #1 0x7fd149dc9952  (/usr/lib/libSDL2-2.0.so.0+0xce952)

==25830==ABORTING
@Ghabry

This comment has been minimized.

Show comment
Hide comment
@Ghabry

Ghabry Mar 13, 2017

Member

One problem is when volume is 0, then "read_bytes" is not initialized. But even after fixing this ASAN still reports a crash.
Interestingly this doesn't crash under Windows, not even with application verifier enabled o.O

Member

Ghabry commented Mar 13, 2017

One problem is when volume is 0, then "read_bytes" is not initialized. But even after fixing this ASAN still reports a crash.
Interestingly this doesn't crash under Windows, not even with application verifier enabled o.O

@Ghabry

This comment has been minimized.

Show comment
Hide comment
@Ghabry

Ghabry Mar 31, 2017

Member

Had to remove the 3ds heapsize change. Crashes on real hardware. So we have to instead reduce liblcf mem footprint ;)

Member

Ghabry commented Mar 31, 2017

Had to remove the 3ds heapsize change. Crashes on real hardware. So we have to instead reduce liblcf mem footprint ;)

@carstene1ns

This comment has been minimized.

Show comment
Hide comment
@carstene1ns

carstene1ns Apr 1, 2017

Member

Merging this now. Will test the Wii version later on hardware...

Member

carstene1ns commented Apr 1, 2017

Merging this now. Will test the Wii version later on hardware...

@carstene1ns carstene1ns merged commit ca2e789 into EasyRPG:master Apr 1, 2017

6 checks passed

Android (armeabi-v7a) Build finished.
Details
GNU/Linux Build finished.
Details
OSX Build finished.
Details
Windows (x64) Build finished.
Details
Windows (x86) Build finished.
Details
web Build finished.
Details

@Ghabry Ghabry deleted the Ghabry:sdl_con_audio_sin_mixer branch Apr 1, 2017

@carstene1ns carstene1ns referenced this pull request May 30, 2017

Merged

MSVC: Update to latest buildscript version. #1192

26 of 26 tasks complete

Ghabry pushed a commit to libretro/easyrpg-libretro that referenced this pull request May 22, 2018

Merge pull request #1100 from Ghabry/sdl_con_audio_sin_mixer
Add GenericAudio Backend, make SDL mixer optional
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment