RFC: Reworking sound sub-system

[calris]

(If you really want to avoid duplicates, you could use the quark_* functions instead, but they seem to be more optimized for the case where you're *usually* adding an unknown string, so YMMV.)

The quark functions use a linear strcmp() search and could benefit from hashing.

For a lark, I did it

https://github.com/calris/angband/commit/fa582bd842a50f6bfe3fe4599c11570a23a4ed7b

[Nick]

Just pushed a couple of fixes for Windows

The Mac, for the time being, we need to not compile sound-code.c - I'll need to look a bit deeper into the Makefiles to sort that out

So I gave these to the autobuilder, and the results were interesting.

Linux still doesn't compile, because the autobuilder configures with --disable-sdl-mixer, and so supported_sound_files[] is only declared and not set. This could (I think) be fixed by something like

#ifndef SOUND_SDL
const struct sound_file_type supported_sound_files[] = {  {"", SDL_NULL} };
#endif

in sound-core.c

Windows has less errors now:

main-win.c: In function ‘load_sound_win’:
main-win.c:1157:19: error: ‘MCI_OPEN_PARMS’ has no member named ‘device’
    if (!sample->op.device) {
                   ^
main-win.c:1167:26: warning: comparison between pointer and integer
    data->loaded =  (NULL != sample->op.wDeviceID);
                          ^
main-win.c:1181:20: error: ‘win_sample’ has no member named ‘filname’
    my_strcpy(sample->filname, filename, strlen(filename) + 1)
                    ^
main-win.c:1182:4: error: expected ‘;’ before ‘data’
    data->loaded = true;
    ^
main-win.c: In function ‘play_sound_win’:
main-win.c:1208:15: error: ‘win_sample’ has no member named ‘device’
     if (sample->device) {
               ^
main-win.c:1215:4: error: case label not within a switch statement
    case WIN_WAV:
    ^
main-win.c:1224:4: error: break statement not within loop or switch
    break;
    ^
main-win.c:1227:1: warning: no return statement in function returning non-void [-Wreturn-type]
 }
 ^
main-win.c: In function ‘unload_sound_win’:
main-win.c:1236:15: error: ‘win_sample’ has no member named ‘device’
     if (sample->device)
               ^
main-win.c:1237:27: error: ‘win_sample’ has no member named ‘device’
      mciSendCommand(sample->device, MCI_CLOSE, MCI_WAIT, (size_t)(&sample->op))
                           ^
main-win.c:1239:5: error: expected ‘;’ before ‘break’
     break;
     ^
main-win.c: At top level:
main-win.c:1270:6: error: conflicting types for ‘init_sound_win’
 bool init_sound_win(struct sound_hooks *hooks, int argc, char **argv)
      ^
In file included from main-win.c:88:0:
snd-win.h:21:6: note: previous declaration of ‘init_sound_win’ was here
 errr init_sound_win(struct sound_hooks *hooks, int argc, char **argv);
      ^

OS X compiled cleanly, I guess because load_sound_hook isn't set, but then I don't understand why it failed the first time.

[calris]

Linux still doesn't compile, because the autobuilder configures with --disable-sdl-mixer, and so supported_sound_files[] is only declared and not set. This could (I think) be fixed by something like

#ifndef SOUND_SDL
const struct sound_file_type supported_sound_files[] = {  {"", SDL_NULL} };
#endif

in sound-core.c

Better to avoid building sound-core.c if there is no sound support at all - Another Makefile issue

OS X compiled cleanly, I guess because load_sound_hook isn't set, but then I don't understand why it failed the first time.

I'm sure there will be more than a few issue to do with Makefiles that will need to be sorted out - keep an eye out

[calris]

Windows has less errors now:

And should have even less now

[takkaria]

Am I right in thinking that with the addition of a global supported_sound_files, you can only compile one sound module in at a time? Although I think there's less need for compiling in multiple audio drivers than there is for compiling multiple frontends, it would be good to preserve the ability to do this. Perhaps there could be another hook to return this data?

[calris]

Further down the rabbit hole...

I've teased out 'sound' functionality an attempted to adjust the build system accordingly. In regards to the build system, I've only touched configure.ac and Makefile.src. Seems to work OK for various build settings under Linux. For other builders, YMMV.

I also 'had' (for varying definitions of had) to muck around with ui-prefs a little. Of course, not building sound-core.c meant that

parser_reg(p, "sound sym type str sounds", parse_prefs_sound);

wasn't going to fly. Wrapping #ifdef SOUND around this meant

static enum parser_error parse_prefs_sound(struct parser *p)

generated an unused static function warning.

Soooo - I moved parse_prefs_sound() into sound-core.c because really, if we change the way sound preferences are handled (or the structure of sound.prf) we really don't want to be touching anything other that sound-core.c

This had a knock on effect of having to expose the previously private prefs_data struct

But all this gives us a very clean cut between the sound module and the rest of angband

• #include "sound.h" in main.c
• Handling the command line args for sound in main() (which I think we can improve upon later)
• print_sound_help() in main.c
• init_sound() in main.c
• #include "sound.h" in ui-prefs.c
• register_sound_pref_parser() in ui-prefs.c

These changes are digging a little deeper into some core code than I had thought, but hopefully for the better

[calris]

Am I right in thinking that with the addition of a global supported_sound_files, you can only compile one sound module in at a time?

Kind of...

EDIT: I missed the bleedingly obvious... Yes, supported_sound_files will cause a symbol conflict. Easy enough to fix - I'll add a hook into the sound_hooks struct to fetch the module specific file formats

EDIT 2: This is now in sound-devel - thanks for pointing it out

In reality, the Windows sound module supports two methods of playing a sound
- MCI for MP3
- PlaySound() for WAV

My guess is that sound was first written for Windows using only WAV files which PlaySound() could handle. Then along came the MP3 files, which PlaySound() couldn't handle, so the MCI code was added in.

Linux only has SDL support, so it really doesn't matter that we only compile in one sound module for.

Mac uses something else again (I'm not going to pretend to understand it)

Although I think there's less need for compiling in multiple audio drivers than there is for compiling multiple frontends, it would be good to preserve the ability to do this. Perhaps there could be another hook to return this data?

I do believe it will handle having multiple sound modules compiled in (see init_sound() in sound-core.c), but there are simply no platforms supporting more than one sound library to test the theory.

I can test it easily enough by creating a raw ALSA sound module

[redlumf]

Just curious here. Has anyone profiled the speed impact of the changes?

[calris]

Just curious here. Has anyone profiled the speed impact of the changes?

The runtime performance should be pretty much identical - There are a couple of extra NULL pointer sanity checks in the code path and a couple more levels in the call tree, but apart from that the runtime code path is the same.

The loading should be slightly faster because we no longer load the same sound file multiple times (if it's used in multiple messages - which some file are). However, using the standard .prf parser is probably a little slower that the custom parsers that were written for the old sound.cfg, but I would hazard a guess that this is more than made up for by the loading efficiency.

Memory footprint should also be lower (again due to not loading sound files multiple times).

And for Windows, we are creating fewer MCI resources (one per sound file, versus one per message sound), no idea what impact that will have on overall performance.

[takkaria]

I also 'had' (for varying definitions of had) to muck around with ui-prefs a little. Of course, not building sound-core.c meant that

parser_reg(p, "sound sym type str sounds", parse_prefs_sound);

wasn't going to fly. Wrapping #ifdef SOUND around this meant

static enum parser_error parse_prefs_sound(struct parser *p)

generated an unused static function warning.

Soooo - I moved parse_prefs_sound() into sound-core.c because really, if we change the way sound preferences are handled (or the structure of sound.prf) we really don't want to be touching anything other that sound-core.c

Putting an ifdef around the sound support has the side effect of producing parser errors when no sound module is present. I think there's two solutions; either split the sound config parsing from the pref parser, or put a stub in the ui-prefs file. To me the former looks cleaner and avoids putting the internal state of the pref parser in a header file but YMMV.

[calris]

Putting an ifdef around the sound support has the side effect of producing parser errors when no sound module is present. I think there's two solutions; either split the sound config parsing from the pref parser, or put a stub in the ui-prefs file. To me the former looks cleaner and avoids putting the internal state of the pref parser in a header file but YMMV.

As in we get errors when we hit 'sound:' lines I'm assuming?

I never see any myself. Where do they get displayed?

EDIT: OK, I see it now - didn't seem to care about it in the main init, but if you include sound: in a user .prf file it complains.

I'm not a fan of splitting out processing of the sound.prf file. And I like the idea that you can just include sound: directives in a user .prf file (makes developing and testing new sounds much easier).

But I also don't like the idea of the parser having to know about the 'sound' directive.

Mmmmm

EDIT: I defined a 'dummy' parser and if SOUND is not defined, an inline function in sound.h tells the parser to use the dummy function for lines starting with sound:

[molybdenum]

OS X's sound loading/config parsing happens in load_sounds() at main-cocoa.m:2660. I don't know when the load sound hook is called, but it's likely not at a great time for the OS X port to do something like that (due to memory model and other launch stuff).

[Nick]

Now compiles cleanly for Linux (although birth tests fail) and OS X, Windows only has one missing #include, by the looks:

In file included from ./ui-prefs.h:25:0,
                 from win/win-layout.c:21:
./ui-keymap.h:60:18: error: unknown type name ‘ang_file’
 void keymap_dump(ang_file *fff);
                  ^
Makefile.win:134: recipe for target 'win/win-layout.o' failed

Looks like you guys are doing a great job in this thread, I'm happy to be the monkey that runs the autobuilder

[calris]

Now compiles cleanly for Linux (although birth tests fail)

How do I run the tests?

[Nick]

How do I run the tests?

You have to have configured with --enable-test, then

./run-tests

for birth tests,

make tests

for unit tests.

If you're interested in writing unit tests for any of your work (which is encouraged, but more honoured in the breach), there's a README in src/tests.