ssj71 / OSC2MIDI

A highly flexible and configurable OSC to JACK MIDI (and back) bridge
GNU General Public License v3.0
45 stars 7 forks source link

note state not make any difference #30

Closed vinieich closed 6 years ago

vinieich commented 6 years ago

Hello, when I try to use midi to osc with the config below, it will activate the osc message whenever there is a "note on" or "note off" message, regardless of the state that is in the config.

It also happens with the "noteon()" midi message.

This is the line that I'm using: /test/tst , : note( channel, 60, velocity, 1 );

This is the command: osc2midi -v -m tst

And this is the log:


pair created: /test/tst  : noteon( channel, 60, velocity )
1 pairs created.
starting osc server on port 57120
 sending osc messages to address osc.udp://localhost:8000
opening client...
assigning process callback...
initializing JACK input: 
creating ringbuffer...
initializing JACK output: 
creating ringbuffer...
Ready.
matches found:
  noteon ( 0, 60, 86 ) -> /test/tst , 

matches found:
  noteoff ( 0, 60, 64 ) -> /test/tst , ```

English is not my primary language but I hope that I'm being clear.
Thanks!
ssj71 commented 6 years ago

Thanks for the thorough report. I think I understand. You do not want it to send OSC when the note-off occurs. I will look at this when I get a chance. Hopefully this weekend.

vinieich commented 6 years ago

Yes, that is it. If I want to start a metronome for example just playing middle C, it will start one time on "note on" and then it will restart again with "note off" even if on the config I clearly stated that it should activate only on a "note on" state. Thanks for seeing this and there is no hurry. If you need any more info I'll happly provide. Have a good weekend.

ssj71 commented 6 years ago

@agraef I think this is due to the code added around pair.c:1691 in commit c5ba977086d0ed8f15dc49fcfd0b9066e5f9e600

Is there a specific use-case that should have this behavior? I don't think its very consistent with the documentation so I'm leaning toward reverting that commit, but if there's a good reason I'll just add a CLI arg to select it optionally. What do you think?

ssj71 commented 6 years ago

@vinieich I'm pretty confident that if you just comment out lines 1691-1698 in pair.c and rebuild it will work the way you expect. (I haven't tested it yet though). I'd like albert's thoughts on why he added this code before I commit a change though. If that doesn't work let me know.

vinieich commented 6 years ago

@ssj71 I removed that lines and everything now works as it supposed to.

agraef commented 6 years ago

@ssj71 There's no line 1691 in that commit, or am I missing something? Do you mean line 1208-1214 in the commit?

As to what that changeset does, it allows an explicit note-off message (0x80 status) to be recognized as a note-on (0x90 status) with velocity 0, which is what the MIDI standard mandates. The problem is that some MIDI devices and programs will never send a proper note-off message, they will always send note-ons with velocity 0 instead (either for convenience, like Pd, or to make good use of running status, like some hardware devices and MIDI drivers do). And that's perfectly in line with the MIDI standard.

So I do think that we need those lines. Otherwise your program, supposed to catch note-ons only, will behave differently depending on whether your device happens to emit proper note-offs or just note-ons with velocity 0. Which is a bad and totally unexpected inconcistency (at least that's what I thought when I first saw this behavior, which prompted this fix in the first place).

I'd really recommend that we stick to the MIDI standard (as crazy as this aspect of it may be), because that's the only standard there is to follow here.

I think that if you only want to catch proper note-ons (i.e., note-ons with nonzero velocity), then the proper way to do that in osc2midi would be to use a range pattern like 1-127 for the velocity parameter.

agraef commented 6 years ago

In case you don't believe what I just said about the MIDI standard's treatment of zero velocity, here's what Jeff Glatt has to say about it: http://midi.teragonaudio.com/tech/midispec/noteon.htm

(This is not the official MIDI standard by the MMA, which is behind a paywall, but Jeff's description of the MIDI standard is really very accurate and a better read than the standard documents anyway.)

agraef commented 6 years ago

@ssj71 I removed that lines and everything now works as it supposed to.

@vinieich I don't think so. Have you really tried this with a device that emits 0x90 with velocity 0 instead of a 0x80 status? (Please check with a MIDI monitor to make sure that it really emits those.)

I'll give your map a whirl with osc2midi as well.

vinieich commented 6 years ago

@agraef No, I never said that my devices emits 0x90 with velocity 0. My device emits 0x80 as note off and OSC2MIDI was matching that even when I clearly config it to NOT match on note off. I don't know what that code was doing but maybe it was changing the 0x80 to 0x90 with velocity 0 BEFORE the matching? My english is not so good but what I understand from the link that you posted was that 0x90 vel 0 must be treated as 0x80 and not the other way arround. Am I wrong? As my device do not send 0x90 with vel 0, I can't understand why would OSC2MIDI match the "note off" when in the config it was configured to state 1, "note on". Sorry if I didn't understand it correctly.

agraef commented 6 years ago

@agraef No, I never said that my devices emits 0x90 with velocity 0.

I didn't imply that, quite the contrary. I merely asked you to test specifically with a device that does generate 0x90 with velocity 0. If osc2midi is working according to the MIDI standard, then it ought to behave exactly the same as if you're sending 0x80 status with any velocity.

Don't blame me, that's just the craziness of having 0x90 with 0 velocity mean note-off instead of a silent note. ;-) Which is in the standard because in the bad old times bandwidth was scarce, so MIDI devices usually operated with running status to save some bytes transmitted over the MIDI interface.

But what's important here is that, according to the MIDI standard, 0x90 with velocity 0 and 0x80 (with any velocity) are both a note-off. Now, as I remember it (@ssj71 please correct me if I'm wrong), that noteon opcode (or, equivalently, note with a nonzero state argument) is supposed to match any note-on message, even if it has zero velocity. Is that right? At least that's how it's implemented right now.

vinieich commented 6 years ago

@agraef Like I said, english is not my best form of expression :)

I understand (or at least I think I do) this part of the MIDI standard.

What I don't understand is: OSC2MIDI is configured, as you can see on the log above, to respond only to "note on"; It creates the /test/tst : noteon( channel, 60, velocity ) [attention to the NOTEON]; My device DON'T send "note ON with vel 0"; It catch that "note off" as a really "note off" like here:

matches found:
  noteoff ( 0, 60, 64 ) -> /test/tst ,

Why, after all of these, would it trigger the OSC message?

If the MIDI standard says that you need to consider 0x90 vel 0 as a 0x80, so be it. But I don't think that it says that every 0x80 must be converted to 0x90 vel 0.

Besides all of that, this behavior does not seems to be compatible with the documentation.

I really tried to be more helpful on my text now and really, no offense meant.

agraef commented 6 years ago

My english is not so good but what I understand from the link that you posted was that 0x90 vel 0 must be treated as 0x80 and not the other way arround. Am I wrong?

Yes, you are wrong, unfortunately. The following paragraph in that page says:

A device that recognizes MIDI Note On messages must be able to recognize both a real Note Off as well as a Note On with 0 velocity (as a Note Off). There are many devices that generate real Note Offs, and many other devices that use Note On with 0 velocity as a substitute.

It's true, a program that's prepared to deal with 0x80 should interpret 0x90 with 0 vel as a 0x80 with medium velocity. But it also goes the other way round: A program that expects to deal with 0x90 with zero velocity must then interpret any 0x80 as a 0x90 with vel 0. So any program that's supposed to deal with any of these messages, must handle them both in a consistent fashion.

agraef commented 6 years ago

Oops, that was supposed to be a block quote, guess I forgot how to properly format this... Fixed now.

vinieich commented 6 years ago

I've read it again and I still can't figure out why it should convert a "note off" to a "note on vel 0", essentially duplicating the note and making the "state" and note[on,off] useless. What I think it should do is, maybe, convert a 0x90v0 to 0x80 so it can be id as a "note off" as per MIDI standard.

Imagine this use cases:

If anyone is SENDING a note off to a midi device using an OSC message, he/she is supposed to know what kind of midi signal his/her device is allowed to receive and then config accordingly (if not, just running osc2midi on default config with verbose will show them what he is supposed to do).

If anyone is using a note off to send an OSC message, I suppose that the "note off" matching filter should trigger the message either on 0x90v0 or 0x80 as per MIDI standard.

If anyone is using a note on, explicitly, to send an OSC message and his/her controller send the 0x80 as "note off", it shouldn't trigger on 0x80 on any occasion, because the standard says that 0x80 and 0x90v0 must be treated as a "note off".

Wrapping all together, the 0x90, exclusively when the the velocity is 0, MUST be treated as a "note off" and not a "note on". At least is what this is saying.

unless the velocity is 0, in which case, you really have a Note Off)

And here recognize both a real Note Off as well as a Note On with 0 velocity (as a Note Off). Emphasis on "(as a Note Off)"

So I still can't figure out why is a 0x80 triggering a "note on" only config when even if I used a 0x90v0 it SHOULD be treated as a "note off" and not a "note on".

But I understand that we both can have different meanings for what the standard means and I don't want to be unpleasant so I won't be taking anymore of your time. Sorry if I'm being repetitive but it's hard to express myself on another language accurately. Thanks and as I said before, english is not my primary language, no offense meant.

agraef commented 6 years ago

What I don't understand is: OSC2MIDI is configured, as you can see on the log above, to respond only to "note on";

Yeah, that behavior is kind of counter-intuitive, I agree.

@ssj71, maybe we need to think about what noteon / noteoff should actually mean. Besides what we have now (where 0x80 and 0x90-vel 0 are considered equivalent, which after all is convenient for the standard "synth" use case), one might argue two other approaches:

@ssj71, what's your take?

agraef commented 6 years ago

Thanks and as I said before, english is not my primary language, no offense meant.

No worries, we developers with our code out in the wild have to be able to take a bit of heat, and as long as we keep to the problem at hand and don't start insulting each other, everything's fine. :)

agraef commented 6 years ago

Thinking about it some, I've come to think that @vinieich makes a valid point. There's too much magic in the note-on/off treatment involved right now. One should be able to control exactly which MIDI messages one wants to receive, and that's just not possible when receiving MIDI note-ons and -offs right now.

So it certainly makes sense to go for the low-level interpretation and just remove all the special treatment of the 0x90 and 0x80 status bytes. OTOH, this does make me feel a bit wary, because it makes it just too easy to mess up if you do need the standard MIDI interpretation -- people then have to provide rules for both noteon(x, 0) and noteoff(x, vel) to properly implement (semantic) note-off.

ssj71 commented 6 years ago

Wow I missed quite some conversation. :)

noteon / noteoff means what it says from a more abstract, semantic perspective, but then noteon should only be matched by 0x90 with vel>0, and noteoff should also be matched by 0x90 with vel=0.

This one. But that's not what we currently have. I was aware of the midi spec that noteoff == noteon 0. osc2midi should work the same regardless of which method a midi controller sends to it. The note(x,x,x,1) really should give the same results as a noteon() opcode too. Its been long enough since I've been through the code that I'm not confident it is, but it should be. The issue with the current implementation is that vinieich cannot specify a pair that only matches note-ons and ignores note-offs, which is a perfectly reasonable thing to be able to do. I think in either noteoff or noteon 0 the behavior should be that it ignores note-off messages as well as a note-on with velocity 0 as vinieich expects. If someone wants a pair that matches both note-on and note-off (using either noteoff or noteon 0) they can do it with 2 pairs with identical osc sides or just a note( , , ,x).

I think the only change necessary is to take out that chunk that vinieich already confirmed gave him that behavior.

The stickier point IMO is whether we should make some way to specify which method of note-off the osc->midi sends. Currently it defaults to a noteoff message but it can be deliberately made with a pair using noteon(chan, note, 0) so maybe no change is necessary for that regard. I'm not familiar with any devices that require noteon 0 and don't properly interpret the noteoff.

ssj71 commented 6 years ago

@agraef since we do have control of how the pair represents the noteoff data (always with the 0x90 code) I think its sufficient to only do conversion from 0x80->0x90 and not the other way 'round. We never have a pair object that will properly be intended to match the other way. After looking through the code a bit I'm pretty confident we are doing it that way and so we can safely remove the code block discussed without any problem of handling either note-off or note-on 0.

EDIT: I just pushed a commit to resolve this and put it the way I think it should be. I'm still open for discussion if you feel we need something more.

agraef commented 6 years ago

@ssj71, so why didn't you just revert the entire commit then? The rest of the commit is just about using a copy mymsg of msg, which isn't needed any more because you now removed the only lines actually mutating the MIDI message. :)

agraef commented 6 years ago

Ok, I see. Apparently the code has changed quite a bit since the commit, so simply reverting it wasn't possible.

agraef commented 6 years ago

The stickier point IMO is whether we should make some way to specify which method of note-off the osc->midi sends. Currently it defaults to a noteoff message but it can be deliberately made with a pair using noteon(chan, note, 0) so maybe no change is necessary for that regard.

I completely agree with that.

I'm not familiar with any devices that require noteon 0 and don't properly interpret the noteoff.

Such a device would violate the MIDI standard, so no need to cater to those. :)

vinieich commented 6 years ago

Ok, I see that the issue now is about code so that's my cue. @agraef thanks for the patience @ssj71 thanks for this nice piece of software Have a good weekend both of you. Ciao

agraef commented 6 years ago

Ok, so this issue seems to be fixed, right?

Meanwhile, while trying to test @vinieich's example, I've run into a different issue: my osc2midi doesn't actually read any midi at all! I think I had this working at some point, but now it seems that there's a call to jack_ringbuffer_write_advance missing in jackdriver.c, so let me prepare a PR for that...

agraef commented 6 years ago

Oops, false alarm, MIDI is working all right over here after all. Silly case of PEBKAC. 🤦‍

And yes, I can confirm that everything is working properly now, so this issue can be closed. Even better, Jack MIDI actually seems to map 0x90 with vel 0 to proper 0x80 note-offs with medium velocity anyway (I checked that with Pd which allows me to send either), so the issue I described about accidentally messing up the note-off handling in a map doesn't really exist in the first place. Thus just specifying a noteoff or note(, , , 0) will reliably catch a note-off in any case.

The bottom line is that Jack does exactly the right thing here to make the low-level (physical) and high-level (semantic) interpretations match, so that we get both at the same time. Which is nice.

@vinieich, thanks for your patience and reporting the bug. This stupid piece of code that @ssj71 just removed was written by me, it seemed a good idea at the time... And have a nice weekend, too, both of you. 😃

osc2midi is such a nice and useful program. The only thing it lacks right now is direct OSC <-> OSC and MIDI <-> MIDI mapping, then it would be the controller mapping tool for Linux. (Well, you can do that by just doing chains of osc2midi instances, but that's a little inconvenient.)

ssj71 commented 6 years ago

@agraef just a thought though, I think when other midi backends are implemented (its on the unwritten todo list) some may not do this conversion for us. Rather than converting note off to note-on 0 we'll do the other way which I think provides what we really want.

I've just pushed a commit to master which implements this (I know I should have done a branch but I was feeling a bit lazy).

agraef commented 6 years ago

So the mymsg copy now did find a good use after all. ;-)

Maybe add mymsg[2] = 64; as well there, making it a note-off at medium velocity? At least that's what Jack MIDI apparently does.

Also, Jack MIDI will always do the conversion, while the code you added will only do it for a noteoff() pattern, so a vel 0 0x90 would still be matched against noteon(), right? So maybe the p->opcode == 0x80 part of the condition should be deleted, to emulate the behavior of Jack MIDI.

ssj71 commented 6 years ago

Alright, that is done.

Looking at it, I didn't like that the conversion would execute 1x for every pair in the map so I instead did the conversion where the midi is received rather than when trying to match the pair. This then removes the possibility for a map of osc/something : noteon(chan, n, 0) but I don't think thats really a "feature" that is needed. I'm closing this issue, but of course am open to further discussion.

agraef commented 6 years ago

Looking good to me.

There are some rare use cases where you actually might want to send a proper note on with 0 velocity to some special MIDI equipment, but that's on the output side (and I'm not sure that this is even supported by Jack MIDI anyway; if it's doing the conversion on the input side, it might do the same on output).