Revise the sequencer's event queue

[derselbst]

Proposing a new event queue for the sequencer, based on prior discussion:
https://lists.nongnu.org/archive/html/fluid-dev/2019-12/msg00001.html

With this change fluidsynth will require a C++98 compliant compiler.

Consider this as RFC, feedback is welcome.

The "pain-points" from the discussion:

1. It is slow.

Done (thanks to heap sort), see runtime of test_seq_event_queue_sort.

2. A meaningful ordering for events with the same tick has not been considered.

Done, see comments in fluid_seq_queue.cpp.

3. Complicated implementation

Now uses one single event queue, which requires C++98. Implemented similarly to std::priority_queue by using heap sort.

The "queue" I use currently is of type std::deque. deque does not provide preallocation. std::vector does provide it. However, std::deque has the huge advantage that appending additional elements is cheap. For std::vector appending new elements would require to reallocate all the memory and copy it to the new array. So,

• either use std::deque with the risk that memory allocation may occur during fluid_sequencer_send_at(), or
• use std::vector with a preallocated pool of events and make fluid_sequencer_send_at() when the vector runs out of capacity.

Comments?

4. Events that have been processed are deleted and gone.

After having thought about this more, this is the correct behavior. After events have been dispatched, they must be released to free underlying memory, see point 3. For the very rare case that a client (e.g. fluid_player) may require those events in the future, the client should be responsible for storing the events somewhere.

5. The sequencer supports the system timer as alternative time source.

The conclusion from the mailing list was that the system timer can be removed. This has been done.

6. Time Scaling

Time scaling can now be used for arbitrary tempo changes. The previous implementation was capable of that as well, however, the time-scale was limited to 1000. The only limitation for the scale is now >0, see test_seq_scale.

Other Points

• fluid_sequencer_remove_events() turned out to be broken before, as it did not remove all events from the queue. This has been fixed, see test_seq_event_queue_remove.

• Consider the following code executed by threadA:

fluid_sequencer_send_at(event0);
fluid_sequencer_set_time_scale(); // new scale
fluid_sequencer_send_at(event1);

The new scale will be definitely applied to event1. However, if another concurrently running threadB executes fluid_sequencer_process(), it was previously not clear, whether the new scale was also applied to event0. This depends on whether event0 was still in the preQueue, and this depends on event0.time and the tick count that fluid_sequencer_process() is being called with. This has been changed. As of now, events are queued with their timestamp AS-IS. And only the latest call to fluid_sequencer_set_time_scale() is being considered during fluid_sequencer_process(). This makes the implementation very simple, i.e. no events need to be changed and the sequencer doesn't have to be locked down. On the other hand, it means that fluid_sequencer_set_time_scale() can only be used for tempo changes when called from the sequencer callback. In other words, if threadA executes the code above followed by fluid_sequencer_process(), event0 and event1 will be executed with the same tempo, which is the latest scale provided to the seq. Is this acceptable? The old implementation had the same limitation. And when looking through the internet, I only find users who call fluid_sequencer_set_time_scale() from the sequencer callback. Still, this is a point I'm raising for discussion.

[lgtm-com]

This pull request fixes 1 alert when merging 76e22b6 into 9b069fa - view on LGTM.com

fixed alerts:

• 1 for Comparison of narrow type with wide type in loop condition

[lgtm-com]

This pull request fixes 1 alert when merging 4ef7c55 into 9b069fa - view on LGTM.com

fixed alerts:

• 1 for Comparison of narrow type with wide type in loop condition

[mawe42]

Thanks for this, it definitely looks like an improvement! But let me reiterate my concerns about two points:

1. The ordering of events

I don't think that we can decide what a "meaningful ordering" of events on the same tick is. Only the party that created those events can know that. By forcing our custom ordering, we take away the callers ability to decide what's meaningful and what is not.

2. The new C++ dependency

In my opinion there is no real need to use C++ here. The queue and heap sort could be implemented in C as well. I understand that it is more convenient to use C++ (at least for people who know C++), but I feel like that is not a good reason to introduce a new compile time dependency and another programming language to the code base.

The ordering just doesn't feel right, but it's not a strong objection. But adding larger bits of C++ to the code base is more problematic, in my opinion.

[derselbst]

Thank you Marcus for your feedback. Below my thoughts.

---

1. The ordering of events
   I don't think that we can decide what a "meaningful ordering" of events on the same tick is.

Ordering of events at the same tick is undefined. Because heap sort is not stable, we have no other choice to give it a "meaningful ordering" (that is, an ordering that will work for 99% of our users without any visible side-effects). In this case, we can and we have to decide which order makes the most sense given fluidsynths implementation. And this is what I do:

• With the proposed ordering, it is ensured that all control changes have been processed before a note is started. While respecting proper bank selection and program-fallback.
• Notes that are shorter than the chosen time-resolution (i.e. zero duration notes) are not a supported use-case.
• Sending various control changes of the same type, on the same channel, at the same tick, is not a supported use-case either.

With this solution, I currently do not see any side-effects.

2. The new C++ dependency
   The queue and heap sort could be implemented in C as well.

Ofc they could be implemented in C89. No offense, but this "do-it-yourself" mentally dates back to the old C days decades ago. Nowadays, there is a standard available, that already provides this implementation for us. Well-tested, performant and widely established. It is called C++ and dates back to the year 1998. Now, it's 2020. And let me put it clearly: I will not be the one reimplementing a custom heap sort and std::deque. If an equivalent C implementation magically fell from heaven, I would probably take it though.

introduce a new compile time dependency and another programming language to the code base.

Is there any specific compiler or environment you are referring to that nowadays does not provide a C++98 compliant tooling?

I am convinced that everybody nowadays compiling fluidsynth already has a C++98 compiler in his toolbox that just waits for being used. In other words, all the different ways to build fluidsynth already provide a C++ compiler out-of-the-box. And even for rarely used OS/2 and Solaris I am convinced this should work as-is. If I missed something, please correct me.

And if it's about the necessary runtime dependency to the standard C++ library, pls. keep in mind that when fluidsynth is compiled with support for the jack audio driver, the fluidsynth library already has that dependency:

ldd /usr/lib64/libfluidsynth.so.2
        libstdc++.so.6 => /usr/lib64/libstdc++.so.6 (0x00007f2291dc0000)

[mawe42]

Because heap sort is not stable, we have no other choice to give it a "meaningful ordering" (that is, an ordering that will work for 99% of our users without any visible side-effects).

I don't agree. We don't have to give it a meaningful ordering, but a defined (and documented) ordering. We should leave the "meaningful" part to the user. And the only way to do that is to use the order in which the events arrived, which I'm sure could easily be added as a tie-breaker sort key to the comparison function.

In practice it probably doesn't matter which approach we implement. So like I already said, I have no strong feelings against this solution. But from a purely technical, API design and interoperability standpoint, I think it's the wrong approach.

Ofc they could be implemented in C89. No offense, but this "do-it-yourself" mentally dates back to the old C days decades ago. Nowadays, there is a standard available, that already provides this implementation for us.

Well, fluidsynth is implemented in C. And C++ is not just another library, but another programming language. To me this feels similar to somebody proposing to solve a problem with a particular Ruby Gem in a project that is written in Perl. Granted, the C++ you introduce here is mostly glue code, but it's the precedent that bothers me. Will C++ also be a good solution to the next problem that has an easy solution in the C++ stdlib?

I'm not opposed to moving fluidsynth to C++... would give me an incentive to actually learn it... but I think that such a move should be a conscious decision taken by the community.

And if it's about the necessary runtime dependency to the standard C++ library, pls. keep in mind that when fluidsynth is compiled with support for the jack audio driver, the fluidsynth library already has that dependency

You are right, and personally the dependency doesn't worry me. But we have some users who use fluidsynth on very resource constrained hardware who will most likely not be using jack. So for them, this would be a new dependency. And the reason to introduce this is not even to solve a problem that many of our users face. The reason is simply that we choose one of many possible solutions to a performance problem in the very rare (edge-)use-case of adding thousands of events to the sequencer ahead of time.

[derselbst]

And the reason to introduce this is not even to solve a problem that many of our users face.

One could also think it differently: Because the sequencer performs very poorly at the moment, nobody uses it :)

I think that such a move should be a conscious decision taken by the community.

Ok, so you are suggesting to ask at the mailing list. Will we have a deadline for people to object?

[mawe42]

One could also think it differently: Because the sequencer performs very poorly at the moment, nobody uses it :)

Some people seem to use it. Not many hits for direct calls to new_fluid_sequencer and new_fluid_sequencer2 on GitHub, but there are a number of wrappers for other languages that expose the sequencer, for example pyfluidsynth. And there are definitely people using that, for example: https://github.com/oscgonfer/python-generative-music/blob/e94a3d6b1c08e210baca663ce6f594d59f6a5de9/endlessplayer.py

Ok, so you are suggesting to ask at the mailing list. Will we have a deadline for people to object?

That would probably be a good idea, otherwise we'll "hang in the air" if nobody responds. And we should also think about which question to ask. In my opinion the questions should be bigger than "should we use C++ for this issue". Some ideas:

• Do we want C++ code in fluidsynth?
• If so, do we want to port fluidsynth to C++ completely?
• Or should only new code/features be C++, existing C code stays untouched unless a complete rewrite is required?
• Or do we only want minimal glue code for C++ stdlib features, rest stays C?

[lgtm-com]

This pull request fixes 1 alert when merging 8068ab1 into fdd577b - view on LGTM.com

fixed alerts:

• 1 for Comparison of narrow type with wide type in loop condition

[lgtm-com]

This pull request fixes 1 alert when merging a9fa702 into fdd577b - view on LGTM.com

fixed alerts:

• 1 for Comparison of narrow type with wide type in loop condition

[lgtm-com]

This pull request fixes 1 alert when merging 7ff9a98 into f15b8e5 - view on LGTM.com

fixed alerts:

• 1 for Comparison of narrow type with wide type in loop condition

[derselbst]

As you probably notice, I started working on this again. Also because the feedback from the mailing list about switching to C++ was quite positive.

As proof of concept, I basically implemented my own MIDI player (in my app ANMP). Previously, my MIDI player was responsible for correct timing of events, creating tempo map etc. With this new sequencer queue, however, I am able to push events with their raw MIDI tick to the queue, making fluidsynth's sequencer take care of tempo change (by changing the sequencers scale on every tempo change). While doing so, I found a few bugs in this PR and fixed them. Now, this queue works perfectly fine as MIDI player.

I also reverted my app to the previous behaviour using this new queue and it still works fine. I, therefore, assume that the changes I've made here are backward compatible and should not break any other app currently using fluidsynth's sequencer.

Long story short: I consider this ready for review.

[jjceresa]

Suggestion to avoid warning.
if(top.time <= (unsigned int)cur_ticks)

[derselbst]

You're right. But a cast is not correct here. cur_ticks should be unsigned instead. I'll fix it.

[lgtm-com]

This pull request fixes 1 alert when merging 4b0dce8 into 85cf123 - view on LGTM.com

fixed alerts:

• 1 for Comparison of narrow type with wide type in loop condition

[jjceresa]

A bit out of subject. Fluidsynth being a library with components selected at CMake time, I wonder if we could have a new CMake option to have the fluidsynth library with or without the sequencer ?.

[derselbst]

I wonder if we could have a new CMake option to have the fluidsynth library with or without the sequencer ?.

The sequencer is an inherent part of fluidsynth. Without the sequencer, a huge part of fluidsynth's API would become unusable. IMO, it would be a bad idea making it a compile-time option. Why are you asking?

[jjceresa]

The sequencer is an inherent part of fluidsynth. Without the sequencer, a huge part of fluidsynth's API would become unusable.

In this case, please ignore my question.

[lgtm-com]

This pull request fixes 1 alert when merging ea85a1d into a893994 - view on LGTM.com

fixed alerts:

• 1 for Comparison of narrow type with wide type in loop condition

[lgtm-com]

This pull request fixes 1 alert when merging 1e9cdc7 into 9995fd8 - view on LGTM.com

fixed alerts:

• 1 for Comparison of narrow type with wide type in loop condition

[derselbst]

During the past weeks and months I have intensively tested this implementation in my app ANMP. Thus, I consider this implementation to be ready now.