RFC: Reworking sound sub-system

[redlumf]

Recently for a toy project I needed a hash function.

http://www.cse.yorku.ca/~oz/hash.html

The first one djb2 performs very well.

Perfomance is always nice! I've compiled angband-4.0.1 for the Amiga and it's very slow (for reference 2.7.x was flying on the amiga).

( Amiga compilation http://eab.abime.net/showthread.php?t=79222)

Incidently, I had no issues with sound on amiga either! :P (I had to pull an ami-snd.c of course, and recode the mp3s to .8svx)

[calris]

Recently for a toy project I needed a hash function.

http://www.cse.yorku.ca/~oz/hash.html

The first one djb2 performs very well.

Now that I have ripped out the trees and implemented hash searches, I can easily play around with various hash functions. I started with FNV because it was easy to find complete reference source in C. No hash collisions in FNV over the 330ish sound file names.

Perfomance is always nice! I've compiled angband-4.0.1 for the Amiga and it's very slow (for reference 2.7.x was flying on the amiga).

( Amiga compilation http://eab.abime.net/showthread.php?t=79222)

Incidently, I had no issues with sound on amiga either! :P (I had to pull an ami-snd.c of course, and recode the mp3s to .8svx)

Why isn't the Amiga port in mainline? It's always good to have an architecture at the bottom end of the performance scale - it really helps identify performance issues

[redlumf]

Why isn't the Amiga port in mainline?

Dead platform, no maintainer(s), three users...well, maybe four. Pick one!

It's always good to have an architecture at the bottom end of the performance scale - it really helps identify performance issues

OH YES!


Now back on subject, I am confused, I've looked at the source and play_sound has no search at all. Just array lookup. What are you trying to improve? The building of the array on startup?

[calris]

Now back on subject, I am confused, I've looked at the source and play_sound has no search at all. Just array lookup. What are you trying to improve? The building of the array on startup?

• Every playform defines it's own processing of sound.cfg
• No platform used the standard parser
• list-message.h had an extra column for the sole purpose of mapping message types to sound message types instead of just using the message type strings already used by the standard parser
• Supporting a new sound arch required reimplementing slabs of what should be common code

The list goes on...

Essentially - the sound code is a mess and I figured I could help clean it up

EDIT: Using the standard .prf parser had some unexpected, but very nice consequences. For example, you can now map custom sounds (including your own sound files) in your user .prf files

[takkaria]

I would have thought this would be a seriously risky proposition. From my limited understanding of allocators, I thought they tracked chunks and sizes - what would happen if the pointer was at the start of a chunk and subtracting one would push it back into another chunk?

Because mem_alloc(x) allocates an x bytes + sizeof size_t to and returns a pointer to the result of malloc + sizeof size_t. Then mem_realloc() just backtracks. That functionality was put in there a while ago to help track down some memory issues, but it hasn't been used in a while.

[Nick]

I'll be interested to see what this hashing code looks like - I have a feeling we may be able to use the idea elsewhere.

[molybdenum]

Just use a separate repository + the "submodule" functionality of git. That would mean that fetching the sounds is a simple "git submodule init && git submodule update" command away.

An alternate way of breaking up the repository is to move just the source into a submodule (of the main Angband repo), leaving the heavy resources where they are. It doesn't make much sense to move the resources to a new repo and then try to wipe the old history or something. By moving just the source, someone can clone and pull request against that, while everybody with the big repo gets those changes normally. The only drawback is that you don't have history in the source repo, but that's easily looked up on Github if needed. I've done this before and it seemed to work fairly well.

And seriously, why do we have z-virt.c - I saw something in trac about nuking them. Seriously, custom wrappers around standard C functions are evil as.

I think it was because, even up to a few years ago, there were environments and ports that didn't have consistent behavior for standard library stuff. The codebase had to be kept at a point where it could be compilable at a C89-level. It seems some of that has changed a bit though.

I'll be interested to see what this hashing code looks like - I have a feeling we may be able to use the idea elsewhere.

I could have sworn there was a conversation about using hash tables a few years ago. It's also possible this conversation was with myself during some experiment with the architecture. I'm in favor of hash tables since they're relatively simple to implement and understand.

[calris]

I'll be interested to see what this hashing code looks like

I've pushed it into my sound-devel branch

I have a feeling we may be able to use the idea elsewhere.

It could be helpful anywhere we do repeated searches using the same compare string. For example:

chunk_list_remove(), chunk_find_name(), - Add a hash to struct chunk

Probably not going to gain much here - chunk_list_remove(() is never called, and chunk_find_name() is only used to find the "town" chunk

cave_profile() - Add a hash to struct cave_profile
quark_add()- Would require creating a 'quark' struct to hold the name and hash

[calris]

I just push a new commit into sound-devel which migrates Windows to the new sound architecture.

It probably won't compile, let alone work

Can someone try and compile it and tell me what I need to do to get it working properly? All the sound code is grouped together starting at line 1173. We can move this out to snd-win.c later

This patch also pushes the iteration of file extensions into sound-core.c. Each platform sound module simply needs to declare the supported_sound_files array and sound-core will take care of the rest.

EDIT: About the empty commit 45d83fc - I think it has something to do with me using stg (stacked git)

[Nick]

I just push a new commit into sound-devel which migrates Windows to the new sound architecture.

It probably won't compile, let alone work

Correct

Linux says

/home/worker/base/linux-master/build/src/sound-core.c:145: undefined reference to `supported_sound_files'
/home/worker/base/linux-master/build/src/sound-core.c:148: undefined reference to `supported_sound_files'
/home/worker/base/linux-master/build/src/sound-core.c:151: undefined reference to `supported_sound_files'
/home/worker/base/linux-master/build/src/sound-core.c:141: undefined reference to `supported_sound_files'

Windows says

In file included from main-win.c:87:0:
snd-win.h:21:28: warning: ‘struct sound_hooks’ declared inside parameter list
 errr init_sound_win(struct sound_hooks *hooks, int argc, char **argv);
                            ^
snd-win.h:21:28: warning: its scope is only this definition or declaration, which is probably not what you want
main-win.c:1180:30: error: array type has incomplete element type
 const struct sound_file_type supported_sound_files[] = { {".mp3", WIN_MP3},
                              ^
main-win.c:1186:2: error: unknown type name ‘win_sound_type’
  win_sound_type type;
  ^
main-win.c:1194:72: warning: ‘struct sound_data’ declared inside parameter list
 static bool load_sound_win(const char *filename, int file_type, struct sound_data *data)
                                                                        ^
main-win.c: In function ‘load_sound_win’:
main-win.c:1198:30: error: dereferencing pointer to incomplete type
  sample = (win_sample *)(data->plat_data);
                              ^
main-win.c:1206:15: error: ‘win_sample’ has no member named ‘device’
    if (!sample->device) {
               ^
main-win.c:1207:5: error: ‘op’ undeclared (first use in this function)
     op.dwCallback = 0;
     ^
main-win.c:1207:5: note: each undeclared identifier is reported only once for each function it appears in
main-win.c:1216:8: error: dereferencing pointer to incomplete type
    data->loaded =  (NULL != sample->op.wDeviceID);
        ^
main-win.c:1216:26: warning: comparison between pointer and integer
    data->loaded =  (NULL != sample->op.wDeviceID);
                          ^
main-win.c:1218:13: error: dereferencing pointer to incomplete type
    if (!data->loaded) {
             ^
main-win.c:1219:55: error: dereferencing pointer to incomplete type
     plog_fmt("Sound: Failed to load sound '%s')", data->name);
                                                       ^
main-win.c:1230:20: error: ‘win_sample’ has no member named ‘filname’
    my_strcpy(sample->filname, filename, strlen(filename) + 1)
                    ^
main-win.c:1231:4: error: expected ‘;’ before ‘data’
    data->loaded = true;
    ^
main-win.c:1236:8: error: dereferencing pointer to incomplete type
    data->loaded = false;
        ^
main-win.c:1240:6: error: dereferencing pointer to incomplete type
  data->plat_data = (void *)sample;
      ^
main-win.c: At top level:
main-win.c:1248:35: warning: ‘struct sound_data’ declared inside parameter list
 static bool play_sound_win(struct sound_data *data)
                                   ^
main-win.c: In function ‘play_sound_win’:
main-win.c:1252:42: error: dereferencing pointer to incomplete type
  win_sample *sample = (win_sample *)(data->plat_data);
                                          ^
main-win.c:1257:15: error: ‘win_sample’ has no member named ‘device’
     if (sample->device) {
               ^
main-win.c:1264:4: error: case label not within a switch statement
    case WIN_WAV:
    ^
main-win.c:1273:4: error: break statement not within loop or switch
    break;
    ^
main-win.c:1276:1: warning: no return statement in function returning non-void [-Wreturn-type]
 }
 ^
main-win.c: At top level:
main-win.c:1278:37: warning: ‘struct sound_data’ declared inside parameter list
 static bool unload_sound_win(struct sound_data *data)
                                     ^
main-win.c: In function ‘unload_sound_win’:
main-win.c:1280:42: error: dereferencing pointer to incomplete type
  win_sample *sample = (win_sample *)(data->plat_data);
                                          ^
main-win.c:1285:15: error: ‘win_sample’ has no member named ‘device’
     if (sample->device)
               ^
main-win.c:1286:27: error: ‘win_sample’ has no member named ‘device’
      mciSendCommand(sample->device, MCI_CLOSE, MCI_NOTIFY)
                           ^
In file included from /usr/share/mingw-w64/include/winnt.h:9:0,
                 from /usr/share/mingw-w64/include/minwindef.h:146,
                 from /usr/share/mingw-w64/include/windef.h:8,
                 from /usr/share/mingw-w64/include/windows.h:69,
                 from main-win.c:154:
main-win.c:1286:6: error: too few arguments to function ‘mciSendCommandA’
      mciSendCommand(sample->device, MCI_CLOSE, MCI_NOTIFY)
      ^
In file included from main-win.c:180:0:
/usr/share/mingw-w64/include/mmsystem.h:1791:28: note: declared here
   WINMMAPI MCIERROR WINAPI mciSendCommandA(MCIDEVICEID mciId,UINT uMsg,DWORD_PTR dwParam1,DWORD_PTR dwParam2);
                            ^
main-win.c:1287:7: error: expected ‘;’ before ‘Mix_FreeChunk’
       Mix_FreeChunk(sample->sample_data.chunk);
       ^
main-win.c:1300:7: error: dereferencing pointer to incomplete type
   data->plat_data = NULL;
       ^
main-win.c:1301:7: error: dereferencing pointer to incomplete type
   data->loaded = false;
       ^
main-win.c: At top level:
main-win.c:1320:35: warning: ‘struct sound_hooks’ declared inside parameter list
 static bool init_sound_win(struct sound_hooks *hooks, int argc, char **argv)
                                   ^
main-win.c:1320:13: error: conflicting types for ‘init_sound_win’
 static bool init_sound_win(struct sound_hooks *hooks, int argc, char **argv)
             ^
In file included from main-win.c:87: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);
      ^
main-win.c: In function ‘init_sound_win’:
main-win.c:1322:7: error: dereferencing pointer to incomplete type
  hooks->open_audio_hook = open_audio_win;
       ^
main-win.c:1323:7: error: dereferencing pointer to incomplete type
  hooks->close_audio_hook = close_audio_win;
       ^
main-win.c:1324:7: error: dereferencing pointer to incomplete type
  hooks->load_sound_hook = load_sound_win;
       ^
main-win.c:1325:7: error: dereferencing pointer to incomplete type
  hooks->unload_sound_hook = unload_sound_win;
       ^
main-win.c:1326:7: error: dereferencing pointer to incomplete type
  hooks->play_sound_hook = play_sound_win;
       ^
main-win.c: In function ‘hook_quit’:
main-win.c:4824:9: warning: unused variable ‘j’ [-Wunused-variable]
  int i, j;
         ^
main-win.c: In function ‘WinMain’:
main-win.c:5140:2: warning: implicit declaration of function ‘init_sound’ [-Wimplicit-function-declaration]
  init_sound("win", 0, NULL);
  ^
main-win.c: At top level:
main-win.c:879:13: warning: ‘tokenize_whitespace’ defined but not used [-Wunused-function]
 static s16b tokenize_whitespace(char *buf, s16b num, char **tokens)
             ^
main-win.c:1320:13: warning: ‘init_sound_win’ defined but not used [-Wunused-function]
 static bool init_sound_win(struct sound_hooks *hooks, int argc, char **argv)
             ^

and OS X says

Undefined symbols for architecture i386:
  "_supported_sound_files", referenced from:
      _load_sound in angband.o
ld: symbol(s) not found for architecture i386
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Hope that helps.

[calris]

Well the Linux build issue has me stumped... I did the following

make distclean
./configure --with-no-install --enable-sdl-mixer 
make

And it compiles fine

[calris]

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

[myshkin]

I think the best option is to make snd-sdl a module in it's own right (i.e. not a sub-module of sound-core) and ditch the '-s' switch (instead use -msnd-sdl) so then we could have something like:

angband -mx11 -- -n6 -msnd-sdl -- -pn -fogg

to run a 6 window x11 display, SDL sounds, no preloading of sounds (-pn), forcing loading of ogg files

I'm not sure if it makes sense to make the sound module a main module because that would mean having multiple main modules. Surely you can get the same sound option behaviour using -s as you can using -m?

That reminds me...one of these days, we should give main.c a slightly better argument parser so as to remove the need for the -- separator.

[AnonymousHero]

I could have sworn there was a conversation about using hash tables a few years ago. It's also possible this conversation was with myself during some experiment with the architecture. I'm in favor of hash tables since they're relatively simple to implement and understand.

You may want to re-read some of the later posts. This is not about hash tables!

All it is, really, is a way to avoid unnecessary strcmp() calls and the associated pointer chasing.

[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