Skip to content

Commit 796dacd

Browse files
derselbstderselbst
authored
Merge pull request #1709 from FluidSynth/issue-1695
Review findings of Issue 1695
2 parents 0ec65c7 + c403aa9 commit 796dacd

File tree

5 files changed

+276
-101
lines changed

5 files changed

+276
-101
lines changed

‎src/midi/fluid_seq.c‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -462,12 +462,13 @@ fluid_sequencer_send_now(fluid_sequencer_t *seq, fluid_event_t *evt)
462462
* @note The sequencer sorts events according to their timestamp \c time. For events that have
463463
* the same timestamp, fluidsynth (as of version 2.2.0) uses the following order, according to
464464
* which events will be dispatched to the client's callback function.
465-
* - #FLUID_SEQ_SYSTEMRESET events precede any other event type.
465+
* - #FLUID_SEQ_NOTEOFF events precede any other event type.
466+
* - #FLUID_SEQ_SYSTEMRESET events are only preceded by #FLUID_SEQ_NOTEOFF events.
466467
* - #FLUID_SEQ_UNREGISTERING events succeed #FLUID_SEQ_SYSTEMRESET and precede other event type.
467468
* - #FLUID_SEQ_NOTEON and #FLUID_SEQ_NOTE events succeed any other event type.
468469
* - Otherwise the order is undefined.
469470
* \n
470-
* Or mathematically: #FLUID_SEQ_SYSTEMRESET < #FLUID_SEQ_UNREGISTERING < ... < (#FLUID_SEQ_NOTEON && #FLUID_SEQ_NOTE)
471+
* Or mathematically: #FLUID_SEQ_NOTEOFF < #FLUID_SEQ_SYSTEMRESET < #FLUID_SEQ_UNREGISTERING < ... < (#FLUID_SEQ_NOTEON && #FLUID_SEQ_NOTE)
471472
*
472473
* @warning Be careful with relative ticks when sending many events! See #fluid_event_callback_t for details.
473474
*/

‎src/midi/fluid_seq_queue.cpp‎

Lines changed: 18 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,8 @@ static bool event_compare(const fluid_event_t& left, const fluid_event_t& right)
5656
// Both events have the same tick value. Per MIDI standard, the order is undefined. However, most implementations use a FIFO ordering here,
5757
// which we cannot use, because heap sort is not stable. To make sure that fluidsynth behaves correctly from a user perspective,
5858
// we do the following:
59-
// * System reset events are always first,
60-
// * Unregistering events are second (this gives clients the chance to reset themselves before unregistering at the same tick),
59+
// * NoteOff events have the highest precedence and are processed before System reset events (this allows turning off notes before a channel is potentially disabled via channel mode messages on the same tick),
60+
// * Unregistering events are third (this gives clients the chance to reset and silence themselves before unregistering at the same tick),
6161
// * Bank changes must precede Prog changes (to ensure correct preset fallback AND preset selection within a certain bank),
6262
// * NoteOn events are always last (this makes sure that all other "state-change" events have been processed and NoteOff events
6363
// with the same key as the NoteOn have been processed (zero-length notes are not a use-case here)).
@@ -75,31 +75,34 @@ static bool event_compare(const fluid_event_t& left, const fluid_event_t& right)
7575
// * X meaning any other event type, and
7676
// * the '*' means that it could be zero, but making it 1 simplifies the boolean expression.
7777
//
78-
// | ltype \ rtype | SYSR | UNREG | BANK | PROG | NOTEON | X |
79-
// | SYSR | 1 | 1 | 1 | 1 | 1 | 1 |
80-
// | UNREG | 0 | 1 | 1 | 1 | 1 | 1 |
81-
// | BANK | 0 | 0 | 1 | 1 | 1 | 1* |
82-
// | PROG | 0 | 0 | 0 | 1 | 1 | 1* |
83-
// | NOTEON | 0 | 0 | 0 | 0 | 1 | 0 |
84-
// | X | 0 | 0 | 0 | 0 | 1 | 1 |
78+
// | ltype \ rtype | NOTEOFF | SYSR | UNREG | BANK | PROG | NOTEON | X |
79+
// | NOTEOFF | 1 | 1 | 1 | 1 | 1 | 1 | 1 |
80+
// | SYSR | 0 | 1 | 1 | 1 | 1 | 1 | 1 |
81+
// | UNREG | 0 | 0 | 1 | 1 | 1 | 1 | 1 |
82+
// | BANK | 0 | 0 | 0 | 1 | 1 | 1 | 1* |
83+
// | PROG | 0 | 0 | 0 | 0 | 1 | 1 | 1* |
84+
// | NOTEON | 0 | 0 | 0 | 0 | 0 | 1 | 0 |
85+
// | X | 0 | 0 | 0 | 0 | 0 | 1 | 1 |
8586
//
8687
// The values in the diagonal (i.e. comparison with itself) must be true to make them become false after leaving this
8788
// function in order to satisfy the irreflexive requirement, i.e. assert(!(a < a))
8889

8990
leftIsBeforeRight =
91+
// zero'th row in table
92+
(ltype == FLUID_SEQ_NOTEOFF)
9093
// first row in table
91-
(ltype == FLUID_SEQ_SYSTEMRESET)
94+
|| (rtype != FLUID_SEQ_NOTEOFF && ltype == FLUID_SEQ_SYSTEMRESET)
9295
// the rtype NOTEON column
9396
|| (rtype == FLUID_SEQ_NOTEON || rtype == FLUID_SEQ_NOTE)
9497
// the second row in table
95-
|| (rtype != FLUID_SEQ_SYSTEMRESET && ltype == FLUID_SEQ_UNREGISTERING)
98+
|| (rtype != FLUID_SEQ_NOTEOFF && rtype != FLUID_SEQ_SYSTEMRESET && ltype == FLUID_SEQ_UNREGISTERING)
9699
// the third row in table
97-
|| (rtype != FLUID_SEQ_SYSTEMRESET && rtype != FLUID_SEQ_UNREGISTERING && ltype == FLUID_SEQ_BANKSELECT)
100+
|| (rtype != FLUID_SEQ_NOTEOFF && rtype != FLUID_SEQ_SYSTEMRESET && rtype != FLUID_SEQ_UNREGISTERING && ltype == FLUID_SEQ_BANKSELECT)
98101
// the fourth row in table
99-
|| (rtype != FLUID_SEQ_SYSTEMRESET && rtype != FLUID_SEQ_UNREGISTERING && rtype != FLUID_SEQ_BANKSELECT && ltype == FLUID_SEQ_PROGRAMCHANGE)
102+
|| (rtype != FLUID_SEQ_NOTEOFF && rtype != FLUID_SEQ_SYSTEMRESET && rtype != FLUID_SEQ_UNREGISTERING && rtype != FLUID_SEQ_BANKSELECT && ltype == FLUID_SEQ_PROGRAMCHANGE)
100103
// the bottom right value, i.e. any other type compared to any other type
101-
|| (ltype != FLUID_SEQ_SYSTEMRESET && ltype != FLUID_SEQ_UNREGISTERING && ltype != FLUID_SEQ_BANKSELECT && ltype != FLUID_SEQ_PROGRAMCHANGE && ltype != FLUID_SEQ_NOTEON && ltype != FLUID_SEQ_NOTE &&
102-
rtype != FLUID_SEQ_SYSTEMRESET && rtype != FLUID_SEQ_UNREGISTERING && rtype != FLUID_SEQ_BANKSELECT && rtype != FLUID_SEQ_PROGRAMCHANGE && rtype != FLUID_SEQ_NOTEON && rtype != FLUID_SEQ_NOTE);
104+
|| (ltype != FLUID_SEQ_NOTEOFF && ltype != FLUID_SEQ_SYSTEMRESET && ltype != FLUID_SEQ_UNREGISTERING && ltype != FLUID_SEQ_BANKSELECT && ltype != FLUID_SEQ_PROGRAMCHANGE && ltype != FLUID_SEQ_NOTEON && ltype != FLUID_SEQ_NOTE &&
105+
rtype != FLUID_SEQ_NOTEOFF && rtype != FLUID_SEQ_SYSTEMRESET && rtype != FLUID_SEQ_UNREGISTERING && rtype != FLUID_SEQ_BANKSELECT && rtype != FLUID_SEQ_PROGRAMCHANGE && rtype != FLUID_SEQ_NOTEON && rtype != FLUID_SEQ_NOTE);
103106
}
104107
else
105108
{

‎test/test_seq_event_queue_sort.c‎

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,8 @@ static short order = 0;
1010
void callback_stable_sort(unsigned int time, fluid_event_t *event, fluid_sequencer_t *seq, void *data)
1111
{
1212
static const int expected_type_order[] =
13-
{ FLUID_SEQ_NOTEOFF, FLUID_SEQ_NOTEON, FLUID_SEQ_SYSTEMRESET, FLUID_SEQ_UNREGISTERING
14-
/* technically, FLUID_SEQ_NOTEOFF and FLUID_SEQ_NOTEON are to follow, but we've already unregistered */ };
13+
{ FLUID_SEQ_NOTEOFF, FLUID_SEQ_NOTEOFF, FLUID_SEQ_NOTEON, FLUID_SEQ_SYSTEMRESET, FLUID_SEQ_UNREGISTERING
14+
/* technically, FLUID_SEQ_NOTEON is to follow, but we've already unregistered */ };
1515

1616
TEST_ASSERT(fluid_event_get_type(event) == expected_type_order[order++]);
1717
}
@@ -26,24 +26,27 @@ void test_order_same_tick(fluid_sequencer_t *seq, fluid_event_t *evt)
2626
fluid_event_set_source(evt, -1);
2727
fluid_event_set_dest(evt, seqid);
2828

29-
for(i = 1; i <= 2; i++)
30-
{
31-
fluid_event_noteoff(evt, 0, 64);
32-
TEST_SUCCESS(fluid_sequencer_send_at(seq, evt, i, 1));
33-
fluid_event_noteon(evt, 0, 64, 127);
34-
TEST_SUCCESS(fluid_sequencer_send_at(seq, evt, i, 1));
35-
}
29+
fluid_event_noteon(evt, 0, 64, 127);
30+
TEST_SUCCESS(fluid_sequencer_send_at(seq, evt, 1, 1));
31+
fluid_event_noteoff(evt, 0, 64);
32+
TEST_SUCCESS(fluid_sequencer_send_at(seq, evt, 1, 1));
33+
34+
fluid_event_noteoff(evt, 0, 64);
35+
TEST_SUCCESS(fluid_sequencer_send_at(seq, evt, 1, 1));
36+
37+
fluid_event_noteon(evt, 0, 64, 127);
38+
TEST_SUCCESS(fluid_sequencer_send_at(seq, evt, 2, 1));
3639

3740
fluid_event_system_reset(evt);
3841
TEST_SUCCESS(fluid_sequencer_send_at(seq, evt, 2, 1));
3942
fluid_event_unregistering(evt);
4043
TEST_SUCCESS(fluid_sequencer_send_at(seq, evt, 2, 1));
4144

4245
fluid_sequencer_process(seq, 1);
43-
TEST_ASSERT(order == 2);
46+
TEST_ASSERT(order == 3);
4447

4548
fluid_sequencer_process(seq, 2);
46-
TEST_ASSERT(order == 4);
49+
TEST_ASSERT(order == 5);
4750

4851
fluid_sequencer_unregister_client(seq, seqid);
4952
TEST_ASSERT(fluid_sequencer_count_clients(seq) == 0);

‎test/test_seq_evt_order.c‎

Lines changed: 0 additions & 73 deletions
This file was deleted.

0 commit comments

Comments
 (0)