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

Libtimidity could be trivially enabled to play music through mixer.cpp #7333

Closed
rsn8887 opened this issue Mar 6, 2019 · 10 comments
Closed

Comments

@rsn8887
Copy link

rsn8887 commented Mar 6, 2019

Version of OpenTTD

1.9.0 beta git from March 03 2019

Expected result

Configuring via --with-libtimidity finds the lib and enables the libtimidity.cpp source code. However, the music data is never passed back to mixer.cpp.

Actual result

Just like what is already done with fluidsynth, the libtimidity sample stream should be passed back (via callback) to mixer.cpp so it becomes audible in the game

Steps to reproduce

Compile with libtimidity. Observe no music.

Here is some example code that could fix this (guarded for Switch hardware because of my testing):
https://github.com/rsn8887/OpenTTD/blob/88398068e99232993839346111ed72cfad7d875e/src/music/libtimidity.cpp#L50

Music plays just fine this way through libtimidity, at least on my Switch.

If there's interest, I could make a quick PR to enable this in the main codebase.

@PeterN
Copy link
Member

PeterN commented Mar 6, 2019

libtimidity support has been removed in master by #7326

@rsn8887
Copy link
Author

rsn8887 commented Mar 6, 2019

Oh no, how sad. I am glad I forked before that, or I would have pulled my hair out getting music to play.

The reason for removing it states "it was only available on PSP". But i can verify that is not true at all. It was not limited to PSP, for example it worked for me by enabling it on Switch. It was quite general code and very nice. With just that one added callback function, it could work great on any platform, not just PSP.

I really liked the code because

  • libtimidity is super-easy to port and small with zero dependencies and clear code
  • it can be statically linked, dynamic linking is not available on many mobile platforms.
  • it renders sound perfectly using internal mixer by just adding one callback.
  • this is much easier to port than the current implementation extmidi timidity driver, which depends on some external player / dll being installed on the host system. External players are mostly only available on Win/Unix/Mac and it is generally complicated to get them to work. AFAIK external players are not available on mobile and embedded.
  • I think it is a great idea to render the midi internally using a static lib. It causes it to "just work" without additional installation of external components. Currently, I think only the fluidsynth driver does that. At least that is the only driver that implements that callback. It might be good to keep at least one alternative to fluidsynth.

Thanks for the great project btw. I just got introduced to OpenTTD a year or so ago, when the first Vita port came about. Truly amazing!

@nielsmh
Copy link
Contributor

nielsmh commented Mar 6, 2019

You're welcome to re-add support for libtimidity, since supposedly someone has started maintaining it again. The previous implementation of it was not straightforward to get compiled in and also used on Linux when I tried.

And for your information, while it's true that only the fluidsynth driver currently mixes with the general sound mixer and shares output stream with the sound effects output, on both Windows and Mac the OS default is to use a software synth which will also render in-process to the user's default audio device, and be controlled by the same volume control. I looked a bit in to getting DirectMusic on Windows to also mix audio via the sound effects mixer, and it turned out to be more trouble than worth.

@PeterN
Copy link
Member

PeterN commented Mar 6, 2019

@rsn8887 Yes, if you have a good case for it to be reintroduced, that is acceptable, however have you tried our libfluidsynth support instead which is not being removed.

It is great that you are developing Switch support but obviously we know nothing about your port, so could not take that into consideration. We would also welcome PRs to address porting issues you find, or perhaps going even further.

@rsn8887
Copy link
Author

rsn8887 commented Mar 6, 2019

Thank you for the encouragement. I will do my best. I have not tried libfluidsynth. It also looks great.

Yes, of course, it was just a coincidence that I happened to stumble on this while trying to get music to play on the Switch.

I only just started working on the Switch port and some enhancements to the pre-existing Vita port one week ago. So there would have been no way of having this dialog in time before that libtimidity commit.

@rsn8887
Copy link
Author

rsn8887 commented Mar 6, 2019

For the record, the up-to-date and easy to compile version of libtimidity I used on the Switch and Vita is this one:
https://github.com/sezero/libtimidity

@nikolas
Copy link
Member

nikolas commented Mar 7, 2019

@rsn8887 this updated version of libtimidity isn't available in Debian's package repo. I don't see it in Homebrew either. If we can get libtimidity back into these package repositories, then it might make more sense to consider adding back to OpenTTD.

I've opened a Debian issue for this here: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=921597

@TrueBrain
Copy link
Member

More people want libtimidity back: #7454

I said most what needs to be said there ;) We have no problems readding it, but having some targets that we compile for that uses it would be greatly appreciated ;)

I think all is said about this what we can say for now. If we can help or you need more information etc, let us know. Or drop by on IRC. We are chatty there :)

Tnx!

@rsn8887
Copy link
Author

rsn8887 commented Jul 3, 2019

I am trying to upstream the Switch port but I need libtimidity back. So there’s a platform that needs it. It would help my efforts greatly if the person who removed libtimidity here could re-add it, because the missing libtimidity is causing a hell of merge errors for me during my tries to rebase.

@LordAro
Copy link
Member

LordAro commented Jul 3, 2019

Reverting f1a298c should fix most of your rebase issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants