Music

Do you want to help out with Freeciv development? Then check out this forum.
Post Reply
jwrober
Posts: 35
Joined: Thu Jul 11, 2019 2:05 pm

Music

Post by jwrober »

I've been testing out an issue with the new musicset feature of 2.6.x, previously mentioned in the Patching post a few weeks ago.

The main issue is that while in either peace or combat, only one song plays and then the music stops. I added a couple lines of log_verbose() to client/audio.c to see if I could shed some light on what is going on. Here are my observations. I think there is a bug, but wanted to discuss here before posting to HRM. Here is a diff of my change in audio_play_tag():

Code: Select all

--- audio.c.orig        2019-12-18 07:55:54.000000000 -0600
+++ audio.c     2020-02-06 10:35:48.095611512 -0600
@@ -414,6 +414,8 @@
 
   if (sfile) {
     soundfile = secfile_lookup_str(sfile, "files.%s", tag);
+    log_verbose("soundfile = %s", soundfile);
+
     if (soundfile == NULL) {
       const char *files[MAX_ALT_AUDIO_FILES];
       int excluded = -1;
@@ -424,6 +426,8 @@
       for (i = 0; i < MAX_ALT_AUDIO_FILES; i++) {
         const char *ftmp = secfile_lookup_str(sfile, "files.%s_%d", tag, i);
 
+       log_verbose("excluded = %i, i = %i, j = %i, tag = %s", excluded, i, j, tag);
+
         if (ftmp == NULL) {
           if (excluded != -1 && j == 0) {
             /* Cannot exclude the only track */
After a recompile w/ debug enabled, I have captured log entries to show the new log details. Starting with game start, using default ruleset, selecting Romans. Everything looks good so far.

Code: Select all

4: [T000 - 2020/02/06 10:50:03] in send_packet_player_info_100() [packets_gen.c::11217]:   field 'mood' has changed
4: in receive_packet_player_info_100() [packets_gen.c::10581]:   got field 'mood'
4: in audio_play_sound() [audio.c::509]: audio_play_sound('e_game_start', '(null)')
3: in audio_play_tag() [audio.c::417]: soundfile = (null)
3: in audio_play_tag() [audio.c::429]: excluded = -1, i = 0, j = 0, tag = e_game_start
3: in audio_play_tag() [audio.c::466]: No sound file for tag e_game_start
3: in audio_play_sound() [audio.c::514]: Neither of tags e_game_start or (null) found
4: in real_audio_play_music() [audio.c::530]: audio_play_music('music_classical_peace', '(null)')
Continuing we see the for loop iterate through all 5 possibilities without breaking. The 5 possibilities are set by MAX_ALT_AUDIO_FILES defined in client/audio.h. I think that number could be bigger, but not relevant to this discussion.

Code: Select all

3: in audio_play_tag() [audio.c::417]: soundfile = (null)
3: in audio_play_tag() [audio.c::429]: excluded = -1, i = 0, j = 0, tag = music_classical_peace
3: in audio_play_tag() [audio.c::429]: excluded = -1, i = 1, j = 1, tag = music_classical_peace
3: in audio_play_tag() [audio.c::429]: excluded = -1, i = 2, j = 2, tag = music_classical_peace
3: in audio_play_tag() [audio.c::429]: excluded = -1, i = 3, j = 3, tag = music_classical_peace
3: in audio_play_tag() [audio.c::429]: excluded = -1, i = 4, j = 4, tag = music_classical_peace
The code eventually exists the for loop, continues to pick a random value and plays a song

Code: Select all

4: in fc_rand_debug() [rand.c::127]: fc_rand(5) = 0 at audio.c:449
3: in sdl_audio_play() [audio_sdl.c::108]: Playing file "/home/jwrober/.freeciv/2.6/SongOfDoom/RB-ASongofIceandFire.ogg" on music channel
At this point I sit and wait for the song to stop. Play a few turns. At Turn 5 I meet an AI and do cease fire and then at Turn 6 purposely break the cease fire causing war:

Code: Select all

4: [T006 - 2020/02/06 11:02:09] in send_packet_player_info_100() [packets_gen.c::11217]:   field 'mood' has changed
4: in receive_packet_player_info_100() [packets_gen.c::10581]:   got field 'mood'
4: in real_audio_play_music() [audio.c::530]: audio_play_music('music_classical_combat', '(null)')
3: in audio_play_tag() [audio.c::417]: soundfile = (null)
3: in audio_play_tag() [audio.c::429]: excluded = -1, i = 0, j = 0, tag = music_classical_combat
3: in audio_play_tag() [audio.c::429]: excluded = -1, i = 1, j = 1, tag = music_classical_combat
3: in audio_play_tag() [audio.c::429]: excluded = -1, i = 2, j = 2, tag = music_classical_combat
3: in audio_play_tag() [audio.c::429]: excluded = -1, i = 3, j = 3, tag = music_classical_combat
3: in audio_play_tag() [audio.c::429]: excluded = -1, i = 4, j = 4, tag = music_classical_combat
4: in fc_rand_debug() [rand.c::127]: fc_rand(5) = 2 at audio.c:449
3: in sdl_audio_play() [audio_sdl.c::108]: Playing file "/home/jwrober/.freeciv/2.6/SongOfDoom/RB-Olympus.ogg" on music channel
Notice that we go into the for loop again and cycle through all five times. Get a new music file to play and then it stops. Two turns later, redo the cease fire:

Code: Select all

4: [T008 - 2020/02/06 11:17:54] in send_packet_player_info_100() [packets_gen.c::11217]:   field 'mood' has changed
4: in receive_packet_player_info_100() [packets_gen.c::10581]:   got field 'mood'
4: in real_audio_play_music() [audio.c::530]: audio_play_music('music_classical_peace', '(null)')
3: in audio_play_tag() [audio.c::417]: soundfile = (null)
3: in audio_play_tag() [audio.c::429]: excluded = -1, i = 0, j = 0, tag = music_classical_peace
3: in audio_play_tag() [audio.c::429]: excluded = -1, i = 1, j = 1, tag = music_classical_peace
3: in audio_play_tag() [audio.c::429]: excluded = -1, i = 2, j = 2, tag = music_classical_peace
3: in audio_play_tag() [audio.c::429]: excluded = -1, i = 3, j = 3, tag = music_classical_peace
3: in audio_play_tag() [audio.c::429]: excluded = -1, i = 4, j = 4, tag = music_classical_peace
4: in fc_rand_debug() [rand.c::127]: fc_rand(5) = 1 at audio.c:449
3: in sdl_audio_play() [audio_sdl.c::108]: Playing file "/home/jwrober/.freeciv/2.6/SongOfDoom/TJ-IntoTheShadows.ogg" on music channel
I continue moving through turns, discover another AI do a cease fire and then break it on purpose again:

Code: Select all

4: [T027 - 2020/02/06 11:37:45] in send_packet_player_info_100() [packets_gen.c::11217]:   field 'mood' has changed
4: in receive_packet_player_info_100() [packets_gen.c::10581]:   got field 'mood'
4: in real_audio_play_music() [audio.c::530]: audio_play_music('music_classical_combat', '(null)')
3: in audio_play_tag() [audio.c::417]: soundfile = (null)
3: in audio_play_tag() [audio.c::429]: excluded = -1, i = 0, j = 0, tag = music_classical_combat
3: in audio_play_tag() [audio.c::429]: excluded = -1, i = 1, j = 1, tag = music_classical_combat
3: in audio_play_tag() [audio.c::429]: excluded = -1, i = 2, j = 2, tag = music_classical_combat
3: in audio_play_tag() [audio.c::429]: excluded = -1, i = 3, j = 3, tag = music_classical_combat
3: in audio_play_tag() [audio.c::429]: excluded = -1, i = 4, j = 4, tag = music_classical_combat
4: in fc_rand_debug() [rand.c::127]: fc_rand(5) = 2 at audio.c:449
3: in sdl_audio_play() [audio_sdl.c::108]: Playing file "/home/jwrober/.freeciv/2.6/SongOfDoom/RB-Olympus.ogg" on music channel
I can eventually get both AI's into peace and the mood changes back and then a peace song will play. However only one song will play if I stay in either peace or combat. Combat does revert to peace on its own somewhere between 11 and 12 turns if I do nothing.

Some questions:
- The for loop seems to be part of code that is never called from audio_finished_callback(), instead all calls come from real_audio_play_music(). audio_finished_callback() gives the number of the song that was played so it can be excluded at next run opportunity. It does not seem to work, however or I have not been able to get the callback to wake up so to speak.
- I cannot figure out why one only song plays unless I force the issue. I cannot tell if its from the for loop deal or something else I have not discovered yet.

Any thoughts on this would be great. I think improving on the soundtrack idea would be a great addition to the game.

Thanks
cazfi
Elite
Posts: 3069
Joined: Tue Jan 29, 2013 6:54 pm

Re: Music

Post by cazfi »

It's unfortunate that you never opened hrm ticket with reference to your musicset. I think I managed to debug this really quickly with the help of your musicset now I saw this, but the fix is unlikely to make it to freeciv-2.6.3 at this point.
cazfi
Elite
Posts: 3069
Joined: Tue Jan 29, 2013 6:54 pm

Re: Music

Post by cazfi »

The ticket -> Bug #905818
cazfi
Elite
Posts: 3069
Joined: Tue Jan 29, 2013 6:54 pm

Re: Music

Post by cazfi »

cazfi wrote:the fix is unlikely to make it to freeciv-2.6.3 at this point.
Update: When I wrote that I was thinking about the convulated music system code where any bugfix has high risk of introducing other regressions, and the limited time this would be in testing before 2.6.3 release.
In the end the fix does not touch those parts of the code, but should be relatively safe to include even with limited testing time. It's in my current 2.6.3 plans (being in current plans does not guarantee being in final plans, of course)
jwrober
Posts: 35
Joined: Thu Jul 11, 2019 2:05 pm

Re: Music

Post by jwrober »

thanks cazfi, I wasn't sure about how to write up an HRM ticket. I can test the patch out on 2.6.2.1 and see how things go. I'd also like to submit a small patch to audio.h.
cazfi
Elite
Posts: 3069
Joined: Tue Jan 29, 2013 6:54 pm

Re: Music

Post by cazfi »

jwrober wrote:The 5 possibilities are set by MAX_ALT_AUDIO_FILES defined in client/audio.h. I think that number could be bigger
That's now Feature #905821.
jwrober
Posts: 35
Joined: Thu Jul 11, 2019 2:05 pm

Re: Music

Post by jwrober »

cazfi wrote:
jwrober wrote:The 5 possibilities are set by MAX_ALT_AUDIO_FILES defined in client/audio.h. I think that number could be bigger
That's now Feature #905821.
I posted a patch for the feature to the dev mailing list. I also tested the patch for the music playback issue and it works great for me.
cazfi
Elite
Posts: 3069
Joined: Tue Jan 29, 2013 6:54 pm

Re: Music

Post by cazfi »

jwrober wrote:I posted a patch for the feature to the dev mailing list.
I hadn't seen that. Now checked the archive. Apparently it was not delivered to gmail addresses due to Google blackout.

Anyway, I've submitted a patch to make the limit 25. Unfortunately it won't get accepted to stable branches with datafile format frozen (musicspec using more than 5 alternative tracks wouldn't work for already released versions), but only to development master.
cazfi
Elite
Posts: 3069
Joined: Tue Jan 29, 2013 6:54 pm

Re: Music

Post by cazfi »

jwrober wrote:I also tested the patch for the music playback issue and it works great for me.
It turned out to be broken. It's not the correct fix (the root-cause of the bug was not what it was thought to be) and it causes music not to stop in some cases. However, I don't think it makes things worse overall. It seemingly fixes one bug, and introduces another. So probably not going to revert it from 2.6.3, given that it's less than 48h to planned release time, and all the 2.6.3 tests so far have already run with the patch in (not going to restart 2.6.3 testing with patch reverted now)
Post Reply