-
Notifications
You must be signed in to change notification settings - Fork 275
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
Implement a reliable port selection API #30
base: master
Are you sure you want to change the base?
Conversation
If you would consider to merge the autotools patch #22 (which I recommend) I'd suggest that you merge that patch first. It is easier for me to adopt my changes to Sebastian's patch set than vice versa. |
Ok. I have merged the last chanegs on the master branch. @garyscavone some remark to your memory leak fix. CFRelease should be called before the error check as error(...) may not return, but the application may continue to run. |
Even though this feature is currently unused we should use a different library version for future development. While we provide workarounds for the API the ABI has changed.
…c ALSA calls. This layer provides an API that is more consistent with the data used by RtMidi. On the other hand it allows to optionally use a locking mechanism for all sequencer calls so that ALSA may use one client for all threads for certain tasks (or if it is requested).
The ALSA output devices should be still considered broken.
The ALSA output devices should be still considered broken. # Conflicts: # RtMidi.cpp
MidiInAlsa::openPort MidiInAlsa::getPortList MidiOutAlsa::getPortList Now, we can do some first tests. Test files follow…
This was broken as we use ALSA addresses to store port ids, now. These are stored as unsigned values.
Signed-off-by: Tobias Schlemmer <keinstein@users.sourcforge.net>
WinMM does not support a good port selection scheme. At least wine seems to ensure reliable port ids.
# Conflicts: # RtMidi.cpp # RtMidi.h
# Conflicts: # RtMidi.cpp # RtMidi.h # configure.ac
…ster-ts3 # Conflicts: # RtMidi.cpp
…ster-ts3 # Conflicts: # RtMidi.cpp # RtMidi.h
# Conflicts: # README.md # RtMidi.cpp # RtMidi.h # configure.ac # doc/doxygen/tutorial.txt
1st step towards better comparability to upstream
This matches the layout of upstream RtMidi.h.
…ster-ts3 # Conflicts: # Makefile.am # README.md # RtMidi.cpp # RtMidi.h # configure.ac # rtmidi_c.cpp # rtmidi_c.h
# Conflicts: # configure.ac
8f24b42
to
3f6ef4e
Compare
I have integrated all changes and bug fixes so far in this fork. I have reorganized the code to better match upstream. Some additional thoughts behind the structure:
In order to not clobber the global namespace more than necessary, everything is encapsulated in a namespace “rtmidi”. The API itself is also kept minimal:
getPortName has been exteded to support different needs of users and programmers. All backends except Windows are organized in two levels: devices/clients and ports. The naming schemes are not consistent. Even not in within one backend. This will be always the case as the naming is left to the device driver or MIDI client. So sometimes the port name contains the client name and sometimes not. Leaving one out, may lead unclear descriptions, while keeping everything leads sometimes to overly long strings, where the important part is hidden because of space or other limitations. This is part is related to #100. Some bugs have been fixed a long time ago, but I had no time to port them to upstream and back. Most of the changes in the structure of the old functions are simply copy & paste operations to a place that looked useful to avoid code duplications or produced clashes with the needs of the new API. Some further simplifications have been intentionally left out to in order to keep some fixed points to align code diffs. Unfortunately, git diff seems to ignore these points. At least it mixes the different APIs in the diffs. The build system has been developed following the Autotools include paradigm. This has been done in parallel with Mutabor development. If someone is willing to play around with these ideas I'm open for discussions. Design decisions have been the result of an evolutionary process and are not carved in stone. But they will be much deeper when I cleaned up the code and removed some further duplications. I could also offer to split up the patch into several parts for reviews (Namespace, getPortList/openPort for each backend separately, other API functions for each backend separately). But I currently don't have the time to keep this bunch of patches in sync with upstream for several years. This time would be better used to update the fork and to fix bugs. |
Hi, Thanks for all that work. What's the advantage of the PortDescriptor scheme over just adding |
There are DAWs that use JACK for its internal routing backend. In fact JACK allows you to store JACK sessions, so that you even don't need a real DAW in order to achieve that. Theoretically you can build up a DAW from hundreds of independent small JACK clients. The same is possible with ALSA. So in the end There are different reasons: I'm thinking about a feature to save and restore port descriptors. But this would be based on numeric ids and/or UUIDs, if possible, so that it allows mostly automatic reconfiguration after a system restart without the problems that arise from reordering devices/modules/plugins/filters during restart. On a linux system you could open three shell windows. Try the following commands (each in a different window:)
You will get something like:
As you can see there are two clients announced as “TiMidity”. Each of them has a port named “TiMidity port 0” so writing “TiMidity port 0”. They can only be differenciated by the port descriptor 128:0 and 129:1. Yes, you can serialize the port descriptor into a string representation. But both represent different information. And users dislike the three-fold redundency when you put everything into one string just to make sure, that the port connection is stable. What I forgot in my last comment is the ALL_API mode, in which case the calling software does not care about the available APIs and can use all ports from all supported interfaces with one library call. In fact the portDescriptor API can easily be extended to provide the necessary information for tree based presentation of MIDI ports as it is commonly used in GUIs like WINE, qjackctl or in the above example of aconnect. Actually, the tree presentation is the best compromise between screen usage and completeness of port names. |
# Conflicts: # README.md # RtMidi.cpp # RtMidi.h # configure.ac
In addition to the last answer danomatika/ofxMidi#21 mentions port name changes due to port renumbering. My approach is simply: Don't try to be more clever than the underlying API. Otherwise you will have to deal with many unexpected side effects. |
The current RtMidi implementation has a race condition that may be very annoying in certain cirumstances.
Steps to reproduce the problem:
The current patch set contains a suggestion for a reliable API.
The main problem that shall be addressed is that the current API does not store port descriptors together with the meta data (port names/port numbers). Thus, both can get out of sync.
The patches are based both on master and winks include:
• Winks patches as well as reverting them to avoid problems synchronizing with the master branch.
• some code reorganization (extract common API into a separate class)
• Add a port descriptor class (With a more flexible port naming interface).
• Add port descriptor API: getPortList, openPort(PortDescriptor &, const std::string&), getPortDescriptor()
• Add Example programs.
• Add an automatic test case for all backends that support virtual devices. This has been inspired by the automatic loopback test case of the node-midi project.
I came across the following questions:
• What's the purpose of class RtMidi? Is it used directly?
• RtMidiIn and RtMidiOut serve as API containers. Why do you use virtual inline functions, here? I'd suggest to remove the virtual classifier from the member functions and declare them only in RtMidi if they are used both by RtMidiIn and RtMidiOut. This would render CommonMidiApi useless and I would reintegrate it into MidiApi. Otherwise the inline functions should be marked extern to avoid linker errors.
• There is no function call ot set or get the current API object from RtMidi(In|Out). This and the fact that the implementation classes Midi(In|Out)(Alsa|Jack|WinMM|WinKS|Core|Dummy) are not documented separately suggest that these classes are private to the RtMidi library. Is that right? In this case I'd suggest to remove these classes from RtMidi.h. Otherwise I'd suggest to add two functions to set and get the API object that is used by the RtMidi(In|Out).
• Maybe… probably… I might add a container API that allows to collect several APIs into one common API.