[patch] add API for real time midi stream parsing

[derselbst]

Hi.

It does look like fluidsynth currently does
not have any API that allows to deal with the
simple midi stream in real time.
For instance, the linux OSS sequencer has
SEQ_MIDIOUT() macro for that, and alsa has
snd_rawmidi_write().

This patch adds fluid_sequencer_add_midi_data_to_buffer()
function and a few more.

Reported by: stsp

Original Ticket: fluidsynth/tickets/152

[derselbst]

so, it seems this patch, if accepted, would superseded #108 ?

[derselbst]

@stsp Is this still an issue? If so, I would rather prefer making fluid_midi_parser_parse() public since this gives the user greater flexibility and possibly avoids introducing even more parsing related functions.

[stsp]

Hi!

fluidsynth is alive? Great news!
In fact, I simply misnamed an entry.
It should have been named
"add API for real time midi stream playing".
It does not replace parser, for example I
use parser to write the midi stream into a
.mid file, and I use this API for converting
midi stream into PCM.
But sure, if you have parser API public,
then the user can code up this function
himself - with bigger code, and he must
be familiar with what midi parser is. This
API allows him to just relay the data bytes.

[stsp]

My current solution is to embed the
fluidsynth bits and pieces into my source
tree, but it is sub-optimal. If you release
something workable, I'll drop the hacks and
bump up the required fluidsynth version.

[stsp]

IIRC there were more patches from me,
will you process all of them?

[derselbst]

Ok, then let's make it public. I also dont share David's concerns that parsing might change, since fluid_midi_event_t is fluidsynths MIDI compliant struct. It is private and as long as the MIDI standard doesnt change, parsing wont change.

IIRC there were more patches from me,
will you process all of them?

So far only see #148, which made it into 1.1.7.

[stsp]

My preference would be to allow both APIs -
they both have its uses and are already used
in my code.

Yes, I meant #148, thanks!

[derselbst]

My preference would be to allow both APIs

And with both you mean the patch of this ticket and making fluid_midi_parser_parse() public? As I've understood making fluid_midi_parser_parse() public would be sufficient, since you're free to call fluid_sequencer_add_midi_event_to_buffer() or any other fluid_midi_event_t function after parsing?

[stsp]

Yes, but this will require you some
extra steps. When people work with raw
midi streams, they may want an API that
works with it, because both alsa and oss
have such API - there is no need to set
up the parser to just play the raw midi stream.

[stsp]

Also timidity has such API (I coded the
patch for them).

[stsp]

munt has such API too (looks quite close
to fluidsynth wrt APIs). I think this makes
more sense if there are different playing
backends. Eg while fluidsynth only outputs
to PCM, it may need parsing in any case.
But those systems that can output to real midi
devices, may completely bypass the parsing
stage, so requiring the parsing would in this
case be a waste of time.

[derselbst]

My problem with fluid_sequencer_add_midi_data_to_buffer() is that it doesnt quite fit to the seqbind. It looks more like a hack introducing that fluid_seqbind_priv_t and clobbering thefluid_midi_parser_t into it, while we're also having fluid_seqbind_t but both are only visible as void* to the user.

When making fluid_midi_parser_parse() public anyway, fluid_sequencer_add_midi_data_to_buffer() would provide some redundant functionality, since without it all the user would have to do is:

int ret=FLUID_OK;
for (i = 0; i < length && ret == FLUID_OK; i++) {
		fluid_midi_event_t* event = fluid_midi_parser_parse(parser, data[i]);
		if (event != NULL) {
			ret = fluid_sequencer_add_midi_event_to_buffer(seq, event);
		}
	}

Which is basically my understanding of having an API that just works.

[stsp]

Yes, implementation was hacky for sure.
Other systems takes different approach
to this, for example oss has a sequencer
protocol that allows to pass various data
types, including parsed midi events and
raw midi data. In terms of seqbind, this
could probably mean extending fluid_midi_event_t
to allow embedding other data types, and
then teaching the synth core to handle
these new types. But maybe its a too big
work for too small task.

[derselbst]

fluid_midi_event_t is and will stay the MIDI compliant struct. We have fluid_event_t for more custom things.

[stsp]

Looking into fluid_seq_event_type it seems
there is nothing new, but maybe you mean this
can be a candidate for extending. So indeed
fluid_sequencer_send_at() looks exactly like
the API I need, provided fluid_midi_event_t
is extended a bit. I use exactly that kind of API
in other projects.

[stsp]

Would it make sense for the public parser
API to create fluid_event_t instead of
fluid_midi_event_t by any chance?
This will allow me to move to seq.h API
(though I really don't know what's the
difference).

[derselbst]

I see: you basically want fluid_sequencer_add_midi_event_to_buffer() with an additional send_at_tick param, right? Perhaps we should really rethink the API design. The user should just be able to parse raw data to fluid_midi_event_t and convert that to fluid_event_t. This would also allow us getting rid of fluid_sequencer_add_midi_event_to_buffer() which is actually doing two things: convert fluid_midi_event_t to fluid_event_t and add it seq sending it now.

Looking into fluid_seq_event_type it seems
there is nothing new,

Unfortunately due to FLUID_SEQ_LASTEVENT this enum cannot be expanded, which is why fluidsynth requires an SOVERSION bump and this also gives us the opportunity to revise the API. The master branch docs can be found here.

[stsp]

I see: you basically want fluid_sequencer_add_midi_event_to_buffer()
with an additional send_at_tick param, right?

Yes, that would be a convenient API.
Currently I use fluid_sequencer_process()
for timestamping.

The user should just be able to parse raw data
to fluid_midi_event_t and convert that to fluid_event_t.

But is it better than to just immediately parse
into fluid_event_t?

requires an SOVERSION bump and this also gives us the opportunity to revise the API.

Sounds good. :)

[derselbst]

But is it better than to just immediately parse
into fluid_event_t?

IMO yes, because then the user enjoys full freedom of whatever he wants to do.

[stsp]

IIRC I was not able to find an API that
converts form fluid_midi_event_t to fluid_event_t.
So if you add one, that would be nice.
Of course I don't know what is better:
parse to fluid_event_t and have an optional
converter to fluid_midi_event_t or the other
way around - I'll use whatever you introduce,
but I just think that the common case should
not involve the conversion and the converter
should only exist for the user to " enjoys full
freedom of whatever he wants to do". That's
why to me it looks slightly better to parse to
fluid_event_t and optionally convert back.

[stsp]

As of 1.1.7 this enum value is deprecated and will be
removed in a future release, because it prevents adding
new enum values without breaking ABI compatibility.

Just out of curiosity, could you please
clarify why? I try to think up a breakage
scenario but can't. Do you think some
program could use this value to define
an array size? Or what is the breakage
if the program thinks this value is smaller
than it actually is?

[derselbst]

to parse to fluid_event_t and optionally convert back

fluid_event_t is the more powerful one, so converting back from it might be problematic. Those *_LAST are as well as every other enum value guaranteed to stay the same in order to provide ABI compatibility. I dont know if or how downstream uses them (sure, could be they use it in an array or for iteration, idk). In fact it doesnt matter, it's a public enum so they are allow to do this. That's why one usually adds new enum values at the end, i.e. after *_LAST. However due to the semantic of *_LAST it needs to stay the very last enum value. The only option upstream has is to add new values prior to that. But this would change the value of *_LAST. It's a dilemma... so we'll get completely rid of it.

[stsp]

In fact also in fluidsynth I can
think of the only place where you
should export fluid_midi_event_t -
this is after a midi parser. In all other
places you can likely export only
fluid_event_t, which can carry midi
events too, but not limited to that.
Same with OSS: it does not export
internal midi event representation,
but it is not needed, as it does not
export the parser either.

[derselbst]

I dont want to tweak fluid_event_t to carry midi events, rawmidi binary blobs or anything else. fluid_event_t is solely meant to carry event types that are unique to fluidsynth and only one at a time. And therefore IMO we need to provide small parser. For now it will create fluid_midi_event_t, because

• creating fluid_event_t would mean too much changes for a maintenance release,
• fluid_midi_event_t is fluidsynths highly midi compliant but still custom data type when it comes to the midi.h API,
• it is currently not clear if removal of fluid_midi_event_t will happen at all, and
• still I wanted to provide you with a workable solution for your problem in 1.1.9.

[stsp]

I dont want to tweak fluid_event_t to carry midi events

But doesn't it do this already?
It even has the accessors functions
to get different parts of the midi event.

fluid_event_t is solely meant to carry event types that are unique to fluidsynth and only one at a time.

This does not differ from what OSS does.
He also uses the notion of "sequencer events" -
the ones that are unique to OSS. And they
can embed different things, one at a time
(for rawmidi - just one byte at a time).

For now it will create fluid_midi_event_t, because

I have no doubts that parser should create
fluid_midi_event_t - the parsed midi event.
This is the same what OSS does, just it doesn't
export them to the user, but that doesn't matter
here.

The one and only difference I can see between
what you say and what OSS does, is the variety
of data types that fluidsynth supports natively,
and, as the result, the fluid_event_t can carry.
OSS natively supports different data types, and
so its sequencer event can carry them all.
fluidsynth only supports the midi events, and
so its fluid_event_t can carry only them.
This is perfectly fine and architecturally similar.
The question is only whether fluidsynth can be
extended to natively support more input data
types in the future, or it will always ask the user
to convert any other data types to the one it
supports.

[derselbst]

I dont want to tweak fluid_event_t to carry midi events

But doesn't it do this already?

No. It offers events that are semantically similar. Neither does their type id match with GM MIDI, nor are they limited to 16 channels.

I have no doubts that parser should create fluid_midi_event_t - the parsed midi event.

Forget about fluid_midi_event_t, it's AFAIK a completely redundant part of fluidsynth that only causes confusion. Any fluid_midi_event_t must be converted to fluid_event_t before it can be used. It only exists because it's "established".

The question is only whether fluidsynth can be extended to natively support more input data types in the future, or it will always ask the user to convert any other data types to the one it supports.

It will always only support data types that are directly supported by fluidsynth. There are so many midi-like sequenced formats that starting to introduce foreign data types would be a never ending story.

[stsp]

Forget about fluid_midi_event_t, it's AFAIK a completely
redundant part of fluidsynth that only causes confusion.

But other projects also have 2 different representations:
external (for sequencer, user-filled, extensible) and internal
(for synth engine, parser-filled)... I thought this similarity in
fluidsynth is (or was?) intentional. The only thing that is
redundant compared to OSS, is an accessors of fluid_event_t -
other than that its all the same! Can't believe it wasn't
intentional. :)

[derselbst]

Accessors of fluid_event_t are not redundant. Again fluid_event_t is used by the sequencer. The user must be able to fill those events appropriately without being bound to the restrictions of midi.

FYI:

Fluidsynths external events:

• fluid_event_t (for sequencer, user-filled, extensible, etc.)
• fluid_midi_event_t (some strange established special case of fluid_event_t)

Fluidsynths internal event:

• fluid_rvoice_event_t (small helper synthesis events on a per-voice-level for internal use only)

[stsp]

The user must be able to fill those events appropriately

By "accessors" I only meant getters.
User of course should be able to construct such events.
It is not unexpected to have another, more restricted
event type that does have getters, in this case fluid_midi_event_t.
But at the end of the day, it doesn't matter of course. :)

[stsp]

Be fluid_event_t really extensible, it could probably
embed fluid_midi_event_t, perhaps as a union member.
This will make a conversion between them worth
1 line of C code, mostly eliminating the need to
remove either one, at least for a short-term goal.
Since fluid_midi_event_t is parser-created, it really
can't use any current/future extensions of fluid_event_t,
and it can provide the getters that will always work,
so IMHO it can just serve its small goals well.

[derselbst]

it could probably embed fluid_midi_event_t, perhaps as a union member.

Unnecessary, fluid_event_t is powerful enough to store fluid_midi_event_t in it's own members. Also I dont believe in short-term-thinking when preparing a major version bump with API cleanup.

Since fluid_midi_event_t is parser-created, it really can't use any current/future extensions of fluid_event_t,

Generated by a parser that only understands rawmidi. rawmidi doesnt understand or provide any extensions of fluid_event_t, so this is not relevant.

[stsp]

Unnecessary, fluid_event_t is powerful enough to store fluid_midi_event_t in it's own members.

Yes, but the conversion is more difficult in this case.
I only propose the way to reduce redundancy w/o
removing anything. If fluid_event_t embeds
fluid_midi_event_t instead of using its own members
for this - redundancy reduced. If fluid_event_t
then also removes its getters (you'll be able to use
the getters of fluid_midi_event_t if its embedded) -
redundancy reduced, plus full match to OSS arch. :)

Generated by a parser that only understands rawmidi.
rawmidi doesnt understand or provide any extensions of fluid_event_t, so this is not relevant.

Yes, that's exactly what I say.
Since any possible extensions of fluid_event_t are
irrelevant for parser-generated event, it can use the
fixed subset of it - fluid_midi_event_t. It doesn't need
to use the full thing.

[derselbst]

Sry, but this is a hack. If a downstream app only knows about fluid_event_t and wants to know the channel of it, it would need to:

fluid_event_t * some_event;
fluid_midi_event_t* some_embedded_midievent = fluid_event_get_midi_event(some_event);
int chan = fluid_midi_event_get_channel(some_embedded_midievent);

How would that make anything clearer?

Yes, that's exactly what I say.

Ok, then I misread that. Anyway if fluid_midi_event_t is removed there will no reason to keep it just because the parser uses it.

[stsp]

channel of it, it would need to:

But will it really need that?
According to what you said, I suppose
the core would work with fluid_rvoice_event_t.
I think it shouldn't just use the getters to
fluid_midi_event_t and/or fluid_event_t.
But getters for fluid_midi_event_t are important
for the user code.

The problem with having getters directly to
fluid_event_t is that in this case you will really
never be able to extend it to carry something
else than the midi event, or the getters will stop
working. OTOH getters for fluid_midi_event_t
will never stop working. Of course it is not a problem
as long as you are not supposed to even actually
extend fluid_event_t, but who knows. :)

[derselbst]

But will it really need that?

Yes, because you cannot rely on downstream apps solely using fluid_midi_event_t.

But getters for fluid_midi_event_t are important for the user code.

All the getters of fluid_midi_event_t are available for fluid_event_t, I dont see the problem.

The problem with having getters directly to fluid_event_t is that in this case you will really never be able to extend it to carry something else than the midi event, or the getters will stop working.

I still dont see the problem. Why should the getters suddenly stop working?

[stsp]

Yes, because you cannot rely on downstream apps solely using fluid_midi_event_t.

No-no, it would be much better if downstream apps
use mostly fluid_event_t! It works well with sequencer.
I wonder if there are too many downstream apps
(besides my own) that uses the getters, rather than
to just feed events to sequencer? And if it does, I think
it already works with fluid_midi_event_t? (mine does)

All the getters of fluid_midi_event_t are available for fluid_event_t, I dont see the problem.

Redundancy.

I still dont see the problem. Why should the getters suddenly stop working?

If you teach fluid_event_t to carry something else
than it currently does, for example the rawmidi byte,
then the use of the midi-event getters will become
invalid, they will give wrong results.

[derselbst]

I wonder if there are too many downstream apps (besides my own) that uses the getters, rather than to just feed events to sequencer?

Cant tell, but since fluid_event_t is the more powerful struct of the twos, we have to keep its getters.

Redundancy.

And that's why I want fluid_midi_event_t to be removed.

If you teach fluid_event_t to carry something else than it currently does, for example the rawmidi byte,
then the use of the midi-event getters will become invalid, they will give wrong results.

That's what you have fluid_event_get_type() for. If the user uses the getters incorrectly they will surely get incorrect results, which is even possible nowadays:

fluid_event_noteoff(evt, ch, key); // no velocity!
fluid_event_get_velocity(evt); // undefined

But this doesnt mean they stop working or we're unable to expand fluid_event_t.

[stsp]

The idea of distinction comes from the fact
that fluid_event_t is used as an input to
sequencer from an app, while fluid_midi_event_t
is an opposite direction: from parser to an app.
You want to combine them and also unify the path:
from_app->to_parser->to_app->to_sequencer.
This is a possible solution, but for my app these
pathes are clearly separated, and I need only:
from_app->to_sequencer (never need accessors)
from_app->to_parser->to_app (need all accessors, never need sequencer)

So from my point of view these types are different
and serves different purposes. Of course, as you
explained above, it is possible to combine them.
The questions are only:

• does anyone need to ever put into sequencer
  the events that comes from the parser, provided
  you can put rawmidi bytes to sequencer as well
• does anyone need to ever access the members
  of the event that didn't came from the parser
  (i.e. was created by the user for sequencer).

If the answers are both "no", then I'd say your
proposal is not the only possible one. For my app
both answers are "no", that's why I assume so,
plus the currently existing fluid_midi_event_t
suggests that I used the model that was intended
in fluidsynth at least in the past...

[derselbst]

The idea of distinction comes from the fact that fluid_event_t is used as an input to sequencer from an app, while fluid_midi_event_t is an opposite direction:

I disagree. While fluid_event_t was indeed designed for the sequencer, fluid_midi_event_t seems to originate from the fluid_player.

So from my point of view these types are different and serves different purposes.

I dont see such a separation.

And given your path "from_app->to_parser->to_app": I dont understand why you need fluidsynth types at all, as you dont seem to be doing anything with fluidsynth in this case.

Also I'm not the first one having the idea of removing fluid_midi_event_t. Look at the TODO. This suggestion has been made by Peter H. himself.

[stsp]

And given your path "from_app->to_parser->to_app":
I dont understand why you need fluidsynth types at all,
as you dont seem to be doing anything with fluidsynth in this case.

Yes, just using its parser, and the getters to
get the values out. This is similar to be a separate
parser library, completely unrelated to fluidsynth.
This is why I expect it to have its own types - it
may really be made completely separate. In fact,
before switching to fluidsynth, I used the hand-written
lib for parsing, with its own types etc. So for my
use-case you mix the completely unrelated things,
but of course there are other use cases where this
change may be for good (unless the above answers
evaluate to "no")

[derselbst]

Yes, just using its parser, and the getters to get the values out.

Great, so after having had so much off topic talk about #308, this statement makes me believe that exposing fluid_midi_parser_parse() for your specific use-case is a bad idea, as you seem to rely on the fact that fluid_midi_event_t is a somewhat midi-compliant struct that doesnt modify or discards any data when parsing from rawmidi (as the case for sysex), so that you can just copy and paste this data to use it somewhere else but not for fluidsynth.

"Abusing" fluidsynth as parser library replacement is not what originally was intended by exposing the parser and I'm currently thinking about reverting this.

[stsp]

the fact that fluid_midi_event_t is a somewhat midi-compliant struct that doesnt modify
or discards any data when parsing from rawmidi (as the case for sysex), so that you
can just copy and paste this data to use it somewhere else but not for fluidsynth.

If it does discard things, surely it will do so also
when passing to fluidsynth?

"Abusing" fluidsynth as parser library replacement is not
what originally was intended by exposing the parser and
I'm currently thinking about reverting this.

That's my point too, partially.
I think it:

• can be reverted if the alternative for rawmidi is embedded directly into sequencer
• can't be reverted, to allow people to still use it
  outside of fluidsynth, and yet clearly separate it
  with different types (not used anywhere in fluidsynth,
  except perhaps with conversions)

So to me reverting it from core but still providing
separately is exactly the best solution, and this is
exactly why I am thinking about a separate types
too. But I'll take any other API you can offer of course.

[stsp]

I mean, in an absence of any other rawmidi
support, I am using the parser both ways:
with sequencer and (in different code) separately.
But these uses are very different, that's why
I am a bit uncomfortable about combining them
the way you suggest.

[derselbst]

If it does discard things, surely it will do so also when passing to fluidsynth?

Yes, but it doesnt matter, because fluidsynth doesnt use such long sysex messages.

if the alternative for rawmidi is embedded directly into sequencer

Wont happen. Let fluidsynth use the alsa_raw midi driver, connect to this port in your app and feed it with raw midi events as it arrives. Or properly parse them yourself and push them to fluidsynth manually.

to allow people to still use it outside of fluidsynth,

This the point, because fluid_midi_parser is limited and relies on fluidsynths data types, it doesnt make any sense to allow the parser being used outside of fluidsynth.

[stsp]

"Abusing" fluidsynth as parser library replacement is not what originally
was intended by exposing the parser

But you have a getters for fluid_midi_event_t.
Something that isn't supposed to be used outside
of fluidsynth, should not have getters, because
I don't see the need to use getters before just
passing an event to sequencer. Getters are the
door to the standalone usage, aren't they?
That's why maybe if you don't want to have
fluid_event_t being used outside, you can
just remove getters from it?
I use getters for fluid_midi_event_t, and this
is the only way I use fluid_midi_event_t at all.
I think it is a valid way, not an abuse, as otherwise
there would be no getters to do that?

This the point, because fluid_midi_parser is limited and relies on fluidsynths data types

Even once you cease using fluid_midi_event_t
in fluidsynth?

it doesnt make any sense to allow the parser being used outside of fluidsynth.

It would be a pity if I'll have to dig for another
library just for the parser. Why not to make this
a valid use instead? Which is the whole point of
all my ideas above actually.

[stsp]

In fact, I have a parser rip-off since it was
unexported. This rip-off only contains
enum fluid_midi_event_type and
fluid_midi_event_t.
So I really think the parser can be spared
if fluid_midi_event_t is made exclusive
for it. This should really be a better solution
than to ask the user to please go for the
search of another library for parsing...

[derselbst]

Getters are the door to the standalone usage, aren't they?

Getters are used to lookup events, e.g. if you receive them in a callback function to see what's inside and transform the event if need be. I wont remove any getters.

It would be a pity if I'll have to dig for another library just for the parser. Why not to make this a valid use instead?

You even said yourself that these are completely separate paths in your app. And since for this use-case you dont need fluidsynth at all, it is invalid to abuse fluidsynth as parser, esp. because it doesnt even suit your needs (sysex).

To me it seems that you want to go with the most comfortable solution by solely using fluidsynth. However it is not fluidsynth's job to solve your problem of custom rawmidi processing. Ofc internally fluidsynth is capable of that, but only in means to synthesize sound. This use-case however applies to the alsa_raw midi driver.

[stsp]

And since for this use-case you dont need fluidsynth at all, it is invalid to abuse fluidsynth as parser

And the solution is?
Keep using my own rip-off of the fluidsynth's parser?

This use-case however applies to the alsa_raw midi driver.

But this doesn't give me the parser, which I need too
(besides synthesizing a sound). I use alsa_raw when
it is available, but I can't, for example, get the pcm stream
from it back into an app easily, and there are many
other problems. munt is the most friendly synthesizer
so far, but its not GM.

[derselbst]

And the solution is?

Use a proper parsing library.

But this doesn't give me the parser, which I need too

Srsly, you dont need fluidsynth's limited parser, you need a parsing library.