Small extension to the interactive interface

[parlance]

Hello libopenmpt users and developers.

I am developing a project where libopenmpt is being used as the primary sound engine for both music and interactive sound effects. While the existing interactive API was good, there were a few problems with it:

• Some of the interactive functions are safe to use asynchronously, but the most important ones (play_note, stop_note) were not. Safe asynchronous operation for the interactive API vastly simplifies implementation in my case.
• A few basic functions for getting and setting channel panning or a "pitch factor" were conspicuously missing.

My changes:

• play_note now explicitly checks the channel mix list for the module and will not use a currently mixing channel, this removes the need for the bug-fix that modified the mix list in the function and allows safe asynchronous operation.
• play_note now no longer looks for fading channels, as those channels in the mix list, AND cutting off a channel if there are completely free ones further down the list is not ideal.
• stop_note now no longer nulls the current sample pointer for the channel, and instead works more like the new note_off function where it will set the channel note_off and fade out, but then also set the volume to 0. This is required for safe asynchronous operation.
• Added the missing note_off function so I can actually use instrument envelopes interactively.
• Added the missing functions for channel pitch factor and panning. Channel "pitch-factor" works by setting the "C-5" speed of the channel based on it's sample and the pitch factor.

I read the contributing doc and I realize this is not the appropriate channel. I am not a regular open source developer and I simply wanted to share these modifications both because I am required to if I ship the project, but also because someone else might find a use for it, even if this isn't merged. While I followed the code's naming conventions I didn't follow your formatting style, which quite frankly I hate .

Lastly, thank you for the hard work on libopenmpt.

Edit: I did find a few more problems now that I am using the interactive interface more. "Global" volume and pitch factors don't affect OPL instruments for example, if I find more I will go back and try to fix some of them.

[Saga Musix]

Welcome, and many thanks for your contribution. I will look into it in greater detail in the next few days, but some parts will definitely need some rework.

play_note now no longer looks for fading channels, as those channels in the mix list, AND cutting off a channel if there are completely free ones further down the list is not ideal.

Fading channels were only supposed to be a backup when no other free channels were found, so the current code is buggy (definitely not doing what it's originally meant to do). I think in the end this code should simply be replaced by a call to CSoundFile::GetNNAChannel, as the logic there is tried and proven.

Added the missing functions for channel pitch factor and panning. Channel "pitch-factor" works by setting the "C-5" speed of the channel based on it's sample and the pitch factor.

The pitch factor is an interesting idea, but it will require more work because it will only work with frequency-based formats that use nC5Speed, but not period-based formats that use finetune + transpose.

"Global" volume and pitch factors don't affect OPL instruments for example, if I find more I will go back and try to fix some of them.

The pitch factor is now fixed. However, I don't see how global volume wouldn't work with OPL instruments, as it just updates the same variable as a Vxx effect in the pattern would do. Do you have an example where it doesn't work?

[parlance]

Welcome, and many thanks for your contribution. I will look into it in greater detail in the next few days, but some parts will definitely need some rework.

play_note now no longer looks for fading channels, as those channels in the mix list, AND cutting off a channel if there are completely free ones further down the list is not ideal.

Fading channels were only supposed to be a backup when no other free channels were found, so the current code is buggy (definitely not doing what it's originally meant to do). I think in the end this code should simply be replaced by a call to CSoundFile::GetNNAChannel, as the logic there is tried and proven.

As for cutting off fading channels, this is the offending bit of code in 0.5.4 libopenmpt_ext_impl.cpp: play_note:

Code Select Expand

// Find a free channel
CHANNELINDEX free_channel = MAX_CHANNELS - 1;
// Search for available channel
for(CHANNELINDEX i = MAX_CHANNELS - 1; i >= get_num_channels(); i--)
{
const ModChannel &chn = m_sndFile->m_PlayState.Chn[i];
if ( chn.nLength == 0 ) {
free_channel = i;
break;
} else if ( chn.dwFlags[CHN_NOTEFADE] ) {
// We can probably still do better than this.
free_channel = i;
}
}
The channel list is only searched in one pass, and if nLength isn't 0 it will accept any channel with the CHN_NOTEFADE flag. This is what I wanted to remove. I agree that this function essentially duplicates NNA functionality, I will take a look at GetNNAChannel when I get back to my home office. It is important to me to preserve safe asynchronous operation for the interactive interface, so if GetNNAChannel doesn't check if a channel is in the current mix list then I still need at least that check.

Added the missing functions for channel pitch factor and panning. Channel "pitch-factor" works by setting the "C-5" speed of the channel based on it's sample and the pitch factor.

The pitch factor is an interesting idea, but it will require more work because it will only work with frequency-based formats that use nC5Speed, but not period-based formats that use finetune + transpose.

I did notice that my "channel pitch factor" didn't work on OPL instruments after I posted. Originally I wasn't going to care as I intend to use samples exclusively, but the libopenmpt code is clean and compact enough that fixing this shouldn't be difficult.

"Global" volume and pitch factors don't affect OPL instruments for example, if I find more I will go back and try to fix some of them.

The pitch factor is now fixed. However, I don't see how global volume wouldn't work with OPL instruments, as it just updates the same variable as a Vxx effect in the pattern would do. Do you have an example where it doesn't work?

The example module "Yuzu - Yu-Lib.mptm" was the module I was using when I tested this.

[parlance]

I've attached an updated version that hopefully addresses your concerns.

• The logic for checking the mix list was moved into the GetNNAChannel function, disabled by a default boolean parameter.
• play_note was updated to use GetNNAChannel as described, but with mix list checking enabled (it IS the "interactive" API after-all). This also still removes the need for the mix-list bug-fix hack at the end of the function.
• Channel pitch factors recalculate the channel's nPeriod using it's current fine tuning, to comply with that. If instruments respect this in ReadNote (do they?) then it should work for any instrument.

I would appreciate getting this merged as I would like to get the global pitch factor fix. I have a sneaky suspicion it may have had something to do with nPeriod .

[parlance]

I cleaned up the code, added the appropriate exception checks and doc strings, and fixed a few mismatches in naming convention. I also actually attached all the files I modified this time. Oops. I noticed that play_note doesn't seem to work with OPL instruments as well... not sure what the issue might be since the OPL play_patch function is called in ReadNote whenever the instrument changes, and the play_note function was already flagging that through SetInstrument.

Thanks again for helping me out.

[manx]

I am not a regular open source developer and I simply wanted to share these modifications

Please suggest changes as a patch in unified diff format https://en.wikipedia.org/wiki/Diff#Unified_format instead of copies of source files which give no clue as to which code revision they are based on. Every source code management system can generate patches.

because I am required to if I ship the project

We of course welcome your contributions, but the libopenmpt license (BSD-3-Clause) does not actually require you to share any source, even if you are shipping binaries.

I did not look into the changes in detail yet, however a few things I noticed so far:

You cannot extend existing interface definitions in libopenmpt_ext by adding virtual functions (C++) or functions pointers (C). This breaks binary compatibility when compiling against a newer version but runtime linking against an older version. You need to add a new separate interface "interactive2" in addition to the existing ones.

While I followed the code's naming conventions I didn't follow your formatting style, which quite frankly I hate

This really is not a good reason. Please stick to established code formatting in surrounding code.

[parlance]

I am not a regular open source developer and I simply wanted to share these modifications

Please suggest changes as a patch in unified diff format https://en.wikipedia.org/wiki/Diff#Unified_format instead of copies of source files which give no clue as to which code revision they are based on. Every source code management system can generate patches.

because I am required to if I ship the project

We of course welcome your contributions, but the libopenmpt license (BSD-3-Clause) does not actually require you to share any source, even if you are shipping binaries.

I did not look into the changes in detail yet, however a few things I noticed so far:

You cannot extend existing interface definitions in libopenmpt_ext by adding virtual functions (C++) or functions pointers (C). This breaks binary compatibility when compiling against a newer version but runtime linking against an older version. You need to add a new separate interface "interactive2" in addition to the existing ones.

While I followed the code's naming conventions I didn't follow your formatting style, which quite frankly I hate

This really is not a good reason. Please stick to established code formatting in surrounding code.

Okay, but I don't really agree with that kind of design decision. I'll keep my changes in my own fork.

[manx]

I am not a regular open source developer and I simply wanted to share these modifications

Please suggest changes as a patch in unified diff format https://en.wikipedia.org/wiki/Diff#Unified_format instead of copies of source files which give no clue as to which code revision they are based on. Every source code management system can generate patches.

because I am required to if I ship the project

We of course welcome your contributions, but the libopenmpt license (BSD-3-Clause) does not actually require you to share any source, even if you are shipping binaries.

I did not look into the changes in detail yet, however a few things I noticed so far:

You cannot extend existing interface definitions in libopenmpt_ext by adding virtual functions (C++) or functions pointers (C). This breaks binary compatibility when compiling against a newer version but runtime linking against an older version. You need to add a new separate interface "interactive2" in addition to the existing ones.

While I followed the code's naming conventions I didn't follow your formatting style, which quite frankly I hate

This really is not a good reason. Please stick to established code formatting in surrounding code.

Okay, but I don't really agree with that kind of design decision. I'll keep my changes in my own fork.

Which design decision specifically do you not agree with? I am somewhat confused.

[parlance]

Binary compatibility is important for libraries that might actually be used in a shared context, like for example the OpenGL DLL that ships with Windows. 99.999% of applications that would link with something like libopenmpt would ship the DLL with the application.

There is a non-zero cost to maintaining that compatibility (the pain of having to write my function signatures in slightly different formats across half a dozen different files, the general messiness of having something like "interactive" and "interactive2" functionally separate from each other, developers have to initialize both, check which interface version has the functions they need, etc.) Developers going forward have to deal with that irritation and messiness forever, while the proportion of people who benefit from it slowly shrinks from 0.001% to 0.0%.

As I stated in my first post, my goal is not to become a contributor, I just wanted to share a useful modification that I intend to use myself (though I have seen feature requests for precisely what I've added on your forums).

[Saga Musix]

I had a look at the modified patch. Please note that libopenmpt is only guaranteed to be thread-safe in the sense that you can render different modules in different threads. However, calls on the same openmpt::module object all must come from the same thread or must be synchronized by the caller. This is also how OpenMPT's multithreading works and I'm really no fan of declaring that some library function may potentially be thread-safe (and then break accidentally in a later revision). All those modifications for checking the current mix list are not required if all calls to the same openmpt::module are synchronized (e.g. with a mutex). Given this precondition, I think that it's more important that libopenmpt's play_note function tries as hard as possible to find a playable channel (including fading channels) than being maybe a little bit more thread-safe than it was before. Of course you can do whatever you like to do in your own fork, however this unfortunately means that I cannot merge the play_note / GetNNAChannel modification as-is. I will of course fix play_note so that it doesn't steal just any fading channel, but it will keep using fading channels as a backup if no other free channels are found. I would really recommend that you wrap your libopenmpt function calls in a shared_mutex or similar if you expect them to origin from different channels, or send all requests for libopenmpt state manipulation to a single thread - anything else has no guarantee that it won't break at some point, and isn't even guaranteed to work on all platforms with your current code (e.g. this code is almost guaranteed to not work as intended with ARM's weak memory model if called from more than one thread).

On to the other suggestions / submissions: I really like the idea of being able to change the pitch of the note (presumably for sound effects?), I think the updated code should work with all formats in the sense that a change will be heard, however it will still not be correct in all cases (depending on the format, a lower nPeriod value may indicate a higher note, or it might indicate a lower note... modules are messy). You don't need to spend any time on fixing this though; I can do this before merging your changes.

The example module "Yuzu - Yu-Lib.mptm" was the module I was using when I tested this.

Can you provide some example code how you play the module? Because here it works just fine, in particular with this module. There is no reason why it should work only for samples but not for OPL instruments. The only reason why it could potentially not work is using MPT 1.16 or OpenMPT 1.17RC1 mix modes (CSoundFile::m_nMixLevels), but it is not possible to choose those in any OpenMPT version that uses OPL instruments (and in particular Yuzu's track doesn't use them).

[parlance]

I did look through ReadNote when I was trying to determine if asynchronous usage could cause any problems without explicit synchronization, but my assumption was enforced memory / cache coherence across threads. I understand the implications for other (inferior ) platforms so I understand not wanting to go that way. I will use explicit synchronization primitives.

I tried and was unable to replicate the former issue I had with OPL instruments not respecting global volume. It may have been a mistake on my part, or possibly related to asynchronous access. Either way, sorry to waste your time.

Thanks also for being willing to work with me in spite of my inexperience contributing. If I use explicit synchronization primitives then I should be able to wait for next revision and avoid forking with these changes included. If I find anything else I need to change I will submit the diffs rather than copies of code files.

Thanks again.

[manx]

Binary compatibility is important for libraries that might actually be used in a shared context, like for example the OpenGL DLL that ships with Windows. 99.999% of applications that would link with something like libopenmpt would ship the DLL with the application.

This assumption is wrong on Linux, macOS (Homebrew, Macports, Fink), all BSD platforms, and also on Windows when using vcpkg. libopenmpt maintains binary compatibility since version 0.2, as documented. This is not something that we will change.

Developers going forward have to deal with that irritation and messiness forever, while the proportion of people who benefit from it slowly shrinks from 0.001% to 0.0%.

Applications do have to check for libopenmpt_ext interface availability. libopenmpt_ext exists precisely for us to be able to avoid having to support old interfaces forever; they can get removed in future versions. Applications must thus expect that getting any particular interface pointer may return nullptr/NULL and that a particular interface might not be declared at compile-time by checking for the corresponding macro. This needs better documentation, though.
If libopenmpt_ext did not exist and any _ext interface would be added as a regular libopenmpt interface function, we would never be able to remove these somewhat experimental (and most of them actually are experimental features for which cleaner interfaces are envisioned for the future) features in the future without breaking each and every binary distribution.

Without this "messiness" you dislike so much, libopenmpt_ext would not exist, and you would thus not have any interface for doing any interactive things with libopenmpt. You would not be able to implement your application at all.

Not caring about binary compatibility, like you suggest, would also significantly increase the amount of work for 100% of all package maintainers on each and every libopenmpt update/release.

You may very well consider bianry compatibility useless for your particular usecases, however in reality it is not for a lot of other people.

I did look through ReadNote when I was trying to determine if asynchronous usage could cause any problems without explicit synchronization, but my assumption was enforced memory / cache coherence across threads. I understand the implications for other (inferior ) platforms so I understand not wanting to go that way.

Other architecture's memory coherency model is not "inferior" to x86's. If at all, x86 is inferior because it makes certain parallel algorithms difficult or impossible to implement in a truely scalable manner because the implicit overhead of the coherence protocol hinders scalability. However, none of this actually matters here. Unless explicit synchronization primitives are used, the C++ language allows the compiler to reorder EVERYTHING as long as single-thread-observable effects remain unchanged. You cannot make any assumptions about multi-threaded behaviour here.


If you suggest code for inclusion in libopenmpt, it is in your own interest to follow our design decisions (and not to repeatedly declare them wrong based on incomplete or false assumptions). If you do not, you decreae the chance of getting your changes included because it in turn increases the amount of work that we libopenmpt maintainers would have to put into it.

[Saga Musix]

I forgot to mention here, the NNA bugfix has found its way into libopenmpt in the meantime, and I also have a commit ready that will add most of the suggested APIs. One that is currently still missing is the one for changing the channel pitch. As I wanted to avoid duplicate code for this feature that will just be used by libopenmpt_ext, I thought that this new API could be based on the recently-added high-precision finetune commands. The resulting API semantics could end up slightly different, though:
The effective depth of the Set Finetune command can be configured individually for each instrument. It's at least +/-1 semitone, but every instrument can in theory have a different finetune depth. I see this both as an upside and a downside:
- Depending on how the module instruments are set up, it would be more difficult to consistently change the pitch for several instruments.
- However, this can also be seen as an advantage: If the API is intended to be used for adding variations to sound effects, you can set up the optimal pitch range for each individual sound effect in the module.

As a compromise, the depth could be restricted to +/-1 semitone for all instruments at the API level, which would result in a lower pitch resolution for instruments that internally have a higher depth configured, though.

Maybe you could give a quick description of what you want to achieve with this API, so that we can better judge which approach would be the best.

[parlance]

Thank you for your continued work on this!

The idea was to use libopenmpt as the main audio mixing and rendering component for real-time game sound. Sound banks are constructed with the visual interface OpenMPT provides, and the developer can then reference those and play instruments as sound patches with libopenmpt's efficient mixing and rendering, as well as the game music of course. This kind of game sound system would be useful for games that need very dynamic sound control; dynamic music you can actually speed up and slow down or pitch and down, the ability to seek smoothly and seamlessly, etc.

Libraries that are used for this are low in supply and high in demand, and sometimes expensive, like https://www.fmod.com/. libopenmpt already has 98% of the features, while being open source and free. You may find other developers looking for something similar a use case for the library.

[Saga Musix]

Doesn't time fly... as of r16054, there is the new "interactive2" libopenmpt_ext interface which adds the new functionality. As long libopenmpt 0.6 isn't released, the design of these functions may still change, so if you have the possibility, please give them a try.

For the frequency stuff, I went for a design that allows to set the finetune in a +/-1 range, with the exact meaning of that range depending on the instrument setup (the instrument's pitch wheel depth will determine how many semitones this range translates into). In your scenario, this would allow a sound designer to set the desired range how far a note should be able to deviate from the original frequency in OpenMPT, and the same +/-1 range can be used throughout the code for all instruments.