RFC: Reworking sound sub-system

Collapse
X
 
  • Time
  • Show
Clear All
new posts
  • redlumf
    Scout
    • Aug 2015
    • 25

    #46
    Recently for a toy project I needed a hash function.



    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)

    Comment

    • calris
      Adept
      • Mar 2016
      • 194

      #47
      Originally posted by redlumf
      Recently for a toy project I needed a hash function.



      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

      Comment

      • redlumf
        Scout
        • Aug 2015
        • 25

        #48
        Originally posted by calris
        Why isn't the Amiga port in mainline?
        Dead platform, no maintainer(s), three users...well, maybe four. Pick one!

        Originally posted by calris
        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?

        Comment

        • calris
          Adept
          • Mar 2016
          • 194

          #49
          Originally posted by redlumf
          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

          Comment

          • takkaria
            Veteran
            • Apr 2007
            • 1951

            #50
            Originally posted by calris
            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.
            takkaria whispers something about options. -more-

            Comment

            • Nick
              Vanilla maintainer
              • Apr 2007
              • 9634

              #51
              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.
              One for the Dark Lord on his dark throne
              In the Land of Mordor where the Shadows lie.

              Comment

              • molybdenum
                Apprentice
                • May 2013
                • 84

                #52
                Originally posted by AnonymousHero
                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.

                Originally posted by calris
                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.

                Originally posted by 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.
                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.

                Comment

                • calris
                  Adept
                  • Mar 2016
                  • 194

                  #53
                  Originally posted by Nick
                  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

                  Comment

                  • calris
                    Adept
                    • Mar 2016
                    • 194

                    #54
                    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)

                    Comment

                    • Nick
                      Vanilla maintainer
                      • Apr 2007
                      • 9634

                      #55
                      Originally posted by 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
                      Correct

                      Linux says
                      Code:
                      /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
                      Code:
                      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
                      Code:
                      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.
                      One for the Dark Lord on his dark throne
                      In the Land of Mordor where the Shadows lie.

                      Comment

                      • calris
                        Adept
                        • Mar 2016
                        • 194

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

                        Code:
                        make distclean
                        ./configure --with-no-install --enable-sdl-mixer 
                        make
                        And it compiles fine

                        Comment

                        • calris
                          Adept
                          • Mar 2016
                          • 194

                          #57
                          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

                          Comment

                          • myshkin
                            Angband Devteam member
                            • Apr 2007
                            • 334

                            #58
                            Originally posted by takkaria
                            Originally posted by calris
                            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.

                            Comment

                            • AnonymousHero
                              Veteran
                              • Jun 2007
                              • 1393

                              #59
                              Originally posted by molybdenum
                              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.

                              Comment

                              • calris
                                Adept
                                • Mar 2016
                                • 194

                                #60
                                Originally posted by AnonymousHero
                                (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

                                A free, single-player roguelike dungeon exploration game - Add hashing to quarks calris/angband@fa582bd

                                Comment

                                Working...
                                😀
                                😂
                                🥰
                                😘
                                🤢
                                😎
                                😞
                                😡
                                👍
                                👎