Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Music ms_after hangs scenario story screen #4030

Open
slavrenyuk opened this issue Apr 18, 2019 · 13 comments

Comments

Projects
None yet
3 participants
@slavrenyuk
Copy link

commented Apr 18, 2019

I will describe 2 buggy use cases below. The bug only affects story screen, during a scenario ms_after works well.

a) ms_after setting for music tag directly under campaign tag hangs story starting
Steps to reproduce

  1. Go to <Wesnoth directory>/data/core/macros/sound-utils.cfg
  2. In SCENARIO_MUSIC macro add ms_after=10000 for intro music "revelation.ogg"
  3. Launch the game, start "Son of the Black-Eye" campaign (it uses that macro)
  4. There is a 10 second lag before you can move through the story pages

b) ms_after setting for music in a prestart event hangs story ending
Steps to reproduce

  1. Go to <Wesnoth directory>/data/core/macros/sound-utils.cfg
  2. In SCENARIO_MUSIC macro change ms_after=2000 with ms_after=10000 for a prestart event music
  3. Launch the game, start "Son of the Black-Eye" campaign (it uses that macro)
  4. Finish reading the story (or skip it)
  5. There is a 10 second lag before the scenario starts
@jyrkive

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

As unfortunate as it is, this is currently by design. Hence the enhancement label.

Mix_FadeOutMusic() blocks the main thread during the fade-out in this line:

Mix_FadeOutMusic(fadingout_time);

SDL_Mixer isn't documented to be thread safe, so we can't move that call into a worker thread.

The only way to keep fade-outs from hanging the main thread would be a full rewrite of the sound engine.

@slavrenyuk

This comment has been minimized.

Copy link
Author

commented Apr 18, 2019

Thanks for the quick reply.

@jyrkive

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

Sure thing. (It's easy to reply quickly because we get real-time notifications when new issues are filed, and I'm familiar with sound code.)

@slavrenyuk

This comment has been minimized.

Copy link
Author

commented Apr 18, 2019

ms_before doesn't cause similar bug because of this line?

// FIXME: we don't pause ms_before on this first track. Should we?

There is still something strange about ms_after. If I set ms_after for the intro music, it hangs before the intro music starts to play. And if I set ms_after in the prestart event, it hangs before that music starts to play. Doesn't it mean that the sound engine takes ms_after of the wrong track?

@jyrkive

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

ms_before doesn't cause similar bug because of this line?

From what I can tell with code reading, ms_before should also block the main thread. I can investigate this in the evening.

There is still something strange about ms_after. If I set ms_after for the intro music, it hangs before the intro music starts to play. And if I set ms_after in the prestart event, it hangs before that music starts to play. Doesn't it mean that the sound engine takes ms_after of the wrong track?

Indeed, it seems to use ms_after of the new track.

@slavrenyuk

This comment has been minimized.

Copy link
Author

commented Apr 18, 2019

I would like to update the music related wiki page with the information that ms_before and ms_after must not be used for music tag directly under the campaign tag or it will lead to a story screen lag. Ok?

@jyrkive

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

That's fine for me.

Do you have a wiki account, though? If not, it would be easier if I were to update it myself.

@slavrenyuk

This comment has been minimized.

Copy link
Author

commented Apr 18, 2019

I have a wiki account.

@jyrkive

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

All right, go ahead. 👍

@jyrkive

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

I fixed ms_after setting fade-out time of the wrong track in commit b023b81.

Regarding ms_before not blocking the main thread, a quick check in a debugger shows that Mix_FadeInMusic() does not block.

@slavrenyuk

This comment has been minimized.

Copy link
Author

commented Apr 18, 2019

I think we can close this ticket after your fix and my update on the wiki page https://wiki.wesnoth.org/index.php?title=MusicListWML&type=revision&diff=60956&oldid=56285
It doesn't make sense to rewrite the sound engine only because of the ms_after key of a story music.

@jyrkive

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

That would be true if music fade-out was the only reason, but there are also other things which would be nice such as music crossfade. And seriously, SDL_Mixer should never have been used in the first place: it's just a sample audio mixer library, not intended for serious use.

I think it's better to leave this issue open. It's already labeled as an enhancement request.

@soliton-

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

It could use a better title if it's supposed to be a general "replace SDL_Mixer" issue though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.