RFC: Reworking sound sub-system

Collapse
X
 
  • Time
  • Show
Clear All
new posts
  • calris
    Adept
    • Mar 2016
    • 194

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

    • Nick
      Vanilla maintainer
      • Apr 2007
      • 9647

      #62
      Originally posted by 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
      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
      Code:
      #ifndef SOUND_SDL
      const struct sound_file_type supported_sound_files[] = {  {"", SDL_NULL} };
      #endif
      in sound-core.c

      Windows has less errors now:
      Code:
      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.
      One for the Dark Lord on his dark throne
      In the Land of Mordor where the Shadows lie.

      Comment

      • calris
        Adept
        • Mar 2016
        • 194

        #63
        Originally posted by Nick
        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
        Code:
        #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

        Comment

        • calris
          Adept
          • Mar 2016
          • 194

          #64
          Originally posted by Nick
          Windows has less errors now:
          And should have even less now

          Comment

          • takkaria
            Veteran
            • Apr 2007
            • 1951

            #65
            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?
            takkaria whispers something about options. -more-

            Comment

            • calris
              Adept
              • Mar 2016
              • 194

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

              Code:
              parser_reg(p, "sound sym type str sounds", parse_prefs_sound);
              wasn't going to fly. Wrapping #ifdef SOUND around this meant

              Code:
              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

              Comment

              • calris
                Adept
                • Mar 2016
                • 194

                #67
                Originally posted by 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?
                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
                Last edited by calris; March 27, 2016, 13:52.

                Comment

                • redlumf
                  Scout
                  • Aug 2015
                  • 25

                  #68
                  Just curious here. Has anyone profiled the speed impact of the changes?
                  Last edited by redlumf; March 27, 2016, 13:50. Reason: (typo)

                  Comment

                  • calris
                    Adept
                    • Mar 2016
                    • 194

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

                    Comment

                    • takkaria
                      Veteran
                      • Apr 2007
                      • 1951

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

                      Code:
                      parser_reg(p, "sound sym type str sounds", parse_prefs_sound);
                      wasn't going to fly. Wrapping #ifdef SOUND around this meant

                      Code:
                      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.
                      takkaria whispers something about options. -more-

                      Comment

                      • calris
                        Adept
                        • Mar 2016
                        • 194

                        #71
                        Originally posted by takkaria
                        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:
                        Last edited by calris; March 27, 2016, 15:24.

                        Comment

                        • molybdenum
                          Apprentice
                          • May 2013
                          • 84

                          #72
                          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).

                          Comment

                          • Nick
                            Vanilla maintainer
                            • Apr 2007
                            • 9647

                            #73
                            Now compiles cleanly for Linux (although birth tests fail) and OS X, Windows only has one missing #include, by the looks:
                            Code:
                            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
                            One for the Dark Lord on his dark throne
                            In the Land of Mordor where the Shadows lie.

                            Comment

                            • calris
                              Adept
                              • Mar 2016
                              • 194

                              #74
                              Originally posted by Nick
                              Now compiles cleanly for Linux (although birth tests fail)
                              How do I run the tests?

                              Comment

                              • Nick
                                Vanilla maintainer
                                • Apr 2007
                                • 9647

                                #75
                                Originally posted by calris
                                How do I run the tests?
                                You have to have configured with --enable-test, then
                                Code:
                                ./run-tests
                                for birth tests,
                                Code:
                                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.
                                One for the Dark Lord on his dark throne
                                In the Land of Mordor where the Shadows lie.

                                Comment

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